860581 - Add support for Mixed Content Blocking - Android

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Some background: https://blog.mozilla.org/tanvi/2013/04/10/mixed-content-blocking-enabled-in-firefox-23/

Bug 815321 is the meta bug for desktop.

Most of the UI code seems to be in IndentityHandler. The version in Fennec is way out of date now:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6411

The popup code is here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6445

Comment 1

[:Margaret Leibovic]

I think this will be hard for us to do with our current UI because we don't have a way to show an indicator for a dismissed doorhanger notification (I'm having trouble finding the bug that I know was filed about this). I'm not sure if we want to address that issue first, or come up with different UX than desktop.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

If we need UI work, let's get Ian thinking about this...

Comment 3

[Chris Peterson [:cpeterson]]

I'd like to work on this for Q2.

Comment 4

[Chris Peterson [:cpeterson]]

Update: I won't have time to look at this bug in Q2.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Margaret and I talked about re-using the Lock icon for mixed content notification. It's a good start and won't cause much UI churn.

Comment 6

[Tanvi Vyas[:tanvi]]

This appears to be a duplicate of 801945.

Comment 8

[:Margaret Leibovic]

Ian, this is on the roadmap for Fx24, and I'm wondering if you've thought about it at all. Because we don't have persistent doorhanger notification icons, our UX will need to be different than desktop's. As mfinkle mentioned in comment 5, I think our best option right now would be to build on top of the lock icon and site identity popup, but I'm not sure of how we'd want to add buttons to that for the "Keep Blocking" and "Disable Protection on This Page" options.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

e could try to add the "Keep Blocking" and "Disable Protection on This Page" options to the Larry popup, displayed when tapping the lock/favicon.

Comment 10

[Ian Barlow (:ibarlow)]

Attachment: Design explorations

Margaret, this is what I have so far -- mostly I'm exploring how best to communicate this in the title bar, as well as what the notification would look like. 

Rolling this into the Larry menu wasn't an option I had considered. I worry that might overload the user with information that isn't relevant to the message we are trying to communicate about blocking content though...

Comment 11

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #10)
> Created attachment 759103 [details]
> Design explorations
> 
> Margaret, this is what I have so far -- mostly I'm exploring how best to
> communicate this in the title bar, as well as what the notification would
> look like. 
> 
> Rolling this into the Larry menu wasn't an option I had considered. I worry
> that might overload the user with information that isn't relevant to the
> message we are trying to communicate about blocking content though...

From a technical perspective, if we're using the site identity icon to show/hide this popup, it's probably easiest to just use the same view for the popup. However, whether or not we show similar "Larry" UI is a UX decision.

One thing to keep in mind is that if we reuse the site identity icon for a blocked content indicator is that there isn't a separate place to view site identity information like there is on desktop, so we probably do want to combine the site identity information from the normal Larry popup into the blocked content popup (e.g. blocked content on an SSL site).

I started porting over some code from desktop's gIdentityHandler into our IdentityHandler, and our implementation will need to look a bit different to handle this "blocked content as an identity state" design.

Tanvi, do you have any good test pages you used while developing this feature?

Comment 12

[Tanvi Vyas[:tanvi]]

I am glad we are reusing the shield icon and trying to be consistent with security UX across platforms.  I think that is really important for our users.

The UX for Mixed Content Blocker on Android is tricky.  On one hand, users are used to seeing the lock icon and that is how they look to see if a site is secure.  Replacing the lock with the shield icon may cause some confusion.  But adding a third icon will really crowd the address bar.

For the case where user's "disable protection on this page" can we consider using the icon that we use on desktop, instead of the broken shield: https://people.mozilla.com/~tvyas/FigureD.jpg

If the user is visiting an HTTP page, what icons show up on the mobile firefox?  Just the favicon?

Margaret, I put together some test cases on people.mozilla.org.  Here are a few basic ones that show an alert box when mixed content protection is disabled:
https://people.mozilla.com/~tvyas/mixedcontent.html
https://people.mozilla.com/~tvyas/mixedboth.html
https://people.mozilla.com/~tvyas/mixeddisplay.html

If you'd like more complicated test cases (mixed grandchild iframes, delayed mixed content, mixed content caused by document.open), let me know and I'll compile a list of those.

Thanks for working on this!  As far as I know, we will be the first mobile browser to have a Mixed Content Blocker UI.

Comment 13

