This should be a checkbox in the privacy preferences.

Potential point of confusion with this UI: It looks like the enforcement feature is coupled to the DNT feature. A spacer might be warranted to separate the two options and make sure people don't think they have to specify a DNT option to use tracking protection. Space could also probably be better used here if the radio buttons were converted to a menulist instead. This would also make it much simpler. As-is, it looks somewhat like a 4 choice option at first glance.

(In reply to Dave Garrett from comment #4) > Potential point of confusion with this UI: It looks like the enforcement > feature is coupled to the DNT feature. A spacer might be warranted to > separate the two options and make sure people don't think they have to > specify a DNT option to use tracking protection. Space could also probably > be better used here if the radio buttons were converted to a menulist > instead. This would also make it much simpler. As-is, it looks somewhat like > a 4 choice option at first glance. Thanks for the feedback. This bug is for the new preference only. I added space and left DNT alone.

Current mockup: attachment 8459851 [details]

(In reply to Dave Garrett from comment #4) > Potential point of confusion with this UI: It looks like the enforcement > feature is coupled to the DNT feature. A spacer might be warranted to > separate the two options and make sure people don't think they have to > specify a DNT option to use tracking protection. Space could also probably > be better used here if the radio buttons were converted to a menulist > instead. This would also make it much simpler. As-is, it looks somewhat like > a 4 choice option at first glance. Agreed - I thought there wasn't enough contextual separation between the two things when I first saw the mockup. Philipp: Thoughts?

FYI: we're going back to the single checkbox for the DNT pref (see bug 1042135)... mentioning it here in case it matters for design.

Comment on attachment 8448270 [details] newpref.png Obsoleting this screenshot since it will change when the DNT pref gets changed.

Ah! Thanks, Sid. No extra info needed from Philipp then.

FWIW, This mockup shows the intended design: https://bug1029193.bugzilla.mozilla.org/attachment.cgi?id=8459849 It still shows radio buttons for DNT options, but you see how that section is separated by space and the »Learn More« link.

Comment on attachment 8478722 [details] [diff] [review] Expose tracking protection pref in privacy pref pane Review of attachment 8478722 [details] [diff] [review]: ----------------------------------------------------------------- Hi Drew, I spent an embarrassingly long time trying to get an icon to show up for the checkbox, but no go. There weren't too many models of things in prefs that show a related image, with the exception of the application menu items. Any help is welcome. I also wasn't sure how to test non-in-content prefs from Nightly anymore, or what kind of automated tests are required for exposing a bool pref. Thanks, Monica

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #17) > There weren't too many models of things in prefs that show a related image The translation prefs seems to have an image added by bug 1022856: http://hg.mozilla.org/mozilla-central/rev/2d9116d8711b#l2.22 An existing image usage is for the Sync Prefs UI: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/sync.xul#66 http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/sync.xul#65 I don't know much about how in-content prefs are implemented though.

(In reply to Ed Lee :Mardak from comment #18) > (In reply to [:mmc] Monica Chew (please use needinfo) from comment #17) > > There weren't too many models of things in prefs that show a related image > The translation prefs seems to have an image added by bug 1022856: > http://hg.mozilla.org/mozilla-central/rev/2d9116d8711b#l2.22 You should be able to see that UI by setting browser.translation.ui.show true and opening preferences -> Content -> Languages

Comment on attachment 8478733 [details] [diff] [review] Expose tracking protection pref in privacy pref pane Review of attachment 8478733 [details] [diff] [review]: ----------------------------------------------------------------- Ed was right, as usual. Thanks!

Comment on attachment 8478733 [details] [diff] [review] Expose tracking protection pref in privacy pref pane Review of attachment 8478733 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Monica, Ed. I don't think this needs a test. ::: browser/components/preferences/in-content/privacy.xul @@ +90,5 @@ > + </hbox> > + <label id="trackingProtectionLearnMore" > + class="text-link" > + value="&trackingProtectionLearnMore.label;" > + onclick="gAdvancedPane.openTextLink(event)"/> Is there a reason for this onclick? I don't think you need it since you're setting the label's href. Actually openTextLink isn't defined on the in-content gAdvancedPane, so if you right-click this link (to bypass the href but trigger the onclick), you get: JavaScript error: about:preferences#privacy, line 1: gAdvancedPane.openTextLink is not a function http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/advanced.js ::: browser/components/preferences/privacy.xul @@ +96,5 @@ > + </hbox> > + <label id="trackingProtectionLearnMore" > + class="text-link" > + value="&trackingProtectionLearnMore.label;" > + onclick="gAdvancedPane.openTextLink(event)"/> Same here, except here when I right click I get: JavaScript error: chrome://browser/content/preferences/preferences.xul, line 1: gAdvancedPane is not defined Which I don't understand because advanced.js is included in preferences.xul (via advanced.xul). ::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd @@ +10,5 @@ > <!ENTITY dntTrackingNotOkay.accesskey "n"> > <!ENTITY dntTrackingOkay.label2 "Tell sites that I want to be tracked"> > <!ENTITY dntTrackingOkay.accesskey "h"> > +<!ENTITY trackingProtection.label "Prevent sites from tracking me"> > +<!ENTITY trackingProtection.accesskey "P"> P is already used in this file, and the Privacy pane, for privateBrowsingPermanent2.accesskey: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/privacy.dtd#76 There it's lowercase, but we should avoid using the same key at all. T would be good but it's taken, too. So is S. How about "m" for "me"? That'll cause the M in "from" to be underlined on platforms that actually draw underlines, but at least it's still the first letter of one of the words. Plus it's me-centered which is always nice. :-) But if you have a better idea that's not already used, feel free to choose it.

Let's hold off on checking this in, please, until you get a sign-off from me or Chad.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #24) > Let's hold off on checking this in, please, until you get a sign-off from me > or Chad. OK -- can I check in the string changes only for maximum flexibility?

Yes, we should check the strings in.

Comment on attachment 8480155 [details] [diff] [review] Expose tracking protection pref in privacy pref pane Carrying over review from adw in comment 23 > Is there a reason for this onclick? Nope, removed. Also thanks for the accesskey tip, fixed :) String changes are in.

This can't be enabled on fennec until https://bugzilla.mozilla.org/show_bug.cgi?id=1063831 is fixed.

This is a modified version of attachment 8480155 [details] [diff] [review] that is unbitrotted, and makes this UI conditional on privacy.trackingprotection.ui.enabled (added in bug 1081343), and Nightly-only for the moment (since bug 1081343 is for now).

Comment on attachment 8510687 [details] [diff] [review] patch with pref control on whether this UI appears Review of attachment 8510687 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Seems redundant to have #ifdef NIGHTLY around _initTrackingProtection since privacy.trackingprotection.ui.enabled is already #ifdef NIGHTLY'ed in bug 1081343. Thanks for unbitrotting.

Comment on attachment 8510687 [details] [diff] [review] patch with pref control on whether this UI appears Review of attachment 8510687 [details] [diff] [review]: ----------------------------------------------------------------- You could avoid the #ifdef if you try-catch the getBoolPref("privacy.trackingprotection.ui.enabled") and return early on exception. Not a big deal, but that would mean we wouldn't have to return to this once we add privacy.trackingprotection.ui.enabled to all builds. ::: browser/components/preferences/in-content/privacy.js @@ +27,5 @@ > + let link = document.getElementById("trackingProtectionLearnMore"); > + let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "tracking-protection"; > + if (url) { > + link.setAttribute("href", url); > + } else, hide `link`? Actually, `url` should never be falsey, since you're concatenating the return value with a nonempty string.

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #33) > LGTM. Seems redundant to have #ifdef NIGHTLY around _initTrackingProtection > since privacy.trackingprotection.ui.enabled is already #ifdef NIGHTLY'ed in > bug 1081343. Thanks for unbitrotting. As Drew mentions, the ifdef is needed since if the default pref doesn't exist, the getBoolPref call will throw, and I didn't want to add a try/catch. (In reply to Drew Willcoxon :adw from comment #34) > You could avoid the #ifdef if you try-catch the > getBoolPref("privacy.trackingprotection.ui.enabled") and return early on > exception. Not a big deal, but that would mean we wouldn't have to return > to this once we add privacy.trackingprotection.ui.enabled to all builds. I was assuming we'd just rip out all the #ifdef NIGHTLYs at that point. > else, hide `link`? Actually, `url` should never be falsey, since you're > concatenating the return value with a nonempty string. Yep, fixed that. Thanks.

Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5: - using latest Aurora 36.0a2 (07-01-2015) : no checkbox for tracking protection is displayed in the privacy preferences (because the UI for tracking protection is not uplifted yet to Aurora) - using latest Nightly 37.0a1 (07-01-2015) : the checkbox for tracking protection is displayed in the privacy pane (if you set the privacy.trackingprotection.ui.enabled pref to true)