1042135 - Revert do not track preferences to be a simple on/off switch

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

We currently have three options for DNT.

The one saying that a user explicitly wants to be tracked seems to be superfluous. Being tracked is (unfortunately) the standard on the web. Even our SUMO page says that this option is effectively the same as not telling the website about tracking preferences. By presenting more options to the users here, we make it less likely for them to change the setting at all.

We should therefore remove that option which would allow us to reduce the UI to one simple checkbox:

[ ] Tell websites that I do not want to be tracked

The main reason for switching away from this design (bug 765398) seems to have been the ad industry wanting the explicit opt-in. Two years later, I don't think that's an issue anymore. Especially since we're the only browser offering the "Do track" option.

Sid, it looks like you've been very involved in this discussion. What's your take on this?

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

While I hate to flip-flop on this UI too much, we're seeing very little adoption of the "track me" feature, so it's probably fine to remove it.  

Here's a patch to do what we want.  Not sure if we need to migrate old pref values to the new ones or not.  Flagging dolske, since he reviewed the other UI changes here.

Comment 2

[Dave Garrett]

Simplifying it this much re-introduces a user confusion point that was fixed with the 3 option version: it implies that setting DNT disables tracking, whilst doing so actually notably increases HTTP header fingerprintability. (this wouldn't be the case if DNT were on by default, as it REALLY should be, especially at this point) If the 3rd option of an explicit yes were to be removed, then preferably it should remain as a radio option:

(*) Do not tell sites anything about my tracking preferences
( ) Tell websites that I do not want to be tracked

It takes up more space but it's more explicit about the available options.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Dave: DNT on by default makes it a worthless signal since if we do that "DNT:1" is more about Mozilla's opinion than the individual user's, but that's an old conversation that we don't need to re-hash in this bug.[0] 

We _could_ add a note below the checkbox that explains that some sites ignore this request...  "Note: This Do Not Track request may affect your browsing experience and not all companies will honor it."  IMO, the key words in this feature's text is "Tell sites".

[0] http://blog.sidstamm.com/2011/11/firefox-wont-activate-dnt-by-default.html

Comment 4

[Dave Garrett]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #3)
> Dave: DNT on by default makes it a worthless signal [...]

I don't intend to re-argue it either; I merely can't in good conscience say anything on the topic without mentioning the inherent flaws in the system.

> We _could_ add a note below the checkbox that explains that some sites
> ignore this request...

The problem is not just that it will be ignored, but that the very act of saying you wish to not be tracked volunteers the user for more effective HTTP header fingerprinting based tracking. (the general consensus in the related bugs was header based fingerprinting is far more worthy of concern than client based fingerprinting due to its ease and inability to be detected) If you'd like to make that clear in a warning along with the known problem of it just being comically unenforceable, be my guest. Having an explicit "tell them nothing" option is at least better than nothing. A simple checkbox with a warning confirmation prompt explaining the situation in detail would be ideal, but more complicated.

Comment 5

[Dave Garrett]

Actually, comment 4 isn't the ideal. The ideal solution is of course the obvious one: just force every user to make a decision in a first-run page with a DNT explanation and big yes or no buttons that can't be dismissed without a response. DNT is then based on the actual choice of all users, rather than just those who found the buried checkbox, and DNT enabling no longer increases entropy in any meaningful way because most users would obviously say no to tracking and yes to DNT, thus making DNT set to off the unusual option and more susceptible to fingerprinting. (yeah, out of scope of this bug, but I can't avoid saying it anyway)

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 8460325 [details] [diff] [review]
proposed patch

Review of attachment 8460325 [details] [diff] [review]:
-----------------------------------------------------------------

Honestly, I think we should just remove this UI entirely. AFAICT (from the distant sidelines) "Do Not Track" has failed to gain any industry interest, and a some major sites have recently backtracked and now explicitly ignore it. The original pitch for DNT was that in lieu of a technical blocking ability (akin to AdBlock), it's effectiveness would come from getting the industry to respect it via user demand... If that's not happening, this isn't much more than a functionless "feel good" checkbox. [At best -- at worse it's actively misleading/harming users into thinking it's affording significant privacy protection.]

But this patch is at least moving in the right direction.

::: browser/components/preferences/in-content/privacy.xul
@@ +80,5 @@
>    <caption><label>&tracking.label;</label></caption>
> +  <checkbox id="privacyDoNotTrackCheckbox"
> +            label="&dntTrackingNotOkay.label2;"
> +            accesskey="&dntTrackingNotOkay.accesskey;"
> +            preference="privacy.donottrackheader.enabled"/>

Hmm. You're not doing any migration, so for those (few) people who enabled the "tell sites I want to be tracked", they'll now see UI indicating the browser is doing exactly the opposite. Which feels like an embarrassing footgun.

I'd suggest one of:

1) Retain the prefsync code, but treat privacy.trackingprotection.enabled == true + privacy.donottrackheader.value != 1 as being unchecked in the UI. (And if the user checks it, .value gets set back to 1)

2) Remove the privacy.donottrackheader.value pref entirely (and reset .enabled to false for users who had it set to != 0)

