763420 - Telemetry doorhanger appears after checking the "Send performance data" in settings

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andreea Pod]

Build: Firefox 16.0a1 (2012-06-10)
Device: LG Optimus 2X (Android 2.2)

Steps to reproduce:
1. Create a new profile
2. Open the browser
3. After the telemetry doorhanger appears tap on back button to dismiss it
4. Go to settings and set to yes the "Send performance data" option
5. Restart the application using the menu > More > Quit button.

Expected results:
- doorhanger should not appear and the "Send performance data" option should be set on yes

Actual result:
- the telemetry prompt appears and the "Send performance data" option is not checked.

Comment 1

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

I'm not sure this is a bug. I think using the BACK button is not considered a real "no", so the next time the app has a chance it will show the prompt again. Using the "No" button is the explicit "no" action.

Comment 2

[Andreea Pod]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> I'm not sure this is a bug. I think using the BACK button is not considered
> a real "no", so the next time the app has a chance it will show the prompt
> again. Using the "No" button is the explicit "no" action.

Yes, you are right here Mark, but the doorhanger reappears at restart after I checked the "Send performance data" option in Fennec settings (meaning it is set on "Yes"), as I mentioned in the description.

Comment 3

[:Margaret Leibovic]

Looks like we decided whether or not to prompt purely on the "toolkit.telemetry.prompted" pref, and we only set that if the user explicitly taps a yes/no button [1]. I think it would probably be more correct to just set the pref as soon as we show the doorhanger, since it seems mostly designed to keep track of the version of the privacy message we've shown the user. If we really want to re-show the notification until the user makes a choice, we could just check to see if "toolkit.telemetry.enabled" or "toolkit.telemetry.rejected" were ever set.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#357

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Set telemetry prompted when telemetry preference changed

This patch does a small amount of refactoring to put the telemetry code in its own Telemetry class. When the telemetry enabled option is changed in the prefs screen, this acts as if a button on the prompt were clicked (in other words, the toolkit.telemetry.prompted pref is set).

I originally wanted to use standard pref observers to implement this, but that would conflict with our call to "Services.prefs.clearUserPref(this._PREF_TELEMETRY_ENABLED)".

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Set telemetry prompted when telemetry preference changed

Part of another patch got mixed up with this one; fixed.

Comment 6

[Brian Nicholson (:bnicholson)]

This code was taken from desktop, so I think the intended behavior is to repeatedly show the prompt until the user makes a decision. AFAIK, there's no desktop setting to disable telemetry like we have on mobile, which is why desktop isn't hitting this issue. We could change this to only prompt once since we do have our own pref in the UI, but we might see a decline in telemetry opt-ins since this defaults to "no".

Comment 7

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

Comment on attachment 631967 [details] [diff] [review]
Set telemetry prompted when telemetry preference changed

>+var Telemetry = {

>+  observe: function observe(aSubject, aTopic, aData) {
>+    } else if (aTopic == "Telemetry:Add") {
>+      let json = JSON.parse(aData);
>+      var telemetry = Cc["@mozilla.org/base/telemetry;1"]
>+            .getService(Ci.nsITelemetry);

no need to wrap

>+  showTelemetryPrompt: function showTelemetryPrompt() {

Let's rename to "prompt" since the extra "Telemetry" is redundant

Comment 8

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/dbf0598205ce

Comment 9

[Matt Brubeck (:mbrubeck)]

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

Comment 10

[Cristian Nicolae (:xti)]

This issue is not reproducible anymore on the latest Nightly build. Closing bug as verified fixed on:

Firefox 16.0a1 (2012-06-13)
Device: Galaxy Nexus
OS: Android 4.0.2

Comment 11

[microrffr]

Comment on attachment 631967 [details] [diff] [review]
Set telemetry prompted when telemetry preference changed

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

telemetry doorhanger buttons no longer work

::: mobile/android/chrome/content/browser.js
@@ +5321,5 @@
> +    let buttons = [
> +      {
> +        label: Strings.browser.GetStringFromName("telemetry.optin.yes"),
> +        callback: function () {
> +          Services.prefs.setIntPref(this._PREF_TELEMETRY_PROMPTED, this._TELEMETRY_PROMPT_REV);

wrong "this" in callback

Comment 12

[Aaron Train [:aaronmt]]

That explains why the buttons are not working on Nightly.

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: fix "this" references

(In reply to microrffr from comment #11)
> Comment on attachment 631967 [details] [diff] [review]
> Set telemetry prompted when telemetry preference changed
> 
> Review of attachment 631967 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> telemetry doorhanger buttons no longer work
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +5321,5 @@
> > +    let buttons = [
> > +      {
> > +        label: Strings.browser.GetStringFromName("telemetry.optin.yes"),
> > +        callback: function () {
> > +          Services.prefs.setIntPref(this._PREF_TELEMETRY_PROMPTED, this._TELEMETRY_PROMPT_REV);
> 
> wrong "this" in callback

Thanks, nice catch.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/68fa58281b57

Comment 15

[Cristian Nicolae (:xti)]

Telemetry doorhanger buttons are working again as expected on the latest Nightly build. Closing bug as verified fixed on:


Firefox 16.0a1 (2012-06-19)
Device: Galaxy Nexus
OS: Android 4.0.2