For MCB, it does (STR: go to https://espn.go.com, get redirected, edit URL bar to point to https://) and the shield shows up, with disable protection, etc. For tracking protection, it doesn't seem to although I think some things are being blocked.

The shield is also not non-dismissible in fennec for MCB.

Behavior should be the same as on Desktop 1) Go to https://m.espn.go.com 2) See MCB shield pop up 3) Click "Disable protection" Expected behavior Shield should persist with a red stripe as it does on desktop Actual behavior Shield disappears wesj sez on irc that changes need to be made to this file: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6805 Margaret, Wes: can you confirm that changes made in https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1043803&attachment=8477636 and https://bugzilla.mozilla.org/show_bug.cgi?id=1043801 need to be added to the mobile browser.js?

Also, can you confirm if changes made to browser/components/preferences/in-content/privacy.js affect fennec or are desktop only?

Those prefs are desktop only. Ours are in mobile/android/app/mobile.js. We'll need to identify this state here with a new const: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6805 That const will be sent to Java and used to fill in some information here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/SiteIdentity.java#68 (i.e. add the new mode to the SecurityMode enum there). Finally, the popup itself will need to be updated to show the right icon/text: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/SiteIdentityPopup.java#144 Actually, looking at this, we already have an enable button: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/SiteIdentityPopup.java#119 So maybe we just need to show an icon in this state.... Margaret will probably know better.

Thanks, wesj. In that case we don't need to wait on fennec to expose tracking protection in the prefs on desktop, but it would be nice to have them both.

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #2) > Behavior should be the same as on Desktop > 1) Go to https://m.espn.go.com > 2) See MCB shield pop up > 3) Click "Disable protection" > > Expected behavior > Shield should persist with a red stripe as it does on desktop > > Actual behavior > Shield disappears If you disable protection, we will reload the page, and you should see a yellow triange icon that allows you to re-enable protection. Do you not see that? Should we update that to a different icon to be consistent with desktop? Is that the main issue in this bug, or is there also a separate issue about adding support for tracking protection? (In reply to Wesley Johnston (:wesj) from comment #4) > Those prefs are desktop only. Ours are in mobile/android/app/mobile.js. Small correction: the file mmc pointed to was for the settings UI, so we'll need to add this to our settings UI here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/xml/preferences_privacy.xml > So maybe we just need to show an icon in this state.... Margaret will > probably know better. wesj is correct that we'll need to implement UI for any new site security features. It would be great to keep this in sync with desktop. Can you sum up what changes need to happen for Fennec? (cc'ing Robin to keep her in the loop about potential settings changes)

Hi, sorry for the lack of context. In desktop support for tracking protection is going to look somewhat like this and overload the mixed content shield, since both features block elements from loading: https://intranet.mozilla.org/TrackingProtectionForFirefox#How_do_you_use_it.3F Here is the UX bug with the original mocks: https://bugzilla.mozilla.org/show_bug.cgi?id=1029193 You can see that the main changes are that the mixed content shield icon should not longer be dismissible, should show a red strike-through state when either type of protection is disabled, should have the ability to re-enable mixed content blocking from the doorhanger, and also support an additional stanza to reflect tracking protection information. None of these changes have been made in fennec. Currently there are only bugs to expose the tracking protection preference in desktop: https://bugzilla.mozilla.org/show_bug.cgi?id=1031033 If you want to support this in fennec (and I think we should) then someone needs to take ownership of porting the desktop changes in browser.js to fennec. That person should probably not be me :)

filter on [mass-p5]

We should do this for parity with desktop.

It looks like mobile users are particularly susceptible if they use Verizon: http://webpolicy.org/2014/10/24/how-verizons-advertising-header-works/ I think this could be even more valuable on mobile than desktop.

