Bug 1043797 will make the mixed content doorhanger more generic to allow adding different types of blocked content. Content blocked by tracking protection will be that other type of blocked content. Same controls as mixed content: * Heading * Description * Combined button/dropdown control Mockup: attachment 8459851 [details]

I feel the need to point out a specific detail from the mockup: Disabling tracking protection is for the *site*, not for that tab session like mixed content blocking is. ie, disabling tracking protection will mean it's disabled for that site until the person explicitly re-enables it for that site.

Comment on attachment 8467487 [details] [diff] [review] tracking protection notifications show up in the generic security notification doorhanger, interactions with permissionManager to disable/enable tracking protection for a page The new generic security doorhanger now reflects the state of tracking protection in manner symmetrical to how it reflects the mixed content state.

Comment on attachment 8467487 [details] [diff] [review] tracking protection notifications show up in the generic security notification doorhanger, interactions with permissionManager to disable/enable tracking protection for a page Review of attachment 8467487 [details] [diff] [review]: ----------------------------------------------------------------- This'll need test coverage. ::: browser/base/content/browser.js @@ +6605,5 @@ > + this.showBadContentDoorhanger(state); > + > + // Ensure the doorhanger is shown when tracking content is detected but not blocked. > + if (state & nsIWebProgressListener.STATE_LOADED_TRACKING_CONTENT) > + this.showBadContentDoorhanger(state); As mentioned in another bug, this leads to the possibility of calling showBadContentDoorhanger() more than once here. Should coalesce these conditions. ::: browser/base/content/urlbarBindings.xml @@ +1616,5 @@ > + <xul:description anonid="trackingContentProtectionDisabled" hidden="true" class="popup-notification-item-message popup-notification-item-message-critical" xbl:inherits="popupid"> > + &trackingContentBlocked.disabled.message; > + </xul:description> > + </xul:vbox> > + <xul:toolbarbutton type="menu" label="Options" class="popup-notification-item-button" xbl:inherits="popupid"> Localize label. Also, use a menulist like the webRTC doorhanger does. @@ +1647,5 @@ > + } > + if (this.notification.options.state & Ci.nsIWebProgressListener.STATE_LOADED_TRACKING_CONTENT) { > + document.getAnonymousElementByAttribute(this, "anonid", "trackingContent").hidden = false; > + document.getAnonymousElementByAttribute(this, "anonid", "trackingContentProtectionDisabled").hidden = false; > + document.getAnonymousElementByAttribute(this, "anonid", "trackingContentAction.block").hidden = false; These should be memoized in fields. @@ +1672,5 @@ > + let gPermissionManager = > + Components.classes["@mozilla.org/permissionmanager;1"] > + .getService(Components.interfaces.nsIPermissionManager); > + > + // get document URL, normalize it *Why* normalize it? And, for that matter, what do you mean by "normalize"? @@ +1679,5 @@ > + .getService(Components.interfaces.nsIIOService); > + let url = ioService.newURI( > + gBrowser.selectedBrowser.contentDocument.location.href, null, null); > + let normalizedUrl = ioService.newURI( > + "https://" + url.hostPort, null, null); This is a very verbose way of doing it. You can just clone gBrowser.selectedBrowser.currentURI (nsIURI objects have a clone() method), and set the scheme property directly. @@ +1681,5 @@ > + gBrowser.selectedBrowser.contentDocument.location.href, null, null); > + let normalizedUrl = ioService.newURI( > + "https://" + url.hostPort, null, null); > + > + gPermissionManager.add(normalizedUrl, Services.jsm is your friend (already imported in browser.js, so available here). |Services.perms| will give you the permissions manager. @@ +1705,5 @@ > + > + BrowserReload(); > + ]]></body> > + </method> > + <method name="_IsTrackingContentBlocked"> As mentioned elsewhere, we generally try to avoid adding methods only used for testing.

Here's the learn more link: https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/tracking-protection