I'd probably prefer the latter.

::: modules/libpref/src/init/all.js
@@ +849,3 @@
>  //   0 = tracking is acceptable
>  //   1 = tracking is unacceptable
> +//   (our current UI only provides for 1 or no header)

I'd remove this comment, since it's the kind of thing that will bitrot quickly (i.e., someone changing the UI again probably isn't going to see this comment, and someone seeing this comment shouldn't trust it for that reason ;).

Comment 7

[Justin Dolske [:Dolske]]

Also, other issues aside, I don't think the issue in comment 2 is a significant concern. A checkbox is fine.

Comment 8

[Dave Garrett]

(In reply to Justin Dolske [:Dolske] from comment #6)
The success of DNT relied on 3 decisions being made:
1) Users had to decide to opt to be tracked or not
2) Advertisers had to decide to honor the flag in some capacity (& publicly)
3) Legislators had to decide to make the flag somewhat enforceable

Not a one of those actually happened. Browser vendors have been too afraid to ask users for a mandatory decision, because they all knew what the answer would be and advertisers wouldn't like it. Advertisers generally did the obvious thing and came up with excuses to ignore it. Legislators did the obvious thing and the topic was never heard from again.

Work is being done on an active tracking blocker (bug 1029886), so at this point I wholeheartedly agree that DNT should just be gutted out of the codebase entirely and banished to an addon for the few that want to stick around with this failed experiment.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Attachment: firefox desktop patch

Thanks for the quick turnaround, dolske.

I opted for migration path (2), except I didn't remove the privacy.donottrack.value pref (I did reset it on migration if necessary).  It's possible an add-on is using that pref to send a specific header value, and necko currently uses that pref to figure out what value to send.  If you still think we should remove it, please r- me again and I'll go and do that.

Looks like I still need to make patches for:
* Metro support (see bug 854077)
* Fennec support (see bug 648654)
* Firefox OS support (not sure of relevant bug)

Comment 10

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Regarding user expectation: Perhaps we can make it clearer what the checkbox does by using wording like
»Notify websites that I don't want to be tracked«

Comment 11

[Dave Garrett]

Dumb L10N question: Is it 100% safe to reuse the string as-is after converting to a checkbox? Sometimes different languages need different wording/spellings depending on where the string is used, though I can't think of any way this could come up with such a simple change as this.

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 8460517 [details] [diff] [review]
firefox desktop patch

Review of attachment 8460517 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, in other cases I might be OK with an obscure pref that resulted in quirky UI. But I think in the context of a privacy feature we've advertised, having UI that can indicate the _opposite_ of what's actually happening is too big of a footgun. So I think the .value pref should indeed be removed, especially given that we've removed more popular prefs due to them being maintenance burdens (and this one is clearly not a popular pref).