[:Margaret Leibovic]

(In reply to Tanvi Vyas [:tanvi] from comment #12)

> For the case where user's "disable protection on this page" can we consider
> using the icon that we use on desktop, instead of the broken shield:
> https://people.mozilla.com/~tvyas/FigureD.jpg

I agree that it would be good to be consistent with desktop on the icons here.

> If the user is visiting an HTTP page, what icons show up on the mobile
> firefox?  Just the favicon?

Yes, we only show the favicon, to try to save URL bar real estate.

> Margaret, I put together some test cases on people.mozilla.org.  Here are a
> few basic ones that show an alert box when mixed content protection is
> disabled:
> https://people.mozilla.com/~tvyas/mixedcontent.html
> https://people.mozilla.com/~tvyas/mixedboth.html
> https://people.mozilla.com/~tvyas/mixeddisplay.html
> 
> If you'd like more complicated test cases (mixed grandchild iframes, delayed
> mixed content, mixed content caused by document.open), let me know and I'll
> compile a list of those.

Thanks for the links to these pages! I think this is good for now, and we can try out more complicated testcases once we get something implemented.

> Thanks for working on this!  As far as I know, we will be the first mobile
> browser to have a Mixed Content Blocker UI.

:)

Comment 14

[:Margaret Leibovic]

