Closed Bug 1185163 Opened 6 years ago Closed 6 years ago

Create tri-state tracking protection pref

Categories

(Firefox for Android Graveyard :: Settings and Preferences, defect)

35 Branch
defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: Margaret, Assigned: Margaret)

Details

Attachments

(3 files)

I think we should turn our user-facing tracking protection pref into a tri-state pref:
* Tracking protection enabled everywhere
* Tracking protection enabled only in private browsing
* Tracking protection disabled

The default would be the second choice, but we would be giving users an option to upgrade their tracking protection.

Before bug 1175969, we exposed this pref in Nightly, so at the very least, I think we should land this tri-state pref in Nightly, but ideally, I would like to see this ride the trains, at least to beta.

Barbara, what do you think? Can you own figuring out the product decision around shipping this feature?
Flags: needinfo?(bbermes)
Bug 1185163 - Create tri-state tracking protection pref (for Nightly only). r=liuche
Attachment #8635610 - Flags: review?(liuche)
I wrote this patch to make this UI only appear on Nightly builds, but my one concern with that is that we won't get Nightly testers on the checkbox UI, so we'd be waiting until Aurora for that to get some more testing.
I like the idea of having the option to enable TP for non-private browsing (I actually emailed about this earlier today).

So if there is nothing too specific about this from an eng effort, we should move this to Nightly.

I'll confirm with Karen & Javaun about their plans around non-private browsing and TP on Desktop.
Flags: needinfo?(bbermes)
Comment on attachment 8635610 [details]
MozReview Request: Bug 1185163 - Create tri-state tracking protection pref (for Nightly only). r=liuche

https://reviewboard.mozilla.org/r/13577/#review12187

Ship It!

::: mobile/android/base/resources/values/arrays.xml:53
(Diff revision 1)
> +    <string-array name="pref_tp_values">

We should just name this pref_trackingprotection_values - let's be explicit.

::: mobile/android/base/strings.xml.in:200
(Diff revision 1)
> +  <string name="pref_tp_enabled">&pref_tp_enabled;</string>

Nit: We use pref_tracking_protection_title and summary right above this, so we might as well use the full string (pref_tracking_protection_enabled) here too.

::: mobile/android/base/locales/en-US/android_strings.dtd:203
(Diff revision 1)
> +<!ENTITY pref_tp_enabled_pb "Actively block tracking elements in Private Browsing">

Do we need "Actively"? That starts making this string very long (which will likely be a problem for certain localizations). Maybe this could be "Block tracking elements" and "Block tracking elements only in Private Browsing".

::: mobile/android/chrome/content/browser.js:1560
(Diff revision 1)
> +          case "0":

Do we prefer using strings here for some reason? I think it's a little confusing that it's string representations of numbers, but not a huge deal.

::: mobile/android/chrome/content/browser.js:1566
(Diff revision 1)
> +            Services.prefs.clearUserPref("privacy.trackingprotection.pbmode.enabled");