Comment on attachment 8473918 [details] [diff] [review] tracking protection notifications show up in the generic security notification doorhanger, interactions with permissionManager to disable/enable tracking protection for a page Review of attachment 8473918 [details] [diff] [review]: ----------------------------------------------------------------- Addressed review comments. URL is normalized/transformed that way because the back-end does the same when trying to figure out if a URL matches a host rule in its white list.

Comment on attachment 8476988 [details] [diff] [review] tracking protection notifications show up in the generic security notification doorhanger, interactions with permissionManager to disable/enable tracking protection for a page Review of attachment 8476988 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, I just have the same nits here that I did in bug 1043803, minus the one about setting the link href. ::: browser/base/content/browser.js @@ +6588,5 @@ > // Show the doorhanger when: > // - mixed active content is blocked > // - mixed active content is loaded (detected but not blocked) > + // - tracking content is blocked > + // - tracking content is not blocked I think you want "(detected but not blocked)" here, too, for clarity? ::: browser/base/content/urlbarBindings.xml @@ +1794,5 @@ > </method> > + <method name="disableTrackingContentProtection"> > + <body><![CDATA[ > + // get document URI, normalize it > + // (any scheme turned into https is correct) A more useful comment here might say that this is how nsChannelClassifier::ShouldEnableTrackingProtection constructs URIs when it tests permissions. @@ +1811,5 @@ > + <body><![CDATA[ > + // Remove the current host from the 'trackingprotection' consumer > + // of the permission manager. This effectively removes this host > + // from the tracking protection white list (any list actually). > + Services.perms.remove(gBrowser.selectedBrowser.currentURI.host, This seems weird... we add the permission with https://host:port, but remove the permission with host only? Oh, nsIPermissionManager.remove takes a host name as its first arg... @@ +1816,5 @@ > + "trackingprotection"); > + BrowserReload(); > + ]]></body> > + </method> > + <method name="_IsTrackingContentBlocked"> Same here as in bug 1043803, let's make this a read-only property isTrackingContentBlocked.

(In reply to Drew Willcoxon :adw from comment #23) > Comment on attachment 8476988 [details] [diff] [review] > tracking protection notifications show up in the generic security > notification doorhanger, interactions with permissionManager to > disable/enable tracking protection for a page > > Review of attachment 8476988 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, I just have the same nits here that I did in bug 1043803, minus > the one about setting the link href. > > ::: browser/base/content/browser.js > @@ +6588,5 @@ > > // Show the doorhanger when: > > // - mixed active content is blocked > > // - mixed active content is loaded (detected but not blocked) > > + // - tracking content is blocked > > + // - tracking content is not blocked > > I think you want "(detected but not blocked)" here, too, for clarity? > Actually no. With mixed content we detect it in a page and decide not to block it (if the page is white-listed). With tracking protection we decide not to enable the protection without looking at the page elements. (which may or may not have stuff that should have been blocked had the protection been enabled) > ::: browser/base/content/urlbarBindings.xml > @@ +1794,5 @@ > > </method> > > + <method name="disableTrackingContentProtection"> > > + <body><![CDATA[ > > + // get document URI, normalize it > > + // (any scheme turned into https is correct) > > A more useful comment here might say that this is how > nsChannelClassifier::ShouldEnableTrackingProtection constructs URIs when it > tests permissions. > > @@ +1811,5 @@ > > + <body><![CDATA[ > > + // Remove the current host from the 'trackingprotection' consumer > > + // of the permission manager. This effectively removes this host > > + // from the tracking protection white list (any list actually). > > + Services.perms.remove(gBrowser.selectedBrowser.currentURI.host, > > This seems weird... we add the permission with https://host:port, but remove > the permission with host only? Oh, nsIPermissionManager.remove takes a host > name as its first arg... > It is weird but yes remove() expects a host name while add() expects a URI. > @@ +1816,5 @@ > > + "trackingprotection"); > > + BrowserReload(); > > + ]]></body> > > + </method> > > + <method name="_IsTrackingContentBlocked"> > > Same here as in bug 1043803, let's make this a read-only property > isTrackingContentBlocked.

Comment on attachment 8475335 [details] [diff] [review] tests asserting doorhanger correctly reflects tracking protection state Review of attachment 8475335 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/beningpage.html @@ +1,1 @@ > +<!DOCTYPE HTML> s/bening/benign ::: browser/base/content/test/general/browser_bug1043801-trackingUI.js @@ +48,5 @@ > + dbService.finishStream(); > + dbService.finishUpdate(); > +} > + > +function testBeningPage() s/Bening/Benign/g

