Closed Bug 1081343 Opened 10 years ago Closed 10 years ago

Create a pref to control Polaris

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal
Points:
2

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 36
Iteration:
36.1

People

(Reporter: Dolske, Assigned: tomasz)

References

Details

Attachments

(2 files, 5 obsolete files)

polaris.patch
7.16 KB, patch
tomasz
: review+
Details | Diff | Splinter Review
followup, add ifdefs for more of the patch, and tests
3.93 KB, patch
Details | Diff | Splinter Review 
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.
Flags: needinfo?(jmoradi)
(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)?
Flags: needinfo?(mmc)
(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)?
Flags: needinfo?(mmc)
Flags: qe-verify?
Flags: firefox-backlog+
(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.
Flags: needinfo?(jmoradi)
Points: --- → 2
Assignee: nobody → tkolodziejski
Status: NEW → ASSIGNED
Iteration: --- → 36.1
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?
Flags: needinfo?(dolske)
(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).
Flags: qe-verify? → qe-verify+
Attached patch polaris.patch (obsolete) — — Splinter Review 
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
Attachment #8505665 - Flags: review?(dolske)
Flags: needinfo?(dolske)
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
Attachment #8505665 - Flags: review?(dolske) → feedback+
Attached patch polaris.patch (obsolete) — — Splinter Review 
Done. Please have a look.
Attachment #8505665 - Attachment is obsolete: true
Attachment #8508421 - Flags: review?(gavin.sharp)
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.
Attachment #8508421 - Flags: review?(gavin.sharp) → review+
Oh, and we also need to update bug 1031033's patch to obey the pref you're adding here.
Attached patch polaris.patch (obsolete) — — Splinter Review 
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?
Attachment #8508421 - Attachment is obsolete: true
Flags: needinfo?(gavin.sharp)
Attachment #8508487 - Flags: review+
Attached patch polaris.patch (obsolete) — — Splinter Review 
Forgot to rename the setting in one place. Also added test for default value that would have caught this.
Attachment #8508487 - Attachment is obsolete: true
Attachment #8508489 - Flags: review+
(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.
Flags: needinfo?(gavin.sharp)
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.
Attached patch polaris.patch (obsolete) — — Splinter Review 
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.
Attachment #8508489 - Attachment is obsolete: true
Attachment #8508821 - Flags: review?(gavin.sharp)
Attachment #8508821 - Flags: review?(gavin.sharp) → review+
I am confused about having the UI in bug 1031033 at all based on the behavior described in comment 0.
Keywords: checkin-needed
Can we get a Try run for sanity's sake please?
Keywords: checkin-needed
(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.
Blocks: 1087910
sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=977856&repo=fx-team
Whiteboard: [fixed-in-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.
Attached patch polaris.patch — — Splinter Review 
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.
Attachment #8508821 - Attachment is obsolete: true
Attachment #8510583 - Flags: review+
(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.
https://hg.mozilla.org/mozilla-central/rev/54fab44be5f3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
QA Contact: catalin.varga
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)
?
Flags: needinfo?(gavin.sharp)
It's in my queue, I'll push it shortly (with that change).
Flags: needinfo?(gavin.sharp)
Verified as fixed using:

FF 38
Build id:20150120030203
OS: Win 7 x64, Mac Os X 10.10, Ubuntu 12.04 x86
Status: RESOLVED → VERIFIED
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?
Flags: needinfo?(gavin.sharp)
Polaris is still Nightly-only, so that seems fine.
Flags: needinfo?(gavin.sharp)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: