699806 - make it possible to enable Telemetry by default on Nightly and Aurora channels (Desktop)

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

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

Nightly and Aurora users accept a different level of engagement with Mozilla by nature of installing these early release versions of Firefox. These channels also have significantly smaller install bases. We should enable Telemetry by default on these channels.

Comment 1

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

Telemetry should be enabled by default on both desktop and mobile.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

From a technical privacy standpoint, I support comment 0 and comment 1 assuming:

(a) it's clear that when people install nightly or aurora that they may opt into data collection and other risky features (disclosed via download page or first-run or similar)
(b) there's a way to turn it back off that's available via download, first-run page, or similar
(c) this is confined to nightly and aurora users (who know they're in for a bumpy ride)

In general, I think if we get good enough data in those channels, we can probably limit most of the telemetry probes to those channels and then avoid bothering the Beta and Release users with most of our probes.  :)

Comment 3

[Christopher Jung]

If/when we implement this, we should ensure that users aren't confused since we already have Test Pilot bundled by default on aurora and beta channels (not on nightly).

Comment 4

[Tom Lowenthal]

The privacy team is fine with this plan. Comment 2 is correct that we'll need to put some text on the download page & privacy policy going forward. This should probably not be a blocker.

Comment 5

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

Adding a link to bug 679431 for reference.

Comment 6

[Tom Lowenthal]

Filed two bugs for the changes needed. We need:
- a modification to the About box, which lands in the same merge, and
- added text on the download page whenever possible.

Comment 7

[(dormant account)]

Is there a reason this isn't being done for Beta? It's still a development build.

Comment 8

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

My understanding from speaking with Sid (please correct me if I'm wrong) is that the beta audience is not as much of a dev/test audience as Nightly and Aurora and therefore there is a different privacy expectation.

Comment 9

[Tom Lowenthal]

I can confirm Comment 8. Default in Beta seems a little strong. However, we could potentially do a more aggressive prompt, or an install-time prompt.

Comment 10

[Théo Chevalier [:tchevalier]]

Attachment: Patch proposal

I think this is too easy to work, but tell me if you think it's okay.

Comment 11

[Théo Chevalier [:tchevalier]]

Note that on my own compilation, checkbox on the pref pane is checked, but I don't know if telemetry is really activated, because I guess it's impossible to activate it on a non-official build.

Comment 12

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

Comment on attachment 581339 [details] [diff] [review]
Patch proposal

This looks correct to me...and I would expect that this change would be very easy to implement. I will defer to Taras for the r+.

Taras - Do any other properties need to be set? Does setting this property prevent prompting the user to opt-in?

Comment 13

[(dormant account)]

Comment on attachment 581339 [details] [diff] [review]
Patch proposal

You also need to set toolkit.telemetry.prompted to 2

Comment 14

[(dormant account)]

Theo, to test this, take out #ifdef MOZ_TELEMETRY_REPORTING ..#endif in http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js

Comment 15

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

Taras - Will this change cover desktop and mobile or is there a separate file that must be changed for mobile preferences?

Comment 16

[Théo Chevalier [:tchevalier]]

Thanks a lot ! I will post the patch updated right away, and compile without #ifdef MOZ_TELEMETRY_REPORTING ..#endif during the night to ensure that works.

Comment 17

[Théo Chevalier [:tchevalier]]

Attachment: Patch V2

pref("toolkit.telemetry.prompted", 2); added.

Comment 18

[(dormant account)]

(In reply to Lawrence Mandel [:lmandel] from comment #15)
> Taras - Will this change cover desktop and mobile or is there a separate
> file that must be changed for mobile preferences?

No idea about mobile, mfinkle?

Comment 19

[(dormant account)]

Comment on attachment 581407 [details] [diff] [review]
Patch V2

Looks good.

Comment 20

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

Putting the current prompt rev in these pref files is kind of error prone. It would be nicer to use a magical value that means "never prompt".

Comment 21

[Théo Chevalier [:tchevalier]]

Attachment: Patch V3

PREF_TELEMETRY_NEVER_PROMPT added.

I don't know what's best in nsBrowserGlue.js to check if the preference is set:
>PREF_TELEMETRY_NEVER_PROMPT == true
or only
>PREF_TELEMETRY_NEVER_PROMPT

I've put this test with telemetryPrompted === TELEMETRY_PROMPT_REV test, because action is the same, but I don't kwnow if it's the best way to proceed.

Comment 22

[(dormant account)]

(In reply to Théo Chevalier from comment #21)
> Created attachment 582089 [details] [diff] [review]
> Patch V3
> 
> PREF_TELEMETRY_NEVER_PROMPT added.
> 
> I don't know what's best in nsBrowserGlue.js to check if the preference is
> set:
> >PREF_TELEMETRY_NEVER_PROMPT == true
> or only
> >PREF_TELEMETRY_NEVER_PROMPT
> 
> I've put this test with telemetryPrompted === TELEMETRY_PROMPT_REV test,
> because action is the same, but I don't kwnow if it's the best way to
> proceed.

I would've just added code to check for toolkit.telemetry.rejected and set that.

Comment 23

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

Theo - Think you can rev the patch today to respond to comment 22 in order to land on M-C before the cut over to Aurora tomorrow?

Comment 24

[Théo Chevalier [:tchevalier]]

Yes, sorry I was a little busy these days, but I'm going to work tonight and provide a patch within a few hours. If there are still errors, can anyone correct in order not to miss the merge?

Comment 25

[Théo Chevalier [:tchevalier]]

Attachment: PatchV4

Taras - That is what you was talking about? If not, I fear for not having understood.

Comment 26

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

Comment on attachment 582946 [details] [diff] [review]
PatchV4

PREF_TELEMETRY_REJECTED is a string constant, so adding a null check for it doesn't make sense. I think you want something like:

let telemetryEnabledByDefault = Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED) && !Services.prefs.prefHasUserValue(PREF_TELEMETRY_ENABLED);

if (telemetryEnabledByDefault || telemetryPrompted === TELEMETRY_PROMPT_REV)
  return;

Comment 27

[Théo Chevalier [:tchevalier]]

Attachment: PatchV5

So we only need that? You've done all the work! :) Thanks again Gavin, I can only get better! I begin to study the Mercurial Queues.

Comment 28

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

Comment on attachment 583701 [details] [diff] [review]
PatchV5

Sorry, I should have noticed this earlier, but this will have the unfortunate side effect of disabling telemetry if you switch from Firefox->Nightly/Aurora->Firefox with the same profile... We should probably try to avoid that by using a separate pref. toolkit.telemetry.enabledByDefault?

nit: add newlines to the end of the files, and indent the two lines after "let telemetryEnabledByDefault" by two spaces, and don't remove any of those newlines.

Comment 29

[Théo Chevalier [:tchevalier]]

Attachment: PatchV6

Margaret - Could you (If you have time, or you can ask someone else to feedback, it doesn't matter.) tell me if this patch takes into account all comments from this bug, please?
I think I solved the disabling effect by switching profile, but I want to be sure that the patch is perfect, before requesting a review.

Comment 30

[:Margaret Leibovic]

Comment on attachment 588569 [details] [diff] [review]
PatchV6

I think Gavin should look at this, since he's the one who reviewed previous versions of this patch (and I'm not familiar with this code).

Comment 31

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

Comment on attachment 588569 [details] [diff] [review]
PatchV6

On the right path, but we need to change a couple of things:
- need to remove telemetry.enabled from the firefox-branding.js files
- no need to add enabledByDefault to all.js
- need to make the telemetry service check telemetry.enabled || telemetry.enabledByDefault to determine whether it should be active
- telemetryEnabledByDefault doesn't need to check prefHasUserValue, you can just getBoolPref(PREF_TELEMETRY_ENABLED_BY_DEFAULT)

Comment 32

[Théo Chevalier [:tchevalier]]

Attachment: PatchV7

Comment 33

[Théo Chevalier [:tchevalier]]

Attachment: PatchV8

Comment 34

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

Comment on attachment 589674 [details] [diff] [review]
PatchV8

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

>-    // If the user has seen the latest telemetry prompt, do not prompt again
>+    // If the user has seen the latest telemetry prompt or if telemetry is activated or is activated by default, do not prompt again
>     // else clear old prefs and reprompt
>-    if (telemetryPrompted === TELEMETRY_PROMPT_REV)
>+    let telemetryActive = Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED_BY_DEFAULT) ||
>+     Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED);
>+    
>+    if (telemetryActive || telemetryPrompted === TELEMETRY_PROMPT_REV)

We only want to omit the prompt if telemetry is activated by default, since we want to keep the ability to re-prompt users who have already activated telemetry in the future by bumping the prompt revision. So only check PREF_TELEMETRY_ENABLED_BY_DEFAULT here, and rename it back to "telemetryEnabledByDefault" or somesuch.

This is also still missing the changes to TelemetryPing.js to actually have the service check both prefs to determine enabled state, and I also just realized that we'll need to also fix the prefs UI to not appear when telemetry is enabled by default.

Comment 35

[Théo Chevalier [:tchevalier]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #34)
> Comment on attachment 589674 [details] [diff] [review]
> PatchV8
> 
> >diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
> 
> >-    // If the user has seen the latest telemetry prompt, do not prompt again
> >+    // If the user has seen the latest telemetry prompt or if telemetry is activated or is activated by default, do not prompt again
> >     // else clear old prefs and reprompt
> >-    if (telemetryPrompted === TELEMETRY_PROMPT_REV)
> >+    let telemetryActive = Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED_BY_DEFAULT) ||
> >+     Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED);
> >+    
> >+    if (telemetryActive || telemetryPrompted === TELEMETRY_PROMPT_REV)
> 
> We only want to omit the prompt if telemetry is activated by default, since
> we want to keep the ability to re-prompt users who have already activated
> telemetry in the future by bumping the prompt revision. So only check
> PREF_TELEMETRY_ENABLED_BY_DEFAULT here, and rename it back to
> "telemetryEnabledByDefault" or somesuch.

Oh, okay, I didn't think to this case.

> 
> This is also still missing the changes to TelemetryPing.js to actually have
> the service check both prefs to determine enabled state, and I also just
> realized that we'll need to also fix the prefs UI to not appear when
> telemetry is enabled by default.

I didn't understand that in your previous comment, I confused "make the telemetry service check telemetry.enabled || telemetry.enabledByDefault" with simply the fact to test if one of these two prefs is true. So, I got it, I'll check TelemetryPing.js tomorrow.
So, to put it in a nutshell, I've to do a new function in TelemetryPing.js who check in wich case we are (just enabled/enabled By default), and call this function on nsBrowserGlue.js. If it's "enabledByDefault", I hide the pref in the option pane and return BG__showTelemetryNotification()?

Thanks Gavin, for your quick review!

Comment 36

[Théo Chevalier [:tchevalier]]

Attachment: PatchV9

* UI pref hidden when telemetry is activated by default (Guess it works also with mobile)

* Telemetry is disabled (on TelemetryPing.js) when telemetry.enabled is false OR when enabledBydefault is true and telemetry.enabled has a value set by the user.

Comment 37

[Théo Chevalier [:tchevalier]]

Attachment: PatchV10

Some minimal adjustments in patchV9

Comment 38

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

