829881 - Remove remnants of Telemetry notification preferences

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Gregory Szorc [:gps]]

In bug 804745 we removed the Telemetry-specific notification bar and replaced it with a unified one that applies to {Telemetry, Crash Reports, Firefox Health Report}.

The patch was only partial in that it did not remove lots of Telemetry-centric code still kinda/sorta looking at the preferences used by the notification bar.

In this bug, we will remove the remnants of the legacy preferences.

Legacy preferences include:

* toolkit.telemetry.notifiedOptOut
* toolkit.telemetry.prompted
* toolkit.telemetry.rejected
* @MOZ_TELEMETRY_DISPLAY_REV@

Removal of the above assumes that they are only referenced in frontend code, not in /toolkit. 

This work needs to be performed in the next few weeks and will need to be uplifted to 20 since the work in bug 804745 will be uplifted in the next day or two. (Bug 804745 is on the Aurora fast track because of new strings.)

I will likely perform this work then get sign-off from Telemetry and browser peeps as necessary.

As an aside, yes, I'm aware many of these were recently introduced and it may seem strange to remove them so suddenly. That's how software works sometimes :/

Comment 1

[Théo Chevalier [:tchevalier]]

No worries, I understand.
Also, I think I can help you in there.

I confirm, there is nothing in toolkit, but you will need to remove
* toolkit.telemetry.notifiedOptOut
* toolkit.telemetry.prompted
 from several tests, including Talos.
(And likely replace them with a similar mecanism to hide th THR notification)
And also be sure to permanently hide it in Webapps.js.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for 20 since that's the first version we're shipping FHR in.

Comment 3

[Gregory Szorc [:gps]]

We have a complicating factor: /mobile/android/chrome/content/browser.js. We don't yet have the unified notification bar in /mobile. Until we do, we need the old notification bar to persist. The good news is Chenxia (:liuche) has the mocks and requirements from Product/UX and will be working on this and it should land in Firefox 21. Yay!

As for general behavior in the new world, this is how I'm going to code it to work (and I think this is similar or exactly how things are done today):

On Nightly and Aurora channels, Telemetry is enabled by default via a default pref value. On new profiles, if you go to the Data Choices pref pane or about:telemetry, you'll see the checkbox checked by default. As soon as you launch the app, Telemetry will start collecting and sending data to Mozilla (because Telemetry is enabled by default). Eventually the user will see the unified data reporting notification bar. At that time they have the opportunity to go to the pref pane and disable Telemetry. I've been told the submit-before-notified behavior is acceptable on Nightly and Aurora channels.

On Beta and Release channels, Telemetry is disabled by default via a default pref value. On new profiles, if you go to the Data Choices pref pane or about:telemetry, the checkbox will be unchecked by default. Since this pref is disabled by default, Telemetry will neither collect nor submit data on application start. Eventually the user will see the data reporting notification bar. At that time, they have the opportunity to opt in to Telemetry. If the user goes to the pref pane or about:telemetry and opts in to Telemetry, data recording starts immediately and data submission could occur before the data reporting notification bar is displayed.

Please let me know if this is not acceptable.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Remove references to Telemetry notification bar, v1

Disclaimer: I'm not intimately familiar with the requirements for Telemetry before and of all the places in the tree where the code I touched could have implications. I quite possibly made some huge mistakes. I'm also not sure I tagged the appropriate person for review.

This patch removes references to the Telemetry notification bar. This notification bar since been replaced with a unified "data reporting" notification bar covering {FHR, Telemetry, Crash Reports}.

One might say that this patch stops recording user intent by removing the "toolkit.telemetry.rejected" pref. Yes and no. We don't have a separate pref any more. But, we do have the "is user set" state in the toolkit.telemetry.{enabled, enabledPreRelease} pref as captured by the preferences store. I /think/ this should be sufficient. If not, I can restore the explicit state capture.

I did not touch /mobile because we still need the notification bar. I /might/ have broken something in mobile by removing pref references from about:telemetry. I don't know the code well enough to be sure. I also held off removing the pref references in the test files (e.g. automation.py.in) because I suspect they could be relevant for mobile. We should file a new bug to track the notification bar on mobile (there /might/ already be one) and remove the remaining prompting preferences when that lands.

Comment 5

[Théo Chevalier [:tchevalier]]

Comment on attachment 702407 [details] [diff] [review]
Remove references to Telemetry notification bar, v1

