737600 - When telemetry is disabled in the pref pane, we should update toolkit.telemetry.rejected

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Théo Chevalier]

This bug is part of bug 699806. When a user disable telemetry in the pref pane, we should update toolkit.telemetry.rejected to record that he have made a choice. We have to do this for desktop and mobile (native).

Comment 1

[Théo Chevalier]

Attachment: Patch V1 (mobile)

About using:
 Services.prefs.getBoolPref(this._PREF_TELEMETRY_ENABLED)); 
instead of:
 !Services.prefs.getBoolPref(this._PREF_TELEMETRY_ENABLED)); 

It seems that the observer is called right *before* _PREF_TELEMETRY_ENABLED is toggled.

Comment 2

[Théo Chevalier]

Attachment: Patch V1 (desktop)

In fact, toggle telemetry.rejected is ok, because we use telemetry.enabled value and we can trust it regarding telemetry state.

Comment 3

[Théo Chevalier]

Attachment: Patch V2 (desktop)

Comment 4

[Théo Chevalier]

Attachment: Patch V2 (mobile)

Comment 5

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

Comment on attachment 676023 [details] [diff] [review]
Patch V2 (mobile)

passing to Brian, who is a good reviewer for these changes.

Comment 6

[Brian Nicholson (:bnicholson)]

Comment on attachment 676023 [details] [diff] [review]
Patch V2 (mobile)

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

::: mobile/android/chrome/content/browser.js
@@ +6886,3 @@
>          Services.prefs.setIntPref(this._PREF_TELEMETRY_DISPLAYED, this._TELEMETRY_DISPLAY_REV);
> +        Services.prefs.setBoolPref(this._PREF_TELEMETRY_REJECTED,
> +                                   Services.prefs.getBoolPref(this._PREF_TELEMETRY_ENABLED));

We have the pref value from the JSON parse, so you should be able to set PREF_TELEMETRY_REJECTED to pref.value instead of using getBoolPref(). Also, shouldn't this be set to the opposite of enabled like in the desktop patch?

Comment 7

[Théo Chevalier]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> Comment on attachment 676023 [details] [diff] [review]
> Patch V2 (mobile)
> 
> Review of attachment 676023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +6886,3 @@
> >          Services.prefs.setIntPref(this._PREF_TELEMETRY_DISPLAYED, this._TELEMETRY_DISPLAY_REV);
> > +        Services.prefs.setBoolPref(this._PREF_TELEMETRY_REJECTED,
> > +                                   Services.prefs.getBoolPref(this._PREF_TELEMETRY_ENABLED));
> 
> We have the pref value from the JSON parse, so you should be able to set
> PREF_TELEMETRY_REJECTED to pref.value instead of using getBoolPref(). Also,

Will the JSON parse pref be changed when we toggle multiple times? I will test, but if it's the case, indeed it avoid getting the pref for nothing.

> shouldn't this be set to the opposite of enabled like in the desktop patch?

Like I said in comment 1, it seems that at this point the pref is not toggled yet (I checked on a build, we got the value we want). On desktop, the pref is already toggled when we do this.

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Théo Chevalier [:tchevalier] from comment #7)
> Will the JSON parse pref be changed when we toggle multiple times? I will
> test, but if it's the case, indeed it avoid getting the pref for nothing.

The value should always match the state of the checkbox when the pref is set, so yes.

> > shouldn't this be set to the opposite of enabled like in the desktop patch?
> 
> Like I said in comment 1, it seems that at this point the pref is not
> toggled yet (I checked on a build, we got the value we want). On desktop,
> the pref is already toggled when we do this.

That's because we set the pref in a different observer callback here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1112

It's a bit racy to assume the execution order of these callbacks; that's why it's safest to use the value that's given directly to the observer (which is pref.value).

Comment 9

[Théo Chevalier]

Attachment: Patch V3

s/this._PREF_TELEMETRY_ENABLED/pref.value

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 682397 [details] [diff] [review]
Patch V3

