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

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

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. I think that we can use the same mechanism that we use to prompt users to opt-in to Telemetry on Beta and Release 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 think we're in agreement in bug 723129 on the text

"$PRODUCTNAME sends information about performance, hardware, usage and customizations back to Mozilla to help improve Firefox."

Please run with this text for now. The Learn more link should point to the Telemetry SUMO page.

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

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: patch to add notification

OK, here's the first cut that seems to work.  I'm not sure how well this integrates with Theo's work in bug 699806.

This patch does have a small change to the text; it reads:

"Nightly/Aurora sends information about performance, hardware, usage and customizations back to Mozilla to help improve Nightly/Aurora."

I didn't know whether including "Firefox" in the actual text would be OK; of course I can fix if desired.  Or we can reword?

Comment 3

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

(In reply to Nathan Froyd (:froydnj) from comment #2)
> Created attachment 600017 [details] [diff] [review]
> patch to add notification
> 
> OK, here's the first cut that seems to work.  I'm not sure how well this
> integrates with Theo's work in bug 699806.

Aside from needing to land at the same time, how else does this notification interact with the work in bug 699806?

> 
> This patch does have a small change to the text; it reads:
> 
> "Nightly/Aurora sends information about performance, hardware, usage and
> customizations back to Mozilla to help improve Nightly/Aurora."
> 
> I didn't know whether including "Firefox" in the actual text would be OK; of
> course I can fix if desired.  Or we can reword?

I think you're right about changing the trailing "Firefox" to "Nightly/Aurora". 

One more thought, above I provided a link to the Telemetry article on SUMO. The link that I provided is for the English version of the article. I think the URL that should be used is actually below, which excludes the language component. 

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

Comment 4

[Théo Chevalier]

Regarding my patch in bug 699806, I think it's ok. 
We'll need to merge this, but it's trivial:
>+    var telemetryEnabledByDefault = null;
>+    try {
>+      telemetryEnabledByDefault = >Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED_BY_DEFAULT);
>+    } catch(e) {}

Comment 5

[Tom Lowenthal]

Please revert the trailing product name to "Firefox".

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Tom Lowenthal [:StrangeCharm] from comment #5)
> Please revert the trailing product name to "Firefox".

The string "Firefox" should not be hard coded into the string.  It should be the product brand name (%3$S) as it is in the attached patch 600017 -- this is for when the code is included in otherwise named products.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 600017 [details] [diff] [review]
patch to add notification

Rather than including an "OK" button that does nothing, it seems like we should just have no buttons, but avoid hiding the default close button (i.e. don't set hideclose="true").

A lot of the code in the block you're adding is duplicating code lower down in the function (e.g. the "var win" and associated variables), that should be avoided.

Unlike the other telemetry notification bar, I don't think it makes sense to re-show the notification if the user clicks the "more info" link - this notification has served its purpose at that point, and there's no more user action required.

r- for those issues.

All that aside, it seems a little weird to be displaying this message without any indication of how you could opt-out. Seems like either we don't think users will care (in which case we shouldn't prompt), or we think they might want to opt-out, in which case we should either point them to prefs, or even better include an "opt-out" button in the notification itself.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to Lawrence Mandel [:lmandel] from comment #0)
> link to a learn more page with details about Telemetry and instructions of how to
> opt-out.

Ah, missed this part. Ignore the last part of comment 7.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Lawrence Mandel [:lmandel] from comment #3)
> Aside from needing to land at the same time, how else does this notification
> interact with the work in bug 699806?

They don't need to land simultaneously; since all the code here is predicated upon the enabled-by-default pref actually existing, the code will lay dormant until enabled-by-default lands.

> One more thought, above I provided a link to the Telemetry article on SUMO.
> The link that I provided is for the English version of the article. I think
> the URL that should be used is actually below, which excludes the language
> component. 

That's a good point, I will fix that.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: patch to add notification

Thanks for the quick review!  Here's a revised patch with somewhat less duplication.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: patch to add notification

Rebasing patch against trunk, since bug 710589 added some of the bits we should be using.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 600928 [details] [diff] [review]
patch to add notification