(In reply to Gregory Szorc [:gps] from comment #3)
> Please let me know if this is not acceptable.

Seems acceptable to me, the only concern I could have is sending data *before* showing anything to the user, but I assume privacy team has already reviewed that for FHR, so it could apply to Telemetry too?
If not, maybe we could use the metrics collection service (I don’t know this code) to tell telemetry (and FHR?) when it’s good to go? (User acceptance, or implicit acceptance)

(In reply to Gregory Szorc [:gps] from comment #4)
> One might say that this patch stops recording user intent by removing the
> "toolkit.telemetry.rejected" pref. Yes and no. We don't have a separate pref
> any more. But, we do have the "is user set" state in the
> toolkit.telemetry.{enabled, enabledPreRelease} pref as captured by the
> preferences store. I /think/ this should be sufficient. If not, I can
> restore the explicit state capture.

If we do that, we have to be sure to never go back to something like the old notification bar, this is a question that I can’t answer.
Either way, telemetry user acceptance record were screwed since the beginning, so…

Also, the text we display in about dialog is only about Telemetry, since FHR is enabled by default too, it could make sense to update it. Maybe something shorter and more global like in the FHR notification "PRODUCTNAME sends some data to Mozilla so that we can improve your experience", but I will open a bug and let privacy people or Matej find the right words.

Now, regarding your patch, I think what you removed is safe, I spotted some little other things we can also remove.

In /toolkit/content/jar.mn
*  content/global/aboutTelemetry.js
You can remove the "*", it is only needed for @MOZ_TELEMETRY_DISPLAY_REV@

In /toolkit/content/aboutTelemetry.js and in /browser/components/nsBrowserGlue.js
You can remove #filter substitution for the same reason.

If someone added something which require #filter after I added it, we will know that at build time :)
Could be a good idea to do a try build to be safe.

r+ with that.

Comment 6

[Gregory Szorc [:gps]]

(In reply to Théo Chevalier [:tchevalier] from comment #5)
> Seems acceptable to me, the only concern I could have is sending data
> *before* showing anything to the user, but I assume privacy team has already
> reviewed that for FHR, so it could apply to Telemetry too?

In short, the Aurora and Nightly channels are exempt from the requirement that notification occur before upload. That means we only turn our scrutinizing eye to Beta and Release. On those channels, Telemetry is disabled by default. So, any data submission will be the result of explicit user action. Health Report is enabled by default, so it has to take extra precautions before upload. It does this, even on all channels (because a single code path is easier).

> If not, maybe we could use the metrics collection service (I don’t know this
> code) to tell telemetry (and FHR?) when it’s good to go? (User acceptance,
> or implicit acceptance)

We now have a "data reporting" XPCOM service. One of the objects that hangs off of it is a "policy" that holds state around what has been agreed to, what's enabled, etc. It currently only supports FHR. But, we could hang Telemetry off of it if we wanted. See https://hg.mozilla.org/mozilla-central/file/default/services/datareporting/policy.jsm
 
> Also, the text we display in about dialog is only about Telemetry, since FHR
> is enabled by default too, it could make sense to update it. Maybe something
> shorter and more global like in the FHR notification "PRODUCTNAME sends some
> data to Mozilla so that we can improve your experience", but I will open a
> bug and let privacy people or Matej find the right words.

Currently, Telemetry and FHR are separate product features. I anticipate they will converge or at least become more complementary over time. Given they are separate, I /think/ the current string on about:telemetry can remain. If we need to change, we can file a follow-up.
 
> In /toolkit/content/jar.mn
> *  content/global/aboutTelemetry.js
> You can remove the "*", it is only needed for @MOZ_TELEMETRY_DISPLAY_REV@
 > In /toolkit/content/aboutTelemetry.js and in
> /browser/components/nsBrowserGlue.js
> You can remove #filter substitution for the same reason.

Good catches! Done.
 
> If someone added something which require #filter after I added it, we will
> know that at build time :)
> Could be a good idea to do a try build to be safe.

I'll just land in s-c. If we get a build failure, we'll know :)

> r+ with that.

Thank you!

Comment 7

[Théo Chevalier [:tchevalier]]

>I've been told the submit-before-notified behavior is acceptable on Nightly and Aurora channels.

Oops, read too fast, sorry :) So then it’s fine.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Remove references to Telemetry notification bar, v2

Incorporated review feedback.

Comment 9

[Théo Chevalier [:tchevalier]]