OK. I am trying to figure out what actually needs to be done in this bug. The patch in bug 1043803 seems to add support for STATE_LOADED_MIXED_ACTIVE_CONTENT, which Fennec already seems to support. I turned on (1) DNT and (2) privacy.trackingprotection.enabled but I have no idea if either of those really affects *this* bug. Then I went to https://m.espn.go.com and saw the Mixed Content UI. Here are some screenshots: Tapped on the favicon/shield: http://people.mozilla.org/~mfinkle/fennec/screenshots/fennec-blocking-espn.png Tapped on "Keep blocking": http://people.mozilla.org/~mfinkle/fennec/screenshots/fennec-blocking-espn-keepblocking.png Tapped on the favicon/shield, then tapped on "Disable protection": http://people.mozilla.org/~mfinkle/fennec/screenshots/fennec-blocking-espn-disableblocking.png Tapped on the favicon/shield: http://people.mozilla.org/~mfinkle/fennec/screenshots/fennec-blocking-espn-reenableblocking.png 1. Is this working as expected? 2. If not, what needs to be changed? 3. How do we know that the "tracking protection" [1] is enabled in the code? It looks like bug 1043801 has the additional STATE_*_TRACKING_CONTENT flags we need to support. 4. The shield icon looks a little too small to me. We should consider if that was on purpose or if we should update it. [1] https://intranet.mozilla.org/TrackingProtectionForFirefox#How_do_you_use_it.3F

Also to note: Bug 1043805 has more UX design and flow for how TRACKING and MIXED content indicators interact.

I think I might known enough to be dangerous here. I'll take a crack at making a patch that will: 1. Separate Identity, MixedContent and TrackingContent flags into different groups. Keeping them all as Identity doesn't feel right and won't scale well I think. 2. Add basic support to the SiteIdentityPopup for Tracking Content

This patch splits apart the flags in browser.js and creates a richer JSON object sent in the message.

This patch makes changes to the SiteIdentity class so it can handle the separate flags coming from JS.

This patch changes the SiteIdentityPopup to be aware of the three different types of flags, and create a Tracking notification if needed. It also adds some strings. I reuse some of the MixedContent strings for actions. The SUMO URL is likely not correct.

This patch addresses the image displayed in the URLBar. It was coded to work from a single combined flag, but now we have three. I only save the image level now. I should probably change the comment's in SiteIdentity.java about keeping the enums in sync with site_security_level.xml too.

Even with these patches, I don't see TRACKING state being passed into Fennec. I see this logged: D/GeckoBrowser(25835): {"type":"Content:SecurityChange","tabID":0,"identity":{"mode":{"identity":"unknown","mixed":"mixed_content_blocked","tracking":"unknown"}}} W/GeckoConsole(25835): [JavaScript Warning: "The resource at "..." was blocked because tracking protection is enabled." {file: "https://m.espn.go.com/wireless/" line: 0}] W/GeckoConsole(25835): [JavaScript Warning: "The resource at "..." was blocked because tracking protection is enabled." {file: "https://m.espn.go.com/wireless/" line: 0}] I see URLs being blocked, but tracking: "unknown"

Same as the v0.1 patch except with a typo fix. Don't feel like you need to stop feedback on the v0.1 patch.

With the typo fix, I see this when visiting https://m.espn.go.com I also see it when visiting https://people.mozilla.com/~mchew/test_tp.html

madhava, can you help us coordinate the UX between mobile/desktop here? I think our current mobile mixed content blocking UI lags behind desktop, and this bug will make it appear much more often, so it's a good time for a refresher. We could also split the UX polish out into a separate bug if there's already too much going on in this bug.

Comment on attachment 8513558 [details] [diff] [review] tracking-splitflags-java v0.1 Review of attachment 8513558 [details] [diff] [review]: ----------------------------------------------------------------- I like where you're going with splitting these different things apart, but I think we need to do more to update the update/reset methods. ::: mobile/android/base/SiteIdentity.java @@ +52,5 @@ > } > } > > + // The order of the items here correspond to image > + // levels in site_security_level.xml This isn't true for these new enums. I see in a future patch you're manually off-setting the index, so you could still keep a comment about how the order is used for the site security level, but I feel like the offset makes this fragile enough that we might want to change how we do this. @@ +121,4 @@ > public SiteIdentity() { > reset(SecurityMode.UNKNOWN); > + mMixedMode = MixedMode.UNKNOWN; > + mTrackingMode = TrackingMode.UNKNOWN; These should probably go in reset. And reset probably shouldn't take a security mode anymore. Maybe we can just rename it resetIdenitity() and use it to reset the identity data, but then directly set mSecurityMode where we're currently calling reset. I don't really like this reset method as it is, since it's nulling out all the identity values, but yet it's setting a security mode, and you can't (or shouldn't) have an verified/identified site without any information about the host.