(In reply to Gavin Sharp (use [e-mail] for email) from comment #34)
> ...and I also just
> realized that we'll need to also fix the prefs UI to not appear when
> telemetry is enabled by default.

As I understand your comment you're suggesting that we should remove the "Submit performance data" checkbox from Preferences->Advanced->General. If my understanding is correct, I don't think that this is the correct action. Users should still have the option to opt-out of Telemetry on Nightly and Aurora, where it is enabled by default. The options should exist but be selected by default.

Comment 39

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

OK. That will make avoiding the problem from comment 28 rather complicated - maybe it's not worth avoiding.

Comment 40

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

(In reply to Gavin Sharp (use [e-mail] for email) from comment #28)
> Sorry, I should have noticed this earlier, but this will have the
> unfortunate side effect of disabling telemetry if you switch from
> Firefox->Nightly/Aurora->Firefox with the same profile... We should probably
> try to avoid that by using a separate pref.
> toolkit.telemetry.enabledByDefault?

Hmm. While I would like to keep Telemetry enabled when a user switches from Nightly/Aurora to Beta/Release I'm not sure that doing so would be correct from a privacy standpoint. Telemetry is opt-out on Beta/Release. Without the user having explicitly opted in on Nightly/Aurora I don't think that we can assume that they opt-in on Beta/Release.

Sid - From a privacy perspective, if a Nightly user with Telemetry enabled (due to our switch to opt-out on this channel) switches to Release, can we keep them opted in to Telemetry? Do we need to prompt them to opt-in on Release?

Comment 41

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Lawrence Mandel [:lmandel] from comment #40)
> Sid - From a privacy perspective, if a Nightly user with Telemetry enabled
> (due to our switch to opt-out on this channel) switches to Release, can we
> keep them opted in to Telemetry? Do we need to prompt them to opt-in on
> Release?

We need to re-prompt them (if we don't it is not opt-in).  If we don't, we can't make blanket statements like "Telemetry is opt-in for users on our release channel".

But we only have to prompt them once.  If they've clicked "yes", then go back to nightly, then come back to release, there's no need for a prompt.

I like one thing tchevalier said in comment 36:
> * Telemetry is disabled (on TelemetryPing.js) when telemetry.enabled 
> is false OR when enabledBydefault is true and telemetry.enabled has 
> a value set by the user.

Just brainstorming here: maybe it makes sense to have a three-state pref?  { unknown : on : off }.  "unknown" could indicate "on" in nightly/aurora, and "off" in beta/release.  Upon switch to beta/release, the user can be prompted, and the result is stored as "on" or "off" which is always honored (regardless of the channel).

Regardless of what we end up with, I think it's important to provide a UI for all builds so users can opt-out of telemetry on nightly and aurora.  Removing that UI is not a good idea.

-Sid

Comment 42

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

(In reply to Sid Stamm [:geekboy] from comment #41)
> Just brainstorming here: maybe it makes sense to have a three-state pref?  {
> unknown : on : off }.  "unknown" could indicate "on" in nightly/aurora, and
> "off" in beta/release.  Upon switch to beta/release, the user can be
> prompted, and the result is stored as "on" or "off" which is always honored
> (regardless of the channel).
> 
We already have this three state. It was added so that we would know whether we should reprompt when the Telemetry prompt is dismissed without clicking No.

Comment 43

[Sid Stamm [:geekboy or :sstamm]]

Fantastic!  Let's use it.

Comment 44

[Théo Chevalier [:tchevalier]]

Attachment: PatchV11

Thank you guys :) 
So:
- Now the pref UI is always displayed, and the checkbox is checked following the same logic used to activate telemetry (checked if telemetry.enabled is true, or if telemetry.enabledByDefault is true and telemetry.enabled hasn't been set by the user.).

- About prompting: Let's take the case where the user creates a fresh profile.

if telemetryEnabledByDefault is true: we are on Nightly/Aurora => we don't prompt anything, and telemetry.prompted remains to false.

The user moves to Beta/GA: telemetryEnabledByDefault and telemetry.prompted are false => we prompt, and telemetry.prompted becomes true.

Now, whatever the user do, we don't prompt again, because telemetry.prompted is true. But, anyways, he can modify the pref in the options.

Lawrence - Regarding comment 40, and comment 41, is that correct?

Additional question: If telemetryEnabledByDefault isn't set (It's only set for Nightly/Aurora, and after this patch), is it considered to be false? I think so, but I'm not sure.

Comment 45

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

(In reply to Théo Chevalier [:tchevalier] from comment #44)
> Now, whatever the user do, we don't prompt again, because telemetry.prompted
> is true. But, anyways, he can modify the pref in the options.
If the user has been prompted but the prompt was dismissed without an explicit yes/no decision we may prompt again. For the simple case this is correct.

> 
> Lawrence - Regarding comment 40, and comment 41, is that correct?
> 
Yes. 

> Additional question: If telemetryEnabledByDefault isn't set (It's only set
> for Nightly/Aurora, and after this patch), is it considered to be false? I
> think so, but I'm not sure.
This sounds right to me. The end goal is to have Telemetry enabled by default (out-out) on Night and Aurora and disabled (opt-in) on Beta and Release.

Comment 46

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

(In reply to Lawrence Mandel [:lmandel] from comment #40)
> Hmm. While I would like to keep Telemetry enabled when a user switches from
> Nightly/Aurora to Beta/Release I'm not sure that doing so would be correct
> from a privacy standpoint.

That actually wasn't the issue at all, sorry for the confusion - our preferences code only serialize values that differ from the default prefs (known as "user prefs"), so if we took the naive approach of just changing the default pref, users switching from Firefox with telemetry enabled (enabled=true is a user pref) to Nightly (enabled=true is a default pref) would lose the "enabled=true" as a user pref. That means that when they'd switch back to Firefox, telemetry would be disabled again. In reverse, the issue would be users disabling telemetry in Nightly (enabled=false is a user pref), switching to Firefox (enabled=false is a default pref) and then back to Nightly, where it would then be enabled again. This is only a potential issue when switching Nightly/Aurora->Firefox->Nightly/Aurora or Firefox->Nightly/Aurora->Firefox, which isn't common.

We're not going to take the naive approach, though, so all this is moot. It does mean the implementation is a little more complicated (need an additional pref).

I'm sorry that I've not been responsive to these review requests, I'm pretty swamped with some other bugs. I'll try to get this some attention soon, either from myself or someone else who can review.

Comment 47

[Théo Chevalier [:tchevalier]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #46)
> That actually wasn't the issue at all, sorry for the confusion - our
> preferences code only serialize values that differ from the default prefs
> (known as "user prefs"), so if we took the naive approach of just changing
> the default pref, users switching from Firefox with telemetry enabled
> (enabled=true is a user pref) to Nightly (enabled=true is a default pref)
> would lose the "enabled=true" as a user pref. That means that when they'd
> switch back to Firefox, telemetry would be disabled again. In reverse, the
> issue would be users disabling telemetry in Nightly (enabled=false is a user
> pref), switching to Firefox (enabled=false is a default pref) and then back
> to Nightly, where it would then be enabled again. This is only a potential
> issue when switching Nightly/Aurora->Firefox->Nightly/Aurora or
> Firefox->Nightly/Aurora->Firefox, which isn't common.
> 
> We're not going to take the naive approach, though, so all this is moot. It
> does mean the implementation is a little more complicated (need an
> additional pref).
> 
> I'm sorry that I've not been responsive to these review requests, I'm pretty
> swamped with some other bugs. I'll try to get this some attention soon,
> either from myself or someone else who can review.


No problem for the delay, Gavin, I can easily imagine that you have a lot of other bugs to work on. Your comments are always usefull :)

About the pref issue, I think this is prevalent for nightly and aurora users, because they often use the same profile with different channels. I'll fix that this weekend.

Just found one other (minor) issue with my patch:
> enabledBydefault = Services.prefs.getBoolPref(PREF_ENABLED_BY_DEFAULT);
Instead of enabledByDefault

Comment 48

[Théo Chevalier [:tchevalier]]

Attachment: PatchV12

First of all, sorry about the delay, some work to do, but now, I've two weeks free for Mozilla :)

I added the new pref: telemetry.chosen, to know everywhere if the user have made his choice by himself.

So, to sum up what we need, according to previous comments:

- We need to prompt one time (and, I guess, while he didn't made a choice?) if we switch Nightly/Aurora(EnabledByDefault) to Beta/Firefox, if the user didn't have already made a choice on this profile -> This is given by checking the new pref on nsBrowserGlue.js.

(NOTE: There is one issue for implementation: when the user chose, by any way, I need to enable telemetry.chosen. Found for the prompt, but I'll search how enable two pref on the same time with one checkbox on the pref pane.)

- We need to keep telemetry enabled (or disabled) across the channels when the user have made a choice, and obviously, while he didn't on Nightly/Aurora, we keep Telemetry enabled by default. (Ok regarding Sid's comment - comment 41)

That's what I understood, please correct me if I'm wrong.

In addition:
>    checkbox.checked = true;
>    if ((enabledByDefault.value == "true" && enabled.value == "false" && chosen.value == "true") ||
>       (enabledByDefault.value == "false" && enabled.value == "false") {
>      checkbox.checked = false;

This is not good, I know it, and it needs to be changed. This should be the exact opposite of that:
>    if ((enabledByDefault && !enabled && chosen) ||
>       (!enabledByDefault && !enabled))

(Because one disable telemetry, and the other check the checkbox)
Even with the help of my computer science professor, when I've invert the first condition, I got something completely wrong, so I wrote that temporarily.

FYI, the patch compiles, but the pref isn't checked by default on my build, dunno why.

Comment 49

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

(In reply to Théo Chevalier [:tchevalier] from comment #48)
> I added the new pref: telemetry.chosen, to know everywhere if the user have
> made his choice by himself.

If I understand what you're saying, this is a boolean preference to simply record whether a user has made a selection.
> 
> So, to sum up what we need, according to previous comments:
> 
> - We need to prompt one time (and, I guess, while he didn't made a choice?)

That's a good point. If the user has previously opted in/out of Telemetry we should respect their choice. In this case I don't see the need to notify the user. The short of this is only prompt if the user has not made an explicit choice.

Sid - Do you agree?

> if we switch Nightly/Aurora(EnabledByDefault) to Beta/Firefox, if the user
> didn't have already made a choice on this profile -> This is given by
> checking the new pref on nsBrowserGlue.js.
> 
> (NOTE: There is one issue for implementation: when the user chose, by any
> way, I need to enable telemetry.chosen. Found for the prompt, but I'll
> search how enable two pref on the same time with one checkbox on the pref
> pane.)
> 
> - We need to keep telemetry enabled (or disabled) across the channels when
> the user have made a choice, and obviously, while he didn't on
> Nightly/Aurora, we keep Telemetry enabled by default. (Ok regarding Sid's
> comment - comment 41)
> 
> That's what I understood, please correct me if I'm wrong.
> 
> In addition:
> >    checkbox.checked = true;
> >    if ((enabledByDefault.value == "true" && enabled.value == "false" && chosen.value == "true") ||
> >       (enabledByDefault.value == "false" && enabled.value == "false") {
> >      checkbox.checked = false;
> 
> This is not good, I know it, and it needs to be changed. This should be the
> exact opposite of that:
> >    if ((enabledByDefault && !enabled && chosen) ||
> >       (!enabledByDefault && !enabled))
> 
> (Because one disable telemetry, and the other check the checkbox)
> Even with the help of my computer science professor, when I've invert the
> first condition, I got something completely wrong, so I wrote that
> temporarily.
> 
> FYI, the patch compiles, but the pref isn't checked by default on my build,
> dunno why.

I think what you want here is:

if((enabled && chosen) || (!chosen && enabledByDefault) {
   ENABLE TELEMETRY
}
else {
   DISABLE TELMETRY
}

Comment 50

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Lawrence Mandel [:lmandel] from comment #49)
> That's a good point. If the user has previously opted in/out of Telemetry we
> should respect their choice. In this case I don't see the need to notify the
> user. The short of this is only prompt if the user has not made an explicit
> choice.
> 
> Sid - Do you agree?

Sounds right.

Comment 51

[Théo Chevalier [:tchevalier]]

(In reply to Lawrence Mandel [:lmandel] from comment #49)
>If I understand what you're saying, this is a boolean preference to simply
>record whether a user has made a selection.

Yes :)

> I think what you want here is:
> 
> if((enabled && chosen) || (!chosen && enabledByDefault) {
>    ENABLE TELEMETRY
> }
> else {
>    DISABLE TELMETRY
> }

Thanks Lawrence, but in the case of a current Beta/GA with Telemetry enabled, chosen doesn't exists, so Telemetry will be disabled and prompted again, no?
(It's the case !enabledByDefault, !chosen (isn't set yet), enabled)


(In reply to Sid Stamm [:geekboy] from comment #50)
> Sounds right.

Okay, thanks, so we keep Telemetry enabled across the channels if the user has made a choice.

Comment 52

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Théo Chevalier [:tchevalier] from comment #51)
> (In reply to Sid Stamm [:geekboy] from comment #50)
> > Sounds right.
> 
> Okay, thanks, so we keep Telemetry enabled across the channels if the user
> has made a choice.

... if that choice is "yes".  And we should probably keep it disabled if the user has chosen "no".

Comment 53

[Théo Chevalier [:tchevalier]]

(In reply to Sid Stamm [:geekboy] from comment #52)
> ... if that choice is "yes".  And we should probably keep it disabled if the
> user has chosen "no".

Yeah, so I'll redo all the conditions with that.

Ho, I just realized something obvious: chosen is true when enable or rejected is true, and false when enabled and rejected are false, so if I'm right, we don't need chosen. (So, I can use your condition, Lawrence, by using these pref and by adding last Sid's condition.)

Comment 54

[Théo Chevalier [:tchevalier]]

Attachment: PatchV13

* telemetry.chosen is now true when the user modify the checkbox on the pref pane. Set by function telemetryChosen(). But I need to improve it when the pref doesn't exists.

* We are going to re-prompt ALL the Beta/GA users since they don't have the chosen pref, and she's needed to avoid the prompt. (It's up to you to tell me if this is a problem or not.)

* If there is a user choice, this choice (enabled or disabled) is kept across the channels. (It was already ok, but I confirm last Sid's comment)

(Forget comment 53, we absolutely need the chosen pref)

Comment 55

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

(In reply to Théo Chevalier [:tchevalier] from comment #54)
> Created attachment 596907 [details] [diff] [review]
> PatchV13
> 
> * telemetry.chosen is now true when the user modify the checkbox on the pref
> pane. Set by function telemetryChosen(). But I need to improve it when the
> pref doesn't exists.

In what circumstance does that pref not exist?

> 
> * We are going to re-prompt ALL the Beta/GA users since they don't have the
> chosen pref, and she's needed to avoid the prompt. (It's up to you to tell
> me if this is a problem or not.)

This is a problem. We do not want to reprompt for an internal code change. At a minimum, we know that a user has clicked yes/no in the Telemetry prompt based on the toolkit.telemetry.prompted value. We also know that if Telemetry is enabled (toolkit.telemetry.enabled = true) the user has made a selection because Telemetry is disabled by default. (This is true across all channels at this point.) The case that we can't cover is when a user has explicitly gone to preferences and clicked to disable Telemetry. (Would need to check and then uncheck the box.) This should be a small subset (as this is the default pref) so we may be able to live with reprompting this crowd.

> 
> * If there is a user choice, this choice (enabled or disabled) is kept
> across the channels. (It was already ok, but I confirm last Sid's comment)
> 
> (Forget comment 53, we absolutely need the chosen pref)

Comment 56

[Théo Chevalier [:tchevalier]]

(In reply to Lawrence Mandel [:lmandel] from comment #55)
> In what circumstance does that pref not exist?

While the user have not made his choice, the pref doesn't exists. When he made his choice by changing the box, I do that:
var chosen = document.getElementById("toolkit.telemetry.chosen");
chosen.value = "true";

In advanced.js, all the pref are already created, but toolkit.telemetry.chosen doesn't exists yet, I have to do something like :

Services.prefs.setBoolPref("toolkit.telemetry.chosen", true);
or
pref("toolkit.telemetry.chosen", true);

But this isn't possible in this file without including some stuff.
Or, another solution is to set chosen to false in modules/libpref/src/init/all.js

> This is a problem. We do not want to reprompt for an internal code change.
> At a minimum, we know that a user has clicked yes/no in the Telemetry prompt
> based on the toolkit.telemetry.prompted value. We also know that if
> Telemetry is enabled (toolkit.telemetry.enabled = true) the user has made a
> selection because Telemetry is disabled by default. (This is true across all
> channels at this point.) The case that we can't cover is when a user has
> explicitly gone to preferences and clicked to disable Telemetry. (Would need
> to check and then uncheck the box.) This should be a small subset (as this
> is the default pref) so we may be able to live with reprompting this crowd.

Fixed :)
>    if (telemetryPrompted === TELEMETRY_PROMPT_REV &&
>       (telemetryEnabledByDefault || telemetryChosen || telemetryEnabled || telemetryRejected))
>      return;


+ added Services.prefs.clearUserPref(PREF_TELEMETRY_CHOSEN);
when we prompt/reprompt, since we can't keep telemetry.chosen to true if we ask the user. Like this, on Beta/GA, if there is a prompt and the user avoid the prompt (don't click on yes or no), telemetry is disabled.
We keep it enabled on Nightly/Aurora (enabledByDefault).

Comment 57

[Théo Chevalier [:tchevalier]]

Attachment: PatchV14

Comment 58

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

Comment on attachment 597067 [details] [diff] [review]
PatchV14

>     const PREF_TELEMETRY_PROMPTED = "toolkit.telemetry.prompted";
>     const PREF_TELEMETRY_ENABLED  = "toolkit.telemetry.enabled";
>+    const PREF_TELEMETRY_ENABLED_BY_DEFAULT = "toolkit.telemetry.enabledByDefault";
>+    const PREF_TELEMETRY_CHOSEN = "toolkit.telemetry.chosen";
>     const PREF_TELEMETRY_REJECTED  = "toolkit.telemetry.rejected";

toolkit.telemetry.rejected sounds to me like the user has performed the action of rejecting telemetry. In this case they've made a choice. Can we deduce the same information from toolkit.telemetry.enabled? If so, is the new toolkit.telemetry.chosen param required?

>-    // If the user has seen the latest telemetry prompt, do not prompt again
>+    // If the user has seen the latest telemetry prompt or if telemetry is
>+    // enabled by default, do not prompt again
>     // else clear old prefs and reprompt
>-    if (telemetryPrompted === TELEMETRY_PROMPT_REV)
>+    let telemetryEnabled = Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED);
>+    
>+    if (telemetryPrompted === TELEMETRY_PROMPT_REV &&
>+       (telemetryEnabledByDefault || telemetryChosen || telemetryEnabled || telemetryRejected))
>       return;

This doesn't look quite right to me. If the user has been prompted and has made a choice we do not reprompt regardless of whether telemetry is enabled or rejected. This bug introduces a new case that the user does not need to be prompted if telemetry is enabled by default.
    
if ((telemetryPrompted === TELEMETRY_PROMPT_REV && telemetryChosen) || telemetryEnabledByDefault)
   return;

Comment 59

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

Taras - Can you review the latest patch to ensure that the Telemetry prefs are being used correctly?

Comment 60

[(dormant account)]

(In reply to Lawrence Mandel [:lmandel] from comment #59)
> Taras - Can you review the latest patch to ensure that the Telemetry prefs
> are being used correctly?

Gavin knows those as well as I do, I trust his judgement over mine for any new pref code.

Comment 61

[Théo Chevalier [:tchevalier]]

(In reply to Lawrence Mandel [:lmandel] from comment #58)
> toolkit.telemetry.rejected sounds to me like the user has performed the
> action of rejecting telemetry. In this case they've made a choice. Can we
> deduce the same information from toolkit.telemetry.enabled? If so, is the
> new toolkit.telemetry.chosen param required?

That's what I was wondering to myself in comment 53, but I realized that telemetry.rejected is not modified if the user change the checkbox in the pref pane. If this is the only issue, we can simply fix it like that:
if the user uncheck the box, we can set telemetry.rejected to false in the telemetryChosen function instead of setting telemetry.chosen to true. =>Now we are able to know if the user have modified the pref.

But, this is not enough because telemetry.enabled have a default value (false), so in this case we can't determine if the user have made a choice. In definitive, we still need telemetry.chosen.

Gavin, could you confirm this please? Maybe I missed something.

> This doesn't look quite right to me. If the user has been prompted and has
> made a choice we do not reprompt regardless of whether telemetry is enabled
> or rejected. This bug introduces a new case that the user does not need to
> be prompted if telemetry is enabled by default.
>     
> if ((telemetryPrompted === TELEMETRY_PROMPT_REV && telemetryChosen) ||
> telemetryEnabledByDefault)
>    return;

Yes, much better, thanks.

Comment 62

[Théo Chevalier [:tchevalier]]

Attachment: PatchV15

- Added in nsBrowserGlue, TelemetryPing, advanced:
>    //If chosen pref isn't set, we can guess some cases where the user have made a choice
>    if (rejected || enabled)
>      chosen = true;
Now we check the chosen pref AND we set it to true when rejected or enabled are true.

- Included Services.jsm on advanced.js to use:
>Services.prefs.setBoolPref("toolkit.telemetry.chosen", true);

Comment 63

[Théo Chevalier [:tchevalier]]

Attachment: Patch V16

Yay, it works! No prompt for Nightly/Aurora, the checkbox is checked by default, telemetry is enabled by default, when the user uncheck the box and click "Ok", telemetry is disabled and his choice is saved, and if he check the box, telemetry is enabled and his choice is saved.
If on Nightly/Aurora, a user with EnabledByDefault go to Beta/GA, Telemetry is prompted. He makes his choice, he comes back to Nightly: his choice have been saved and is applied.

One issue: On the pref pane, when a user with telemetry.EnabledByDefault to true only check/uncheck the Telemetry checkbox, and without clicking on 'Ok' button: 
- telemetry.chosen becomes true
- telemetry.enabled is still false (default)
So only by checking/unchecking the box, it disable telemetry.

One thing I can't test: if Telemetry is really enabled or not.

Tell me if I've missed something. I made a try-build here if you want: https://tbpl.mozilla.org/?tree=Try&rev=ca0cf22081dc

Comment 64

[Théo Chevalier [:tchevalier]]

Sorry I made a mistake in my last comment, about the issue: it's by unchecking, then checking the box, that it disable telemetry. (The box is already checked when telemetry is enabled by default)

Comment 65

[Sonny Piers [:sonny]]

Comment on attachment 600731 [details] [diff] [review]
Patch V16

Théo, as said on IRC I'll take a look on this.

Comment 66

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

In thinking about the scenario when this lands on nightly, what will happen to the Telemetry preference for someone on a release/beta/aurora channel who opens their profile with nightly and then goes back to the prior channel. Will their preference be wiped? Do we need to land this on all channels concurrently? (Or as close to it as possible? - There should be no noticeable change to the beta/release audience.)

Comment 67

[Théo Chevalier [:tchevalier]]

(In reply to Lawrence Mandel [:lmandel] from comment #66)
> In thinking about the scenario when this lands on nightly, what will happen
> to the Telemetry preference for someone on a release/beta/aurora channel who
> opens their profile with nightly and then goes back to the prior channel.
> Will their preference be wiped? Do we need to land this on all channels
> concurrently? (Or as close to it as possible? - There should be no
> noticeable change to the beta/release audience.)

Since we guess the chosen value with:
>if (rejected || enabled)
>      chosen = true;
For these cases, there are no problem on current beta/GA (if telemetry is already enabled or disabled, it will stay like that.)

The only case where we are going to reprompt users on beta/GA is when users have checked then unchecked the box in the pref (and only in the pref), because when the checkbox is unchecked, telemtry.rejected don't change, so we aren't able to determine if we are in this case, or if we are in the default case. In fact, it's exactly what you said in comment 55.

Comment 68

[Théo Chevalier [:tchevalier]]

Oh, and we don't need that anymore in the doorhanger code, since we handle these cases.
>Services.prefs.setBoolPref(PREF_TELEMETRY_CHOSEN, true);

Comment 69

[Théo Chevalier [:tchevalier]]

Hum, in fact the crowd we are going to remprompt are all the people who have enabled telemetry (pref pane or doorhanger notification) and disabled it later with the pref pane. Maybe this crowd could be bigger that we was thinking. Is there a way to determine how many people we are going to reprompt?

Comment 70

[Théo Chevalier [:tchevalier]]

Attachment: Patch V17

As said, removing Services.prefs.setBoolPref(PREF_TELEMETRY_CHOSEN, true);

Bug 725407 is not applied yet here.

Sonny - What I try to do is to call gAdvancedPane.telemetryChosen(); only when toolkit.telemetry.enabled is changed. Thanks again for the feedback :)

Comment 71

[Sonny Piers [:sonny]]

Comment on attachment 601401 [details] [diff] [review]
Patch V17

I was going to tell you to use prefHasUserValue() but gavin already though about it in comment 46.

Ok so the problem is that the preference "toolkit.telemetry.chosen" needs to be updated following the pref "toolkit.telemetry.enabled".

I would say that a hidden checkbox for the pref "toolkit.telemetry.chosen" were the value would be dynamically updated to follow the value of the checkbox for "toolkit.telemetry.chosen" would be a good solution. Like the function updateHardwareAcceleration in advanced.js but were one of the pref would be hidden.

Comment 72

[Sonny Piers [:sonny]]

f+ of course for the great contributions you do!

Comment 73

[Théo Chevalier [:tchevalier]]

First of all, thanks a lot Sonny for you time and your explanations, very useful :)

So, with Sonny, we've discussed a while, and we ended with these conclusions:

- We don't need telemetry.chosen: we turn telemetry.rejected to true in the pref pane, so in any cases, when the user have made a choice, we have telemetry.enabled to true OR telemetry.rejected to true. (We are still going to reprompt people who have disabled telemetry by unchecking the pref pane, it will be great if we found a fix avoiding that, but I think it's not possible.)

- We use a second checkbox, but this one is hidden. It is used to set the telemetry.rejected to true when the primary checkbox (submitTelemetryBox) is unchecked.

I launch a try-build here: https://tbpl.mozilla.org/?tree=Try&rev=be6700c35c3d

I'll post the patch here if everything is okay, then requesting a review from Gavin.

Comment 74

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

(In reply to Théo Chevalier [:tchevalier] from comment #73)
> - We don't need telemetry.chosen: we turn telemetry.rejected to true in the
> pref pane, so in any cases, when the user have made a choice, we have
> telemetry.enabled to true OR telemetry.rejected to true. (We are still going
> to reprompt people who have disabled telemetry by unchecking the pref pane,
> it will be great if we found a fix avoiding that, but I think it's not
> possible.)

If I have understood correctly, the telemetry.enabled and telemetry.rejected preferences are currently only set by the opt-in notification. Setting Telemetry by clicking on the checkbox does not set these properties. If so, I don't see why we would differentiate between someone opting in or out based on the notification or the checkbox. 

If I've read this correctly, we're still only reprompting users who opted in and then went to the preference panel and unchecked the box to opt-out. Correct?

> 
> - We use a second checkbox, but this one is hidden. It is used to set the
> telemetry.rejected to true when the primary checkbox (submitTelemetryBox) is
> unchecked.

Is this a new or the existing mechanism?

> 
> I launch a try-build here:
> https://tbpl.mozilla.org/?tree=Try&rev=be6700c35c3d
> 
> I'll post the patch here if everything is okay, then requesting a review
> from Gavin.

Comment 75

[Théo Chevalier [:tchevalier]]

(In reply to Lawrence Mandel [:lmandel] from comment #74)
> (In reply to Théo Chevalier [:tchevalier] from comment #73)
> > - We don't need telemetry.chosen: we turn telemetry.rejected to true in the
> > pref pane, so in any cases, when the user have made a choice, we have
> > telemetry.enabled to true OR telemetry.rejected to true. (We are still going
> > to reprompt people who have disabled telemetry by unchecking the pref pane,
> > it will be great if we found a fix avoiding that, but I think it's not
> > possible.)
> 
> If I have understood correctly, the telemetry.enabled and telemetry.rejected
> preferences are currently only set by the opt-in notification. Setting
> Telemetry by clicking on the checkbox does not set these properties. If so,
> I don't see why we would differentiate between someone opting in or out
> based on the notification or the checkbox. 
> 
Setting Telemetry by clicking on the checkbox currently set telemetry.enabled, but not telemetry.disabled. Telemetry.disabled is currently only set by the prompt. I think that when the checkbox in the pref pane was added, telemetry.rejected was not set because it would have required what we've done here: a second checkbox. And when this was done, only telemetry.enabled was needed to know if we have to enabled it or not.

> If I've read this correctly, we're still only reprompting users who opted in
> and then went to the preference panel and unchecked the box to opt-out.
> Correct?

Yes, but only on Beta/GA. We are going to reprompt on Beta/GA when we can't determine if they are in the default case (telemetry.enabled and telemetry.rejected are both false) so we have to prompt them; or if they are in the case when they have disabled it by unchecking the checkbox (telemetry.enabled and telemetry.rejected are both false too).


> > - We use a second checkbox, but this one is hidden. It is used to set the
> > telemetry.rejected to true when the primary checkbox (submitTelemetryBox) is
> > unchecked.
> 
> Is this a new or the existing mechanism?

It's new: when existing checkbox is checked, a javascript function check an hidden checkbox, so now we can update telemetry.enabled and telemetry.rejected at the same time.


+ Gah, I just realized we made something wrong with Sonny. Since we are using enabled and rejected, if a Nightly/Aurora user have previously disabled telemetry with the prompt (and only with the prompt): rejected is true, so we are not going to enabled it by default, and we WANT to enabled it by default, even if they have previously disabled it. So coming back to telemetry.chosen, because it's a new pref, so nobody have it on true: we can enabled Telemetry on Nightly/Aurora, whatever the user have previously chosen. Like this, we can, after the bug landed,  disregard previous choices (EnabledByDefault on N/A), but consider future choices. (N/A users are still able to disabled it) 

But we should still guess chosen (In addition by checking the pref itself) by checking telemetry.enabled and telemetry.rejected ONLY when telemetry.enabledByDefault is false.

I don't know, if I'm really clear, feel free to ask me ;)


+ Investigating our pref, I saw that:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#371

Seems that Native Fennec uses it's own telemetry prompt, so if this is not "dead code", I should update that too.

You see an iceberg? This bug is exactly the same thing :)

Comment 76

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

(In reply to Théo Chevalier [:tchevalier] from comment #75)
> + Gah, I just realized we made something wrong with Sonny. Since we are
> using enabled and rejected, if a Nightly/Aurora user have previously
> disabled telemetry with the prompt (and only with the prompt): rejected is
> true, so we are not going to enabled it by default, and we WANT to enabled
> it by default, even if they have previously disabled it. So coming back to
> telemetry.chosen, because it's a new pref, so nobody have it on true: we can
> enabled Telemetry on Nightly/Aurora, whatever the user have previously
> chosen. Like this, we can, after the bug landed,  disregard previous choices
> (EnabledByDefault on N/A), but consider future choices. (N/A users are still
> able to disabled it) 

I think you're close here but if a user has made an explicit choice, even if that choice is to opt-out, I think we need to honour it. We should not discard their choice just because it's not the one we want them to make. :)

> But we should still guess chosen (In addition by checking the pref itself)
> by checking telemetry.enabled and telemetry.rejected ONLY when
> telemetry.enabledByDefault is false.

See above about respecting the user's choice.

> + Investigating our pref, I saw that:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#371
> 
> Seems that Native Fennec uses it's own telemetry prompt, so if this is not
> "dead code", I should update that too.

Yes. AFAIK there is separate code for the Fennec prompt. The opt-in and opt-out messages may differ between mobile and desktop (due to space constraints) but the functionality should be the same.

Comment 77

[Théo Chevalier [:tchevalier]]

(In reply to Lawrence Mandel [:lmandel] from comment #76)
> I think you're close here but if a user has made an explicit choice, even if
> that choice is to opt-out, I think we need to honour it. We should not
> discard their choice just because it's not the one we want them to make. :)

Hum, so we are not going to enabled it for all the Nightly/Aurora users. If I follow you on Nightly/Aurora, we just want to enabled it:
- for new profiles
- when no choice was made
- when it was already enabled
Right?

So, we are going to have the same problem we have for prompting beta/GA who have disabled it by unchecking the box: we can't determine who should be enabled it by default and who have disabled it by unchecking the box. So we are going to enabled it for users who have disabled telemetry by unchecking the box and shouldn't be enabled by default.

And finally, we can do it without telemetry.chosen.

Comment 78

[Robert Kaiser]

(In reply to Théo Chevalier [:tchevalier] from comment #75)
> I think that when the checkbox in the pref
> pane was added, telemetry.rejected was not set because it would have
> required what we've done here: a second checkbox.

I don't think you need an actual checkbox, AFAIK, you should be able to just change the value of the <preference> from an (onchange?) event handler on the checkbox.

Comment 79

[Robert Kaiser]

(In reply to Théo Chevalier [:tchevalier] from comment #77)
> So, we are going to have the same problem we have for prompting beta/GA who
> have disabled it by unchecking the box: we can't determine who should be
> enabled it by default and who have disabled it by unchecking the box. So we
> are going to enabled it for users who have disabled telemetry by unchecking
> the box and shouldn't be enabled by default.

IMHO, that's an edge case we can live with (i.e. people who enabled it actively and then disabled via the checkbox). IIRC, we tell them anyhow that we are enabling it, right?

Comment 80

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

(In reply to Robert Kaiser (:[e-mail]) from comment #79)
> (In reply to Théo Chevalier [:tchevalier] from comment #77)
> > So, we are going to have the same problem we have for prompting beta/GA who
> > have disabled it by unchecking the box: we can't determine who should be
> > enabled it by default and who have disabled it by unchecking the box. So we
> > are going to enabled it for users who have disabled telemetry by unchecking
> > the box and shouldn't be enabled by default.
> 
> IMHO, that's an edge case we can live with (i.e. people who enabled it
> actively and then disabled via the checkbox). IIRC, we tell them anyhow that
> we are enabling it, right?

Yes. We're talking about the users on Nightly and Aurora who have explicitly opted in to Telemetry and then gone into the preference panel and disabled it. I don't have numbers on the size of this group but I would expect this to be a very small edge case.

To summarize:
- Any user that has opted in to Telemetry should remain opted in on all channels.
- Any user that has opted out of Telemetry should remain opted out on all channels.
- Any user that has not made a choice wrt Telemetry should be opted out on Release/Beta and opted in on Nightly/Aurora.
* Any user that has opted in and then subsequently opted out of Telemetry will follow the same path as a user that has not made a selection and be reprompted.

Comment 81

[Théo Chevalier [:tchevalier]]

(In reply to Robert Kaiser (:[e-mail]) from comment #78)
> (In reply to Théo Chevalier [:tchevalier] from comment #75)
> > I think that when the checkbox in the pref
> > pane was added, telemetry.rejected was not set because it would have
> > required what we've done here: a second checkbox.
> 
> I don't think you need an actual checkbox, AFAIK, you should be able to just
> change the value of the <preference> from an (onchange?) event handler on
> the checkbox.

Yes, it will works on Linux and OS X, but not on Windows. I explain myself: on Linux and OS X, preferences are "instant apply", so basically, when you check a box the associated pref is immediately changed. But not on Windows: the associated pref is changed only when the user click on the "OK" button. So this is why with Sonny, we think it is the best solution to add a second hidden box, to be consistent with others preferences on Windows.

Comment 82

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

I'm sorry I haven't been able to track this work very closely and be more active here. I definitely appreciate all the work you've done to investigate various approaches, Théo. I really want to get this bug fixed, since it will be really nice to get the extra Nightly/Aurora telemetry data.

I think the edge cases related to users switching back/forth between builds are making this more complicated than it needs to be. I just thought about another approach that seems like it could be simpler:

- at build time, define a MOZ_TELEMETRY_ON_BY_DEFAULT define, which is controlled by the update channel (MOZ_UPDATE_CHANNEL) (true for "nightly" or "aurora", false otherwise)
- have this build time constant control which of the two prefs (toolkit.telemetry.enabledByDefault or toolkit.telemetry.enabled) exist and are used in the given build, using preprocessor defines. All current uses of .enabled (in the service, in the prompt code, in prefs, etc.) would instead use enabledByDefault in builds with MOZ_TELEMETRY_ON_BY_DEFAULT defined.

Using a different pref name for both situations means that user preferences would never be lost when switching between one build and the other (e.g. switching back and forth between Nightly/release builds won't lose your "opted in" status). It also means that user preferences might not be migrated across builds (regardless of what you configure in a release build, you'll be treated like a new user when switching to Nightly, and vice-versa, and manual configuration in one or the other only applies to that version), but I think that's a fine compromise to make for what should be a much simpler implementation, given that people switching between channels frequently is an edge case.

What do you think? Happy to discuss this idea further on IRC if it isn't clear, or if I'm missing something important.

Comment 83

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

I think you have a good idea to simplify the implementation and I agree that switching channels is an edge case.

Comment 84

[Théo Chevalier [:tchevalier]]

Yeah, awesome Gavin, I think this will be easier now, thanks!
I'll begin to work this week-end, furthermore, I'll add the code for mobile.

Comment 85

[Théo Chevalier [:tchevalier]]

Attachment: Patch V18 (New approach)

This patch include stuff for Mobile, and the new approach discussed with Gavin.

Gavin, Lawrence - I don't know if you've already discussed about the lost of the user choice (Only one time, when we land this) on Nightly/Aurora with this approach?

I think I handle all the cases when we use telemetry.enabled, but the define MOZ_TELEMETRY_ON_BY_DEFAULT don't work yet.

Should we turn on telemetry.rejected when a user uncheck the box on desktop and Mobile? This is NOT needed for this bug since the new approach, but I think it will be great to have this saved for futures implementations, do determine the user choice and avoid the issues found here. What do you think?

Comment 86

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

(In reply to Théo Chevalier [:tchevalier] from comment #85)
> Gavin, Lawrence - I don't know if you've already discussed about the lost of
> the user choice (Only one time, when we land this) on Nightly/Aurora with
> this approach?

I'm OK with this. Nightly/Aurora is a dev/test audience. Change like this is more acceptable on these channels.

> Should we turn on telemetry.rejected when a user uncheck the box on desktop
> and Mobile? This is NOT needed for this bug since the new approach, but I
> think it will be great to have this saved for futures implementations, do
> determine the user choice and avoid the issues found here. What do you think?

This seems like the right approach to me but I'll defer to Gavin for the code review.

Comment 87

[Théo Chevalier [:tchevalier]]

Attachment: Patch V19

Need to check if this define works or not.

The checkbox hack still doesn't work, and we need to find a way to do a "oncommand", or "onchange" event for mobile pref.

Comment 88

[Théo Chevalier [:tchevalier]]

Attachment: Patch V20

I launched a try-server here: https://tbpl.mozilla.org/?tree=Try&rev=4cba933e7f8c
(Note: I forced the define to work in the try server patch.)

We need to check with this build if toolkit.telemetry.rejected and toolkit.telemetry.enabled are both applied in the same time on Windows.

Comment 89

[Théo Chevalier [:tchevalier]]

Attachment: Patch V21

We have split that bug. Now, here, we treat only enabling telemetry by default on Firefox Desktop.
So, here is the patch.

The only thing, IMHO, who needs to be fixed before review and landing is:
+if test "$MOZ_UPDATE_CHANNEL" = nightly -o
+  test "$MOZ_UPDATE_CHANNEL" = aurora; then

Comment 90

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

Comment on attachment 607751 [details] [diff] [review]
Patch V21

>diff --git a/browser/branding/aurora/pref/firefox-branding.js b/browser/branding/aurora/pref/firefox-branding.js
>diff --git a/browser/branding/nightly/pref/firefox-branding.js b/browser/branding/nightly/pref/firefox-branding.js

I realized that we can actually just set this in all.js based on MOZ_TELEMETRY_ON_BY_DEFAULT.

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

>+    let telemetryPrompted = null, telemetryRejected = false,
>+    telemetryChosen = false,
>+    telemetryEnabled = Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED);

nit: this formatting is confusing, it looks like telemetryChosen is undeclared. Just use a seperate "let" for each variable, on its own line.

>+    if (telemetryRejected || telemetryEnabled)
>+      telemetryChosen = true;

>+    if (telemetryPrompted === TELEMETRY_PROMPT_REV && telemetryChosen)

Seems like this could be combined into one line (and eliminate the "telemetryChosen" variable). This change in logic would be a nice thing to also split off into a followup bug, I think, since it isn't strictly related to this bug.

>diff --git a/configure.in b/configure.in

>+# Enable Telemetry by default for nightly and aurora channels
>+if test "$MOZ_UPDATE_CHANNEL" = nightly -o
>+  test "$MOZ_UPDATE_CHANNEL" = aurora; then
>+    AC_DEFINE(MOZ_TELEMETRY_ON_BY_DEFAULT)
>+fi

I think you might need to put this after the MOZ_UPDATE_CHANNEL assignment later in the file? Did this end up working?

The patch needs to be updated to tip (nsBrowserGlue pieces no longer apply).

Comment 91

[Théo Chevalier [:tchevalier]]

Attachment: Patch V22

All the changes you requested ;)

About configure.in:
My change is at the right place, the MOZ_UPDATE_CHANNEL assignment is line 6433, whereas my change is line 8556. I tried it with only AC_DEFINE(MOZ_TELEMETRY_ON_BY_DEFAULT)
And all was working, so, there is an issue in the "if" conditional syntax, but I'm not confident with this language.

Once this is fixed, I think we're done :)

Comment 92

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

I think you want:

if test "$MOZ_UPDATE_CHANNEL" = nightly -o "$MOZ_UPDATE_CHANNEL" = aurora; then
  AC_DEFINE(MOZ_TELEMETRY_ON_BY_DEFAULT)
fi

Comment 93

[Théo Chevalier [:tchevalier]]

Yeah look better, thanks :)
I launched a try: https://tbpl.mozilla.org/?tree=Try&rev=f467706817d3

With
+if test "$MOZ_UPDATE_CHANNEL" = nightly -o
+  "$MOZ_UPDATE_CHANNEL" = aurora -o
+  "$MOZ_UPDATE_CHANNEL" = default; then

to make it works on try.

Comment 94

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

Ah, you'll also need quotes. This should work:

if test "$MOZ_UPDATE_CHANNEL" = "nightly" -o \
        "$MOZ_UPDATE_CHANNEL" = "aurora" -o \
        "$MOZ_UPDATE_CHANNEL" = "default"; then
  AC_DEFINE(MOZ_TELEMETRY_ON_BY_DEFAULT)
fi

Comment 95

[Théo Chevalier [:tchevalier]]

Attachment: Patch V23

Hi, sorry, was sleeping, then school...
Thanks for your responsiveness. Here is the try: https://tbpl.mozilla.org/?tree=Try&rev=533eda55e5e8
Hope this is the last :)

Comment 96

[Théo Chevalier [:tchevalier]]

Ok, it works! Ready for review/landing.

Comment 97

[Théo Chevalier [:tchevalier]]

Attachment: Patch V24

Removing "$MOZ_UPDATE_CHANNEL" = "default", we don't need it anymore ;)

Comment 98

[Théo Chevalier [:tchevalier]]

Attachment: Patch V25

I've forgot to remove telemetryChosen declaration.

Comment 99

[Théo Chevalier [:tchevalier]]

Attachment: Patch V26

Changes:

 if test "$MOZ_TELEMETRY_REPORTING"; then
-    AC_DEFINE(MOZ_TELEMETRY_REPORTING)
+  AC_DEFINE(MOZ_TELEMETRY_REPORTING)
+  # Enable Telemetry by default for nightly and aurora channels
+  if test "$MOZ_UPDATE_CHANNEL" = "nightly" -o \
+      "$MOZ_UPDATE_CHANNEL" = "aurora"; then
+      AC_DEFINE(MOZ_TELEMETRY_ON_BY_DEFAULT)
+  fi
 fi

And in aboutDialog.xul:

-#ifdef MOZ_TELEMETRY_REPORTING
+#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
               &warningDesc.telemetryDesc;
 #endif

Like this, telemetry is only set "on by default" when MOZ_TELEMETRY_REPORTING is defined.
First, it's more logic, and furthermore, the telemetry warning in aboutDialog is only displayed on Nightly and Aurora channels. So, it will never be displayed on others channels where telemetry is not enabled by default (UX, default, etc.)

Comment 100

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

Comment on attachment 612227 [details] [diff] [review]
Patch V26

Théo: apologies for your great work here being left without feedback for so long - other priorities got in the way and we were blocked on getting sign-off to make this change, as we discussed in email.

We're close to having the green light to flip this switch, so if you're interested in picking this back up, it would be great to get an updated patch that applies cleanly to trunk. If you're not available to do that, let me know and I can find someone else to help.

Comment 101

[Théo Chevalier [:tchevalier]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #100)
> Comment on attachment 612227 [details] [diff] [review]
> Patch V26
> 
> Théo: apologies for your great work here being left without feedback for so
> long - other priorities got in the way and we were blocked on getting
> sign-off to make this change, as we discussed in email.

No problem, I'm really happy to see that we still care about this feature :)

> 
> We're close to having the green light to flip this switch, so if you're
> interested in picking this back up, it would be great to get an updated
> patch that applies cleanly to trunk. If you're not available to do that, let
> me know and I can find someone else to help.

I think I can update this patch, and the patch on Bug 737596 (the same, but for mobile)
IIRC, we need a part in Java to update an hidden pref on Mobile, and to be honest, I'm not able to do that part. I think this is Bug 737600.

How much time do we have ?

Comment 102

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

(In reply to Théo Chevalier [:tchevalier] from comment #101)
> I think I can update this patch, and the patch on Bug 737596 (the same, but
> for mobile)
> IIRC, we need a part in Java to update an hidden pref on Mobile, and to be
> honest, I'm not able to do that part. I think this is Bug 737600.

Sounds good - I don't think we need to worry about flipping this switch at the same time on mobile/desktop, so it may make sense to tackle just desktop first (since I think that's easiest).

> How much time do we have ?

To actually land the patch, we're blocked on bug 701178 and a blog post announcing the change (which is being drafted). Those should hopefully be ready within 2 weeks, so there's no huge rush, but it wouldn't hurt to have the patch here ready and reviewed before that.

Comment 103

[Théo Chevalier [:tchevalier]]

Attachment: Patch V27

What I tried:
- Nightly, new profile:
Checkbox is checked, telemetry.enabledByDefault is true, opt-out notification displayed. (Everything is ok)

- GA, old profile, choice made: (I chose to disable telemetry, from the panel)
When coming back to Nightly, telemetry is enabledByDefault, the opt-out notification is shown. (Which is expected since we are treating Nightly/Aurora independently of Beta/GA) So back to GA, telemetry was still disabled

Also, about the patch:
- I now handle the new in-content pref page. (I checked, it works, for us it was the exact same code than in the current panel)
- We already are handling all use of telemetry.enabled

Gavin, I think we can also land the patch for mobile, it was working. (I will rebase it). The only thing that is missing is bug 737600. 
I'll attach a patch for desktop there.

I launched a try build https://tbpl.mozilla.org/?tree=Try&rev=3938ae35e58c for you

Comment 104

[Théo Chevalier [:tchevalier]]

For beta/GA only, we have:

+ if (telemetryPrompted === TELEMETRY_PROMPT_REV &&
+    (telemetryRejected || telemetryEnabled))
return;

We are gonna reprompt users who don't want to answer to the prompt at *each* restart, until they choose. I think we don't want that. But I can't see any fix, unless reverting to the old one: if (telemetryPrompted === TELEMETRY_PROMPT_REV) return;

Gavin, btw, I finished the work for Mobile,(opt-out notification + updating telemetry.rejected) an event has been added on the pref since last time.

Comment 105

[Théo Chevalier [:tchevalier]]

Attachment: Patch V28

telemetry.rejected is a shared pref between Nightly/Aurora and Beta/GA.

So, if telemetry is, or has been, disabled on beta/GA (for now, only by the prompt), we have for Nightly/Aurora:
telemetry.disabled == true
telemetry.enabled == true

It's ok, but we should honor disabled pref rather than enabled. I know we are treating telemetry state differently across channels, but since we can trust disabled state, I think we can disable telemetry everywhere if disabled is true, and in that case, we have nothing to prompt/display. (See the fix in telemetryPing.js)

If, for instance, someone disable it everywhere in one click, he will need to re-enable it separately. That may be not clear for users... but since it is to enable and not to disable, it's not a big deal?

Plus, about having true to two pref, it will cause troubles when we toggle values. We will need to carefully review that.

I'm pretty sure there is other cases like that where we are wrong.

Fixing an edge case where after installing Nightly/Aurora, user disable telemetry at first start. At second start we display the opt-out notification saying Nightly is sending data, whereas telemetry is disabled.
+    if (telemetryNotifiedOptOut || telemetryRejected || !telemetryEnabled)
+      return;

Also about string changes, 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.

Comment 106

[Théo Chevalier [:tchevalier]]

In fact we can't hardcode "Firefox", as Sid said in https://bugzilla.mozilla.org/show_bug.cgi?id=725407#c6

Matej, Sid, can we find an other solution?
Maybe define a new optional ReleaseBrandShortName variable (which could be used in other part, there is a lot of hardcoded "Firefox" http://transvision.mozfr.org/?sourcelocale=en-US&locale=fr&repo=release&recherche=Firefox )
If other project have no ReleaseBrandShortName, we use regular brandShortname instead.
What do you think?

Comment 107

[Matej Novak [:matej]]

I can't speak to this from a coding or implementation perspective, but I'm confused as to why we can't say "Firefox" in the second part of the line. All products serve to ultimately make Firefox better, so that's the language we should be using, as we used to do.

Comment 108

[Théo Chevalier [:tchevalier]]

Attachment: Patch V29

Thanks for your anwser Matej, but I talk to Margaret & mfinkle over IRC about harcoded strings: since we already have a lot of hardcoded "Firefox", it's not a big deal. So we can use "Firefox" :)

Following a discussion with Sid, we stated that we need to be able to re-notify existing Nightly/Aurora users if we bump up TELEMETRY_DISPLAY_REV.
To do that, our pref toolkit.telemetry.notifiedOptOut now contain the rev number, like we do for the prompt.


Finally, it seems that we are not consistent between mobile & desktop about the prompt: in mobile, we do NOT consider the prompt as displayed until the user answer. (So we currently have the infinite prompt I talked about if they don't want to answer)
Gavin, is that fine, or we should change that?

Comment 109

[Théo Chevalier [:tchevalier]]

Attachment: Patch V30

When changing telemetry state from pref pane, we also need to update TELEMETRY_DISPLAY_REV (which is telemetry.prompted or telemetry.notifiedOptOut)
It's needed for at least this case:
- <any channel> first run > going to pref pane > disable telemetry > relaunch. (We don't want to display anything if it's disabled)

For desktop we need the rev number in advanced.js, and for mobile in browser.js so I defined TELEMETRY_DISPLAY_REV in configure.in like this:
+AC_SUBST(MOZ_TELEMETRY_DISPLAY_REV, 2)
(Not sure about where I'm supposed to put this line into the file)

Removing that, because it was a bad idea:
-    if (!enabled) {
+    if (!enabled || rejected) {

Again, if someone disable telemetry by the prompt then enable it after via the pref, it's disabled.


Order to apply patches:
bug 699806 > bug 737600 (desktop) > bug 737596 > bug 725987 > bug 737600 (mobile)

I *think* we are good. (I know, already said that, but maybe it's true this time)

Comment 110

[Théo Chevalier [:tchevalier]]

Attachment: Patch V31

about:telemetry page has landed recently, so I updated the patch.

Comment 111

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

Comment on attachment 680482 [details] [diff] [review]
Patch V31

Hey Théo - thanks again for your work here. Marco has offered to help finish the review, so I'm going to forward the request to him.

Comment 112

[Théo Chevalier [:tchevalier]]

Cool, thanks :) Marco, I will request reviews from you on all the desktop patches, and from mfinkle for mobile patches.

Some tests are using toolkit.telemetry.prompted (http://mxr.mozilla.org/mozilla-central/search?string=toolkit.telemetry.prompted), I don't really know if desktop is concerned, so I will update patch on bug 737596.

Btw, we can land desktop & mobile at the same time.

Comment 113

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

One thing I mentioned in email, but forgot to put in the bug: we should plan to initially land this "preffed off", since we're still blocked on some communication stuff (blog posts etc.) before we can fully enable this. Landing the majority of the patch will mean that we can land a very simple change to actually activate it later on, once we're ready to go. That probably just means adjusting configure.in so that MOZ_TELEMETRY_ON_BY_DEFAULT is not defined when this lands.

Comment 114

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

(In reply to Théo Chevalier [:tchevalier] from comment #105)
> Also about string changes, 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 don't think I agree. If someone is using IceWeasel, it's probably more useful to tell them that the information will be improving IceWeasel, even if that happens indirectly. It's also possible for someone making custom builds to specify their own Telemetry server info, in which case saying that the data is improving "Firefox" would be misleading. Either way, this is kind of an edge case, so I think we should just default to doing what we normally do and using brandShortName.

Comment 115

[Matej Novak [:matej]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #114)
> (In reply to Théo Chevalier [:tchevalier] from comment #105)
> > Also about string changes, 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 don't think I agree. If someone is using IceWeasel, it's probably more
> useful to tell them that the information will be improving IceWeasel, even
> if that happens indirectly. It's also possible for someone making custom
> builds to specify their own Telemetry server info, in which case saying that
> the data is improving "Firefox" would be misleading. Either way, this is
> kind of an edge case, so I think we should just default to doing what we
> normally do and using brandShortName.

I'll defer to you on that one if you feel strongly about it, I just find it strange that we're solving for these edge cases rather than the bigger picture (i.e. that our release channels all serve to make the final product better).

Comment 116

[Robert Kaiser]

I agree with Gavin's comment #14, esp. as IMHO every string with "Firefox" in it outside of branding directories is a bug. People doing custom builds we do not sign off on (the Iceweasel case but applies even to way more strongly changed builds) are not allowed to use our trademarks, and the way to deal with that is using a branding directory using different names. If we do not concentrate all our usage of our trademarks there, we quickly render this mechanism unusable.

Comment 117

[Marco Bonardo [:mak]]

Sorry if it took a bit of time to take off, but there's lot of information to gather from all of these bugs (1 year of comments negating each other hurts), I just finished going through it and reordering my ideas on the issue.

In some comments I read about a telemetry.disabled pref, but this pref doesn't seem to exist anywhere, I assumed this was just a typo for telemetry.rejected.

For Firefox desktop I agree with both Gavin and Robert we should never hardcode Firefox. And I agree we are making the problem bigger than it it, just keep using brandShortName, testers in these channels are unlikely to ask themselves what Aurora or Nightly are.
Mobile is a case apart since custom builds are unlikely, they may hardcode Firefox if peers are fine with that, imo.

I'm now starting looking at the actual code.

Comment 118

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

(In reply to Matej Novak [:matej] from comment #115)
> I'll defer to you on that one if you feel strongly about it, I just find it
> strange that we're solving for these edge cases rather than the bigger
> picture (i.e. that our release channels all serve to make the final product
> better).

You're right, that particular case (MoCo's Nightly/Aurora builds not saying "Firefox") is strange. But this goes back to that issue we have with branding packages, where our Nightly/Aurora builds aren't the only users of the "Nightly"/"Aurora" branding packages. Once we fix that underlying issue, a lot of this branding confusion will go away, and we can get the exact behavior you're asking for. But for now, I think we need to go with the conceptually simple solution.

I filed bug 812247 to track the underlying problem that's causing all these branding headaches.

Comment 119

[Théo Chevalier [:tchevalier]]

Attachment: Patch V32

Marco, thanks for your time, I know all this stuff is not crystal clear.
telemetry.disabled was indeed a typo, sorry for the confusion.

If you started reviewing patch V31, keep going on, I'll backport your comments in a new one.

Here is what's new:

1) Since patch V29 I forgot to re-add that in TelemetryPing.js:

+#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
+const PREF_ENABLED = "toolkit.telemetry.enabledPreRelease";
+#else
+const PREF_ENABLED = "toolkit.telemetry.enabled";
+#endif

2) I replaced toolkit.telemetry.enabledByDefault with toolkit.telemetry.enabledPreRelease (See https://bugzilla.mozilla.org/show_bug.cgi?id=737596#c10)

3) Commented  # AC_DEFINE(MOZ_TELEMETRY_ON_BY_DEFAULT) in configure.in to pref-off

4) As discussed, reverting to &brandShortName; until bug 812247 is fixed

Comment 120

[Marco Bonardo [:mak]]

Comment on attachment 682392 [details] [diff] [review]
Patch V32

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

::: browser/components/nsBrowserGlue.js
@@ +858,3 @@
>                                       true, url, clickCallback);
>      }
>      catch (e) {

nsBrowserGlue.js needs #filter substitution
looks like you just moved it to the wrong queued patch

@@ +905,5 @@
>        description.appendChild(link);
>        return link;
>      }
>  
> +    // Opt-out notification when enabled by default, else opt-in prompt

"Display an opt-out notification when telemetry is enabled by default, an opt-in prompt otherwise.

@@ +915,3 @@
>      } catch(e) {}
> +    if (telemetryNotifiedOptOut === TELEMETRY_DISPLAY_REV || !telemetryEnabled)
> +      return;

you can unify the code checking Prompted/Notified pref and early returning in case it's === TELEMETRY_DISPLAY_REV out of the ifdef/else, just moving the telemetryEnabled check outside of it.
You can use just telemetryNotified or telemetryDisplayed for the var name, the subtle difference with prompted doesn't look valuable.

The !telemetryEnable check needs a separate comment, stating which cases it's guarding for.  At first glance seems to cover just the case user flips the pref in about:config? Any other case worth to mention in such comment?

@@ +921,2 @@
>  
> +    Services.prefs.setIntPref(PREF_TELEMETRY_DISPLAYED, TELEMETRY_DISPLAY_REV);

the code from here on (included this setIntPref) could be unified at the end of the ifdef/else just defining some temp vars for the notification arguments (buttons and hideCloseButton). You may define the listener callback function separately (learnMoreClickHandler) since looks like those are quite different.

@@ +930,5 @@
> +      tabbrowser.selectedTab = tabbrowser.addTab(url);
> +      // Remove the notification on which the user clicked
> +      notification.parentNode.removeNotification(notification, true);
> +    }, false);
> +    return;

This early return is pointless, the ifdef/else goes through the end of the method, and we may unify some code as said above

@@ +946,3 @@
>      Services.prefs.clearUserPref(PREF_TELEMETRY_ENABLED);
>      
> +    var telemetryPrompt = browserBundle.formatStringFromName("telemetryOptInPrompt",

since you are reindenting this code, please remove the trailing spaces

@@ +980,5 @@
>        // Add a new notification to that tab, with no "Learn more" link
>        notifyBox = tabbrowser.getNotificationBox();
>        appendTelemetryNotification(telemetryPrompt, buttons, true);
>      }, false);
> +#endif

so, unifying code, after this you'd just have:
 
Services.prefs.setIntPref(PREF_TELEMETRY_DISPLAYED, TELEMETRY_DISPLAY_REV);
		 
let notification = appendTelemetryNotification(telemetryPrompt, buttons, hideCloseButton);
let link = appendLearnMoreLink(notification);
link.addEventListener('click', learnModeClickHandler, false);

::: configure.in
@@ +8512,5 @@
>  AC_SUBST(MOZ_PKG_SPECIAL)
>  
>  AC_SUBST(MOZILLA_OFFICIAL)
>  
> +AC_DEFINE_UNQUOTED(MOZ_TELEMETRY_DISPLAY_REV,"2")

please add whitespace after comma

@@ +8521,5 @@
> +  # Enable Telemetry by default for nightly and aurora channels
> +  if test "$MOZ_UPDATE_CHANNEL" = "nightly" -o \
> +      "$MOZ_UPDATE_CHANNEL" = "aurora"; then
> +  #    AC_DEFINE(MOZ_TELEMETRY_ON_BY_DEFAULT)
> +  fi

the indentation code style in configure.in is 4 spaces, please respect it

please also add a comment line above the commented out line specifying why it's temporarily commented out (what it's waiting for basically).

Comment 121

[Marco Bonardo [:mak]]

Theo, do you have any news on these bugs or an ETA on updated patches? Do you need help with anything?

Comment 122

[Théo Chevalier [:tchevalier]]

Hi Marco, sorry about the delay (had two foss event & l10n stuff).

This patch is ready, but I still need to update toolkit.telemetry.rejected on about:config (I'll put that on bug bug 737600 so that you can review this one)

Comment 123

[Théo Chevalier [:tchevalier]]

Attachment: Patch V33

Adressing Mak's review.

I don't know why, but @MOZ_TELEMETRY_DISPLAY_REV@ is now replaced by "2" instead of only 2. So if (telemetryDisplayed === TELEMETRY_DISPLAY_REV) is always false.

I just added a space after the coma here: AC_DEFINE_UNQUOTED(MOZ_TELEMETRY_DISPLAY_REV, "2")
So maybe we want AC_DEFINE_UNQUOTED(MOZ_TELEMETRY_DISPLAY_REV, 2)?

Comment 124

[Théo Chevalier [:tchevalier]]

Attachment: Patch V34

It was the quotes, AC_DEFINE_UNQUOTED(MOZ_TELEMETRY_DISPLAY_REV, 2) is okay.
Also, commented if ... fi statement too, because build fails if not.

I see nothing else...

Comment 125

[Marco Bonardo [:mak]]

Attachment: patch V35

This is just a rebase on top of bug 737600, I resplitted the changes to help me reviewing them.
Bug 737600 patch applies before this one.
theo could you please retest this along with bug 737600 and tell me if you notice any issue?

Comment 126

[Théo Chevalier [:tchevalier]]

It works, no issues Marco.

Comment 127

[Théo Chevalier [:tchevalier]]

Attachment: Patch V36

Workaround to avoid to display opt-out notification to everyone: 
It's different from line 110 to 163.

telemetryEnabledMain is toolkit.telemetry.enabled
telemetryEnabledPrerelease is toolkit.telemetry.enabledPreRelease

Yes, I'm adding a new pref, again. If you find another solution to check toolkit.telemetry.enabled only once, you're very welcome.
Without this pref we would have:
if (telemetryEnabledMain)
  return;

And in that case, as long as telemetry is enabled on beta/GA we can't re-notify on Nightly/Aurora. So I record the fact that we already checked it.

Also, about checking if toolkit.telemetry.rejected is false: how do you make a difference between a pref that do not exists and a pref set to false?

Comment 128

[Marco Bonardo [:mak]]

(In reply to Théo Chevalier [:tchevalier] from comment #127)
> Also, about checking if toolkit.telemetry.rejected is false: how do you make
> a difference between a pref that do not exists and a pref set to false?

if getting the pref throws it doesn't exist, so, provided if it exists you need to know it's value, something like:

let pref;
try {
  pref = Services.prefs.getBoolPref(...);
} catch (ex) {}
if (pref === undefined) {
  // pref doesn't exist.
} else if (pref) {
  // pref is true
} else {
  // pref is false
}

Comment 129

[Marco Bonardo [:mak]]

(In reply to Théo Chevalier [:tchevalier] from comment #127)
> And in that case, as long as telemetry is enabled on beta/GA we can't
> re-notify on Nightly/Aurora.

Is not this what we want? If telemetry is enabled in beta the user already accepted to send data and we don't want to notify again for opt-out.

Comment 130

[Théo Chevalier [:tchevalier]]

(In reply to Marco Bonardo [:mak] from comment #129)
> (In reply to Théo Chevalier [:tchevalier] from comment #127)
> > And in that case, as long as telemetry is enabled on beta/GA we can't
> > re-notify on Nightly/Aurora.
> 
> Is not this what we want? If telemetry is enabled in beta the user already
> accepted to send data and we don't want to notify again for opt-out.

What we want is to hide notification when we land this change. But, we want to be able to notify again everyone with an enabled telemetry on Nightly/Aurora (later, if our privacy policy is changed).
So we want to display notification if we bump up TELEMETRY_REV and if telemetry is on.
This is why the first case when we return if rejected is true is not a problem: telemetry is off, so we don't need to notify later.


I understand you, all these cases have already given me headaches :)

Comment 131

[Marco Bonardo [:mak]]

Ok, I'm trying to figure a way out without an additional pref, I think I have something here, but I need some time to reorder my thoughts :)
I think we need something like a flow chart to verify the right fix!

Comment 132

[Marco Bonardo [:mak]]

Comment on attachment 688511 [details] [diff] [review]
Patch V36

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

this is what I'm thinking, I didn't verify or clean it up much, I'd like your feedback.

::: browser/components/nsBrowserGlue.js
@@ +907,5 @@
> +    /**
> +     * Display an opt-out notification when telemetry is enabled by default,
> +     * an opt-in prompt otherwise.
> +     */
> +    var telemetryDisplayed = 0;

do not initialize to zero, we want to know if this is the first run, so we'll use a tri-state bool (undefined,true,false)

@@ +927,5 @@
> +     * - TELEMETRY_DISPLAY_REV has been incremented, but telemetry is disabled.
> +     *
> +     * - toolkit.telemetry.enabled has been set to true (check once)
> +     *
> +     * - toolkit.telemetry.rejected has been set to true

this comment will likely need some change after the code is setup

@@ +936,5 @@
> +    } catch(e) {}
> +    var telemetryEnabledMain = false;
> +    try {
> +      telemetryEnabledMain = Services.prefs.getBoolPref("toolkit.telemetry.enabled");
> +    } catch(e) {}

rename to optInTelemetryEnabled

@@ +940,5 @@
> +    } catch(e) {}
> +    var telemetryEnabledMainCheck = false;
> +    try {
> +      telemetryEnabledMainCheck = Services.prefs.getBoolPref("toolkit.telemetry.telemetryEnabledMainCheck");
> +    } catch(e) {}

I don't think we need this pref, we'll use undefined status of telemetryDisplayed to check if this is the first run with opt-out telemetry

@@ +945,3 @@
>  
> +    if (telemetryRejected)
> +      Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, false);

This code replaces from "if (telemetryRejected)" included to "var telemetryPrompt" excluded

let telemetryEnabled = Services.prefs.getBoolPref(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(PREF_TELEMETRY_ENABLED, false);
  Services.prefs.setIntPref(PREF_TELEMETRY_DISPLAYED, 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(PREF_TELEMETRY_REJECTED, false);
  Services.prefs.setIntPref(PREF_TELEMETRY_DISPLAYED, TELEMETRY_DISPLAY_REV);
  return;
}

Comment 133

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [:mak] from comment #132)
> > +    /**
> > +     * Display an opt-out notification when telemetry is enabled by default,
> > +     * an opt-in prompt otherwise.
> > +     */
> > +    var telemetryDisplayed = 0;
> 
> do not initialize to zero, we want to know if this is the first run, so

well, not a tri-state bool, I actually wrote this comment for the other pref, this will be undefined or a number.

Comment 134

[Théo Chevalier [:tchevalier]]

Attachment: Patch V37

I had to split again this part, since this is now different for opt-in and opt-out. (We have to define telemetryDisplayed before the try for opt-in, in the try for opt-out)
+ if (telemetryDisplayed === TELEMETRY_DISPLAY_REV)
+        return;

I think your code is okay, I'll launch a try build to check if everything is right.

Comment 135

[Théo Chevalier [:tchevalier]]

Try build: https://tbpl.mozilla.org/?tree=Try&rev=0f47f32f790e

Comment 136

[Marco Bonardo [:mak]]

Comment on attachment 689120 [details] [diff] [review]
Patch V37

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

This is quite near to be ready, some things still to cleanup in browser glue, then I'll just do some testing before the final marking.

::: browser/components/nsBrowserGlue.js
@@ +906,5 @@
>  
> +    /**
> +     * Display an opt-out notification when telemetry is enabled by default,
> +     * an opt-in prompt otherwise.
> +     */

nit: use // or /* */ commenting style, this is not a javadoc (no /** */)

@@ +909,5 @@
> +     * an opt-in prompt otherwise.
> +     */
> +#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
> +    /**
> +     * Don't display notification if:

This comment should be splitted, a common part should be put before the ifdef, stating the common cases we don't show the prompt in both configurations. For example when TELEMETRY_DISPLAY_REV is the current one.

Then a specific part for ON_BY_DEFAULT should be put inside the ifdef as "Additionally, in opt-out builds, don't display the notification if:"

@@ +911,5 @@
> +#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
> +    /**
> +     * Don't display notification if:
> +     * - at first launch, toolkit.telemetry.enabledPreRelease has been
> +     *  set to false from about:config, about:telemetry or the pref pane.

I think "at first launch" is wrong, what did you mean? I'd just remove it

@@ +913,5 @@
> +     * Don't display notification if:
> +     * - at first launch, toolkit.telemetry.enabledPreRelease has been
> +     *  set to false from about:config, about:telemetry or the pref pane.
> +     *
> +     * - TELEMETRY_DISPLAY_REV has been incremented, but telemetry is disabled.

This should likely read as "The last accepted/refused policy (either by accepting the prompt or by manually flipping the telemetry preference) is already at version TELEMETRY_DISPLAY_REV"

and this is valid for both configurations, so should be put in the common comment

@@ +928,1 @@
>      } catch(e) {}

I don't understand why we have to define telemetryDisplayed in the try here, and outside in the other case, just

let telemetryDisplayed;
try {
  telemetryDisplayed = Services.prefs.getIntPref(PREF_TELEMETRY_DISPLAYED);
} catch(e) {}

if (telemetryDisplayed === TELEMETRY_DISPLAY_REV)
  return;

should work in both cases, undefined === 2 will return false the same way as 0 === 2.

if the getIntPref throws, telemetryDisplayed will stay undefined, otherwise will get a numeric value from the pref.

@@ +979,3 @@
>        return;
> + 
> +    // Clear old prefs and reprompt

please remove the trailing spaces before this line

@@ +979,4 @@
>        return;
> + 
> +    // Clear old prefs and reprompt
> +    Services.prefs.clearUserPref(PREF_TELEMETRY_DISPLAYED);

I honestly don't understand why we did this, the displayed pref can just stay at the value it is... please remove this pointless line of code.

@@ +983,3 @@
>      Services.prefs.clearUserPref(PREF_TELEMETRY_ENABLED);
>      
> +    var telemetryPrompt = browserBundle.formatStringFromName("telemetryOptInPrompt",

please remove the trailing spaces before this line

@@ +1013,5 @@
>        notifyBox = tabbrowser.getNotificationBox();
>        appendTelemetryNotification(telemetryPrompt, buttons, true);
> +    }
> +#endif
> +    // Set pref to indicate we've shown the notification.

add a newline above this

@@ +1017,5 @@
> +    // Set pref to indicate we've shown the notification.
> +    Services.prefs.setIntPref(PREF_TELEMETRY_DISPLAYED, TELEMETRY_DISPLAY_REV);
> +
> +    let notification = appendTelemetryNotification(telemetryPrompt, buttons, hideCloseButton);
> +    let link = appendLearnMoreLink(notification);

you are using half "let" half "var". For new code we usually stick with "let", but the existing code here uses "var", so whatever. I don't care what you prefer, just be consistent in the code you are touching please, probably I'd stick with "var" just cause of the existing code we are hooking into.

Comment 137

[Théo Chevalier [:tchevalier]]

(In reply to Marco Bonardo [:mak] from comment #136)
> Comment on attachment 689120 [details] [diff] [review]
> Patch V37
> 
> Review of attachment 689120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is quite near to be ready, some things still to cleanup in browser
> glue, then I'll just do some testing before the final marking.
> 

I'll address your comments, then report this code to mobile patch. (today)
Btw, do you still need a flow chart for review? Maybe we can use an etherpad.

> @@ +911,5 @@
> > +#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
> > +    /**
> > +     * Don't display notification if:
> > +     * - at first launch, toolkit.telemetry.enabledPreRelease has been
> > +     *  set to false from about:config, about:telemetry or the pref pane.
> 
> I think "at first launch" is wrong, what did you mean? I'd just remove it

When I wrote that, we weren't checking if telemetry is off.
(It was fixing an edge case: Our notification is displayed at second launch.
So if the user disable telemetry at first launch, we don't need to display it.)
But since we now check each time if (!telemetryEnabled) , this comment is useless.

> @@ +928,1 @@
> >      } catch(e) {}
> 
> I don't understand why we have to define telemetryDisplayed in the try here,
> and outside in the other case, just
> 
> let telemetryDisplayed;
> try {
>   telemetryDisplayed = Services.prefs.getIntPref(PREF_TELEMETRY_DISPLAYED);
> } catch(e) {}
> 
> if (telemetryDisplayed === TELEMETRY_DISPLAY_REV)
>   return;
> 
> should work in both cases, undefined === 2 will return false the same way as
> 0 === 2.
> 
> if the getIntPref throws, telemetryDisplayed will stay undefined, otherwise
> will get a numeric value from the pref.

Oh, of course. I thought I couldn't define it without setting a value.

> @@ +979,4 @@
> >        return;
> > + 
> > +    // Clear old prefs and reprompt
> > +    Services.prefs.clearUserPref(PREF_TELEMETRY_DISPLAYED);
> 
> I honestly don't understand why we did this, the displayed pref can just
> stay at the value it is... please remove this pointless line of code.

Yup, useless now. It was before you optimize the code, I forgot to remove it.

Comment 138

[Marco Bonardo [:mak]]

(In reply to Théo Chevalier [:tchevalier] from comment #137)
> I'll address your comments, then report this code to mobile patch. (today)
> Btw, do you still need a flow chart for review? Maybe we can use an etherpad.

well, I think something reporting all the cases and expected result would be very welcome for QA. Myself still fight figuring out all of the possible cases. It's not needed for the review, but I think we should build something. Yes, even an etherpad could work.

Comment 139

[Théo Chevalier [:tchevalier]]

Attachment: Patch V38

Addressing last review comments

Clearing displayed when we re-prompt was indeed useless (actually, needed for mobile patch only) but we want to clear rejected pref!

+ Services.prefs.clearUserPref(PREF_TELEMETRY_REJECTED);


I will *try* to have a flow chart this weekend.

Comment 140

[Marco Bonardo [:mak]]

Comment on attachment 689428 [details] [diff] [review]
Patch V38

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

This is basically a r+, but I want to do some manual testing before the final mark.
You did an amazing work here.

::: browser/components/nsBrowserGlue.js
@@ +911,5 @@
> +     * 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

there are some trailing spaces in the comment

@@ +921,4 @@
>  
> +    if (telemetryDisplayed === TELEMETRY_DISPLAY_REV)
> +      return;
> +#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT

add a newline before the ifdef

@@ +940,4 @@
>  
> +    var telemetryEnabled = Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED);
> +    if (!telemetryEnabled)
> +      return;

please move this above the other 2 prefs, since it's the earliest return we can do
Also move the telemetryRejected part just before its first use, and same for optInTelemetryEnabled.

So basically reorder the code as (pseudocode):
getPref(enabled)
if (!enabled) ...

//comment
getPref(rejected)
if (rejected) ...

// comment
getPref(optin)
if (optin && ...

@@ +983,5 @@
>                        label:     browserBundle.GetStringFromName("telemetryYesButtonLabel2"),
>                        accessKey: browserBundle.GetStringFromName("telemetryYesButtonAccessKey"),
>                        popup:     null,
>                        callback:  function(aNotificationBar, aButton) {
>                          Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true);

since now we also use rejected to tell if the user made a UI choice, I'd say to set it to false here, like we do in prefs and about:telemetry.

@@ +1012,5 @@
> +    // Set pref to indicate we've shown the notification.
> +    Services.prefs.setIntPref(PREF_TELEMETRY_DISPLAYED, TELEMETRY_DISPLAY_REV);
> +
> +    let notification = appendTelemetryNotification(telemetryPrompt, buttons, hideCloseButton);
> +    let link = appendLearnMoreLink(notification);

use "var" in these, just for consistency with the surrounding code.

Comment 141

[Marco Bonardo [:mak]]

Attachment: patch v39

well since I wanted to test the latest patch I addressed my comments in the meanwhile, so I can save you some time :)

Comment 142

[Marco Bonardo [:mak]]

Attachment: patch v40

re-splitted due to changes in bug 737600

Comment 143

[Marco Bonardo [:mak]]

Attachment: patch v40

oops, wrong attachment.

Comment 144

[Marco Bonardo [:mak]]

Comment on attachment 690024 [details] [diff] [review]
patch v40

Theo, this is basically just an unbitrot against bug 737600, but a double check would be more than welcome.

I'll continue testing tomorrow, did enough discoveries for tonight :)

Comment 145

[Marco Bonardo [:mak]]

Comment on attachment 690024 [details] [diff] [review]
patch v40

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

Once you complete your testing I think this can be pushes, I just sent it to Try.

Comment 146

[Marco Bonardo [:mak]]

since lately trychooser seems to forget to post data to buts for me, here is the direct link
https://tbpl.mozilla.org/?tree=Try&rev=167d1546658c

Comment 147

[Théo Chevalier [:tchevalier]]

Wow, I woke up and you did so much things!

Your build was a not-enabled-by-default build, let me confirm with an enabled-by-default one.
https://tbpl.mozilla.org/?tree=Try&rev=278c9384ca26

I'll try both on a Windows VM, since I can't Cancel on Linux.

Comment 148

[Théo Chevalier [:tchevalier]]

Comment on attachment 690024 [details] [diff] [review]
patch v40

OK for me :)

Comment 149

[Théo Chevalier [:tchevalier]]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14a9d4f5d16

Comment 150

[(no longer active)]

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

Comment 151

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

Thanks so much for your excellent work here, Théo! And for the great review assistance, Marco :)

Comment 152

[Mozilla RelEng Bot]

Try run for 167d1546658c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=167d1546658c
Results (out of 215 total builds):
    success: 194
    warnings: 20
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-167d1546658c