863242 - Add a preference setting "accept tracking me" to Fennec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Add a preference setting "accept tracking me" like Bug 854077 to Fennec for Android.

Comment 1

[:Margaret Leibovic]

Ian is in the middle of redesigning the structure of our settings page, so this may be something he'll want to include in his plans.

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v1

(In reply to :Margaret Leibovic from comment #1)
> Ian is in the middle of redesigning the structure of our settings page, so
> this may be something he'll want to include in his plans.

OK. But I attach the patch which works fine.

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v2

Rebased on latest-central.

Comment 4

[Ian Barlow (:ibarlow)]

Alright. So by the looks of it we should at least update our strings to be consistent with what desktop is doing, which means changing to this: 

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

---

Having said that, I was looking through the original desktop bug 765398 and there were some proposed strings that I feel were better, but never ended up getting used. Matej (our copywriter) and I are going to dig into that later in the week to see if we can find out why they were never added. 

So feel free to land the changes I suggested above, but be aware that we might need to file a followup bug to update them again later.

Comment 5

[:Margaret Leibovic]

Comment on attachment 740814 [details] [diff] [review]
patch v2

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

Thanks for writing this patch, I asked ibarlow to offer some feedback on the strings used for this, and he should comment in the bug soon.

However, I also think we should change the way this is implemented to make it less confusing. I think we should model this after the "plugin.enable" preference, since that's also a tri-state preference in the UI, but really sets two seprate preferences. Some of the code has changed, but this is the original patch that implemented that:
http://hg.mozilla.org/mozilla-central/rev/41ceaff2e5f1

::: mobile/android/base/resources/values/arrays.xml
@@ +54,5 @@
> +    </string-array>
> +    <string-array name="pref_donottrack_values">
> +        <item>not_tell_anythings</item>
> +        <item>accept</item>
> +        <item>not_accept</item>

Following what I mentioned before, I think we should just use 0/1/2, similar to pref_plugins_values.

::: mobile/android/chrome/content/browser.js
@@ +954,5 @@
>            pref.value = MasterPassword.enabled;
>            prefs.push(pref);
>            continue;
> +        } else if (prefName === "privacy.donottrackheader.enabled" ||
> +                   prefName === "privacy.donottrackheader.value") {

Since these are real preference names with real preference values, they can be handled by the switch statement down below.

We should come up with a new unique id for our ListPreference, and check for that here. Something simple like "privacy.donottrackheader" would work, since that's not used for any pref.

Otherwise, if someone tries to set one of these real preferences from Java, it will be handled here, when it shouldn't be.

@@ +961,5 @@
> +          let enableDNT = Services.prefs.getBoolPref("privacy.donottrackheader.enabled");
> +          if (!enableDNT) {
> +            pref.value = "not_tell_anythings";
> +          }
> +          else {

Style nit: Put else on the same line as the close brace above.

@@ +1065,5 @@
>        return;
> +    } else if (json.name === "privacy.donottrackheader.enabled") {
> +      if (json.value === "not_tell_anythings") {
> +        // Don't tell anything about tracking me
> +        Services.prefs.setBoolPref("privacy.donottrackheader.enabled", false);

You should also clear the "privacy.donottrackheader.value" pref here.

@@ +1077,5 @@
> +        else if (json.value === "not_accept") {
> +          // Not accept tracking me
> +          Services.prefs.setIntPref("privacy.donottrackheader.value", 1);
> +        }
> +      }

I think it would be clearer to implement this as a switch statement like PluginHelper.setPluginPreference:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/PluginHelper.js#109

And it would also be good to add comments about what each state means.

Comment 6

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #5)

> Thanks for writing this patch, I asked ibarlow to offer some feedback on the
> strings used for this, and he should comment in the bug soon.

Looks like he already beat me!

Comment 7

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

(In reply to Ian Barlow (:ibarlow) from comment #4)
> Alright. So by the looks of it we should at least update our strings to be
> consistent with what desktop is doing, which means changing to this: 
> 
> Tracking
> * Tell sites that I do not want to be tracked
> * Tell sites that I want to be tracked
> * Do not tell sites anything about my tracking preferences
> 
> ---
> 
> Having said that, I was looking through the original desktop bug 765398 and
> there were some proposed strings that I feel were better, but never ended up
> getting used. Matej (our copywriter) and I are going to dig into that later
> in the week to see if we can find out why they were never added. 
> 
> So feel free to land the changes I suggested above, but be aware that we
> might need to file a followup bug to update them again later.

When looking at string candidates, consider the lack of space and l10n.

Comment 8

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #7)

> When looking at string candidates, consider the lack of space and l10n.

Thanks, good point. Related question: does our Settings UI allow strings to wrap? Or do we only get one line?

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to :Margaret Leibovic from comment #5)
> Thanks for writing this patch, I asked ibarlow to offer some feedback on the
> strings used for this, and he should comment in the bug soon.

Thank you for your speedy review. I reply about some points I concerned.


> However, I also think we should change the way this is implemented to make
> it less confusing. I think we should model this after the "plugin.enable"
> preference, since that's also a tri-state preference in the UI, but really
> sets two seprate preferences. Some of the code has changed, but this is the
> original patch that implemented that:
> http://hg.mozilla.org/mozilla-central/rev/41ceaff2e5f1

Thank you. I'll refer it.


> ::: mobile/android/base/resources/values/arrays.xml
> @@ +54,5 @@
> > +    </string-array>
> > +    <string-array name="pref_donottrack_values">
> > +        <item>not_tell_anythings</item>
> > +        <item>accept</item>
> > +        <item>not_accept</item>
> 
> Following what I mentioned before, I think we should just use 0/1/2, similar
> to pref_plugins_values.

Umm... We add some magic number if we just use 0/1/2. Should we resolve it with adding comments which describes their magic number?


> @@ +1077,5 @@
> > +        else if (json.value === "not_accept") {
> > +          // Not accept tracking me
> > +          Services.prefs.setIntPref("privacy.donottrackheader.value", 1);
> > +        }
> > +      }
> 
> I think it would be clearer to implement this as a switch statement like
> PluginHelper.setPluginPreference:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> PluginHelper.js#109
> 
> And it would also be good to add comments about what each state means.

I tried to use a switch statements on this part. But if we use a switch statements, there are some duplicate parts like this:

switch (json.value) {
  case "not_tell_anythings":
    // Don't tell anything about tracking me
    Services.prefs.setBoolPref("privacy.donottrackheader.enabled", false);
    break;
  case "accept": {
    Services.prefs.setBoolPref("privacy.donottrackheader.enabled", true);
    Services.prefs.setIntPref("privacy.donottrackheader.value", 0);
    break;
  case "not_accept":
    Services.prefs.setBoolPref("privacy.donottrackheader.enabled", true);
    Services.prefs.setIntPref("privacy.donottrackheader.value", 1);
    break;
}

How about do you think? I thought this is wordy a little...

Comment 10

[:Margaret Leibovic]

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #9)

> > ::: mobile/android/base/resources/values/arrays.xml
> > @@ +54,5 @@
> > > +    </string-array>
> > > +    <string-array name="pref_donottrack_values">
> > > +        <item>not_tell_anythings</item>
> > > +        <item>accept</item>
> > > +        <item>not_accept</item>
> > 
> > Following what I mentioned before, I think we should just use 0/1/2, similar
> > to pref_plugins_values.
> 
> Umm... We add some magic number if we just use 0/1/2. Should we resolve it
> with adding comments which describes their magic number?

Yes, that's what we do for the plugin pref, and I was thinking it would be good to be consistent with that.

> 
> > @@ +1077,5 @@
> > > +        else if (json.value === "not_accept") {
> > > +          // Not accept tracking me
> > > +          Services.prefs.setIntPref("privacy.donottrackheader.value", 1);
> > > +        }
> > > +      }
> > 
> > I think it would be clearer to implement this as a switch statement like
> > PluginHelper.setPluginPreference:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> > PluginHelper.js#109
> > 
> > And it would also be good to add comments about what each state means.
> 
> I tried to use a switch statements on this part. But if we use a switch
> statements, there are some duplicate parts like this:
> 
> switch (json.value) {
>   case "not_tell_anythings":
>     // Don't tell anything about tracking me
>     Services.prefs.setBoolPref("privacy.donottrackheader.enabled", false);

(this is where you should also add a |Services.prefs.clearUserPref("privacy.donottrackheader.value");|)

>     break;
>   case "accept": {
>     Services.prefs.setBoolPref("privacy.donottrackheader.enabled", true);
>     Services.prefs.setIntPref("privacy.donottrackheader.value", 0);
>     break;
>   case "not_accept":
>     Services.prefs.setBoolPref("privacy.donottrackheader.enabled", true);
>     Services.prefs.setIntPref("privacy.donottrackheader.value", 1);
>     break;
> }
> 
> How about do you think? I thought this is wordy a little...

Yeah, we do have similar repetition in setPluginPreference, but really only one line is repeated. It may appear a little longer, but I think it's worth it for making it easier to understand exactly what each pref value is doing.

Comment 11

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part0 v1: Use switch statement

Clean up BrowserApp.getPreferences() for using switch statement.

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part1 v3: Update setting for DNT

This patch is main part.

Comment 13

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part1 v3.1: Update setting for DNT

I'm sorry. The old patch has some bug when initializing the setting menu.

Comment 14

[:Margaret Leibovic]

Comment on attachment 741899 [details] [diff] [review]
part1 v3.1: Update setting for DNT

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

This is looking better, but I still have some more concerns.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +84,5 @@
>  
> +<!ENTITY pref_donottrack_menu "Tell sites about to track me">
> +<!ENTITY pref_donottrack_no_pref "Don\'t tell anythings">
> +<!ENTITY pref_donottrack_accept_tracking "Accept to be tracked">
> +<!ENTITY pref_donottrack_not_accept_tracking "Don\'t want to be tracked">

You should use the strings that ibarlow specified in comment 4. It would also be good if we can share a screenshot with him to get feedback about whether or not those strings are too long.

::: mobile/android/base/resources/values/arrays.xml
@@ +54,5 @@
> +    </string-array>
> +    <string-array name="pref_donottrack_values">
> +        <item>0</item>
> +        <item>1</item>
> +        <item>2</item>

We should rearrange the ordering of these pref name/values to match what ibarlow said in comment 4.

::: mobile/android/chrome/content/browser.js
@@ +140,5 @@
>  
> +const kDoNotTrackPrefState = Object.freeze({
> +  NOT_TELL_ANYTHINGS: 0,
> +  ACCEPT: 1,
> +  NOT_ACCEPT: 2,

Instead of needing to make strings out of these values, then calling parseInt() to compare the values we get from JSON, let's just store these as strings (e.g. "0", "1", "2").

I also think we can come up with better names for these states. How about:
NO_PREF
ALLOW_TRACKING
DISALLOW_TRACKING

And then also use matching terms in the variable names for the strings/array values.

@@ +986,5 @@
> +           * this is passed when initializing the setting menu.
> +           */
> +          case "privacy.donottrackheader":
> +          case "privacy.donottrackheader.enabled":
> +          case "privacy.donottrackheader.value": {

I think you might have misunderstood my previous review comment. You shouldn't include "privacy.donottrackheader.enabled" or "privacy.donottrackheader.value" here. Because they are both normal prefs, they can just be handled by the regular pref handing switch statement down below.

@@ +987,5 @@
> +           */
> +          case "privacy.donottrackheader":
> +          case "privacy.donottrackheader.enabled":
> +          case "privacy.donottrackheader.value": {
> +            pref.name = "privacy.donottrackheader";

That means you also don't need to set pref.name here, since this code should only handle the "privacy.donottrackheader" pref.

@@ +1106,5 @@
> +          Services.prefs.setBoolPref("privacy.donottrackheader.enabled", false);
> +          Services.prefs.clearUserPref("privacy.donottrackheader.value");
> +          break;
> +        case kDoNotTrackPrefState.ACCEPT:
> +          // Don't tell anything about tracking me

This comment is incorrect. Probably just a copy/paste error :)

Comment 15

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part1 v3.2: Update setting for DNT

Update!

Comment 16

[:Margaret Leibovic]

Comment on attachment 741936 [details] [diff] [review]
part1 v3.2: Update setting for DNT

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

Thanks for addressing my comments. This looks good.

Ian, the strings do wrap, so it's okay that they're long, but we can update them in a follow-up bug if you think of better ones. Consistency with desktop is nice, though.

Comment 17

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #16)

> Ian, the strings do wrap, so it's okay that they're long, but we can update
> them in a follow-up bug if you think of better ones. Consistency with
> desktop is nice, though.

Sounds good. I still need to check in with Matej, so I'll post a follow up when I do.

Comment 18

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: screenshot of patch v3.2

Comment 19

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I uploaded the screenshot of patch part-1 v3.2. This might help Ian's label polishing :)

Comment 20

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Margaret, may I add the "checkin-needed" flag?

Comment 21

[:Margaret Leibovic]

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #20)
> Margaret, may I add the "checkin-needed" flag?

Yes, excellent work!

Comment 22

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54c62289325a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1d0b408e2c

Comment 23

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I clone this bug as bug 866208 for polishing the labels.

(In reply to :Margaret Leibovic from comment #21)
> (In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #20)
> > Margaret, may I add the "checkin-needed" flag?
> 
> Yes, excellent work!

Thank you very much ;)

Comment 24

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/54c62289325a
https://hg.mozilla.org/mozilla-central/rev/5c1d0b408e2c

Comment 25

[Teodora Vermesan (:TeoVermesan)]

Test Cases added in moztrap:
-https://moztrap.mozilla.org/manage/case/8078/
-https://moztrap.mozilla.org/manage/case/8079/