Comment on attachment 8477679 [details] [diff] [review] tracking protection notifications show up in the generic security notification doorhanger, interactions with permissionManager to disable/enable tracking protection for a page Review of attachment 8477679 [details] [diff] [review]: ----------------------------------------------------------------- Carrying over adw's comment 22.

Comment on attachment 8475335 [details] [diff] [review] tests asserting doorhanger correctly reflects tracking protection state Review of attachment 8475335 [details] [diff] [review]: ----------------------------------------------------------------- All my comments are fairly small potatoes. Hopefully I can r+ the next patch. Please use add_task instead of a series of functions, like Jared mentioned in bug 1045809. The functions you pass to add_task run serially in the order that you add them. Please name the html pages using camel case or underscores or hyphens so the names are easier to read. Please add license headers to all these files: https://www.mozilla.org/MPL/headers/ (We noted the "benign" typo over IRC.) ::: browser/base/content/test/general/browser.ini @@ +85,5 @@ > test_no_mcb_on_http_site_font2.html > test_no_mcb_on_http_site_font2.css > xul_tooltiptext.xhtml > + trackingpage.html > + beningpage.html Add these to a `support-files` directly after the test, like how browser_searchSuggestionUI.js does: [browser_searchSuggestionUI.js] support-files = searchSuggestionUI.html searchSuggestionUI.js @@ +455,5 @@ > [browser_bug1024133-switchtab-override-keynav.js] > [browser_bug1025195_switchToTabHavingURI_ignoreFragment.js] > [browser_addCertException.js] > skip-if = e10s # Bug ?????? - test directly manipulates content (content.document.getElementById) > +[browser_bug1043801-trackingUI.js] Except for the end, this manifest is mostly in alphabetical order, so please move this line appropriately. ::: browser/base/content/test/general/browser_bug1043801-trackingUI.js @@ +1,1 @@ > +// Test that the Tracking Protection Doorhanger appears Please don't include the bug number in the file name. @@ +16,5 @@ > + "a:524:32:" + testData.length + "\n" + > + testData; > + > +var dbService = Cc["@mozilla.org/url-classifier/dbservice;1"] > + .getService(Ci.nsIUrlClassifierDBService); This doesn't need to be a global since you only use it in doUpdate. @@ +21,5 @@ > + > +// Update tracking database > +function doUpdate(update) { > + var listener = { > + QueryInterface: function(iid) Nit: Since this is running in a browser window that should already have XPCOMUtils.jsm imported, you can use XPCOMUtils.generateQI([Ci.nsIUrlClassifierUpdateObserver]) instead. If I'm wrong about it already being imported, then nevermind. http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.jsm @@ +45,5 @@ > + dbService.beginUpdate(listener, "test-track-simple", ""); > + dbService.beginStream("", ""); > + dbService.updateStream(update); > + dbService.finishStream(); > + dbService.finishUpdate(); Does all of this need to be undone at the end of the test, to prevent this test from possibly interfering with subsequent tests? @@ +52,5 @@ > +function testBeningPage() > +{ > + SpecialPowers.pushPrefEnv( > + {"set" : [[PREF, true]]}, > + function loadTest() { If it makes it easier for you, you can just use Services.prefs.setBoolPref to do this. It's a simple synchronous call. @@ +93,5 @@ > + notification.reshow(); > + // make sure the state of the doorhanger includes blocking tracking elements > + ok(PopupNotifications.panel.firstChild._IsTrackingContentBlocked(), "OK: Tracking Content is being blocked"); > + > + // Disable Tracking Content Protection for the page (and reload page) "which reloads the page" would be more clear, IIUC? @@ +176,5 @@ > +{ > + waitForExplicitFinish(); > + > + origPREF = SpecialPowers.getBoolPref(PREF); > + origTABLE = SpecialPowers.getCharPref(TABLE); Instead of remembering the original prefs and setting them in doFinish, please call registerCleanupFunction here, and inside your cleanup function, call SpecialPowers.clearUserPref. https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

