We want a new "Polaris" pref that can be enabled by users to opt-in to privacy features/explorations that are still in an experimental stage. The current plan is for this to be an ongoing program, with the first program feature being Tracking Protection. Apparently the discussion on this resulted in the following requested behavior: * The pref will only be in about:config. No UI for it yet. * When the pref is enabled, it should enable the Tracking Protection pref (privacy.trackingprotection.enabled) and expose the UI in bug 1031033 * When the pref is disabled, it should disable privacy.trackingprotection.enabled and hide the UI in bug 1031033 * Similarly, enabling/disabling the Polaris pref should enable/disable DNT. [Is this already being done by tracking protection?]

One part I'm unclear on -- is this pref expected to ride the trains, or stay on Nightly? We've previously discussed how it _could_ exist on all channels, but I don't know if we want that right away.

(In reply to Justin Dolske [:Dolske] from comment #0) bug 1031033 > * Similarly, enabling/disabling the Polaris pref should enable/disable DNT. > [Is this already being done by tracking protection?] No, they are controlled orthogonally by different prefs.

For completeness sake, the requirements also requested: * no visible list-management UI or list attribution * doorhanger icon appears on sites where tracking protection is active * doorhanger learn more link points to SUMO, allows disabling That seem specific to Tracking Protection... Monica, are there bugs on file for that (if it's not already done)?

(In reply to Justin Dolske [:Dolske] from comment #3) > For completeness sake, the requirements also requested: > > * no visible list-management UI or list attribution This is done by default :p List management is controlled by the following about:config prefs urlclassifier.trackingTable browser.trackingprotection.updateURL browser.trackingprotection.gethashURL > * doorhanger icon appears on sites where tracking protection is active This is done in bug 1043801 > * doorhanger learn more link points to SUMO, allows disabling This is done in the previous bug and the sumo page is https://support.mozilla.org/en-US/kb/tracking-protection-firefox?as=u&utm_source=inproduct, linked from the "Learn More" in the doorhanger Can I now write the SUMO article? > That seem specific to Tracking Protection... Monica, are there bugs on file > for that (if it's not already done)?

(In reply to Justin Dolske [:Dolske] from comment #1) > One part I'm unclear on -- is this pref expected to ride the trains, or stay > on Nightly? We've previously discussed how it _could_ exist on all channels, > but I don't know if we want that right away. Let's do Nightly-only for now.

I'll work on it but I need some info: 1. What's the right name for this pref? 2. What's the right place to implement the privacy.trackingprotection.enabled switching functionality? I'm pretty sure that browser/app/profile/firefox.js is not the right place itself. Is it nsBrowserGlue.js? 3. Do we test functionality like this? If so, do we have similar code somewhere so I can reuse good patterns?

(In reply to Tomasz Kołodziejski [:tomasz] from comment #6) > 1. What's the right name for this pref? browser.polaris is fine for now. > 2. What's the right place to implement the > privacy.trackingprotection.enabled switching functionality? I'm pretty sure > that browser/app/profile/firefox.js is not the right place itself. Is it > nsBrowserGlue.js? dolske mentioned this on IRC, but a pref observer nsBrowserGlue sounds right. > 3. Do we test functionality like this? If so, do we have similar code > somewhere so I can reuse good patterns? A test that setting the pref properly toggles the other two prefs (and then properly disables them when toggled back) might be useful. You could also test interactions that involve flipping the other two prefs independently. This could be done with a browser chrome test. Not sure about similar code, but this is pretty straightforward. I don't think we need to worry about tests for the actual tracking protection UI (that should be handled separately).

Here's the patch with simple tests, try run below. It is just adding the pref and when you change this pref privacy.trackingprotection.enabled is automatically changed as well. Do we want to put this code in a separate file and #include it into nsBrowserGlue.js (which is already huge)? https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4162c4b12912

Comment on attachment 8505665 [details] [diff] [review] polaris.patch Looks good, but a few tweaks needed: - move the code away from the top-level of nsBrowserGlue, and just put it in BG__init / BG_observe - use Services.prefs - this should set the DNT pref as well (privacy.donottrackheader.enabled), per comment 0 - we'll probably need a separate pref for bug 1031033's UI, and have this code toggle that too - per comment 5, let's make this #ifdef NIGHTLY_BUILD for now

Done. Please have a look.

Comment on attachment 8508421 [details] [diff] [review] polaris.patch Review of attachment 8508421 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall, though this could use some manual testing to ensure that the common use cases work acceptably (toggling the main pref on, toggling the individual prefs independently, toggling the main pref off again, etc.). Let's hold off on landing this until we answer the DNT question specifically. ::: browser/app/profile/firefox.js @@ +1763,5 @@ > > pref("browser.apps.URL", "https://marketplace.firefox.com/discovery/"); > + > +pref("browser.polaris.enabled", false); > +pref("browser.polaris.ui.enabled", false); This should be privacy.trackingprotection.ui.enabled since it's tied to tracking protection specifically. ::: browser/components/nsBrowserGlue.js @@ +401,5 @@ > + case "nsPref:changed": > + if (data == POLARIS_ENABLED) { > + let enabled = Services.prefs.getBoolPref(POLARIS_ENABLED); > + Services.prefs.setBoolPref("browser.polaris.ui.enabled", enabled); > + Services.prefs.setBoolPref("privacy.donottrackheader.enabled", enabled); Hmm, I wonder if this will actually work right given that bug 1042135 hasn't landed yet. ::: browser/components/test/browser_polaris_prefs.js @@ +7,5 @@ > +function* setAndCheck(pref, enabled) { > + Services.prefs.setBoolPref(POLARIS_ENABLED, enabled); > + yield spinEventLoop(); > + let prefEnabled = Services.prefs.getBoolPref(pref); > + Assert.equal(prefEnabled, enabled, pref + " should be enabled iff polaris enabled."); Slightly misleading comment since these prefs can all still be toggled independently.

Oh, and we also need to update bug 1031033's patch to obey the pref you're adding here.

I updated the patch with some more tests. If you see any more cases worth testing let me know. > This should be privacy.trackingprotection.ui.enabled I renamed the setting. > I wonder if this will actually work right given that bug 1042135 hasn't landed yet. Why it should not work without that bug? > Let's hold off on landing this until we answer the DNT question specifically. What do you mean by that?

Forgot to rename the setting in one place. Also added test for default value that would have caught this.

(In reply to Tomasz Kołodziejski [:tomasz] from comment #13) > > I wonder if this will actually work right given that bug 1042135 hasn't landed yet. > > Why it should not work without that bug? > > > Let's hold off on landing this until we answer the DNT question specifically. > > What do you mean by that? Bug 1042135 simplifies the DNT prefs. If this patch lands before that one, someone who once set "tell sites to track me" will have privacy.donottrackheader.value=0, and so this patch just flipping privacy.donnottrackheader.enabled won't have the desired effect. I asked in bug 1042135 what we can do to get that landed.

I suppose in the interim we can just have your patch also just clearUserPref privacy.donottrackheader.value. Do you want to take care of comment 12 as well? Best to do it in that bug probably.

So as requested I reset privacy.donottrackheader.value when changing polaris pref. So even if someone explicitly says that he wants to be tracked (value = 0) it will be reset after changing polaris pref. Flagging for review as I'm not entirely sure of all the problems of bug 1042135.

I am confused about having the UI in bug 1031033 at all based on the behavior described in comment 0.

Can we get a Try run for sanity's sake please?

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b1241381e3de (https://tbpl.mozilla.org/?tree=Try&rev=b1241381e3de).

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #18) > I am confused about having the UI in bug 1031033 at all based on the > behavior described in comment 0. Not clear to me what you're saying here, can you elaborate? Do you disagree with the plan in comment 0?

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #21) > (In reply to [:mmc] Monica Chew (please use needinfo) from comment #18) > > I am confused about having the UI in bug 1031033 at all based on the > > behavior described in comment 0. > > Not clear to me what you're saying here, can you elaborate? Do you disagree > with the plan in comment 0? I'm confused why a pref only accessible through about:config without a visible UI (browser.polaris.enabled) is required to expose the UI for a pref that's already available in about:config (privacy.trackingprotection.enabled). There doesn't seem to be any user benefit in implementing bug 1031033 if the user still has to go to about:config.

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=977856&repo=fx-team

So this failed because getIntPref failed because there was not PREF_DNTV because it was removed in bug 1042135 and so this patch got bitrotted. However, changes in that bug were backed out due to failures. But then it re-landed. I'll update this patch to reflect those changes.

So I got rid of privacy.donottrackheader.value handling. It is no longer existent (bug 1042135). Also, since there is migration implementation in that bug we don't have to worry about clearing it (as in the previous patch). Unfortunately, try is closed now so I'll post a try link later.

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #22) > I'm confused why a pref only accessible through about:config without a > visible UI (browser.polaris.enabled) is required to expose the UI for a pref > that's already available in about:config > (privacy.trackingprotection.enabled). There doesn't seem to be any user > benefit in implementing bug 1031033 if the user still has to go to > about:config. The net effect is the same either way, but we want to be able to continue to toggle Polaris features independently from the global Polaris pref. The benefit to implementing bug 1031033 and this bug is that a user who has opted in to Polaris has a visible way to disable the tracking protection feature alone after the fact, without disabling the rest of the Polaris features (including future additions).

> The net effect is the same either way, but we want to be able to continue to > toggle Polaris features independently from the global Polaris pref. The > benefit to implementing bug 1031033 and this bug is that a user who has > opted in to Polaris has a visible way to disable the tracking protection > feature alone after the fact, without disabling the rest of the Polaris > features (including future additions). Thanks for explaining. I'm still confused, but I'm guessing that's out of scope of this bug. If bug 1031033 were not tied to Polaris at all, a user who has opted in to Polaris would still have a visible way to enable and disable tracking protection.

Comment on attachment 8510659 [details] [diff] [review] followup, add ifdefs for more of the patch, and tests Review of attachment 8510659 [details] [diff] [review]: ----------------------------------------------------------------- Hello Gavin, do you still want this patch? ::: browser/components/test/browser_polaris_prefs.js @@ +30,3 @@ > add_task(function* test_default_values() { > + if (isNightly()) { > + ok(true, "Skipping test, not Nightly") if (!isNightly) ?

It's in my queue, I'll push it shortly (with that change).

Verified as fixed using: FF 38 Build id:20150120030203 OS: Win 7 x64, Mac Os X 10.10, Ubuntu 12.04 x86

we have this test case which was authored almost 5 months ago, and it only runs on nightly: https://dxr.mozilla.org/mozilla-central/source/browser/components/test/browser_polaris_prefs.js?from=browser_polaris_prefs.js&case=true Is this still the case? Should we enable this on all branches? is this test still valid?

Polaris is still Nightly-only, so that seems fine.