I reviewed the earlier iteration of this patch, so some of these comments apply to the patch for bug 710589.

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>   _showTelemetryNotification: function BG__showTelemetryNotification() {

>+    const PREF_TELEMETRY_SUPPORTURL = "toolkit.telemetry.supportURL";

Rather than adding this, let's use the existing support URL mechanism (also used in _showPlacesLockedNotificationBox):

let url = Services.urlFormatter.formatURLPref("app.support.baseURL");
url += "how-can-i-help-submitting-performance-data";

(Ricky set this URL up on the SUMO side)

>+    function appendTelemetryNotification(aNotifyBox, message, buttons, hideclose) {

This can just refer to "notifyBox" directly via closure, no need to pass it as an argument (you can move these function definitions below the variable declarations to make that clearer).

>+    function appendLearnMoreLink(aNotification, aBrowserBundle) {

Same thing here with aBrowserBundle.

>+      notification.setAttribute("hideclose", hideclose);

Setting hidden="false" is frowned upon, because some selectors depend on the mere presence of "hidden" (though this genereally applies more to HTML than XUL). Safer to just use:
if (hideclose)
  notification.setAttribute("hideclose", true);

>+    var browser = win.gBrowser;

Can you rename this "tabbrowser" or "gBrowser" while you're at it? "browser" is a very confusing name, since "browser" elements exist and this isn't one of them.

>+    var telemetryEnabledByDefault = true;

Seems like this should default to false.

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties

> telemetryPrompt = Will you help improve %1$S by sending anonymous information about performance, hardware characteristics, feature usage, and browser customizations to %2$S?

>+telemetryOptOutPrompt = %1$S sends information about performance, hardware, usage and customizations back to %2$S to help improve %3$S.

Shouldn't these be consistent?

r- for the default pref value and support URL things, but this looks fine otherwise.

Comment 13

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

(In reply to Gavin Sharp (use [e-mail] for email) from comment #12)
> > telemetryPrompt = Will you help improve %1$S by sending anonymous information about performance, hardware characteristics, feature usage, and browser customizations to %2$S?
> 
> >+telemetryOptOutPrompt = %1$S sends information about performance, hardware, usage and customizations back to %2$S to help improve %3$S.
> 
> Shouldn't these be consistent?

telemetryPrompt is the text for the Telemetry opt-in advertisement for Release/Beta. telemetryOptOutPrompt is the text for the Telemetry opt-out notification on Aurora/Nightly. You're right that the text "performance, hardware, usage and customizations" should match. We should also drop "anonymous" from the telemetryPrompt.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Lawrence Mandel [:lmandel] from comment #13)
> (In reply to Gavin Sharp (use [e-mail] for email) from comment
> #12)
> > > telemetryPrompt = Will you help improve %1$S by sending anonymous information about performance, hardware characteristics, feature usage, and browser customizations to %2$S?
> > 
> > >+telemetryOptOutPrompt = %1$S sends information about performance, hardware, usage and customizations back to %2$S to help improve %3$S.
> > 
> > Shouldn't these be consistent?
> 
> telemetryPrompt is the text for the Telemetry opt-in advertisement for
> Release/Beta. telemetryOptOutPrompt is the text for the Telemetry opt-out
> notification on Aurora/Nightly. You're right that the text "performance,
> hardware, usage and customizations" should match. We should also drop
> "anonymous" from the telemetryPrompt.

Hm, I think if anything, "anonymous" should be added to the opt-out prompt.  Dropping the "anonymous" from the Release/Beta prompt might lead some people to believe that the information being sent can be traced back to them, which is not what we want.  Can you explain why you think "anonymous" should be dropped here?

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: patch to add notification

Changes to address review feedback.

Comment 16

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

(In reply to Nathan Froyd (:froydnj) from comment #14)
> Hm, I think if anything, "anonymous" should be added to the opt-out prompt. 
> Dropping the "anonymous" from the Release/Beta prompt might lead some people
> to believe that the information being sent can be traced back to them, which
> is not what we want.  Can you explain why you think "anonymous" should be
> dropped here?

Sid Stamm and I spoke specifically about the use of the word anonymous as it relates to Telemetry. As Sid explained it to me, the data coming from Telemetry is not personally identifiable but also not truly anonymous. Therefore, from a privacy perspective it's not technically accurate to use this term.

I don't want this conversation to derail the work in this bug. I've opened bug 730955 to handle any change to the existing Telemetry opt-in notification.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 601006 [details] [diff] [review]
patch to add notification

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+    var telemetryEnabledByDefault = null;

>+      var telemetryNotifiedOptOut = null;

nit: initialize these to "false"

Also I noticed some tabs in this file, as mentioned on IRC. It would be nice to clean those up as you push this.

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties

>+telemetryOptOutPrompt = %1$S sends information about performance, hardware characteristics, feature usage and browser customizations back to %2$S to help improve %3$S.

Bug 730955 suggests that we want to consolidate on the shorter string from the previous patch, so let's go back to that for the string that you're adding.

Comment 18

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

(In reply to Gavin Sharp (use [e-mail] for email) from comment #17)
> Bug 730955 suggests that we want to consolidate on the shorter string from
> the previous patch, so let's go back to that for the string that you're
> adding.

Yes. Privacy is on board with the shorter string, which is also in use in the about dialog.

Comment 19

[Nathan Froyd [:froydnj]]

Attachment: patch to add notification

Updating with final review comments.  Carrying over r+.

Comment 20

[Dão Gottwald [:dao]]

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

Comment 21

[Matt Brubeck (:mbrubeck)]

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