Closed Bug 737600 Opened 8 years ago Closed 7 years ago

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

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: tchevalier, Assigned: tchevalier)

References

Details

(Whiteboard: [telemetry][mozfr])

Attachments

(2 files, 11 obsolete files)

1.28 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
11.44 KB, patch
tchevalier
: review+
mak
: review+
Details | Diff | Splinter Review
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).
Whiteboard: [telemetry]
Assignee: nobody → theo.chevalier11
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Blocks: 731433
Attached patch Patch V1 (mobile) (obsolete) — Splinter Review
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.
Attached patch Patch V1 (desktop) (obsolete) — Splinter Review
In fact, toggle telemetry.rejected is ok, because we use telemetry.enabled value and we can trust it regarding telemetry state.
Attached patch Patch V2 (desktop) (obsolete) — Splinter Review
Attachment #673566 - Attachment is obsolete: true
Attachment #673618 - Attachment is obsolete: true
Attached patch Patch V2 (mobile) (obsolete) — Splinter Review
Attachment #676022 - Flags: review?(gavin.sharp)
Blocks: 699806
Attachment #676023 - Flags: review?(mark.finkle)
Attachment #676022 - Flags: review?(gavin.sharp) → review?(mak77)
Blocks: 737596
Comment on attachment 676023 [details] [diff] [review]
Patch V2 (mobile)

passing to Brian, who is a good reviewer for these changes.
Attachment #676023 - Flags: review?(mark.finkle) → review?(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?
Attachment #676023 - Flags: review?(bnicholson) → review-
(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.
(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).
Attached patch Patch V3 (obsolete) — Splinter Review
s/this._PREF_TELEMETRY_ENABLED/pref.value
Attachment #676023 - Attachment is obsolete: true
Attachment #682397 - Flags: review?(bnicholson)
Comment on attachment 682397 [details] [diff] [review]
Patch V3

># HG changeset patch
># Parent 477400ee249decd2f155b13c28a784804ba557ad
># User Theo Chevalier <theo.chevalier11@gmail.com>
>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.
Attachment #682397 - Flags: review?(bnicholson) → review-
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
Attachment #676022 - Flags: review?(mak77)
Attached patch Patch V3 (desktop) (obsolete) — Splinter Review
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.
Attachment #676022 - Attachment is obsolete: true
Attachment #685844 - Flags: review?(mak77)
Attachment #682397 - Attachment is obsolete: true
Attachment #685845 - Flags: review?(bnicholson)
Attachment #685845 - Flags: review?(bnicholson) → review+
Attached patch Patch V4 (obsolete) — Splinter Review
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)
Attachment #685844 - Attachment is obsolete: true
Attachment #685844 - Flags: review?(mak77)
Attachment #687514 - Flags: review?(mak77)
(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.
(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.
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.
Attached patch Patch V3 rebased (desktop) (obsolete) — Splinter Review
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?
Attachment #687514 - Attachment is obsolete: true
Attachment #687514 - Flags: review?(mak77)
Flags: needinfo?(theo.chevalier11)
Attached patch Patch V3 rebased (desktop) (obsolete) — Splinter Review
I actually wanted to attach this 8-lines-context version.
Attachment #688057 - Attachment is obsolete: true
Attached patch Patch V4 (obsolete) — Splinter Review
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?
Attachment #688059 - Attachment is obsolete: true
Flags: needinfo?(theo.chevalier11) → needinfo?(mak77)
(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.
Flags: needinfo?(mak77)
(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?
(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 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.
Attached patch Patch V5 (desktop) (obsolete) — Splinter Review
Attachment #688377 - Attachment is obsolete: true
Attachment #689121 - Flags: review?(mak77)
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
Attachment #689121 - Flags: review?(mak77) → review+
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.
updated to use <preference> elements and re-splitted against bug 699806.
Attachment #689121 - Attachment is obsolete: true
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.
Attachment #690021 - Flags: review?(theo.chevalier11)
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
Attachment #690021 - Flags: review+
Comment on attachment 690021 [details] [diff] [review]
patch V6 (desktop)

Ok here, too
Attachment #690021 - Flags: review?(theo.chevalier11) → review+
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.
Wrong link, sorry.
Desktop patch V6 pushed to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/b19ebbfd9ce4
Blocks: 819732
https://hg.mozilla.org/mozilla-central/rev/b19ebbfd9ce4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Mobile Patch V4 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4bdc7fc6c6
Whiteboard: [telemetry] → [telemetry][mozfr]
You need to log in before you can comment on or make changes to this bug.