[Alternatively, if you've got some compelling reason to keep it, doing the rest of suggestion #1 in comment 6 would be fine.]

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Attachment: firefox desktop patch

Ok, I deep sixed the pref.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Attachment: b2g-gecko patch

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fennec patch

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8460598 [details] [diff] [review]
fennec patch

Hey Margaret: I'm not sure I'm doing this right, would like your feedback.  Trying to reduce the DNT options to just on/off (unset/don't track me).  Does this look like a good direction?  You seem to know about this kind of stuff.

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 8460596 [details] [diff] [review]
firefox desktop patch

Review of attachment 8460596 [details] [diff] [review]:
-----------------------------------------------------------------

r+, though you'll technically need someone else to officially stamp the dom/network bits.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8460596 [details] [diff] [review]
firefox desktop patch

mcmanus, mounir: reducing complexity here.  This look ok?  Pretty minor changes.  Flagging you guys since you know about the files in dom/base and netwerk/protocol/http.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8460597 [details] [diff] [review]
b2g-gecko patch

Fabrice: this doesn't seem like quite enough to make b2g work with this change in the DNT settings.

I'm removing privacy.donottrackheader.value from prefs, so I'll probably have to also update some stuff in the gaia settings UI... no idea how to do that, so if there's any advice you can give me (or if you know someone who can pick this off quickly) it would be helpful.

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 8460597 [details] [diff] [review]
b2g-gecko patch

Review of attachment 8460597 [details] [diff] [review]:
-----------------------------------------------------------------

The gecko side looks fine.

On the gaia side, look at https://github.com/mozilla-b2g/gaia/search?q=privacy.donottrackheader.value&ref=cmdform for places where this setting is used.

I'm pretty sure that you will have to use new l10n ids with the new strings and that you can remove the old ones. The current l10n ids are doNotTrackActions, allowTracking, doNotHavePrefOnTracking (see in https://github.com/mozilla-b2g/gaia/blob/76d9c1c1d1e56fcecd0b01ba604587cf73cb66aa/apps/settings/elements/do_not_track.html). You need to update the english version in https://github.com/mozilla-b2g/gaia/blob/76d9c1c1d1e56fcecd0b01ba604587cf73cb66aa/apps/settings/locales/settings.en-US.properties.

Comment 21

[Patrick McManus [:mcmanus]]

Comment on attachment 8460596 [details] [diff] [review]
firefox desktop patch

Review of attachment 8460596 [details] [diff] [review]:
-----------------------------------------------------------------

r=mcmanus on netwerk bits

Comment 22

[:Margaret Leibovic]

Comment on attachment 8460598 [details] [diff] [review]
fennec patch

Review of attachment 8460598 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the slow response. I think you can make this a lot simpler by getting rid of most of the logic we currently use to implement this special tri-state pref.

If the pref is just flipping a boolean value for "privacy.donottrackheader.enabled", we can get rid of all this special logic in browser.js and use the correct key directly in preferences_privacy.xml. We could probably even just back out this patch to fix the bug:
http://hg.mozilla.org/mozilla-central/rev/5c1d0b408e2c

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fennec patch

Thanks for the feedback, Margaret.  I updated the patch to remove as much of the custom code as I could, but don't want to do a pure backout since that would add-back an old string.  In the interest of avoiding string changes, I deleted all the strings that are no longer used but kept pref_donottrack_disallow_tracking around for use.

Other than that, this patch should look much like backing out the cset you mentioned.

Comment 24

[:Margaret Leibovic]

Comment on attachment 8462874 [details] [diff] [review]
fennec patch

Review of attachment 8462874 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looking good, but when I applied the patch locally I found that the string describing the pref gets cut off. See my comment below for a suggestion about how to fix this.

::: mobile/android/base/resources/xml/preferences_privacy.xml
@@ +8,5 @@
>                    android:title="@string/pref_category_privacy_short"
>                    android:enabled="false">
>  
> +    <CheckBoxPreference android:key="privacy.donottrackheader.enabled"
> +                        android:title="@string/pref_donottrack_disallow_tracking"

We should modify how we display this preference, since this string gets cut off (and probably will be cut off in other locales as well). To solve this issue, we could provide both a title and a summary for the checkbox, similarly to what we do in the "Data Choices" section of the "Mozilla" preferences screen.

Here's what the code for that looks like:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/xml/preferences_vendor.xml#28

Perhaps we could make the title "Do Not Track" and we could make the summary a sentence explaining what that means for a user.

As a follow-up bug we could also add a "Learn more" link.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 8460596 [details] [diff] [review]
firefox desktop patch

Stealing Mounir's review here, DOM bits look good. r=jst

Comment 26

[[:mmc] Monica Chew (no longer reading bugmail)]

What's the status of this bug?

Comment 27

[Sid Stamm [:geekboy or :sstamm]]

Hey monica, sorry for lag here.  The desktop patch is ready to land but the fennec and b2g patches need work.  (I'm frankly not as familiar with the front-end code for those two platforms and it's a lower priority than my other work right now.)

If you want to pick up the fennec/b2g work or help me find someone who can knock them out quickly, I'd be grateful.  Also, we could land the desktop patch first, but I hate to make the UI inconsistent across products.

Comment 28

[:Margaret Leibovic]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #27)
> Hey monica, sorry for lag here.  The desktop patch is ready to land but the
> fennec and b2g patches need work.  (I'm frankly not as familiar with the
> front-end code for those two platforms and it's a lower priority than my
> other work right now.)
> 
> If you want to pick up the fennec/b2g work or help me find someone who can
> knock them out quickly, I'd be grateful.  Also, we could land the desktop
> patch first, but I hate to make the UI inconsistent across products.

I can find the time to finish the fennec patch for you some time this week, if that would help you out.

Comment 29

[Sid Stamm [:geekboy or :sstamm]]

(In reply to :Margaret Leibovic from comment #28)
> I can find the time to finish the fennec patch for you some time this week,
> if that would help you out.

That would be brilliant.  The b2g stuff may take me a while though, too.

Comment 30

[:Margaret Leibovic]

Attachment: fennec screenshot (WIP strings)

The only problem with the fennec patch was that the pref title got cut off on the settings screen. I updated the patch to use an android:summary for the pref so that the text doesn't get cut off. However, I am uncertain about what strings we want to use for this title/summary.

Any opinions here? Sid/Monica, I feel like you both have better insight into what kind of language we use for these privacy features across our products.

Comment 31

[Chenxia Liu [:liuche]]

Bug 1048418 adds a CustomCheckboxPreference that allows wrapping of preference titles, if you wanted to use that.

Comment 32

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to :Margaret Leibovic from comment #30)
> Created attachment 8478718 [details]
> fennec screenshot (WIP strings)
> 
> The only problem with the fennec patch was that the pref title got cut off
> on the settings screen. I updated the patch to use an android:summary for
> the pref so that the text doesn't get cut off. However, I am uncertain about
> what strings we want to use for this title/summary.
> 
> Any opinions here? Sid/Monica, I feel like you both have better insight into
> what kind of language we use for these privacy features across our products.

Looks good to me. Sid told me recently he's one of those needinfo folks :) (welcome, Sid!)

Comment 33

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #31)
> Bug 1048418 adds a CustomCheckboxPreference that allows wrapping of
> preference titles, if you wanted to use that.

Oh, that could also be a good solution. But I'm starting to think the summary is nice, since it gives us even more room to explain what's going on.

Comment 34

[Sid Stamm [:geekboy or :sstamm]]

Sorry for the delay... yes, the screenshot looks good to me.  It uses the same language that we want to use in desktop (except it says Fennec, which is obviously not in desktop).

Comment 35

[:Margaret Leibovic]

Attachment: fennec patch (v2)

I didn't change much since I reviewed the last version, but it's probably good to have an extra set of eyes.

Also, the "Fennec" string comes from the unofficial branding - this will be "Firefox" in the release version.

Comment 36

[Chenxia Liu [:liuche]]

Comment on attachment 8480099 [details] [diff] [review]
fennec patch (v2)

Review of attachment 8480099 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! Are we doing anything about the donnottrack.header.value? I think that's the only loose end - maybe file a follow-up to remove that if we're not sure if we want to get rid of the pref entirely.

Comment 37

[Sid Stamm [:geekboy or :sstamm]]

Ah yeah, we should trash the donottrack.header.value pref.  See "nsBrowserGlue.js" in attachment 8460596 [details] [diff] [review] to see the migration we're gonna do for desktop.

Comment 38

[[:fabrice] Fabrice Desré]

Evelyn, can you help on the gaia side here? Thanks!

Comment 39

[Ed Lee :Mardak]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #37)
> Ah yeah, we should trash the donottrack.header.value pref
Just a heads up that enhanced tiles migration checks value == 1 to turn off enhanced tiles by default:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/DirectoryLinksProvider.jsm#132
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js#594

I don't think the tests fail with the removal because the tests set the pref. :(

Comment 40

[Evelyn Hung]

EJ is able to help. :)

Comment 41

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Since Gaia lives on a different repo, I am not entirely sure how we are going to land everything across products in one bug. Also, Evelyn mentioned that we need to take care of OTA upgrades.

How does Fx Desktop/Fx Android take cares of upgrades in this case?

Comment 42

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

@Jenny, can you provide a UX spec for this change ? Thanks :)

Comment 43

[Jenny Lee]

Attachment: [Settings] Do not track v1.0.pdf

See attached, note that the title on root panel is changed to "tracking", thanks!

Comment 44

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: gaia patch

This is a WIP patch for gaia.

Comment 45

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Keep ni? on me to track this bug.

Comment 46

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Ed Lee :Mardak from comment #39)
> I don't think the tests fail with the removal because the tests set the
> pref. :(

Looks like it should be a simple fix.  If I write the two-line patch to fix this, can you review, Mardak?

Comment 47

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8460597 [details] [diff] [review]
b2g-gecko patch

Fabrice: given these are probably all the changes necessary in gecko for b2g, can you review this for me?  The gaia stuff will be done in other patches (thanks for the help, all!)

Comment 48

[Ed Lee :Mardak]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #46)
> Looks like it should be a simple fix.  If I write the two-line patch to fix
> this, can you review, Mardak?
I can give f+. To be 100% correct, it might not be entirely a simple fix as there's potential timing issues of when does the pref get migrated vs when does directory tiles check the pref.

We can relax that, and I think we're okay of just saying any users that previously had DNT=send (whether value=0 or value=1) just gets directory tiles default off. I.e., only check for dnt.enabled = true -> default directory tiles off.

Comment 49

[[:fabrice] Fabrice Desré]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #47)
> Comment on attachment 8460597 [details] [diff] [review]
> b2g-gecko patch
> 
> Fabrice: given these are probably all the changes necessary in gecko for
> b2g, can you review this for me?  The gaia stuff will be done in other
> patches (thanks for the help, all!)

Where are we doing the migration of users that are on builds with the old setting to this new one? Is that planned to be done as part of the FTU upgrade step?

Comment 50

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8482553 [details] [review]
gaia patch

Hi Arthur & Zac,

can you both help me review this patch ? Thanks :)

Comment 51

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8482553 [details] [review]
gaia patch

Thanks for the patch, EJ. 

We may have to handle the key migration. The reason is that when "privacy.donottrackheader.enabled" is true and "privacy.donottrackheader.value" is 0, it actually means the user wants to be tracked. The user preference will change from "track" to "do not track" if we simply discard the value of "privacy.donottrackheader.value" after OTA.

Comment 52

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8482553 [details] [review]
gaia patch

Hi Arthur & Alive,

just added the migration code in System and fixed some typos in Settings, can you guys help me review again ? Thanks !

Comment 53

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8482553 [details] [review]
gaia patch

Please add unit test in system change and ask review again.

Comment 54

[[:fabrice] Fabrice Desré]

Comment on attachment 8460597 [details] [diff] [review]
b2g-gecko patch

Review of attachment 8460597 [details] [diff] [review]:
-----------------------------------------------------------------

Since comment 52 states that the migration is done in gaia, that's fine.

Comment 55

[:Margaret Leibovic]

Comment on attachment 8480099 [details] [diff] [review]
fennec patch (v2)

Review of attachment 8480099 [details] [diff] [review]:
-----------------------------------------------------------------

I failed to realize that we would need to migrate this pref, so this patch needs updating before it can land.

It looks like we should be able to do that here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#809

Comment 56

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8482553 [details] [review]
gaia patch

Hi Alive, I just added unit tests for migrator and I think the patch is ready to be reviewed. 

Thanks.

Comment 57

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8482553 [details] [review]
gaia patch

r=me with the comments addressed, thanks!

Comment 58

[Florin Strugariu [:Bebe]]

Comment on attachment 8482553 [details] [review]
gaia patch

Can you update the python test with the lost steps pls

The rest of the python code looks OK

Comment 59

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8482553 [details] [review]
gaia patch

r=me + Arthur's comment + l10n key changed

Comment 60

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi :Bebe,

I can't understand the comment you left on github, can you tell more clearly about what's the missing step you mentioned there ? Originally we have three options and we would click one by one and then check whether we really set them to mozSettings db or not.

In my patch, these options are changed to only a checkbox with true/false values, so I just did the same way to check. 

Any tips would be appreciated, thanks !

Comment 61

[Florin Strugariu [:Bebe]]

EJIn that test we used to check if:

1. we can enable the option
2. we can disable the option

With your changes in the test now we check only the first step (1.) where we check if we can't enable that option.

Can you add to the test the second step where after you check that the issue is enabled you disable it  and check if it's disabled.

Comment 62

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8482553 [details] [review]
gaia patch

Thanks :Bebe,

I just updated the patch and please help me review it again, thanks !

Comment 63

[Florin Strugariu [:Bebe]]

Comment on attachment 8482553 [details] [review]
gaia patch

All looks OK now.
Thanks EJ for the updates

Comment 64

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks :Bebe & all,

merged this patch into Gaia/master(2.2) : https://github.com/mozilla-b2g/gaia/commit/b00c0f597a0a0404975fe929230da0b5a805b59c

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

B2g desktop mochitest-1 is now failing with https://tbpl.mozilla.org/php/getParsedLog.php?id=48343284&tree=B2g-Inbound

Reverted in https://github.com/mozilla-b2g/gaia/commit/b68a35efa535851f679fda2a9acc100a967de67e until the test can be fixed.

Comment 66

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Sid,

Gaia patch is done but got backed out, because I am not so familiar with the env in Gecko, can you provide some tips on the broken test ? 

I noticed your firefox desktop patch had already fixed something there but not landed yet (?) Should I land after gecko's change ? What's the situation of your patch now ?

Comment 67

[Sid Stamm [:geekboy or :sstamm]]

I was hoping to land everything about the same time; probably all the gecko/firefox changes first, then the gaia patch to follow.  The only thing left before landing is the Fennec patch.

Margaret: are you able to finish this patch soonish or should I try to do the migration (probably will take me a week or two before I can get to it)?

Comment 68

[:Margaret Leibovic]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #67)
> I was hoping to land everything about the same time; probably all the
> gecko/firefox changes first, then the gaia patch to follow.  The only thing
> left before landing is the Fennec patch.
> 
> Margaret: are you able to finish this patch soonish or should I try to do
> the migration (probably will take me a week or two before I can get to it)?

I have this on my list of things to do, I can try to get to it some time today.

Comment 69

[:Margaret Leibovic]

Attachment: fennec patch (v3)

Updated to include migration (logic copied from the desktop patch). I tested the migration locally and it worked.

Comment 70

[Chenxia Liu [:liuche]]

Comment on attachment 8491861 [details] [diff] [review]
fennec patch (v3)

Review of attachment 8491861 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, a couple of questions on the pref migration part.

::: mobile/android/chrome/content/browser.js
@@ +825,5 @@
> +    // three-state to two-state. (This reverts a setting of "please track me"
> +    // to the default "don't say anything").
> +    try {
> +      if (Services.prefs.getBoolPref("privacy.donottrackheader.enabled") &&
> +          Services.prefs.getIntPref("privacy.donottrackheader.value") != 1) {

Nit: Parens around the second boolean statement.

@@ +827,5 @@
> +    try {
> +      if (Services.prefs.getBoolPref("privacy.donottrackheader.enabled") &&
> +          Services.prefs.getIntPref("privacy.donottrackheader.value") != 1) {
> +        Services.prefs.clearUserPref("privacy.donottrackheader.enabled");
> +        Services.prefs.clearUserPref("privacy.donottrackheader.value");

It looks like the docs say clearUserPref "does nothing if the prefbranch it is called on is a default branch." Do you know if that's the case here?

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch#clearUserPref%28%29

Also, if we're removing privacy.donottractheader.value completely and not using it, maybe we should check for the pref and clear it if it exists, regardless of what the value of privacy.donottrackheader.enabled is.

Comment 71

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #70)

> @@ +827,5 @@
> > +    try {
> > +      if (Services.prefs.getBoolPref("privacy.donottrackheader.enabled") &&
> > +          Services.prefs.getIntPref("privacy.donottrackheader.value") != 1) {
> > +        Services.prefs.clearUserPref("privacy.donottrackheader.enabled");
> > +        Services.prefs.clearUserPref("privacy.donottrackheader.value");
> 
> It looks like the docs say clearUserPref "does nothing if the prefbranch it
> is called on is a default branch." Do you know if that's the case here?
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIPrefBranch#clearUserPref%28%29

No, you need to specifically request the default pref branch, e.g.:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#8170

Services.prefs will return the user pref branch by default.

> Also, if we're removing privacy.donottractheader.value completely and not
> using it, maybe we should check for the pref and clear it if it exists,
> regardless of what the value of privacy.donottrackheader.enabled is.

Sure, this sounds good. I can update the logic and post a new patch.

Comment 72

[:Margaret Leibovic]

Attachment: fennec patch (v4)

Comment 73

[Chenxia Liu [:liuche]]

Comment on attachment 8492414 [details] [diff] [review]
fennec patch (v4)

Review of attachment 8492414 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

Comment 74

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #67)
> I was hoping to land everything about the same time; probably all the
> gecko/firefox changes first, then the gaia patch to follow.  The only thing
> left before landing is the Fennec patch.
> 
> Margaret: are you able to finish this patch soonish or should I try to do
> the migration (probably will take me a week or two before I can get to it)?

Ok thanks, Gaia patch got backed out because lack of Gecko patch that would cause test failed. After things done, please feel free to cherry-pick Gaia patch and merge to master. THanks !

Comment 75

[:Gavin Sharp [email: [e-mail]]]

Too late now, but for future reference - this bug would have been so much easier to track if it was split in three per-product (Desktop, Android, OS).

Comment 76

[:Margaret Leibovic]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #75)
> Too late now, but for future reference - this bug would have been so much
> easier to track if it was split in three per-product (Desktop, Android, OS).

I agree. I just filed bug 1071747 for the Fennec part of this, at least to make things easier for the QA folks on our team.

I can land the Fennec patch there. Sid, is there any reason for me not to go ahead and land it now?

Comment 77

[:Margaret Leibovic]

Comment on attachment 8492414 [details] [diff] [review]
fennec patch (v4)

Moved this over to bug 1071747.

Comment 78

[Sid Stamm [:geekboy or :sstamm]]

(In reply to :Margaret Leibovic from comment #76)
> I can land the Fennec patch there. Sid, is there any reason for me not to go
> ahead and land it now?

I don't think so... but we might as well land all the gecko stuff at the same time.  Are you willing to land it all (except the gaia patch)?

Comment 79

[Sid Stamm [:geekboy or :sstamm]]

It's been a little while, so doing a check before landing (includes patch from bug 1071747):
https://tbpl.mozilla.org/?tree=Try&rev=6bbe5e251c9b

Comment 80

[Sid Stamm [:geekboy or :sstamm]]

Looks like there's an xpcshell failure caused by these patches.

14:01:20  WARNING -  TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js | true == false - See following stack:
14:01:20     INFO -  /builds/slave/test/build/tests/xpcshell/tests/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js:checkDefault:597
14:01:20     INFO -  /builds/slave/test/build/tests/xpcshell/tests/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js:test_DirectoryLinksProvider_setDefaultEnhanced:607
14:01:20     INFO -  resource://gre/modules/Task.jsm:createAsyncFunction/asyncFunction:239
14:01:20     INFO -  resource://gre/modules/Task.jsm:Task_spawn:164
14:01:20     INFO -  /builds/slave/test/build/tests/xpcshell/head.js:_run_next_test:1308
14:01:20     INFO -  /builds/slave/test/build/tests/xpcshell/head.js:do_execute_soon/<.run:570
14:01:20     INFO -  /builds/slave/test/build/tests/xpcshell/head.js:_do_main:191
14:01:20     INFO -  /builds/slave/test/build/tests/xpcshell/head.js:_execute_test:405

I haven't had a chance to debug yet, but we probably want to fix that before landing.

Comment 81

[Justin Dolske [:Dolske]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #80)
> Looks like there's an xpcshell failure caused by these patches.
> 
> test_DirectoryLinksProvider.js

This is a test for the newtab enhanced tiles stuff, which involves DNT. It looks to be testing combinations of the .enabled and .value prefs to test that it disables itself appropriately. Probably just needs updated to only test .enabled = true/false, without additional tests on .value setting.

Comment 82

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix_directorylinks

Update DirectoryLinksProvider to use only the donottrackheader.enabled pref since we are removing donottrack.value.  adw: you have reviewed code in these files before, can you take a peek? 

This applies on top of the desktop patch in this bug.

Mardak: you're still flagged needinfo on this bug; please make sure this is a sane change.

Comment 83

[Sid Stamm [:geekboy or :sstamm]]

Attachment: dnt-checkbox-b2g

I spoke with fabrice in person and we decided we don't need one of the lines in the previous patch, so here's an updated b2g-gecko patch with his review.

Once all the gecko stuff is reviewed and ready to land, we can land it all and follow up with the gaia changes the day after.  Fabrice says nothing should break in gaia if we land all the gecko stuff, wait 24 hours, then take the gaia patch.

Comment 84

[Ed Lee :Mardak]

Comment on attachment 8505550 [details] [diff] [review]
fix_directorylinks

>+++ b/browser/modules/DirectoryLinksProvider.jsm	Wed Oct 15 09:19:09 2014 -0700
>         // Default to not enhanced if DNT is set to tell websites to not track
>-        if (Services.prefs.getBoolPref("privacy.donottrackheader.enabled") &&
>-            Services.prefs.getIntPref("privacy.donottrackheader.value") == 1) {
>+        if (Services.prefs.getBoolPref("privacy.donottrackheader.enabled")) {
This is good with the caveat that it's not the exact behavior from before given how the prefs migrate (for enabled/0).

Comment 85

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Ed Lee :Mardak from comment #84)
> This is good with the caveat that it's not the exact behavior from before
> given how the prefs migrate (for enabled/0).

And as discussed in IRC, we're ok with this behavior.

Comment 86

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #83)
> Once all the gecko stuff is reviewed and ready to land, we can land it all
> and follow up with the gaia changes the day after.  Fabrice says nothing
> should break in gaia if we land all the gecko stuff, wait 24 hours, then
> take the gaia patch.

Except it looks like an existing gaia UI test breaks:
https://tbpl.mozilla.org/?tree=Try&rev=df3ff0f57584

Comment 87

[:Gavin Sharp [email: [e-mail]]]

What's stopping the Desktop patch from landing here? We need this for bug 1081343.

Comment 88

[Sid Stamm [:geekboy or :sstamm]]

My intention was to land it all at once, so I haven't tested the desktop patches alone.  To land it all, we still have to fix the gaia tests (comment 86).  I'll check to see if we can land the desktop/fennec stuff first without breaking anything else.

Comment 89

[Sid Stamm [:geekboy or :sstamm]]

Lets see how it looks on try.  If this goes green I'll land everything but the b2g stuff today.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7483163e9a5b

Leaving the needinfo flag so it stays on my radar.

Comment 90

[Tomasz Kołodziejski [:tomasz]]

As asked in bug 1081343 comment 12 patch introduced in that bug is introducing privacy.trackingprotection.ui.enabled which might be useful here. In particular it is responsible for -- no surprise -- showing/hiding tracking protection ui.

Comment 91

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tomasz Kołodziejski [:tomasz] from comment #90)
> As asked in bug 1081343 comment 12 patch introduced in that bug is
> introducing privacy.trackingprotection.ui.enabled which might be useful
> here. In particular it is responsible for -- no surprise -- showing/hiding
> tracking protection ui.

Not really related to this bug - this is about DNT, which is separate from tracking protection.

Comment 92

[rsx11m]

Tomasz, that discussion is likely more for bug 1031033 on the tracking-protection UI.

Comment 93

[Sid Stamm [:geekboy or :sstamm]]

Looks like the desktop and fennec changes worked alone.  Landed.  Please leave this bug open until the b2g patch lands.

https://hg.mozilla.org/integration/mozilla-inbound/rev/093ea94a1697
https://hg.mozilla.org/integration/mozilla-inbound/rev/dffed5d2a799

Comment 94

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ab40afacc816 - looks like you have some of those lovely robocop tests that hard-code UI strings into test code to update, https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3188854&repo=mozilla-inbound and https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3188498&repo=mozilla-inbound

Comment 95

[:Margaret Leibovic]

Sid, you can just land the desktop changes here. I can work on fixing the robocop tests and landing the fennec patch in bug 1071747.

I don't think it matters that they land at *exactly* the same time, as long as they're in the same release :)

Comment 96

[Sid Stamm [:geekboy or :sstamm]]

Ok.  Thanks Margaret.

Comment 97

[Sid Stamm [:geekboy or :sstamm]]

Relanded without the fennec changes (to avoid the robocop test breakage): 

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a16137bc7b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f6aedc8d6e4

Comment 98

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/37875d52fb9c

Comment 99

[:Margaret Leibovic]

Oops, forgot to add the fx-team changeset link in here:
https://hg.mozilla.org/integration/fx-team/rev/37875d52fb9c

Comment 100

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #99)
> Oops, forgot to add the fx-team changeset link in here:
> https://hg.mozilla.org/integration/fx-team/rev/37875d52fb9c

Doh, I landed that with the old bug number. I'll close bug 1071747 and add the links there for reference.

Comment 101

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a16137bc7b4
https://hg.mozilla.org/mozilla-central/rev/0f6aedc8d6e4

Comment 102

[Tim Guan-tin Chien [:timdream] (please needinfo)]

EJ, Arthur, let's file a separate B2G bug and take the migration logic to Gecko like

https://hg.mozilla.org/mozilla-central/rev/9a16137bc7b4#l2.22

Let's not use the migrator in System app. Thanks.

Comment 103

[Arthur Chen [:arthurcc][inactive after 6/30]]

Bug 1110011 and bug 1110012 were filed for tracking the B2G work.

Comment 104

[Sid Stamm [:geekboy or :sstamm]]

tim: are we still planning to do this pref migration on b2g?  AFAICT, that's the only thing holding this bug open.

Comment 105

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #104)
> tim: are we still planning to do this pref migration on b2g?  AFAICT, that's
> the only thing holding this bug open.

Per bug 1112092 comment 21 it is decided perf migration needs to happen at Gaia System. Gregor, is this something people in your team could work on?

Comment 106

[Gregor Wagner [:gwagner]]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #105)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #104)
> > tim: are we still planning to do this pref migration on b2g?  AFAICT, that's
> > the only thing holding this bug open.
> 
> Per bug 1112092 comment 21 it is decided perf migration needs to happen at
> Gaia System. Gregor, is this something people in your team could work on?

Bug 1112092 is about settings from the settings api and this bug is about prefs as far as I can see. Why do we need a settings migrator for prefs?

Comment 107

[[:fabrice] Fabrice Desré]

See also bug 1161299 where I didn't have time to fix the test failures.

Comment 108

[[e-mail]]

the rest gaia work will be done in bug 1214532