(In reply to Drew Willcoxon :adw from comment #30) > Comment on attachment 8475335 [details] [diff] [review] > tests asserting doorhanger correctly reflects tracking protection state > > Review of attachment 8475335 [details] [diff] [review]: > ----------------------------------------------------------------- > > All my comments are fairly small potatoes. Hopefully I can r+ the next > patch. > > Please use add_task instead of a series of functions, like Jared mentioned > in bug 1045809. The functions you pass to add_task run serially in the > order that you add them. > > Please name the html pages using camel case or underscores or hyphens so the > names are easier to read. > > Please add license headers to all these files: > https://www.mozilla.org/MPL/headers/ > > (We noted the "benign" typo over IRC.) > > ::: browser/base/content/test/general/browser.ini > @@ +85,5 @@ > > test_no_mcb_on_http_site_font2.html > > test_no_mcb_on_http_site_font2.css > > xul_tooltiptext.xhtml > > + trackingpage.html > > + beningpage.html > > Add these to a `support-files` directly after the test, like how > browser_searchSuggestionUI.js does: > > [browser_searchSuggestionUI.js] > support-files = > searchSuggestionUI.html > searchSuggestionUI.js > > @@ +455,5 @@ > > [browser_bug1024133-switchtab-override-keynav.js] > > [browser_bug1025195_switchToTabHavingURI_ignoreFragment.js] > > [browser_addCertException.js] > > skip-if = e10s # Bug ?????? - test directly manipulates content (content.document.getElementById) > > +[browser_bug1043801-trackingUI.js] > > Except for the end, this manifest is mostly in alphabetical order, so please > move this line appropriately. > > ::: browser/base/content/test/general/browser_bug1043801-trackingUI.js > @@ +1,1 @@ > > +// Test that the Tracking Protection Doorhanger appears > > Please don't include the bug number in the file name. > > @@ +16,5 @@ > > + "a:524:32:" + testData.length + "\n" + > > + testData; > > + > > +var dbService = Cc["@mozilla.org/url-classifier/dbservice;1"] > > + .getService(Ci.nsIUrlClassifierDBService); > > This doesn't need to be a global since you only use it in doUpdate. > > @@ +21,5 @@ > > + > > +// Update tracking database > > +function doUpdate(update) { > > + var listener = { > > + QueryInterface: function(iid) > > Nit: Since this is running in a browser window that should already have > XPCOMUtils.jsm imported, you can use > XPCOMUtils.generateQI([Ci.nsIUrlClassifierUpdateObserver]) instead. If I'm > wrong about it already being imported, then nevermind. > > http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils. > jsm > > @@ +45,5 @@ > > + dbService.beginUpdate(listener, "test-track-simple", ""); > > + dbService.beginStream("", ""); > > + dbService.updateStream(update); > > + dbService.finishStream(); > > + dbService.finishUpdate(); > > Does all of this need to be undone at the end of the test, to prevent this > test from possibly interfering with subsequent tests? > Not a problem. > @@ +52,5 @@ > > +function testBeningPage() > > +{ > > + SpecialPowers.pushPrefEnv( > > + {"set" : [[PREF, true]]}, > > + function loadTest() { > > If it makes it easier for you, you can just use Services.prefs.setBoolPref > to do this. It's a simple synchronous call. > > @@ +93,5 @@ > > + notification.reshow(); > > + // make sure the state of the doorhanger includes blocking tracking elements > > + ok(PopupNotifications.panel.firstChild._IsTrackingContentBlocked(), "OK: Tracking Content is being blocked"); > > + > > + // Disable Tracking Content Protection for the page (and reload page) > > "which reloads the page" would be more clear, IIUC? > > @@ +176,5 @@ > > +{ > > + waitForExplicitFinish(); > > + > > + origPREF = SpecialPowers.getBoolPref(PREF); > > + origTABLE = SpecialPowers.getCharPref(TABLE); > > Instead of remembering the original prefs and setting them in doFinish, > please call registerCleanupFunction here, and inside your cleanup function, > call SpecialPowers.clearUserPref. > https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

Comment on attachment 8477775 [details] [diff] [review] tests asserting doorhanger correctly reflects tracking protection state Review of attachment 8477775 [details] [diff] [review]: ----------------------------------------------------------------- All previous review comments addressed.

Comment on attachment 8477775 [details] [diff] [review] tests asserting doorhanger correctly reflects tracking protection state Review of attachment 8477775 [details] [diff] [review]: ----------------------------------------------------------------- Great, thank you, Georgios. ::: browser/base/content/test/general/benignpage.html @@ +1,4 @@ > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<!DOCTYPE HTML> I think the doctype has to be on the first line... I could be wrong. ::: browser/base/content/test/general/browser.ini @@ +412,5 @@ > [browser_tabs_owner.js] > +[browser_trackingUI.js] > +support-files = > + trackingpage.html > + benignpage.html Please use camel case or underscores in these file names to make them easier to read. ::: browser/base/content/test/general/browser_trackingUI.js @@ +8,5 @@ > +var PREF = "privacy.trackingprotection.enabled"; > +var TABLE = "urlclassifier.trackingTable"; > + > +var origPREF = false; > +var origTABLE = ""; You don't need these anymore. @@ +107,5 @@ > + gBrowser.removeCurrentTab(); > + }); > + > + origPREF = SpecialPowers.getBoolPref(PREF); > + origTABLE = SpecialPowers.getCharPref(TABLE); These either.

* * * try: -b do -p linux,macosx64,win32,android,emulator -u all -t none

Comment on attachment 8477786 [details] [diff] [review] tests asserting doorhanger correctly reflects tracking protection state Carrying over adw's review from comment 35

Comment on attachment 8477813 [details] [diff] [review] tests asserting doorhanger correctly reflects tracking protection state Review of attachment 8477813 [details] [diff] [review]: ----------------------------------------------------------------- comment 35

https://hg.mozilla.org/mozilla-central/rev/417e635e26d8 https://hg.mozilla.org/mozilla-central/rev/49ff0d5fcd3a

Hi, quick questions since I've just received all the strings together on m-c: * security notification landed in bug 1043797 has accesskeys on both Enable and Disable, this one doesn't. Is it wanted? * disable button on security notification says "Disable protection for now", this just "Disable protection". Does it mean the behavior is different?

Hi flod, Re: accesskeys, from Gijs's comment https://bugzilla.mozilla.org/show_bug.cgi?id=1043797#c37 I can't tell if we need these. If so, I will add them. Can they be the same as the ones for mixed content blocking? > Hrm. Ideally, this should also have an access key. However, if we're going to add multiple sections, and > each has an identical button that's just labelled "Options" (sigh...) that makes less sense. So whether > I'd want it to have an access key probably depends on whether we're adding real new sections for 34 > still, next cycle, or "sometime Q4" or whatever. Let's leave it for now. * "Disable protection" should have been "Disable protection for this site" (https://bug1029193.bugzilla.mozilla.org/attachment.cgi?id=8459849). I will send a followup bug to adw for this change. May it have the same label name since the original string hasn't been localized yet? The behavior is different. In tracking protection, the disabling persists, and in mixed content, it does not.

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #44) > The behavior is different. In tracking protection, the disabling persists, > and in mixed content, it does not. Hm, given this came up, I think we should try to find some clearer language to make the distinction. Filed bug 1058395. (In reply to [:mmc] Monica Chew (please use needinfo) from comment #44) > Hi flod, > > Re: accesskeys, from Gijs's comment > https://bugzilla.mozilla.org/show_bug.cgi?id=1043797#c37 I can't tell if we > need these. If so, I will add them. Can they be the same as the ones for > mixed content blocking? Yes - access keys for these elements are contextual, so they only work for the selected/open dropdown. So yea, we should fix that.