|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
This is a new setting for the privacy API which does not mirror an existing setting in Chrome's implementation.  There are two prefs in which we are interested: privacy.trackingprotection.enabled (globally) privacy.trackingprotection.pbmode.enabled (only in Private Browsing) I have a few questions about the implementation: 1. Should this be put into an existing namespace, such as privacy.services, or should it be given a new namespace that indicates it is a setting only relevant for Firefox? 2. How should the two above prefs be settable? Do we need two separate settings for trackingprotection, or just one that accepts multiple values. For example, we might have one setting which accepts values that could mean: 1 - disabled globally 2 - enabled in private browsing only 3 - enabled globally Does the above make sense, or should it work differently from that?  https://developer.chrome.com/extensions/privacy
(In reply to Bob Silverberg [:bsilverberg] from comment #0) > 1. Should this be put into an existing namespace, such as privacy.services, > or should it be given a new namespace that indicates it is a setting only > relevant for Firefox? Another option to consider would be a new privacy.tracking.* namespace. This would allow us to expose other anti-tracking settings that are already in about:preferences (e.g. privacy.donottrackheader.enabled) or that we are testing/considering adding to about:preferences (e.g. privacy.resistFingerprinting). I think there are a few addons already (NoScript? PrivacyBadger?) that toggle the Do Not Track pref. > 2. How should the two above prefs be settable? Do we need two separate > settings for trackingprotection, or just one that accepts multiple values. > For example, we might have one setting which accepts values that could mean: > 1 - disabled globally > 2 - enabled in private browsing only > 3 - enabled globally The single setting with the above three values makes sense and matches the about:preferences UI in Nightly. The default is 2.
Thanks François, I like the suggestion of the privacy.tracking namespace. I'm going to go that route.
François, I have one more question about the relationship between privacy.trackingprotection.enabled and privacy.trackingprotection.pbmode.enabled. If privacy.trackingprotection.enabled = true and privacy.trackingprotection.pbmode.enabled = false, does that still mean that tracking protection is enabled globally (i.e., the former overrides the latter)? Or does that mean that tracking protection is only enabled for non-private windows, and if so, wouldn't that need to be a fourth option above?
(In reply to Bob Silverberg [:bsilverberg] from comment #3) > If privacy.trackingprotection.enabled = true and > privacy.trackingprotection.pbmode.enabled = false, does that still mean that > tracking protection is enabled globally (i.e., the former overrides the > latter)? Or does that mean that tracking protection is only enabled for > non-private windows, and if so, wouldn't that need to be a fourth option > above? The global switch (privacy.trackingprotection.enabled) takes precendence over the private browsing one. If TP is enabled globally, then we ignore the private browsing pref. There's no way to have TP enabled only in normal windows and not in private browsing windows.
Comment on attachment 8845424 [details] Bug 1345158 - Implement browser.privacy.trackingProtection API https://reviewboard.mozilla.org/r/118212/#review121130 Without having looked closely at the code, I have two questions about the design: 1. Why have the private browsing setting embedded in the value when we have SettingScope (http://searchfox.org/mozilla-central/rev/b035220d0f939559f4f8cf7b9079bc12f6adfc35/toolkit/components/extensions/schemas/types.json#11) 2. Maybe this is a question for francois, does disabling tracking protection stop us from downloading ths list of trackers? If so, I think this really belongs in privacy.services
(In reply to Andrew Swan [:aswan] from comment #6) > 2. Maybe this is a question for francois, does disabling tracking protection > stop us from downloading ths list of trackers? If so, I think this really > belongs in privacy.services We also download the list of trackers if privacy.trackingprotection.annotate_channels is enabled. If both are disabled, then the list is not downloaded.
Hm, well browser.privacy.services.* is primarily about features that cause the browser to access external services. tracking protection is one of those but the actual features is also obviously closely related to privacy, so I'm not sure what the right place is...
Comment on attachment 8845424 [details] Bug 1345158 - Implement browser.privacy.trackingProtection API https://reviewboard.mozilla.org/r/118212/#review124984 I can't speak for the correctness of the Web Extension code, but as far as I can tell, you are using the prefs correctly and the API makes sense to me.
(In reply to Andrew Swan [:aswan] from comment #6) > Comment on attachment 8845424 [details] > Bug 1345158 - Implement browser.privacy.trackingProtection API > > https://reviewboard.mozilla.org/r/118212/#review121130 > > Without having looked closely at the code, I have two questions about the > design: > 1. Why have the private browsing setting embedded in the value when we have > SettingScope > (http://searchfox.org/mozilla-central/rev/ > b035220d0f939559f4f8cf7b9079bc12f6adfc35/toolkit/components/extensions/ > schemas/types.json#11) We could try to use SettingScope for this, but I am concerned that we'd only be supporting part of it. Maybe that's okay, but I am concerned it would just cause confusion for developers. We don't currently support SettingScope for any of the privacy settings, so using it for just this one may cause confusion. Also, we could map what's currently being called "enabled_globally" to "regular", and we could map what's currently being called "enabled_private_browsing_only" to "incognito_persistent", but there would be inconsistencies with how SettingScope is meant to work. For example, in Chrome, you can use SettingScope to set different values for regular and incognito_persistent scopes. For example you could set regular to enabled and incognito_persistent to disabled, but that wouldn't be possible in our scenario, because if "privacy.trackingprotection.enabled" is set to true, the value of "privacy.trackingprotection.pbmode.enabled" is ignored. I.e., you cannot enable tracking protection for just regular windows and not private windows. For the same reason the SettingScope of "regular_only" wouldn't be possible. So we'd be sort of supporting SettingScope, partially, but, as I think I have illustrated, it would be confusing. It seems to me that it's much simpler to provide the three options as specific settings for browser.privacy.tracking.trackingProtectionEnabled. (Note that I changed it from browser.privacy.tracking.trackingProtection to browser.privacy.tracking.trackingProtectionEnabled, which I think makes more sense as a setting name). > 2. Maybe this is a question for francois, does disabling tracking protection > stop us from downloading ths list of trackers? If so, I think this really > belongs in privacy.services I gather from the conversation above, and François' final comment, that I should go ahead with the privacy.tracking namespace that he suggested.
Mark has started working on a more extensive tracking protection API and has some ideas for naming that I think may be better than what I've proposed, so let's just put this on the back burner for now. If Mark ends up implementing it instead he can just take this patch and use most of the code therein.
When you say "back burner", do you mean not making Firefox 57?
(In reply to Mike Kaply [:mkaply] from comment #14) > When you say "back burner", do you mean not making Firefox 57? No, that's not what I meant. I just meant I won't work on it anymore for now, and Mark can take over. I also removed aswan's r? This is a pretty simple change, and most of the code has been written, so I see no reason why it cannot be implemented prior to 57. At this point I think that's somewhat dependent on Mark's availability to work on it among his other priorities.
Is this dependent on the other experiment? As this will block the current behaviour of the lightbeam extension launching pre/on 57.
(In reply to Jonathan Kingston [:jkt] from comment #17) > Is this dependent on the other experiment? As this will block the current > behaviour of the lightbeam extension launching pre/on 57. Do you mean https://github.com/mstriemer/tracking-protection-experiment? I see no reason why this has to depend on that. The two are unrelated except for the fact that they both involve tracking protection. Mark, do you have plans to implement this privacy setting independent of the work being done on the experiment?
Yeah that experiment. Worst case scenario we can remove the functionality within the extension for the time being.
You're looking for bug 1342265, right? The plan for the experiment was to collect feedback for now and likely look at implementing something post 57. I've poked around in the C++ code that handles tracking protection but haven't written any C++ since university so I'm not confident I'd be able to sneak a patch in exposing this in short order. If you just need to know if something is considered a tracker, you could duplicate that logic. The experiment has an extension included with how the code would look without the experiment . It isn't a whole lot more code but it did show that the experiment has a moderate performance benefit even without hooking into C++ code. You'd include the Disconnect block list and Mozilla's public suffix list then you can check if a request is third party (public suffix list is used for this) and in the Disconnect block list. This is essentially what Tracking Protection does. It isn't perfect but should allow you to keep the functionality until we can expose this through webRequest. Would that work in the interim?  https://github.com/mstriemer/tracking-protection-experiment/blob/master/webRequest-extension/background.js