Ian, can you hook me up with some icons? I need shield icons and yellow triangle icons (if you agree that's better than the broken shield icon, for consistency with desktop), in the same dimensions we use for the lock icon:
http://mxr.mozilla.org/mozilla-central/find?string=site_security_identified&tree=mozilla-central&hint=

For my WIP I stole these shield icons from desktop and they seem to work, so I might not actually need a new set of those:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/mixed-content-blocked-16.png
http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/[e-mail]
http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/mixed-content-blocked-64.png

Comment 15

[:Margaret Leibovic]

Probably want to track this for 24 or 25. Work is definitely coming along on this, so I could probably land it for 24, but we can disable it with a pref easily.

Comment 16

[Ian Barlow (:ibarlow)]

Attachment: Updated mockups

Here is an updated set of mocks that addresses a few things from this discussion. 

* Combines mixed content warnings with the site identity doorhanger
* Uses warning triangle for consistency with desktop
* Allows protection to be turned back on from the doorhanger, if it has been disabled. Note: I notice this is not available on desktop, but it's also something I struggle with there -- if I can turn protection off, it seems like I should have the ability to restore it as well.
* Some visual cleanup to our doorhanger styling (more unified typography, large icons on the left for consistency with desktop)

From my brief conversation with Margaret on IRC this seems like a good step forward, but Tanvi if you have any additional feedback please let us know!

Comment 17

[:Margaret Leibovic]

Attachment: WIP

I think it's time to get some eyes on this. I've been building this on top of the patches in bug 751205, so it will probably need some fixing up once I address the review comments in there.

Other than needing some style love, using a DoorHanger in the SiteIdentityPopup seems to be doing what I want. I only implemented the blocked case for now, haven't gotten around to the loaded mixed content case.

And right now my logic in IdentityHandler is kinda wrong, I need to handle the case of mixed blocked content on sites with various security levels. Desktop handles this differently because they have a separate doorhanger notification icon from the lock/identity icon. I like how currently the state flags correspond with different icons in the toolbar, but I think I need to pass some secondary state for the blocked mixed content case to do this right.

Comment 18

[Larissa Co [:lco]]

I(In reply to Ian Barlow (:ibarlow) from comment #16)
> Created attachment 762188 [details]
> Updated mockups
> 
> Here is an updated set of mocks that addresses a few things from this
> discussion. 
> 
> * Combines mixed content warnings with the site identity doorhanger

I like this! I think it's simplifying. The only issue I have is that in the Site identity doorhanger when Mixed Content is enabled, we still show something like "verified by Equifax.... Encrypted". This is confusing to the user because the following message says that certain content isn't secure. 

On desktop, we're modifying the Larry Menu to accommodate a handful of cases involving mixed active content and mixed passive content, so that the security messaging is consistent with the state of mixed content. See bug 838402. There are a bunch of small tweaks to the text in subsequent comments, but this comment is a good place to start: https://bugzilla.mozilla.org/show_bug.cgi?id=838402#c3


> * Uses warning triangle for consistency with desktop
> * Allows protection to be turned back on from the doorhanger, if it has been
> disabled. Note: I notice this is not available on desktop, but it's also
> something I struggle with there -- if I can turn protection off, it seems
> like I should have the ability to restore it as well.

this was the original design; it's on my to-do list to reiterate the desired interaction in the bug :)

> * Some visual cleanup to our doorhanger styling (more unified typography,
> large icons on the left for consistency with desktop)
> 
> From my brief conversation with Margaret on IRC this seems like a good step
> forward, but Tanvi if you have any additional feedback please let us know!

Comment 19

[Brian Nicholson (:bnicholson)]

Comment on attachment 762217 [details] [diff] [review]
WIP

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

For views like this that are built around Gecko, it would be nice if we could have a native-friendly implementation and Gecko wrapper. Do you think we can separate out the logic in DoorHanger into two different classes -- something like DoorHanger and GeckoDoorHanger (I know we've been abusing the GeckoX class names, but it might actually make sense here)? DoorHanger could be used when being created from Java where the underlying implementation doesn't need to communicate with Gecko. GeckoDoorHanger could extend DoorHanger and provide the necessary JSON wrapping/unwrapping calls from Gecko that it passes to/from DoorHanger.

::: mobile/android/base/SiteIdentityPopup.java
@@ +68,5 @@
> +        // (ab)use the existing DoorHanger logic
> +        mMixedContentDoorHanger = new DoorHanger(mActivity, null, -1, "") {
> +            @Override
> +            public void onClick(View v) {
> +                if (v.getTag().equals("0")) {

Pulling out the tag here that we're using internally breaks encapsulation. It would be nice if we could have a proper native doorhanger implementation for registering these callbacks on the buttons.

@@ +90,5 @@
> +
> +        try {
> +            JSONObject disableButton = new JSONObject();
> +            disableButton.put("label", mActivity.getString(R.string.disable_protection));
> +            disableButton.put("callback", 0);

Yikes, I hate how we have to use JSON just for dealing with our own Java-side API; we aren't even doing any remote messaging. Again, creating a native doorhanger class would fix this.

Comment 20

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) (gone through June 22) from comment #19)
> Comment on attachment 762217 [details] [diff] [review]
> WIP
> 
> Review of attachment 762217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For views like this that are built around Gecko, it would be nice if we
> could have a native-friendly implementation and Gecko wrapper. Do you think
> we can separate out the logic in DoorHanger into two different classes --
> something like DoorHanger and GeckoDoorHanger (I know we've been abusing the
> GeckoX class names, but it might actually make sense here)? DoorHanger could
> be used when being created from Java where the underlying implementation
> doesn't need to communicate with Gecko. GeckoDoorHanger could extend
> DoorHanger and provide the necessary JSON wrapping/unwrapping calls from
> Gecko that it passes to/from DoorHanger.
> 
> ::: mobile/android/base/SiteIdentityPopup.java
> @@ +68,5 @@
> > +        // (ab)use the existing DoorHanger logic
> > +        mMixedContentDoorHanger = new DoorHanger(mActivity, null, -1, "") {
> > +            @Override
> > +            public void onClick(View v) {
> > +                if (v.getTag().equals("0")) {
> 
> Pulling out the tag here that we're using internally breaks encapsulation.
> It would be nice if we could have a proper native doorhanger implementation
> for registering these callbacks on the buttons.
> 
> @@ +90,5 @@
> > +
> > +        try {
> > +            JSONObject disableButton = new JSONObject();
> > +            disableButton.put("label", mActivity.getString(R.string.disable_protection));
> > +            disableButton.put("callback", 0);
> 
> Yikes, I hate how we have to use JSON just for dealing with our own
> Java-side API; we aren't even doing any remote messaging. Again, creating a
> native doorhanger class would fix this.

Yeah, I felt pretty bad while writing this. I think you're right that we need to have a different API from Java.

Comment 21

[:Margaret Leibovic]

Attachment: WIP v2

This takes Brian's suggestion and is built on top of my patch from bug 884069.

Comment 22

[:Margaret Leibovic]

Attachment: WIP v3

This builds on top of my more recent patch for bug 884069 (a slightly newer version than the one in that bug).

I also added the "Protection OFF" doorhanger from the mockup. The buttons are all working, but it doesn't look super pretty. Maybe I can land the functionality and convince Sriram to make it look better :)

I was wondering if I should be doing something smarter to avoid having these views hang around forever. Would it be worth creating and destroying the DoorHangers on the fly, rather than caching them?

Comment 23

[:Margaret Leibovic]

Attachment: screenshot: protection on

Comment 24

[:Margaret Leibovic]

Attachment: screenshot: protection off

Comment 25

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #22)

> I also added the "Protection OFF" doorhanger from the mockup. The buttons
> are all working, but it doesn't look super pretty. Maybe I can land the
> functionality and convince Sriram to make it look better :)
> 

Would you mind filing a follow-up bug for that and cc-ing me (and sriram) on it?

Comment 26

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #25)
> (In reply to :Margaret Leibovic from comment #22)
> 
> > I also added the "Protection OFF" doorhanger from the mockup. The buttons
> > are all working, but it doesn't look super pretty. Maybe I can land the
> > functionality and convince Sriram to make it look better :)
> > 
> 
> Would you mind filing a follow-up bug for that and cc-ing me (and sriram) on
> it?

Sure, I'll do that. If we want this for 24, we need to get moving on landing this ugly version this week, but then we can polish it up and uplift.

However, it would be good if we can pick off some low-hanging fruit before we land. For example, the shield icon I stole from desktop looks good, but I still need to swap in real yellow triage icons (the desktop ones have two images and are cropped with CSS, so they don't work here).

Comment 27

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 765107 [details] [diff] [review]
WIP v3

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

Overall questions:

If MixedContent is tied with SiteIdentityPopup, why are we creating a new doorhanger for it? Can't we just inflate the SiteIdentityPopup's content to have these, and set appropriate values?

The current piece of code returns early if there is a mBlockedMixedContent. Why?

f- to clarify the doubts. :(

::: mobile/android/app/mobile.js
@@ +475,5 @@
>  
> +#ifdef NIGHTLY_BUILD
> +// Block insecure active content on https pages
> +pref("security.mixed_content.block_active_content", true);
> +#endif

I'm leaving this part to :wesj :)

::: mobile/android/base/BrowserToolbar.java
@@ +968,5 @@
>              mSiteSecurity.setImageLevel(2);
> +        } else if (mode.equals(SiteIdentityPopup.BLOCKED_MIXED_CONTENT)) {
> +            mSiteSecurity.setImageLevel(3);
> +        } else if (mode.equals(SiteIdentityPopup.LOADED_MIXED_CONTENT)) {
> +            mSiteSecurity.setImageLevel(4);

Please make all these images level as final ints, so that this piece of code is more readable. :(

::: mobile/android/base/SiteIdentityPopup.java
@@ +89,5 @@
>              Log.e(LOGTAG, "Exception trying to get identity data", e);
> +        }
> +    }
> +
> +    private void blockedMixedContent() {

The method name doesn't convey a meaning to me :(
Are we blocking? Do we want to block?

@@ +95,5 @@
> +            mLoadedMixedContent.setVisibility(View.GONE);
> +        }
> +
> +        if (mBlockedMixedContent != null) {
> +            mBlockedMixedContent.setVisibility(View.VISIBLE);

Wouldn't this avoid updating the mBlockedMixedContent after the first inflation (and setting things)??

@@ +100,4 @@
>              return;
>          }
>  
> +        mBlockedMixedContent = new DoorHanger(mActivity);

This is exactly my confusion in patch for bug 884069. Who owns the arrowpopup that surrounds this doorhanger?

@@ +102,5 @@
>  
> +        mBlockedMixedContent = new DoorHanger(mActivity);
> +
> +        String message = mActivity.getString(R.string.blocked_mixed_content_message_top) + "\n" +
> +                         mActivity.getString(R.string.blocked_mixed_content_message_bottom);

Why do we concatenate here? Can't we do it in XML?

@@ +106,5 @@
> +                         mActivity.getString(R.string.blocked_mixed_content_message_bottom);
> +        mBlockedMixedContent.setMessage(message);
> +
> +        // XXX: Fix this link
> +        mBlockedMixedContent.addLink(mActivity.getString(R.string.learn_more), "http://mozilla.org", "\n");

"\n" ?

@@ +108,5 @@
> +
> +        // XXX: Fix this link
> +        mBlockedMixedContent.addLink(mActivity.getString(R.string.learn_more), "http://mozilla.org", "\n");
> +
> +        DoorHanger.OnButtonClickListener listener = new DoorHanger.OnButtonClickListener() {

This interface name is trying to override the View's click listener. That doesn't sound like a good approach. Please rename it to something specific to DoorHanger.

And try to import DoorHanger.OnNewInterfaceClickListener, so this statement is shorter.

@@ +201,5 @@
>              mHost.setTextColor(mResources.getColor(R.color.identity_identified));
>              mOwner.setTextColor(mResources.getColor(R.color.identity_identified));
> +        } else if (mode.equals(BLOCKED_MIXED_CONTENT)) {
> +            // Show a notification that allows the user to disable mixed content blocking.
> +            blockedMixedContent();

This should be loadBlockedMixedContent() then.

@@ +224,5 @@
> +        super.dismiss();
> +
> +        // Hide the mixed content UI whenever the popup is dismissed.
> +        if (mBlockedMixedContent != null) {
> +            mBlockedMixedContent.setVisibility(View.GONE);

I would like all these null checks to go into private methods.

showBlockedMixedContent(boolean doShow);

This can show/hide it.

Comment 28

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #27)

> If MixedContent is tied with SiteIdentityPopup, why are we creating a new
> doorhanger for it? Can't we just inflate the SiteIdentityPopup's content to
> have these, and set appropriate values?

Instead of making more custom XML, it's nicer to reuse the existing logic we have to create and style DoorHanger notifications. After thinking about it a bit, I'm leaning towards just creating these on the fly and getting rid of them after they're used, since they probably won't be shown very often.

> ::: mobile/android/base/BrowserToolbar.java
> @@ +968,5 @@
> >              mSiteSecurity.setImageLevel(2);
> > +        } else if (mode.equals(SiteIdentityPopup.BLOCKED_MIXED_CONTENT)) {
> > +            mSiteSecurity.setImageLevel(3);
> > +        } else if (mode.equals(SiteIdentityPopup.LOADED_MIXED_CONTENT)) {
> > +            mSiteSecurity.setImageLevel(4);
> 
> Please make all these images level as final ints, so that this piece of code
> is more readable. :(

You mean defined as constants in SiteIndetityPopup?

> ::: mobile/android/base/SiteIdentityPopup.java
> @@ +89,5 @@
> >              Log.e(LOGTAG, "Exception trying to get identity data", e);
> > +        }
> > +    }
> > +
> > +    private void blockedMixedContent() {
> 
> The method name doesn't convey a meaning to me :(
> Are we blocking? Do we want to block?

Yeah, it should probably be something like showBlockedMixedContentNotification(), although that's hella long :)

> @@ +95,5 @@
> > +            mLoadedMixedContent.setVisibility(View.GONE);
> > +        }
> > +
> > +        if (mBlockedMixedContent != null) {
> > +            mBlockedMixedContent.setVisibility(View.VISIBLE);
> 
> Wouldn't this avoid updating the mBlockedMixedContent after the first
> inflation (and setting things)??

The things only need to be set once, they do the same thing for every page. I just did this to be lazy about creating the DoorHanger object. But like I mentioned up above, it could be better to just create them as needed, then destroy them after they're used.

> @@ +100,4 @@
> >              return;
> >          }
> >  
> > +        mBlockedMixedContent = new DoorHanger(mActivity);
> 
> This is exactly my confusion in patch for bug 884069. Who owns the
> arrowpopup that surrounds this doorhanger?

See my new patch in bug 884069, DoorHanger doesn't need to know anything about a popup anymore. It can be anywhere! :)

> @@ +102,5 @@
> >  
> > +        mBlockedMixedContent = new DoorHanger(mActivity);
> > +
> > +        String message = mActivity.getString(R.string.blocked_mixed_content_message_top) + "\n" +
> > +                         mActivity.getString(R.string.blocked_mixed_content_message_bottom);
> 
> Why do we concatenate here? Can't we do it in XML?

Yeah, this is a bit of a hack, but we'd need to change the DoorHanger API to allow for multi-line messages. Perhaps we can do this as a follow-up (we're already doing some concatenation in other parts of this popup, so I figured this wasn't too terrible of a hack for a first pass).

> @@ +106,5 @@
> > +                         mActivity.getString(R.string.blocked_mixed_content_message_bottom);
> > +        mBlockedMixedContent.setMessage(message);
> > +
> > +        // XXX: Fix this link
> > +        mBlockedMixedContent.addLink(mActivity.getString(R.string.learn_more), "http://mozilla.org", "\n");
> 
> "\n" ?

This is a delimiter param I added to addLink. When that code to add a link was originally written, it was hard-coded to just add a space between the end of the message text and the start of the link. Perhaps it would be better to just have an API to completely set the message content of the DoorHanger altogether, since the current API doesn't work too well for this use case.

Comment 29

[:Margaret Leibovic]

Attachment: WIP v4

I think this feels a lot nicer. This patch just adds/removes notifications on the fly, and it consolidates the common logic for the blocked/loaded notifications.

Comment 30

[:Margaret Leibovic]

Attachment: patch

This still has a few issues, but I think they're minor enough that they can be addressed in follow-up bugs, especially since we're trying to land the string-dependent part of this feature before the merge.

Follow-up issues:
* Update orange triangle icons
* Create gray Larry icon
* Add icon to notification (probably want to do this by adding support for it to DoorHanger)
* Move Larry to the left side of the popup (and it looks like we'll want a bigger Larry icon)
* Make font size in the notification smaller
* Change link text color
* Create message contents in a less hacky way once bug 885867 is fixed (this will probably help fix the previous two issues listed)

Comment 31

[Wesley Johnston (:wesj)]

Comment on attachment 766099 [details] [diff] [review]
patch

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

::: mobile/android/base/SiteIdentityPopup.java
@@ +37,5 @@
> +    public static final int LEVEL_UKNOWN = 0;
> +    public static final int LEVEL_IDENTIFIED = 1;
> +    public static final int LEVEL_VERIFIED = 2;
> +    public static final int LEVEL_BLOCKED_MIXED_CONTENT = 3;
> +    public static final int LEVEL_LOADED_MIXED_CONTENT = 4;

Really nitpicky, but I think I'd prefer these as LEVEL_MIXED_CONTENT_<X> so that they fall together in alphabetical order.

@@ +41,5 @@
> +    public static final int LEVEL_LOADED_MIXED_CONTENT = 4;
> +
> +    // FIXME: Update this URL for mobile
> +    private static final String MIXED_CONTENT_SUPPORT_URL =
> +        "https://support.mozilla.org/kb/how-does-content-isnt-secure-affect-my-safety";

File a SUMO bug maybe?

@@ +61,5 @@
>          mResources = aActivity.getResources();
>      }
>  
> +    public static int getSecurityImageLevel(String mode) {
> +        if (mode.equals(IDENTIFIED)) {

Its better to do IDENTIFIED.equals(mode) here (in case mode == null).

@@ +143,5 @@
> +        removeMixedContentNotification();
> +
> +        mMixedContentNotification = new DoorHanger(mActivity);
> +
> +        // FIXME: Do this a less hacky way once bug 885867 is fixed.

Remove this. Maybe we don't event want it....

::: mobile/android/chrome/content/browser.js
@@ +1326,5 @@
>          // Try to use the session history to reload so that framesets are
>          // handled properly. If the window has no session history, fall back
>          // to using the web navigation's reload method.
> +        let flags = allowMixedContent ? Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT :
> +                    Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_PROXY | Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;

We'll wind up doing this even if the pref isn't set. Is that ok?

@@ +6053,5 @@
> +  IDENTITY_MODE_IDENTIFIED: "identified",
> +
> +  // Blocked active mixed content. Shield icon is shown, with a popup option to load content.
> +  // Only used if "security.mixed_content.block_active_content" is enabled.
> +  IDENTITY_MODE_BLOCKED_MIXED_CONTENT: "blocked_mixed_content",

Hmm... Maybe we can add a little note in here that even though mixed_content and identitity_mode are orthogonal, our Java frontend coalesce them into one indicator.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +71,5 @@
>  identity.identified.verified_by_you=You have added a security exception for this site
>  identity.identified.state_and_country=%S, %S
>  identity.identified.title_with_country=%S (%S)
>  identity.encrypted2=Encrypted
> +identity.ownerUnknown3=unknown

I realize this is in the mockups. I'm suspcious it wasn't intentional. Desktop still includes these brackets, but maybe we're just ahead of the times...

Comment 32

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #31)

> ::: mobile/android/chrome/content/browser.js
> @@ +1326,5 @@
> >          // Try to use the session history to reload so that framesets are
> >          // handled properly. If the window has no session history, fall back
> >          // to using the web navigation's reload method.
> > +        let flags = allowMixedContent ? Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT :
> > +                    Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_PROXY | Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;
> 
> We'll wind up doing this even if the pref isn't set. Is that ok?

I think that's fine. We only send a message with this "allowMixedContent" flag from the mixed content UI, which wouldn't ever show up if the pref isn't set.

Comment 33

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b1e686b983

Filed bug 885962 for the polish follow-ups.

Comment 34

[Phil Ringnalda (:philor)]

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

Comment 35

[Tanvi Vyas[:tanvi]]

Great work Margaret!  I have a few questions/comments.

* What does the UI look like for these two cases:

https://people.mozilla.com/~tvyas/mixedboth.html - In this case, we have Mixed Active blocked by default, but there is still Mixed Passive content on the page.  We show the globe instead of the lock on Desktop.  My guess is that you show the shield in this case.

https://people.mozilla.com/~tvyas/mixeddisplay.html - In this case, we have Mixed Display content loaded.  We show the globe instead of the lock on Desktop (although, that is changing - we are going to show a grey triangle instead - https://bugzilla.mozilla.org/show_bug.cgi?id=865352).  My guess is that you show the lock or maybe no site identity icon?

* Can we enable mixed content mochitests for Android?  I wrote mochitests for Desktop and explicitly disabled them for Android since Android didn't have the mixed content blocker.  Maybe we can enable them now?  Or maybe not, because they require SSL and back when I wrote the tests, Android mochitests didn't support SSL.
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/android.json#23

* When creating flags/variable names for mixed content that was blocked or loaded, things got tricky because I found I had to distinguish between Mixed Active Blocked / Mixed Active Loaded / Mixed Passive Blocked / Mixed Passive Loaded.  In the Android patch, you don't specify Active vs Passive.  But maybe this is okay, because we know that when we are talking about blocked content, it is most likely active (since passive is allowed by default).  Hence, maybe we don't have to be so explicit.

Thanks!

Comment 36

[:Margaret Leibovic]

(In reply to Tanvi Vyas [:tanvi] from comment #35)
> Great work Margaret!  I have a few questions/comments.
> 
> * What does the UI look like for these two cases:
> 
> https://people.mozilla.com/~tvyas/mixedboth.html - In this case, we have
> Mixed Active blocked by default, but there is still Mixed Passive content on
> the page.  We show the globe instead of the lock on Desktop.  My guess is
> that you show the shield in this case.

Yeah, we show the shield in this case. The contents of the popup are actually the same for this case and the mixedcontent.html case, which makes me think we need a follow-up to address that, since we shouldn't be showing the identity bits in that case, if the connection isn't entirely encrypted.

I also now realize that we should probably do the same thing for when mixed content is enabled (the yellow triangle case).

> https://people.mozilla.com/~tvyas/mixeddisplay.html - In this case, we have
> Mixed Display content loaded.  We show the globe instead of the lock on
> Desktop (although, that is changing - we are going to show a grey triangle
> instead - https://bugzilla.mozilla.org/show_bug.cgi?id=865352).  My guess is
> that you show the lock or maybe no site identity icon?

Correct, we don't show any site identity icon in this case.

> * Can we enable mixed content mochitests for Android?  I wrote mochitests
> for Desktop and explicitly disabled them for Android since Android didn't
> have the mixed content blocker.  Maybe we can enable them now?  Or maybe
> not, because they require SSL and back when I wrote the tests, Android
> mochitests didn't support SSL.
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/android.
> json#23

I'm not sure what the current state of our mochitest capabilities are, but I can look into this. Unfortunately, our test coverage is often pretty bad on Android, but it would be nice if we could make this work. We can file a different bug about this.

> * When creating flags/variable names for mixed content that was blocked or
> loaded, things got tricky because I found I had to distinguish between Mixed
> Active Blocked / Mixed Active Loaded / Mixed Passive Blocked / Mixed Passive
> Loaded.  In the Android patch, you don't specify Active vs Passive.  But
> maybe this is okay, because we know that when we are talking about blocked
> content, it is most likely active (since passive is allowed by default). 
> Hence, maybe we don't have to be so explicit.

Yeah, I made an assumption here that we're just blocking active content, so I just made flags about mixed active content being blocked/loaded. I'm not exactly sure what would happen if users play with about:config flags to block mixed passive content, but there wouldn't be UI for that. I think that's okay, though, since we're not exposing that pref.

Comment 37

[bhavana bajaj [:bajaj]]

Release note for this feature will be added once the remaining polish work is done and is preffed-on by default.Will check back on this if we are ready when preparing Fx24 beta notes.

Comment 38

[Tanvi Vyas[:tanvi]]

Thanks Margaret for your reply!  See comments inline.

(In reply to :Margaret Leibovic from comment #36)
> > https://people.mozilla.com/~tvyas/mixedboth.html - In this case, we have
> > Mixed Active blocked by default, but there is still Mixed Passive content on
> > the page.  We show the globe instead of the lock on Desktop.  My guess is
> > that you show the shield in this case.
> 
> Yeah, we show the shield in this case. The contents of the popup are
> actually the same for this case and the mixedcontent.html case, which makes
> me think we need a follow-up to address that, since we shouldn't be showing
> the identity bits in that case, if the connection isn't entirely encrypted.
> 
In the case where a page has just mixed active content, we block the content and show the shield.  The user can see the shield as something that is protecting them (similar to the lock).  By showing the shield in the case where we  have both mixed active and mixed display content, we are not being completely honest with the user, since they aren't on a fully encrypted page.  I don't have any idea how to fix this UI problem though, since the android url bar already has the favicon and the shield, adding a globe or a grey triangle will just end up being too many icons and too confusing to the user.  We should at least change what the site identity box says though.  For a page that has mixed active content blocked and mixed dispaly content loaded, we will use a message in Desktop that is similar to this:
"Interactive content that isn't encrypted (such as scripts) have been blocked for your protection."
(The final version of this is still under discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=838402#c15)

> I also now realize that we should probably do the same thing for when mixed
> content is enabled (the yellow triangle case).
>
For pages that load mixed active content and show the yellow triangle, we have this message:
"This website contains interactive content that isn't encrypted (such as scripts). Other people can view or modify the website's behavior."
 
> > https://people.mozilla.com/~tvyas/mixeddisplay.html - In this case, we have
> > Mixed Display content loaded.  We show the globe instead of the lock on
> > Desktop (although, that is changing - we are going to show a grey triangle
> > instead - https://bugzilla.mozilla.org/show_bug.cgi?id=865352).  My guess is
> > that you show the lock or maybe no site identity icon?
> 
> Correct, we don't show any site identity icon in this case.
> 
Okay good; that seems like the right thing to do in that case.


> > * When creating flags/variable names for mixed content that was blocked or
> > loaded, things got tricky because I found I had to distinguish between Mixed
> > Active Blocked / Mixed Active Loaded / Mixed Passive Blocked / Mixed Passive
> > Loaded.  In the Android patch, you don't specify Active vs Passive.  But
> > maybe this is okay, because we know that when we are talking about blocked
> > content, it is most likely active (since passive is allowed by default). 
> > Hence, maybe we don't have to be so explicit.
> 
> Yeah, I made an assumption here that we're just blocking active content, so
> I just made flags about mixed active content being blocked/loaded. I'm not
> exactly sure what would happen if users play with about:config flags to
> block mixed passive content, but there wouldn't be UI for that. I think
> that's okay, though, since we're not exposing that pref.

We are just going to block mixed active content for now.  Maybe in a year or 5 years we will block mixed display content too.  The web isn't at a place where we can do that today.  It might help for the future to be explicit and add in the word Active in some cases.  Maybe while you are working on another patch you can sneak that in ;)

In Desktop, if users play with the mixed display content pref and then go to a page with just mixed display, they won't see any UI.  If they go to a page with both mixed active and mixed display content, then they will both be blocked and they will see the shield.  Disabling protection will cause everything on the page (the active and the display) to load.


I realize I'm commenting on a closed bug.  Is there another bug we should move this discussion to?  Thanks!

Comment 39

[:Margaret Leibovic]

(In reply to Tanvi Vyas [:tanvi] from comment #38)

> I realize I'm commenting on a closed bug.  Is there another bug we should
> move this discussion to?  Thanks!

I'm sorry, my work on this feature went on the back burner for a bit, I just filed bug 896048 as a follow-up to hopefully address the remaining concerns.

Comment 40

[:Margaret Leibovic]

This should get pushed to tracking-fennec:25+

Comment 41

[:Margaret Leibovic]

Updating the target milestone to 25 to help keep track of release notes (per bajaj's request).

Comment 42

[Alex Keybl [:akeybl]]

Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.