Comment on attachment 8513560 [details] [diff] [review] tracking-doorhanger v0.1 Review of attachment 8513560 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/toolbar/SiteIdentityPopup.java @@ +80,5 @@ > } > > + final MixedMode mixedMode = mSiteIdentity.getMixedMode(); > + final TrackingMode trackingMode = mSiteIdentity.getTrackingMode(); > + if (mixedMode != MixedMode.UNKNOWN || trackingMode != TrackingMode.UNKNOWN) { I don't think this is right... I think we should show the identity data no matter what the tracking state is, and I think we should only hide it if we've loaded mixed content (which actually looks like a bug in the current logic). This is tricky because where desktop has two icons, we combine them into one. I would like to see some UX help here. @@ +172,5 @@ > + "enable", mButtonClickListener); > + } > + > + mContent.addView(mTrackingContentNotification); > + } I assume this is temporary until we get more UX direction, but if the code is really going to be this similar between the mixed content/tracking notifications, I would factor out the shared stuff into a helper method. @@ +203,1 @@ > Log.e(LOGTAG, "Can't show site identity popup in non-identified state"); You should update this error message. @@ +203,5 @@ > Log.e(LOGTAG, "Can't show site identity popup in non-identified state"); > return; > } > > updateUi(); We should rename this method to something like updateIdentity(), since it just updates the identity bit of the popup. @@ +231,5 @@ > JSONObject data = new JSONObject(); > + if (dh == mMixedContentNotification) { > + data.put("allowMixedContent", tag.equals("disable")); > + } > + if (dh == mTrackingContentNotification) { dh can't be both things, so I would use an else if statement. Or you could even just use a ternary operator to figure out which key to set.

Comment on attachment 8513566 [details] [diff] [review] tracking-toolbar v0.1 Review of attachment 8513566 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/toolbar/ToolbarDisplayLayout.java @@ +441,5 @@ > + // Adjust for mixed content > + imageLevel = 2 + mixedMode.ordinal(); > + } else if (trackingMode != TrackingMode.UNKNOWN) { > + // Adjust for tracking content > + imageLevel = 2 + trackingMode.ordinal(); This feels kinda fragile, but I guess it's not really that bad, since we're not changing these enums often.

Comment on attachment 8513920 [details] [diff] [review] tracking-splitflags-js v0.1 Review of attachment 8513920 [details] [diff] [review]: ----------------------------------------------------------------- Nice, I like that this moves the logic for coalescing all these different states over to Java, since that leaves the front-end-most layer in charge of figuring out what to show the user. ::: mobile/android/chrome/content/browser.js @@ +6656,5 @@ > // Blocked active mixed content. Shield icon is shown, with a popup option to load content. > IDENTITY_MODE_MIXED_CONTENT_BLOCKED: "mixed_content_blocked", > > // Loaded active mixed content. Yellow triangle icon is shown. > IDENTITY_MODE_MIXED_CONTENT_LOADED: "mixed_content_loaded", Nit: I would update these different constant names to reflect the fact that they're used for their own modes, not the identity mode. Maybe MIXED_MODE_CONTENT_BLOCKED, TRACKING_MODE_CONTENT_BLOCKED, etc. @@ +6713,5 @@ > + if (aState & Ci.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL) > + return this.IDENTITY_MODE_IDENTIFIED; > + > + if (aState & Ci.nsIWebProgressListener.STATE_IS_SECURE) > + return this.IDENTITY_MODE_DOMAIN_VERIFIED; Nit: While you're here, you could add braces.

Hi all - I think we can get started by getting Anthony to talk to Philipp, who did the desktop version of the feature.

Sounds good. Leaving NI on for now so I don't forget.

Updated patch based on feedback: * Made unique flag names * Added braces

Updated patch based on feedback: * Tweaked the enum comments to mention they "relate" to the XML drawable, but not maych it exactly. * Re-worked the "reset" method to "resetIdentityData" * Treat mSecurityMode just like the other modes

Updated patch: * Add a helper for adding buttons to notifications * Updated the error message * Renamed "updateUi" to "updateIdentity" * Used a ternary operator to set the allowType

Added slightly better comments here, linking the image level logic to the SiteIdentity enums.

I found a problem in DoorHanger.java where the divider used between DoorHangers was not set to the right color. This fixes it.

New screenshot blocking mixed content and tracking content on m.espn.go.com

Comment on attachment 8515552 [details] [diff] [review] tracking-splitflags-js v0.2 Review of attachment 8515552 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +4428,5 @@ > identity: identity > }; > > + dump("XXX") > + dump(JSON.stringify(message)); Remove debug logging.

Comment on attachment 8515553 [details] [diff] [review] tracking-splitflags-java v0.2 Review of attachment 8515553 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but just be careful about the logic to reset the identity data. ::: mobile/android/base/SiteIdentity.java @@ +164,5 @@ > + mSupplemental = identityData.optString("supplemental", null); > + mVerifier = identityData.getString("verifier"); > + mEncrypted = identityData.getString("encrypted"); > + } catch (Exception e) { > + resetIdentityData(); The current logic also sets mSecurityMode to UNKNOWN in this case, so I think you should do that here as well. To simplify things a bit, you could just make that part of resetIdentityData, instead of calling it separately, since any time we set mSecurityMode to UNKNOWN, we also want to reset all the identity data.

Comment on attachment 8515554 [details] [diff] [review] tracking-doorhanger v0.2 Review of attachment 8515554 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I think the divider show/hide logic is wrong. I also think that there's a problem with hiding the identity data, but it's a pre-existing issue, so we can address that in a follow-up. ::: mobile/android/base/toolbar/SiteIdentityPopup.java @@ +80,5 @@ > } > > + final MixedMode mixedMode = mSiteIdentity.getMixedMode(); > + final TrackingMode trackingMode = mSiteIdentity.getTrackingMode(); > + if (mixedMode != MixedMode.UNKNOWN || trackingMode != TrackingMode.UNKNOWN) { You didn't address my comment about this in comment 23. I don't think this is right - I don't think loading/blocking tracking data should make us hide the identity UI. Desktop still shows the Larry popup with the site identity data. I also think that there's a bug here with the mixed content, since we should still show the identity data if the mixed content is blocked (since that guarantees that all the content that's loaded is in fact from the identified domain). Although it looks like we purposefully did this in bug 966866... I think we paved over a bug where we're resetting the identity data when we shouldn't be. Can you file a follow-up about that? @@ +194,5 @@ > Log.e(LOGTAG, "Can't show site identity popup for undefined state"); > return; > } > > + final SecurityMode identityMode = mSiteIdentity.getSecurityMode(); We should probably update this method to be named getIdentityMode(). @@ +212,5 @@ > + if (trackingMode != TrackingMode.UNKNOWN) { > + addTrackingContentNotification(trackingMode == TrackingMode.TRACKING_CONTENT_BLOCKED); > + > + // If we showed a mixed content notification, we need a divider between them > + mMixedContentNotification.showDivider(); mMixedContentNotification can be null here. And won't this add an unwanted divider the next time we show a mixed content notification without a tracking mode notification? In DoorHangerPopup, we have this terrible logic to iterate through all the notifications and show/hide dividers as appropriate: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DoorHangerPopup.java#326

Comment on attachment 8515555 [details] [diff] [review] tracking-toolbar v0.2 Review of attachment 8515555 [details] [diff] [review]: ----------------------------------------------------------------- I didn't catch this earlier, but there's a problem here that we could show a shield when tracking content is loaded. ::: mobile/android/base/toolbar/ToolbarDisplayLayout.java @@ +440,5 @@ > + int imageLevel = securityMode.ordinal(); > + if (mixedMode != MixedMode.UNKNOWN) { > + // Adjust for mixed content. The 'shield' images start at level 2. > + imageLevel = 2 + mixedMode.ordinal(); > + } else if (trackingMode != TrackingMode.UNKNOWN) { This is a bit tricky because it gives priority to the mixed mode over the tracking mode. Thinking about this a bit more, we should probably be giving priority to the warning icon over the shield. Such that if there's any non-blocked mixed or tracking content, we're showing the shield (with this current logic, you could have loaded tracking content, but blocked mixed content, and we would show a shield). I think it might be clearer to declare constants for the different levels, then explicitly settings them. How about something like this: private final int LEVEL_SHIELD = 3; private final int LEVEL_WARNING = 4; ... // Show the warning icon if any tracking or mixed content is loaded. if (trackingMode == TrackingMode.TRACKING_CONTENT_LOADED || mixedMode == MixedMode.MIXED_CONTENT_LOADED) { imageLevel = LEVEL_WARNING; } else if (trackingMode == TrackingMode.TRACKING_CONTENT_BLOCKED || mixedMode == MixedMode.MIXED_CONTENT_BLOCKED) { imageLevel = LEVEL_SHIELD; }

Comment on attachment 8515556 [details] [diff] [review] divider-color v0.1 Review of attachment 8515556 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch.

(In reply to :Margaret Leibovic from comment #36) > Comment on attachment 8515554 [details] [diff] [review] > tracking-doorhanger v0.2 > > Review of attachment 8515554 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good, but I think the divider show/hide logic is wrong. I also > think that there's a problem with hiding the identity data, but it's a > pre-existing issue, so we can address that in a follow-up. About hiding the Identity data. Desktop use two click targets: One for content blocking and one for identity. We only have one. Given that we currently do not show Identity when blocking content, I'm OK with dealing with it in a new bug. But even so, we run out of space for showing three items. I could propose that we add a "Show Identity" button in the Content Tracking popup, which could flip the Doorhanger to an Identity popup. > > ::: mobile/android/base/toolbar/SiteIdentityPopup.java > @@ +80,5 @@ > > } > > > > + final MixedMode mixedMode = mSiteIdentity.getMixedMode(); > > + final TrackingMode trackingMode = mSiteIdentity.getTrackingMode(); > > + if (mixedMode != MixedMode.UNKNOWN || trackingMode != TrackingMode.UNKNOWN) { > > You didn't address my comment about this in comment 23. I don't think this > is right - I don't think loading/blocking tracking data should make us hide > the identity UI. Desktop still shows the Larry popup with the site identity > data. You just said that we'd handle that in a new bug, right? I purposefully did not handle it because we don't show it now, and we'll need some UX help to get it right. > I also think that there's a bug here with the mixed content, since we should > still show the identity data if the mixed content is blocked (since that > guarantees that all the content that's loaded is in fact from the identified > domain). Although it looks like we purposefully did this in bug 966866... I > think we paved over a bug where we're resetting the identity data when we > shouldn't be. Can you file a follow-up about that? Are we talking about the same "new bug" as in the above paragraphs? I am confused about what new bugs to file. > > + final SecurityMode identityMode = mSiteIdentity.getSecurityMode(); > > We should probably update this method to be named getIdentityMode(). I was waiting for all of this to land before making any sweeping "rename" changes. > > + if (trackingMode != TrackingMode.UNKNOWN) { > > + addTrackingContentNotification(trackingMode == TrackingMode.TRACKING_CONTENT_BLOCKED); > > + > > + // If we showed a mixed content notification, we need a divider between them > > + mMixedContentNotification.showDivider(); > > mMixedContentNotification can be null here. And won't this add an unwanted > divider the next time we show a mixed content notification without a > tracking mode notification? You're right about it being NULL, but we remove the notifications every time we show the Popup so the divider won't stay around.

What's needed to get this finished up? Are we waiting on UX designs at this point?

After Mark's patches in this bug Fennec will be on par with Desktop, which means that you can turn on the feature in about:config and it will work as https://support.mozilla.org/en-US/kb/tracking-protection-firefox?as=u&utm_source=inproduct describes.

(In reply to Mark Finkle (:mfinkle) from comment #39) Sorry I missed this comment earlier. > (In reply to :Margaret Leibovic from comment #36) > > Comment on attachment 8515554 [details] [diff] [review] > > tracking-doorhanger v0.2 > > > > Review of attachment 8515554 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This looks good, but I think the divider show/hide logic is wrong. I also > > think that there's a problem with hiding the identity data, but it's a > > pre-existing issue, so we can address that in a follow-up. > > About hiding the Identity data. Desktop use two click targets: One for > content blocking and one for identity. We only have one. Given that we > currently do not show Identity when blocking content, I'm OK with dealing > with it in a new bug. > > But even so, we run out of space for showing three items. I could propose > that we add a "Show Identity" button in the Content Tracking popup, which > could flip the Doorhanger to an Identity popup. I filed bug 1095643 about this. > > ::: mobile/android/base/toolbar/SiteIdentityPopup.java > > @@ +80,5 @@ > > > } > > > > > > + final MixedMode mixedMode = mSiteIdentity.getMixedMode(); > > > + final TrackingMode trackingMode = mSiteIdentity.getTrackingMode(); > > > + if (mixedMode != MixedMode.UNKNOWN || trackingMode != TrackingMode.UNKNOWN) { > > > > You didn't address my comment about this in comment 23. I don't think this > > is right - I don't think loading/blocking tracking data should make us hide > > the identity UI. Desktop still shows the Larry popup with the site identity > > data. > > You just said that we'd handle that in a new bug, right? I purposefully did > not handle it because we don't show it now, and we'll need some UX help to > get it right. Yes, we can deal with this in bug 1095640 and bug 1095643. > > I also think that there's a bug here with the mixed content, since we should > > still show the identity data if the mixed content is blocked (since that > > guarantees that all the content that's loaded is in fact from the identified > > domain). Although it looks like we purposefully did this in bug 966866... I > > think we paved over a bug where we're resetting the identity data when we > > shouldn't be. Can you file a follow-up about that? > > Are we talking about the same "new bug" as in the above paragraphs? I am > confused about what new bugs to file. Yeah, sorry, this will be handled by bug 1095643. > > > + final SecurityMode identityMode = mSiteIdentity.getSecurityMode(); > > > > We should probably update this method to be named getIdentityMode(). > > I was waiting for all of this to land before making any sweeping "rename" > changes. Sounds good. We can do more clean-up in bug 1095640. > > > + if (trackingMode != TrackingMode.UNKNOWN) { > > > + addTrackingContentNotification(trackingMode == TrackingMode.TRACKING_CONTENT_BLOCKED); > > > + > > > + // If we showed a mixed content notification, we need a divider between them > > > + mMixedContentNotification.showDivider(); > > > > mMixedContentNotification can be null here. And won't this add an unwanted > > divider the next time we show a mixed content notification without a > > tracking mode notification? > > You're right about it being NULL, but we remove the notifications every time > we show the Popup so the divider won't stay around. But we don't destroy the views for the notifications. So if you re-add that mixed content notification, it will still have the divider.

Using logic from DoorHangerPopup to control the dividers. It's not an exact copy/paste because this ViewGroup has other layouts, in addition to DoorHangers. Someday we could move this somewhere else.

This patch changes the image level logic to favor the "disable protection" state.

Comment on attachment 8520129 [details] [diff] [review] tracking-doorhanger v0.3 Review of attachment 8520129 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/toolbar/SiteIdentityPopup.java @@ +218,5 @@ > super.show(); > } > > + // Show the right dividers > + private void showDividers() { I'm not in love with needing to copy this logic, but we're probably going to change the UI in the follow-up, so hopefully we can update this there.

Let's add some simple tests! I stole the core of this patch from a Desktop test: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_trackingUI.js Instead of testing the actual popup notifications (or DoorHangers in our case) this patch listens for the "Content:Security" messages on the Java side. It caches the "tracking" value from the message. The test then sends over a message with the expected values at various times. If the last cached value matches the expected value, we win. This passed on local builds. I'll push to Try as well.

(In reply to Mark Finkle (:mfinkle) from comment #47) > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ac3ffa7505f9 Try run is green

Comment on attachment 8520416 [details] [diff] [review] tracking-test v0.1 Review of attachment 8520416 [details] [diff] [review]: ----------------------------------------------------------------- Nice, this is a good little test. I mainly have some feedback about the use of promises, but you can file a mentor bug as a follow-up if you want, since this old syntax is already a problem elsewhere in our tree. ::: mobile/android/base/tests/testTrackingProtection.java @@ +27,5 @@ > + JSONObject mode = identity.getJSONObject("mode"); > + mLastTracking = mode.getString("tracking"); > + mAsserter.dumpLog("Security change (tracking): " + mLastTracking); > + } catch (Exception e) { > + } We should probably fail an assertion if there's an exception. ::: mobile/android/base/tests/testTrackingProtection.js @@ +10,5 @@ > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/Messaging.jsm"); > + > +XPCOMUtils.defineLazyModuleGetter(this, "Promise", "resource://gre/modules/Promise.jsm"); ES6 promises landed, no? I think you can/should just use the ones built into the JS engine. @@ +17,5 @@ > + do_report_result(passed, text, Components.stack.caller, false); > +} > + > +function promiseLoadEvent(browser, url, eventType="load") { > + let deferred = Promise.defer(); I think you should use the new `new Promise()` syntax. So this function body would be: return new Promise((resolve, reject) => { ... function handle(event) { ... resolve(event); } ... }); You can find some examples in the tree. @@ +58,5 @@ > + testData; > + > + let dbService = Cc["@mozilla.org/url-classifier/dbservice;1"].getService(Ci.nsIUrlClassifierDBService); > + > + let deferred = Promise.defer(); I know you just copied this from desktop, but the same thing still applies about the new Promise syntax. I think this Promise.defer() syntax will be deprecated since it's not in ES6. We're probably fine to just leave this as-is for the test, but it would be good to avoid adding more Promise.jsm dependencies if we can. However, looking into this a bit, MDN says Promise.defer() has been obsolete since Gecko 30: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm#defer%28%29 https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Deferred I can file some mentor bugs to update the other Promise usage in the tree. @@ +104,5 @@ > + > + // Load a blank page > + let url = "about:blank"; > + browser = BrowserApp.addTab(url, { selected: true, parentId: BrowserApp.selectedTab.id }).browser; > + browser.addEventListener("load", function startTests(event) { I see other similar browser load listeners in tests in the tree check if event.target == browser.contentDocument. I'm not sure where the other load events can come from, but it might be worth adding this check. @@ +106,5 @@ > + let url = "about:blank"; > + browser = BrowserApp.addTab(url, { selected: true, parentId: BrowserApp.selectedTab.id }).browser; > + browser.addEventListener("load", function startTests(event) { > + browser.removeEventListener("load", startTests, true); > + Services.tm.mainThread.dispatch(run_next_test, Ci.nsIThread.DISPATCH_NORMAL); Wouldn't this already be on the main thread? Why not just call run_next_test?

(In reply to :Margaret Leibovic from comment #49) > ::: mobile/android/base/tests/testTrackingProtection.java > @@ +27,5 @@ > > + JSONObject mode = identity.getJSONObject("mode"); > > + mLastTracking = mode.getString("tracking"); > > + mAsserter.dumpLog("Security change (tracking): " + mLastTracking); > > + } catch (Exception e) { > > + } > > We should probably fail an assertion if there's an exception. Yes! Done. This also acts as a test for the JSON. > ::: mobile/android/base/tests/testTrackingProtection.js > > +XPCOMUtils.defineLazyModuleGetter(this, "Promise", "resource://gre/modules/Promise.jsm"); > > ES6 promises landed, no? I think you can/should just use the ones built into > the JS engine. I removed this and switched to your suggestions. No sense adding more deprecated code to the tree. > > +function promiseLoadEvent(browser, url, eventType="load") { > > + let deferred = Promise.defer(); > > I think you should use the new `new Promise()` syntax. So this function body > would be: Done and it works locally. > > + browser = BrowserApp.addTab(url, { selected: true, parentId: BrowserApp.selectedTab.id }).browser; > > + browser.addEventListener("load", function startTests(event) { > > I see other similar browser load listeners in tests in the tree check if > event.target == browser.contentDocument. I'm not sure where the other load > events can come from, but it might be worth adding this check. I think target and originaltarget can be the same for some events, like "load", but I changed ti since it's shorter. Still works fine. > > + browser.addEventListener("load", function startTests(event) { > > + browser.removeEventListener("load", startTests, true); > > + Services.tm.mainThread.dispatch(run_next_test, Ci.nsIThread.DISPATCH_NORMAL); > > Wouldn't this already be on the main thread? Why not just call run_next_test? This isn't about threads. This is about allowing everything to settle and calling into the next tests (the real one) when we're more sure that no other races are happening. This is me being wary of tests.

remote: https://hg.mozilla.org/integration/fx-team/rev/25488ca11bf4 remote: https://hg.mozilla.org/integration/fx-team/rev/1e7ae3625898 remote: https://hg.mozilla.org/integration/fx-team/rev/7a44ec776c5c remote: https://hg.mozilla.org/integration/fx-team/rev/ef5b2f939fc1 remote: https://hg.mozilla.org/integration/fx-team/rev/1d031d65e414 remote: https://hg.mozilla.org/integration/fx-team/rev/514c57232708

https://hg.mozilla.org/mozilla-central/rev/25488ca11bf4 https://hg.mozilla.org/mozilla-central/rev/1e7ae3625898 https://hg.mozilla.org/mozilla-central/rev/7a44ec776c5c https://hg.mozilla.org/mozilla-central/rev/ef5b2f939fc1 https://hg.mozilla.org/mozilla-central/rev/1d031d65e414 https://hg.mozilla.org/mozilla-central/rev/514c57232708

That's awesome. :)

Will continue UX efforts in bug 1095640. Conversing with Philipp from the desktop side ATM.