725987 - Create Telemetry (opt-out) notification for Nightly and Aurora (mobile)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Lawrence Mandel [:lmandel] (use needinfo)]

Telemetry will be enabled by default (opt-out) on Nightly and Aurora channels. We need to inform our users that data is being collected. The notification should appear on first run after installation of a Nightly or Aurora build or on first run after Telemetry changes to opt-out on these channels. 

The message for the notification is being developed in bug 723129. The notification should include some form of "OK" button and a link to a learn more page with details about Telemetry and instructions of how to opt-out.

Comment 1

[Lawrence Mandel [:lmandel] (use needinfo)]

I have opened bug 725990 to track the addition of link capability within a notification on mobile.

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

The message for use in mobile is tracked in bug 728340.

Comment 4

[Théo Chevalier]

Brian - Do you think it's possible to have this in Fennec 13? We may likely be able to land Telemetry enabled by default (Bug 699806) before the next cut, but we can't land it without this stuff.

Comment 5

[Brian Nicholson (:bnicholson)]

Do we have a page to link to from the "Learn more" link?

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

We do. https://support.mozilla.org/kb/how-can-i-help-submitting-performance-data

Comment 7

[Brian Nicholson (:bnicholson)]

(In reply to Lawrence Mandel [:lmandel] from comment #6)
> We do.
> https://support.mozilla.org/kb/how-can-i-help-submitting-performance-data

This is the same URL you gave in bug 702319 - is this correct? The page at that URL says, "Telemetry is an opt-in feature", yet this bug is about making an opt-out message.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: patch

This patch updates the doorhanger message for Nightly/Aurora users.

Searching MXR, I don't see where "toolkit.telemetry.enabledByDefault" gets set. I assume this will be set elsewhere?

Also, we do a number of try/catches in here for getting prefs. Any reason not to just set these prefs in app/mobile.js instead?

Comment 9

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #8)

> Searching MXR, I don't see where "toolkit.telemetry.enabledByDefault" gets
> set. I assume this will be set elsewhere?

The patch in bug 699806 sets that in branding-specific prefs files. Do we have those for mobile as well?

Comment 10

[Théo Chevalier]

(In reply to Margaret Leibovic [:margaret] from comment #9)
> (In reply to Brian Nicholson (:bnicholson) from comment #8)
> 
> > Searching MXR, I don't see where "toolkit.telemetry.enabledByDefault" gets
> > set. I assume this will be set elsewhere?
> 
> The patch in bug 699806 sets that in branding-specific prefs files. Do we
> have those for mobile as well?

Yes, I will add in bug 699806 the code for mobile too. Thanks for the patch, Brian!

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Brian Nicholson (:bnicholson) from comment #7)
> (In reply to Lawrence Mandel [:lmandel] from comment #6)
> > We do.
> > https://support.mozilla.org/kb/how-can-i-help-submitting-performance-data
> 
> This is the same URL you gave in bug 702319 - is this correct? The page at
> that URL says, "Telemetry is an opt-in feature", yet this bug is about
> making an opt-out message.

I had intended to use the same URL, which originally had text specifically about Aurora/Nightly. You're right that the current page seems like an odd link from an opt-out dialog. I'll see about an alternative that makes more sense.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Here is the URL for mobile. This SUMO article now includes information that is specific for Aurora/Nightly.

https://support.mozilla.org/kb/how-can-i-help-submitting-mobile-performance-data

Comment 13

[Théo Chevalier]

We changed our approach to fix bug 699806, so you will need to replace:
1)
const PREF_TELEMETRY_ENABLED = "toolkit.telemetry.enabled";

with:
#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
    const PREF_TELEMETRY_ENABLED = "toolkit.telemetry.enabledByDefault";
#else
    const PREF_TELEMETRY_ENABLED  = "toolkit.telemetry.enabled";
#endif

2) you will need to remove:

const PREF_TELEMETRY_ENABLED_BY_DEFAULT = "toolkit.telemetry.enabledByDefault";

let telemetryEnabledByDefault = false;

telemetryEnabledByDefault = Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED_BY_DEFAULT);

3) and finally you will need to replace:

 if (telemetryEnabledByDefault) {
 }

with:

#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
#endif

Btw, you can look at the patch V18 in bug 699806, if needed, to see what I've done for desktop opt-out notification.

Please, tell me if I missed something.

Comment 14

[Théo Chevalier]

We've split bug 699806, now you can look bug 737596 for Mobile stuff. You'll see 
// Opt-out notification will be there
in the patch.

Comment 15

[Théo Chevalier]

Attachment: Patch V2

Rebased and adapted patch, regarding last changes in Bug 737596.

I changed the displayed message
As noticed by Matej (https://bugzilla.mozilla.org/show_bug.cgi?id=709583#c4), we should use an hardcoded "Firefox" instead of $PRODUCTNAME when we talk about the improved product.

I also fixed an edge case where after installing Nightly/Aurora, user disable Telemetry before we display the prompt (I agree, this is more doable on desktop since we display the prompt at second launch rather than in mobile where we display it at first start)
+    if (telemetryNotifiedOptOut || telemetryRejected || !telemetryEnabled)
+      return;

Comment 16

[Théo Chevalier]

Attachment: Patch V3

Before asking review, we need:

-To find a solution about using "Firefox" in string on Nightly/Aurora without harcoding it.
-Gavin's agreement about reverting to:
if (telemetryPrompted === this._TELEMETRY_PROMPT_REV)
       return;

This is being discussed on bug 699806.

Try build: https://tbpl.mozilla.org/?tree=Try&rev=600b4a171269
(But before reverting the return condition, so we still display prompt indefinitely to beta/GA user if they don't choose.)

Comment 17

[Théo Chevalier]

Attachment: Opt-out notification screenshot

Comment 18

[Théo Chevalier]

Attachment: Patch V4

Comment 19

[Théo Chevalier]

We have a Mochitest Robocop orange. This is caused by the notification itself.

I made two builds:
One with our opt-out message + ok button : https://tbpl.mozilla.org/?tree=Try&rev=de6259a4dd79 [RC fail]

A second replacing opt-out message & ok button with opt-in message + Ok/no buttons : https://tbpl.mozilla.org/?tree=Try&rev=48599522f946 [RC success]

My guess is that tests are programmed to detect the prompt and automatically close it, but can't do the same with our opt-out notification.

Should we open a follow up bug for that?
mfinkle, margaret?

Comment 20

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

Actually, the test framework disables all telemetry prompting by setting the preference before tests are run:

http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#411

Since your patch changes the preferences, this no longer works. We need to update the preference in automation.py.in to never show a prompt in Nightly, Aurora, Beta and Release. This might be harder to do now that two different prefs control the state.

Comment 21

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

I guess the simple fix is to add both prefs to automation.py.in

Comment 22

[Brian Nicholson (:bnicholson)]

Looks like Théo is taking this - please undo if that's not the case.

Comment 23

[Théo Chevalier]

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

Thanks Mark, it worked.
I also fixed other tests, I will update bug 737596, since it's not related to opt-out notification.

Comment 24

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

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

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

Comment 25

[Brian Nicholson (:bnicholson)]

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

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

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +63,5 @@
>  blockPopups.label=Block Popups
>  
>  # Telemetry
> +# %1$S = product name (Firefox); %2$S = server owner (Mozilla)
> +telemetry.optin.message3=Send info to %2$S so that we can improve Firefox?

If we're no longer using the product name, there's no need to pass it as a variable here. This can just be:

# %S = server owner (Mozilla)
telemetry.optin.message3=Send info to %S so that we can improve Firefox?

The formatStringFromName() call in browser.js should also be updated accordingly.

@@ +68,4 @@
>  telemetry.optin.learnMore=Learn more
>  telemetry.optin.yes=Yes
>  telemetry.optin.no=No
> +telemetry.optout.message=%1$S sends information about performance, hardware, usage and customizations back to %2$S to help improve Firefox.

To help translators, please add a comment above this string like we have for telemetry.optin.message3.

Comment 26

[Brian Nicholson (:bnicholson)]

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

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

::: mobile/android/chrome/content/browser.js
@@ +6857,2 @@
>  #ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
>    _PREF_TELEMETRY_ENABLED: "toolkit.telemetry.enabledByDefault",

Also, should we really be changing toolkit.telemetry.enabledByDefault? It seems like that should just be a set value set at build-time that we can pull and use here (so we don't need these "#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT" blocks). Could we just always use toolkit.telemetry.enabled?

Comment 27

[Théo Chevalier]

(In reply to Brian Nicholson (:bnicholson) from comment #26)
> Comment on attachment 676025 [details] [diff] [review]
> Patch V4
> 
> Review of attachment 676025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +6857,2 @@
> >  #ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
> >    _PREF_TELEMETRY_ENABLED: "toolkit.telemetry.enabledByDefault",
> 
> Also, should we really be changing toolkit.telemetry.enabledByDefault? It
> seems like that should just be a set value set at build-time that we can
> pull and use here (so we don't need these "#ifdef
> MOZ_TELEMETRY_ON_BY_DEFAULT" blocks). Could we just always use
> toolkit.telemetry.enabled?

Hum no, we treat telemetry state independently, so when we use Nightly/Aurora, we read/update toolkit.telemetry.enabledByDefault , and when we use Beta/GA we read/update toolkit.telemetry.enabled.
We can't use one single pref. toolkit.telemetry.enabledByDefault is defined at build time, but we want the user be able to switch channel.

Gavin is explaining that better than me :) See https://bugzilla.mozilla.org/show_bug.cgi?id=699806#c82

Comment 28

[Brian Nicholson (:bnicholson)]

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

(In reply to Théo Chevalier [:tchevalier] from comment #27)
> Hum no, we treat telemetry state independently, so when we use
> Nightly/Aurora, we read/update toolkit.telemetry.enabledByDefault , and when
> we use Beta/GA we read/update toolkit.telemetry.enabled.
> We can't use one single pref. toolkit.telemetry.enabledByDefault is defined
> at build time, but we want the user be able to switch channel.
> 

All right. But let's fix the strings as described in comment 25.

Comment 29

[Théo Chevalier]

Attachment: Patch V5

Strings fixed, I also fixed comment for desktop since we use brandShortName, not brandFullName

Added a comment to explain
+ if (!telemetryEnabled)
+   return;

Comment 30

[Brian Nicholson (:bnicholson)]

Comment on attachment 687483 [details] [diff] [review]
Patch V5

Looks good.

Comment 31

[Théo Chevalier]

Attachment: Patch V6

Here are a last modification, as we discussed, to avoid to display the notification/enable telemetry by default to everyone.
(You can look at the last comments of bug 699806, where we detailed this code)

In order to save you some time, since you already r+'ed this patch, here are the changed lines:


    /*
     * Display an opt-out notification when telemetry is enabled by default,
     * an opt-in prompt otherwise.
     *
     * But do not display this prompt/notification if:
     *
     * - The last accepted/refused policy (either by accepting the prompt or by 
     *   manually flipping the telemetry preference) is already at version 
     *   TELEMETRY_DISPLAY_REV
     */
    let telemetryDisplayed;
    try {
      telemetryDisplayed = Services.prefs.getIntPref(self._PREF_TELEMETRY_DISPLAYED);
    } catch(e) {}

    if (telemetryDisplayed === TELEMETRY_DISPLAY_REV)
      return;
#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
    /*
     * Additionally, in opt-out builds, don't display the notification if:
     *
     * - Telemetry is disabled
     * - Telemetry was explicitly refused through the UI
     * - Opt-in telemetry was enabled and this is the first run with opt-out
     */
    let telemetryRejected = false;
    try {
      telemetryRejected = Services.prefs.getBoolPref(self._PREF_TELEMETRY_REJECTED);
    } catch(e) {}
    let optInTelemetryEnabled = false;
    try {
      optInTelemetryEnabled = Services.prefs.getBoolPref("toolkit.telemetry.enabled");
    } catch(e) {}

    let telemetryEnabled = Services.prefs.getBoolPref(self._PREF_TELEMETRY_ENABLED);
    if (!telemetryEnabled)
      return;

    // If telemetry was explicitly refused through the UI,
    // also disable opt-out telemetry and bail out.
    if (telemetryRejected) {
      Services.prefs.setBoolPref(self._PREF_TELEMETRY_ENABLED, false);
      Services.prefs.setIntPref(self._PREF_TELEMETRY_DISPLAYED, self._TELEMETRY_DISPLAY_REV);
      return;
    }

    // If opt-in telemetry was enabled and this is the first run with opt-out,
    // don't notify the user.
    if (optInTelemetryEnabled && telemetryDisplayed === undefined) {
      Services.prefs.setBoolPref(self._PREF_TELEMETRY_REJECTED, false);
      Services.prefs.setIntPref(self._PREF_TELEMETRY_DISPLAYED, self._TELEMETRY_DISPLAY_REV);
      return;
    }

    message = Strings.browser.formatStringFromName("telemetry.optout.message",
                                                    [brandShortName, serverOwner, brandShortName], 3);
    buttons = [
      {
        label: Strings.browser.GetStringFromName("telemetry.optout.ok"),
        callback: function () {
          Services.prefs.setIntPref(self._PREF_TELEMETRY_DISPLAYED, self._TELEMETRY_DISPLAY_REV);
        }
      }
    ];
#else
    // Clear old prefs and reprompt
    Services.prefs.clearUserPref(self._PREF_TELEMETRY_DISPLAYED);
    Services.prefs.clearUserPref(self._PREF_TELEMETRY_ENABLED);
    Services.prefs.clearUserPref(self._PREF_TELEMETRY_REJECTED);

Comment 32

[Théo Chevalier]

Comment on attachment 689431 [details] [diff] [review]
Patch V6

I need to backport here the last modifications done in bug 699806 by Marco.

Comment 33

[Théo Chevalier]

Attachment: Patch V7

Finally, here is what's new since the r+'ed V5:
(Desktop patches has landed, this will likely not be changed)



    /*
     * Display an opt-out notification when telemetry is enabled by default,
     * an opt-in prompt otherwise.
     *
     * But do not display this prompt/notification if:
     *
     * - The last accepted/refused policy (either by accepting the prompt or by
     *   manually flipping the telemetry preference) is already at version
     *   TELEMETRY_DISPLAY_REV.
     */
    let telemetryDisplayed;
    try {
      telemetryDisplayed = Services.prefs.getIntPref(self._PREF_TELEMETRY_DISPLAYED);
    } catch(e) {}
    if (telemetryDisplayed === self._TELEMETRY_DISPLAY_REV)
      return;

#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
    /*
     * Additionally, in opt-out builds, don't display the notification if:
     *
     * - Telemetry is disabled
     * - Telemetry was explicitly refused through the UI
     * - Opt-in telemetry was enabled and this is the first run with opt-out.
     */

    let telemetryEnabled = Services.prefs.getBoolPref(self._PREF_TELEMETRY_ENABLED);
    if (!telemetryEnabled)
      return;

    // If telemetry was explicitly refused through the UI,
    // also disable opt-out telemetry and bail out.
    let telemetryRejected = false;
    try {
      telemetryRejected = Services.prefs.getBoolPref(self._PREF_TELEMETRY_REJECTED);
    } catch(e) {}
    if (telemetryRejected) {
      Services.prefs.setBoolPref(self._PREF_TELEMETRY_ENABLED, false);
      Services.prefs.setIntPref(self._PREF_TELEMETRY_DISPLAYED, self._TELEMETRY_DISPLAY_REV);
      return;
    }

    // If opt-in telemetry was enabled and this is the first run with opt-out,
    // don't notify the user.
    let optInTelemetryEnabled = false;
    try {
      optInTelemetryEnabled = Services.prefs.getBoolPref("toolkit.telemetry.enabled");
    } catch(e) {}
    if (optInTelemetryEnabled && telemetryDisplayed === undefined) {
      Services.prefs.setBoolPref(self._PREF_TELEMETRY_REJECTED, false);
      Services.prefs.setIntPref(self._PREF_TELEMETRY_DISPLAYED, self._TELEMETRY_DISPLAY_REV);
      return;
    }

    message = Strings.browser.formatStringFromName("telemetry.optout.message",
                                                    [brandShortName, serverOwner, brandShortName], 3);
    buttons = [
      {
        label: Strings.browser.GetStringFromName("telemetry.optout.ok"),
        callback: function () {
          Services.prefs.setIntPref(self._PREF_TELEMETRY_DISPLAYED, self._TELEMETRY_DISPLAY_REV);
        }
      }
    ];
#else
    // Clear old prefs and reprompt
    Services.prefs.clearUserPref(self._PREF_TELEMETRY_DISPLAYED);
    Services.prefs.clearUserPref(self._PREF_TELEMETRY_ENABLED);
    Services.prefs.clearUserPref(self._PREF_TELEMETRY_REJECTED);

Comment 34

[Brian Nicholson (:bnicholson)]

Comment on attachment 690153 [details] [diff] [review]
Patch V7

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

::: mobile/android/chrome/content/browser.js
@@ +7204,5 @@
> +      return;
> +    }
> +
> +    // If opt-in telemetry was enabled and this is the first run with opt-out,
> +    // don't notify the user.

Is it correct to say "and this is the first run"? You're setting self._PREF_TELEMETRY_DISPLAYED to the current display revision, so wouldn't this also prevent notifications for all future runs (until the revision changes)?

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +68,5 @@
>  telemetry.optin.yes=Yes
>  telemetry.optin.no=No
> +# LOCALIZATION NOTE (telemetryOptOutPrompt): %1$S and %3$S will be replaced by
> +# brandShortName, and %2$S by the value of the toolkit.telemetry.server_owner preference.
> +telemetry.optout.message=%1$S sends information about performance, hardware, usage and customizations back to %2$S to help improve %3$S.

Please change "LOCALIZATION NOTE (telemetryOptOutPrompt)" to "LOCALIZATION NOTE (telemetry.optout.message)".

Comment 35

[Théo Chevalier]

(In reply to Brian Nicholson (:bnicholson) from comment #34)
> Comment on attachment 690153 [details] [diff] [review]
> Patch V7
> 
> Review of attachment 690153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +7204,5 @@
> > +      return;
> > +    }
> > +
> > +    // If opt-in telemetry was enabled and this is the first run with opt-out,
> > +    // don't notify the user.
> 
> Is it correct to say "and this is the first run"? You're setting
> self._PREF_TELEMETRY_DISPLAYED to the current display revision, so wouldn't
> this also prevent notifications for all future runs (until the revision
> changes)?
Hm. We want to say that we check optInTelemetryEnabled only at the first run in opt-out builds, not that the notification will be disabled only for this first run. (Indeed, it will be disabled until the revision changes) I don't know how the reword that correctly. Maybe:

"If opt-in telemetry was enabled, don't notify the user until next policy update. (Do the check only at first run with opt-out builds)"

What do you think?

So if we change this comment, we probably want to change it for Desktop too. Can I update it in this patch?

> 
> ::: mobile/android/locales/en-US/chrome/browser.properties
> @@ +68,5 @@
> >  telemetry.optin.yes=Yes
> >  telemetry.optin.no=No
> > +# LOCALIZATION NOTE (telemetryOptOutPrompt): %1$S and %3$S will be replaced by
> > +# brandShortName, and %2$S by the value of the toolkit.telemetry.server_owner preference.
> > +telemetry.optout.message=%1$S sends information about performance, hardware, usage and customizations back to %2$S to help improve %3$S.
> 
> Please change "LOCALIZATION NOTE (telemetryOptOutPrompt)" to "LOCALIZATION
> NOTE (telemetry.optout.message)".
Oops, corrected

Comment 36

[Brian Nicholson (:bnicholson)]

(In reply to Théo Chevalier [:tchevalier] from comment #35)
> Hm. We want to say that we check optInTelemetryEnabled only at the first run
> in opt-out builds, not that the notification will be disabled only for this
> first run. (Indeed, it will be disabled until the revision changes) I don't
> know how the reword that correctly. Maybe:
> 
> "If opt-in telemetry was enabled, don't notify the user until next policy
> update. (Do the check only at first run with opt-out builds)"
> 
> What do you think?

It might be a bit clearer if you say "already enabled" instead of just "enabled" to indicate that the pref is being carried over.

> So if we change this comment, we probably want to change it for Desktop too.
> Can I update it in this patch?

I think it'd be better to have that as a mini follow-up patch in the desktop bug so the hg annotation refers to the correct bug.

Comment 37

[Théo Chevalier]

Attachment: Patch V8

Comment 38

[Brian Nicholson (:bnicholson)]

Comment on attachment 690532 [details] [diff] [review]
Patch V8

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

r+ with this last change. Thanks for the patch!

::: mobile/android/chrome/content/browser.js
@@ +7205,5 @@
> +      return;
> +    }
> +
> +    // If opt-in telemetry was enabled and this is the first run with opt-out,
> +    // don't notify the user.

Looks like this comment needs to be changed in two places. You got one of them, but here's another.

Comment 39

[Théo Chevalier]

Thanks to you!
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad491d4e39da

Comment 40

[Ed Morley [:emorley]]

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

Comment 41

[Ioana Chiorean]

Firefox 20.0a1 (2012-12-17)
Device: Galaxy Nexus
OS: Android 4.1.1

This feature is now implemented correctly in Nightly (Fx 20). Marking bug as VERIFIED FIXED.