># HG changeset patch
># Parent 477400ee249decd2f155b13c28a784804ba557ad
># User Theo Chevalier <[e-mail]>
>Bug 737600 - When telemetry is disabled in the pref pane, we should update toolkit.telemetry.rejected
>
>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>--- a/mobile/android/chrome/content/browser.js
>+++ b/mobile/android/chrome/content/browser.js
>         Services.prefs.setIntPref(this._PREF_TELEMETRY_DISPLAYED, this._TELEMETRY_DISPLAY_REV);
>+        Services.prefs.setBoolPref(this._PREF_TELEMETRY_REJECTED,
>+                                   Services.prefs.getBoolPref(pref.value));
>+      }

pref.value should be the actual boolean value of the pref, so it doesn't make sense to call Services.prefs.getBoolPref() on it. You should just be able to do this, right?:

    Services.prefs.setBoolPref(this._PREF_TELEMETRY_REJECTED, !pref.value);

As I said in comment 8, pref.value should always match the state of the checkbox when the pref is set. So if the user just enabled the telemetry preference, this will set rejected to false - and vice versa.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 676022 [details] [diff] [review]
Patch V2 (desktop)

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

I hope I got this correct logic-wise, since there appear to be edge cases that we decided to not handle.
If something is unclear, before getting mad at it, feel free to immediately needinfo me.

::: browser/components/nsBrowserGlue.js
@@ +2,1 @@
>  # -*- indent-tabs-mode: nil -*-

Doesn't look like this patch makes any change in nsBrowserGlue, so why is this needed?
Maybe you just moved this to the wrong patch in the queue?

::: browser/components/preferences/advanced.js
@@ +169,5 @@
> +   /**
> +   * When Telemetry is enabled by default, check if it have not been rejected
> +   * on not enabled by default builds
> +   */
> +  initTelemetry: function ()

I'd like something more clear

"When telemetry is opt-out, verify if the user explicitly rejected the telemetry prompt, and if so reflect his choice in the current preference value.
This doesn't cover the case where the user refused telemetry in the prompt but later enabled it in preferences (when we were not yet setting the rejected value). Though, as far as we only run this adjustment for opt-out builds, that's a minor loss of reports VS a major privacy hit."

If I got this correctly, this is sort of a value check, we don't want to have enabled and rejected true at the same time, so we should exit from this method with a properly fixed situation.
Thus I think we should set the enabled pref according to that and then just use the new value.

So, to sum up, I think we want to check if both enabled and rejected are true and, if so, set enabled pref to false. The checkbox should adapt to the pref change automatically, and onchange handler should also be invoked when that happens. This should be the only case we want to handle explicitly, doing something in all cases doesn't seem necessary.

the whole method could also be #ifdef MOZ_TELEMETRY_ON_BY_DEFAULT

@@ +193,5 @@
> +    var rejected = false;
> +    try {
> +      rejected = Services.prefs.getBoolPref(this._PREF_TELEMETRY_REJECTED);
> +    } catch (e) {}
> +    Services.prefs.setBoolPref(this._PREF_TELEMETRY_REJECTED, !enabled);

looks like you are reading rejected but not using it, indeed we likely don't need its current value.

also, afaict onchange fires before the pref is written (just tried with some reportError here), so here you are basing the rejected value on the old enabled value, not on the new choice done by the user.
I think you should use the checkbox .checked value

@@ +195,5 @@
> +      rejected = Services.prefs.getBoolPref(this._PREF_TELEMETRY_REJECTED);
> +    } catch (e) {}
> +    Services.prefs.setBoolPref(this._PREF_TELEMETRY_REJECTED, !enabled);
> +    Services.prefs.setIntPref(this._PREF_TELEMETRY_DISPLAYED,
> +                                this._TELEMETRY_DISPLAY_REV);

wrong indentation

::: browser/components/preferences/advanced.xul
@@ +56,5 @@
>                    name="toolkit.telemetry.enabledByDefault"
>  #else
>                    name="toolkit.telemetry.enabled"
>  #endif
> +                  onchange="gAdvancedPane.telemetryChanged();"