Comment on attachment 702456 [details] [diff] [review]
Remove references to Telemetry notification bar, v2

You can also remove them from /browser/components/preferences/advanced.js and in-content, I forgot those, sorry.

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/921437db35f9

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 702456 [details] [diff] [review]
Remove references to Telemetry notification bar, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bugs 809094 and 804745
User impact if declined: Possibly none. There is potentially a UX bug related to the Telemetry checkbox lingering in here, however.
Testing completed (on m-c, etc.): Manual testing of checkboxes in local builds (not automated test coverage).
Risk to taking this patch (and alternatives if risky): See "user impact"
String or UUID changes made by this patch: None

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

triage drive-by, waiting for landing to central.

Comment 13

[Gregory Szorc [:gps]]

Attachment: Part 2: Fix about:telemetry bustage, v1

For some reason I didn't notice bustage until I performed a clobber build. Perhaps /telemetry/content/jar.mn isn't abiding by the rules I expected it to...

Anyway, there is still preprocessor foo in aboutTelemetry.js, so we need the '*' in jar.mn. Also, I missed a reference to a deleted pref.

I performed a clobber with this patch applied and about:telemetry works again.

Comment 14

[Théo Chevalier [:tchevalier]]

(In reply to Gregory Szorc [:gps] from comment #6)
> > Also, the text we display in about dialog is only about Telemetry, since FHR
> > is enabled by default too, it could make sense to update it. Maybe something
> > shorter and more global like in the FHR notification "PRODUCTNAME sends some
> > data to Mozilla so that we can improve your experience", but I will open a
> > bug and let privacy people or Matej find the right words.
> 
> Currently, Telemetry and FHR are separate product features. I anticipate
> they will converge or at least become more complementary over time. Given
> they are separate, I /think/ the current string on about:telemetry can
> remain. If we need to change, we can file a follow-up.

Of course we will keep the text as is in about:telemetry.

I wasn’t clear, sorry, I was rather talking about the dialog in Firefox > Help > About $PRODUCTNAME.
And yes, this text is about Telemetry stuff, but we don’t say it to the user, it could be understood as "what Nightly/Aurora is and what it may or is sending".
Since we don’t only have telemetry enabled by default in those builds, and as you said it, the features are different, the data sent by default are now different, this is why I think we could (should?) update it.
But if you think this couldn’t be confusing, it’s ok.

Comment 15

[Théo Chevalier [:tchevalier]]

Oh, wait. Big issue. about:telemetry can be opened in mobile...

Comment 16

[Gregory Szorc [:gps]]

(In reply to Théo Chevalier [:tchevalier] from comment #15)
> Oh, wait. Big issue. about:telemetry can be opened in mobile...

I'm not sure how this would be a big issue. I believe the current patches would work on mobile - Telemetry just wouldn't be using the extra preferences (even though they are still saved on mobile - for now).

Comment 17

[Théo Chevalier [:tchevalier]]

Comment on attachment 702601 [details] [diff] [review]
Part 2: Fix about:telemetry bustage, v1

(In reply to Gregory Szorc [:gps] from comment #16)
> (In reply to Théo Chevalier [:tchevalier] from comment #15)
> > Oh, wait. Big issue. about:telemetry can be opened in mobile...
> 
> I'm not sure how this would be a big issue. I believe the current patches
> would work on mobile - Telemetry just wouldn't be using the extra
> preferences (even though they are still saved on mobile - for now).

Hm, you’re right, everything is ok. My bad (These days I’m a little bit paranoid when it comes to Telemetry prefs)

Comment 18

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/e50dfda1a872

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 702601 [details] [diff] [review]
Part 2: Fix about:telemetry bustage, v1

[Approval Request Comment]

Followup to part 1. Part 1 actually broke about:telemetry :/

Comment 20

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/921437db35f9
https://hg.mozilla.org/mozilla-central/rev/e50dfda1a872

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 702456 [details] [diff] [review]
Remove references to Telemetry notification bar, v2

Early enough in Aurora to take changes FHR that keep us consistent between FF20 and 21, even if the user impact is not fully known.

Comment 22

[Scoobidiver (away)]

Pushed to Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/77ac5f84e113
http://hg.mozilla.org/releases/mozilla-aurora/rev/b08ac3e59f63

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Not relnoting since we're not shipping FHR yet and this bug on its own doesn't really have a user-friendly phrasing for notes.