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?

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.

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.

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

(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.

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 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 ;).

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

(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.

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)

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«

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 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.]

Ok, I deep sixed the pref.

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 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 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 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 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 on attachment 8460596 [details] [diff] [review] firefox desktop patch Review of attachment 8460596 [details] [diff] [review]: ----------------------------------------------------------------- r=mcmanus on netwerk bits

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

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 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 on attachment 8460596 [details] [diff] [review] firefox desktop patch Stealing Mounir's review here, DOM bits look good. r=jst

What's the status of this bug?

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.

(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.

(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.

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.

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

(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!)

(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.

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).

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 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.

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.

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

(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. :(

EJ is able to help. :)

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?

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

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

This is a WIP patch for gaia.

Keep ni? on me to track this bug.

(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 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!)

(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.

(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 on attachment 8482553 [details] gaia patch Hi Arthur & Zac, can you both help me review this patch ? Thanks :)

Comment on attachment 8482553 [details] 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 on attachment 8482553 [details] 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 on attachment 8482553 [details] gaia patch Please add unit test in system change and ask review again.

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 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 on attachment 8482553 [details] gaia patch Hi Alive, I just added unit tests for migrator and I think the patch is ready to be reviewed. Thanks.

Comment on attachment 8482553 [details] gaia patch r=me with the comments addressed, thanks!

Comment on attachment 8482553 [details] gaia patch Can you update the python test with the lost steps pls The rest of the python code looks OK

Comment on attachment 8482553 [details] gaia patch r=me + Arthur's comment + l10n key changed

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 !

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 on attachment 8482553 [details] gaia patch Thanks :Bebe, I just updated the patch and please help me review it again, thanks !

Comment on attachment 8482553 [details] gaia patch All looks OK now. Thanks EJ for the updates

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

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.

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 ?

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)?

(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.

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

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.

(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 on attachment 8492414 [details] [diff] [review] fennec patch (v4) Review of attachment 8492414 [details] [diff] [review]: ----------------------------------------------------------------- Great!

(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 !

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).

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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 on attachment 8492414 [details] [diff] [review] fennec patch (v4) Moved this over to bug 1071747.

(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)?

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

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.

(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.

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.

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 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).

(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.

(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

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

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.

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.

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.

(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.

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

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

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

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 :)

Ok. Thanks Margaret.

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

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

(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.

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

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.

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

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

(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?

(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?

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

the rest gaia work will be done in bug 1214532