822366 - Implement Mixed Content Blocker New Icon - Frontend Changes

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

This bug is for the frontend changes needed for the Mixed Active Content Icon.  The patch has already been reviewed and r+'ed (https://bugzilla.mozilla.org/attachment.cgi?id=689834&action=edit), so I'll carry that over to here.


+++ This bug was initially created as a clone of Bug #782654 +++

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Change Site Identity Icon for Mixed Active Content v5

Carrying over patch and r+ from Dao in bug 782654.

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 693103 [details] [diff] [review]
Change Site Identity Icon for Mixed Active Content v5

Review of attachment 693103 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/pinstripe/browser.css
@@ +1361,5 @@
>  #identity-box[open=true] > #page-proxy-favicon {
>    -moz-image-region: rect(0, 32px, 16px, 16px);
>  }
>  
>  @media (min-resolution: 2dppx) {

Oops, this doesn't work on hi-dpi OS X -- you'll get http://cl.ly/image/161t2Z3m060Z.

You need to to:

* add a [e-mail] image that's twice the resolution + jar.mn entry

* add an appropriate rule for it in this @media block

Comment 3

[Tanvi Vyas[:tanvi]]

Attachment: Change Site Identity Icon for Mixed Active Content v6

I'm adding a new patch that should fix the bug that Justin identified in comment 2.  Except, that I need a 32x32 image of the mixed content triangle icon.  For now, I have populated /browser/themes/pinstripe/[e-mail] with a copy of /browser/themes/pinstripe/identity-icons-https-mixed-active.png

I'm going to mark this for review now in case there are any changes.  I won't land until I have the new image and have updated /browser/themes/pinstripe/[e-mail] with it.  Thanks!

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 697717 [details] [diff] [review]
Change Site Identity Icon for Mixed Active Content v6

Review of attachment 697717 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/pinstripe/jar.mn
@@ +33,5 @@
>    skin/classic/browser/[e-mail]
>    skin/classic/browser/identity-icons-https-ev.png
>    skin/classic/browser/[e-mail]
> +  skin/classic/browser/identity-icons-https-mixed-active.png
> +  skin/classic/browser/[e-mail]

The suffix used on hidpi images is "@2x", not "@2px".

The patch looks fine, otherwise.

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: Change Site Identity Icon for Mixed Active Content v7

(In reply to Justin Dolske [:Dolske] from comment #4)
> Comment on attachment 697717 [details] [diff] [review]
> Change Site Identity Icon for Mixed Active Content v6
> 
> Review of attachment 697717 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/pinstripe/jar.mn
> @@ +33,5 @@
> >    skin/classic/browser/[e-mail]
> >    skin/classic/browser/identity-icons-https-ev.png
> >    skin/classic/browser/[e-mail]
> > +  skin/classic/browser/identity-icons-https-mixed-active.png
> > +  skin/classic/browser/[e-mail]
> 
> The suffix used on hidpi images is "@2x", not "@2px".
> 
> The patch looks fine, otherwise.

Sorry, changed it to 2x.  Now we just need the icon from Stephen.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 697816 [details] [diff] [review]
Change Site Identity Icon for Mixed Active Content v7

Looks good. (Well, I only looked at the tiny hidpi changes, so this is really r=dao)

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Contingency icon

Here's a crudely-scaled version of the icons. 10 seconds of work. It would be ok to land with this to take the immediate pressure off our beloved shorlander. ;-) [If this happens, please file a followup to get true hidpi images in before final release.]

Comment 8

[Stephen Horlander [:shorlander]]

Attachment: Mixed Content Warning Icon @2x

Comment 9

[Tanvi Vyas[:tanvi]]

Attachment: Change Site Identity Icon for Mixed Active Content v8

This incorporates Stephen's new icon.  Dolske, can you test it?

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 698022 [details] [diff] [review]
Change Site Identity Icon for Mixed Active Content v8

Yep, icon works properly in hidpi mode now.

Comment 11

[Tanvi Vyas[:tanvi]]

Comment on attachment 698022 [details] [diff] [review]
Change Site Identity Icon for Mixed Active Content v8

Carrying over r+.

Comment 12

[Tanvi Vyas[:tanvi]]

Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/abba54950354

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/abba54950354