This is fine right now, but should we be setting this value explicitly to true or false, rather than assuming what the default value is going to be?
Attachment #8635610 - Flags: review?(liuche) → review+
(In reply to Chenxia Liu [:liuche] from comment #7)

> ::: mobile/android/base/locales/en-US/android_strings.dtd:203
> (Diff revision 1)
> > +<!ENTITY pref_tp_enabled_pb "Actively block tracking elements in Private Browsing">
> 
> Do we need "Actively"? That starts making this string very long (which will
> likely be a problem for certain localizations). Maybe this could be "Block
> tracking elements" and "Block tracking elements only in Private Browsing".

Good point.

antlam, can you help me with strings here? I'm not in a huge rush to land this, but it would be good to try to get this in before the merge, in case we do for some reason want to enable this for 42.

> ::: mobile/android/chrome/content/browser.js:1560
> (Diff revision 1)
> > +          case "0":
> 
> Do we prefer using strings here for some reason? I think it's a little
> confusing that it's string representations of numbers, but not a huge deal.

It's not that we prefer using strings, it's that we get strings back from Java. But... I was probably lazy here and could try to make this better. It would probably be best to pull these out into constants.

> ::: mobile/android/chrome/content/browser.js:1566
> (Diff revision 1)
> > +            Services.prefs.clearUserPref("privacy.trackingprotection.pbmode.enabled");
> 
> This is fine right now, but should we be setting this value explicitly to
> true or false, rather than assuming what the default value is going to be?

Hm... yeah, we should probably do that. Good call.
Flags: needinfo?(alam)
Matej OK'd that copy there in bug 1174242 I believe, so, keeping the current subtitle and title in Settings would be nice. 

As we think about moving this beyond Nightly, we might want to get a final pass from him as well.

For now, on press we could do:

Enabled
Enabled in Private Browsing
Disabled

(I'm taking a page from the copy in our "Cookies" Settings)

Note: Can we change the subtitle strings to reflect the chosen setting as well? "Actively block tracking elements", "Actively block tracking elements in Private Browsing", "Disabled" :D
Flags: needinfo?(alam)
Comment on attachment 8635610 [details]
MozReview Request: Bug 1185163 - Create tri-state tracking protection pref (for Nightly only). r=liuche

Bug 1185163 - Create tri-state tracking protection pref (for Nightly only). r=liuche
Barbara, I noticed that desktop Firefox has a global tracking protection pref available in the user-visible settings on Nightly, so I'd like to land this for us on Nightly as well. Ideally I'd like to ship this on beta or release, but first things first :) What do you think?
Flags: needinfo?(bbermes)
https://hg.mozilla.org/integration/fx-team/rev/617a31bdd44cdffe594db6e21d5a5de06e9edbc7
Bug 1185163 - Create tri-state tracking protection pref (for Nightly only). r=liuche
https://hg.mozilla.org/mozilla-central/rev/617a31bdd44c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
When using a restricted profile "private browsing" is disabled/hidden by default, but the setting shows up defaulting to "Enabled in Private Browsing". Should I file a follow-up bug?
Flags: needinfo?(margaret.leibovic)
(In reply to Sebastian Kaspari (:sebastian) from comment #16)
> When using a restricted profile "private browsing" is disabled/hidden by
> default, but the setting shows up defaulting to "Enabled in Private
> Browsing". Should I file a follow-up bug?

Is there a bug in my patch that exposes this pref to restricted profile users now? I would expect that we would hide this pref for restricted profiles. Sounds like we need a new bug.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(bbermes)
If I go to about:config and set:
privacy.trackingprotection.enabled=true
privacy.trackingprotection.pbmode.enabled=false

Then go to private browsing and load espn.go.com, the shield will be displayed in the URL Bar.
Tracking protection will appear as "Enabled" in Settings.
Flags: needinfo?(margaret.leibovic)
(In reply to Teodora Vermesan (:TeoVermesan) from comment #18)
> If I go to about:config and set:
> privacy.trackingprotection.enabled=true
> privacy.trackingprotection.pbmode.enabled=false
> 
> Then go to private browsing and load espn.go.com, the shield will be
> displayed in the URL Bar.
> Tracking protection will appear as "Enabled" in Settings.

That sounds correct, yes. Does this not match your expectations in some way?
Flags: needinfo?(margaret.leibovic) → needinfo?(teodora.vermesan)
I thought that if "privacy.trackingprotection.pbmode.enabled" pref is set to false, then the shield shouldn't be displayed in the URL Bar. But it appears and tracking protection is enabled in PB.
Flags: needinfo?(teodora.vermesan)
(In reply to Teodora Vermesan (:TeoVermesan) from comment #20)
> I thought that if "privacy.trackingprotection.pbmode.enabled" pref is set to
> false, then the shield shouldn't be displayed in the URL Bar. But it appears
> and tracking protection is enabled in PB.

The about:config pref names are implementation details. What should matter is what the user sees in settings. When the user sees "Tracking protection: Enabled", it is enabled everywhere (including PB).
(In reply to :Margaret Leibovic from comment #21)
> (In reply to Teodora Vermesan (:TeoVermesan) from comment #20)
> > I thought that if "privacy.trackingprotection.pbmode.enabled" pref is set to
> > false, then the shield shouldn't be displayed in the URL Bar. But it appears
> > and tracking protection is enabled in PB.
> 
> The about:config pref names are implementation details. What should matter
> is what the user sees in settings. When the user sees "Tracking protection:
> Enabled", it is enabled everywhere (including PB).

Perhaps we need the text to be more explicit: "Always enabled"
(In reply to :Margaret Leibovic from comment #17)
> Is there a bug in my patch that exposes this pref to restricted profile
> users now? I would expect that we would hide this pref for restricted
> profiles. Sounds like we need a new bug.

I created bug 1216116.
Flags: needinfo?(s.kaspari)
No longer depends on: 1216116
QA Contact: teodora.vermesan
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.