1185163 - Create tri-state tracking protection pref

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

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?

Comment 1

[:Margaret Leibovic]

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

Comment 2

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a943705263

Comment 3

[:Margaret Leibovic]

Attachment: Screenshot: privacy settings

Comment 4

[:Margaret Leibovic]

Attachment: Screenshot: tracking protection options

Comment 5

[:Margaret Leibovic]

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.

Comment 6

[Barbara Bermes [:barbara] - NI please!]

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.

Comment 7

[Chenxia Liu [:liuche]]

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?

Comment 8

[:Margaret Leibovic]

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

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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

Comment 10

[:Margaret Leibovic]

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

Comment 11

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cf81e801222

Comment 12

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cf81e801222

Uh, I wonder how this is passing...
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testTrackingProtection.js#155

Comment 13

[:Margaret Leibovic]

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?

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/617a31bdd44cdffe594db6e21d5a5de06e9edbc7
Bug 1185163 - Create tri-state tracking protection pref (for Nightly only). r=liuche

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/617a31bdd44c

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 17

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

Comment 18

[Teodora Vermesan (:TeoVermesan)]

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.

Comment 19

[: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?

Comment 20

[Teodora Vermesan (:TeoVermesan)]

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.

Comment 21

[:Margaret Leibovic]

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

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

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