the handler should be more specific like telemetryEnabledChanged() since we may add other telemetry preferences in future (unlikely, but can't tell)

::: browser/components/preferences/in-content/advanced.js
@@ +1,1 @@
> +#filter substitution

basically the same comments apply

Comment 12

[Théo Chevalier]

Attachment: Patch V3 (desktop)

1. Set telemetry.enabled && telemetry.rejected to true.
2. Open about:telemetry.
What happens:
telemetry.enabled is set to false
button on about:telemetry is updated (saying telemetry is disabled)
checkbox on pref pane is unchecked

And vice-versa when we uncheck the checkbox from pref pane. Except that when you do that from the pref pane, telemetry.rejected is set to false too.
We want to keep it to true.

Comment 13

[Théo Chevalier]

Attachment: Patch V4 (mobile)

Comment 14

[Théo Chevalier]

Attachment: Patch V4

Problem solved, I replaced the OnChange with an EventListener.

Ready for review !

(Btw that was the last blocking part, so we should be ready now)

Comment 15

[Marco Bonardo [:mak]]

(In reply to Théo Chevalier [:tchevalier] from comment #12)
> Created attachment 685844 [details] [diff] [review]
> Patch V3 (desktop)
> 
> 1. Set telemetry.enabled && telemetry.rejected to true.
> 2. Open about:telemetry.
> What happens:
> telemetry.enabled is set to false
> button on about:telemetry is updated (saying telemetry is disabled)
> checkbox on pref pane is unchecked
> 
> And vice-versa when we uncheck the checkbox from pref pane. Except that when
> you do that from the pref pane, telemetry.rejected is set to false too.
> We want to keep it to true.

Could you give me better steps here, 1 and 2 is not enough to reproduce the issue, I have difficulties understanding the interaction of the pref pane with about:telemetry and viceversa. Do I have to keep them open at the same time?

aboutTelemetry.js changes look wrong, but before commenting on that I'd like to reproduce the issue you are pointing in comment 12.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [:mak] from comment #15)
> aboutTelemetry.js changes look wrong

well, not exactly wrong, the problem is that the patches here and in bug 699806 are intermixed and have reciprocal connections (Something that should be here is there and viceversa), so this patch is missing pieces. I think I'll re-split them for you.

Comment 17

[Marco Bonardo [:mak]]

I actually think I may have misread your onchange handler thinking it was on the checkbox, rather than on the preference, so V4 may not be needed. Indeed we should not use .checked in a preference change handler. Testing locally, likely my fault.

Comment 18

[Marco Bonardo [:mak]]

Attachment: Patch V3 rebased (desktop)

So, this is the rebased split patch. This patch can be applied before bug 699806.

Apart moving changes across the 2 patches (this and bug 699806) that will help me reviewing, I only fixed a couple comments and replaced checkbox.checked with event.target.value in the change handler, compared to V3. This should fix the problem in comment 12, AFAICT.

Theo, could you please retest with these patches?

Comment 19

[Marco Bonardo [:mak]]

Attachment: Patch V3 rebased (desktop)

I actually wanted to attach this 8-lines-context version.

Comment 20

[Théo Chevalier]

Attachment: Patch V4

As we discussed over IRC, I used the pref event listener to update rejected & rev, and now the click event listener only update enabled.

I tried a build, it do what we want.

I have a last concern, since now we have event listeners everywhere, if about:telemetry, or a pref pane is open, when you change enabled from about:config, rejected is dynamically updated (because of telemetry.enabled listeners currently running on about:telemetry or a pref pane), but, when you only have about:config opened (so, when about:telemetry or the pref panes are not open) there is no event listener running so in that case you can change enabled from about:config and rejected & rev will NOT be updated dynamically.

In a nut shell, you can set enabled and rejected values from about:config to whatever you want only when about:telemetry or pref pane are closed.

Do you see what I mean?
This is an edge case, but should we add an event listener on about:config or we don't really care about that particular case?

Comment 21

[Marco Bonardo [:mak]]

(In reply to Théo Chevalier [:tchevalier] from comment #20)
> when
> you only have about:config opened (so, when about:telemetry or the pref
> panes are not open) there is no event listener running so in that case you
> can change enabled from about:config and rejected & rev will NOT be updated
> dynamically.

we can't prevent the users from shooting themselves in the foot, about:config case is an edge case, we handle it when some other panel handling the pref is open cause otherwise the UI would be not consistent, but if the user goes over our warning and plays with about:config values, we are not really responsible of unwanted damage.

Comment 22

[Théo Chevalier]

(In reply to Marco Bonardo [:mak] from comment #21)
> (In reply to Théo Chevalier [:tchevalier] from comment #20)
> > when
> > you only have about:config opened (so, when about:telemetry or the pref
> > panes are not open) there is no event listener running so in that case you
> > can change enabled from about:config and rejected & rev will NOT be updated
> > dynamically.
> 
> we can't prevent the users from shooting themselves in the foot,
> about:config case is an edge case, we handle it when some other panel
> handling the pref is open cause otherwise the UI would be not consistent,
> but if the user goes over our warning and plays with about:config values, we
> are not really responsible of unwanted damage.

Allright, thanks for the fast anwser!
So, if you don't see anything else, I think it's ready?

Comment 23

[Marco Bonardo [:mak]]

(In reply to Théo Chevalier [:tchevalier] from comment #22)
> So, if you don't see anything else, I think it's ready?

May be, just needs final review pass (I'm going to do some testing as well). Will let you know shortly.

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 688377 [details] [diff] [review]
Patch V4

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

Just a couple minor adjustements needed, then I think we're ready here.

::: toolkit/content/aboutTelemetry.js
@@ +642,5 @@
>        let value = getPref(PREF_TELEMETRY_ENABLED, false);
>        Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, !value);
> +      Services.prefs.setBoolPref(PREF_TELEMETRY_REJECTED, value);
> +      Services.prefs.setIntPref(PREF_TELEMETRY_DISPLAYED,
> +                                TELEMETRY_DISPLAY_REV);

We don't need to set rejected and displayed here (the click handler) anymore, since when changing the pref the pref handler is invoked and updates them.

@@ +692,2 @@
>    // Set up event listeners
>    setupListeners();

the special code setting enabled should move just after setupListeners(), so the pref change handler will take care of setting rejected (will be a no-op at this point) and display prefs.

Comment 25

[Théo Chevalier]

Attachment: Patch V5 (desktop)

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 689121 [details] [diff] [review]
Patch V5 (desktop)

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

looks good!
Please wait to land until we also have a reviewed final patch for bug 699806

Comment 27

[Marco Bonardo [:mak]]

So, there's an issue here with the onchange handler, it works fine in in-content prefs since they are instantApply, but in the common preferences dialog the prefs can go out of sync when you Cancel.
I think the best solution for both cases is to add <preference> for both the prefs we have to set and let the panel write them when "enabled" is written. This should work correctly in both panes (dialog and in-content). I have updated the patch and I'm testing it.

Comment 28

[Marco Bonardo [:mak]]

Attachment: patch V6 (desktop)

updated to use <preference> elements and re-splitted against bug 699806.

Comment 29

[Marco Bonardo [:mak]]

Comment on attachment 690021 [details] [diff] [review]
patch V6 (desktop)

Theo, could you please double check my changes here?
Note I moved all the TELEMETRY_ON_BY_DEFAULT stuff to the patch in bug 699806 to properly split the issues.

Comment 30

[Marco Bonardo [:mak]]

Comment on attachment 690021 [details] [diff] [review]
patch V6 (desktop)

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

my testing was positive, once your testing is complete I think this can be pushed

Comment 31

[Théo Chevalier]

Comment on attachment 690021 [details] [diff] [review]
patch V6 (desktop)

Ok here, too

Comment 32

[Théo Chevalier]

Desktop patch V6 just pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14a9d4f5d16

Please leave this bug open until mobile patch is pushed.

Comment 33

[Théo Chevalier]

Wrong link, sorry.
Desktop patch V6 pushed to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/b19ebbfd9ce4

Comment 34

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/b19ebbfd9ce4

Comment 35

[Théo Chevalier]

Mobile Patch V4 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4bdc7fc6c6

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4c4bdc7fc6c6