Georg Fritzsche [:gfritzsche] Reporter
	Description • 9 years ago

If we want to turn off FHR & DRS on desktop in 38 or later, we need to port the infobar handling over to a Telemetry or generic module.

Bug 862563 touched the datachoices infobar code etc. and should be good to find starting points.

	Alessio Placitelli [:Dexter] Assignee
	Comment 1 • 9 years ago

This is missing the test coverage, but it's coming.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 2 • 9 years ago

Comment on attachment 8623171 [details] [diff] [review]
part 1 - Add the data choices infobar UI to toolkit/Telemetry

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

::: toolkit/components/telemetry/moz.build
@@ +30,5 @@
>  ]
>  
>  EXTRA_JS_MODULES += [
>      'TelemetryArchive.jsm',
> +    'TelemetryInfobar.jsm',

Is this desktop specific or does it apply to all platforms?

	Alessio Placitelli [:Dexter] Assignee
	Comment 3 • 9 years ago

I've added some comments and changed the build file so that this only gets shipped on Desktop: Android is using OS notifications [1] for that.

[1] - https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/DataReportingNotification.java#78

	Alessio Placitelli [:Dexter] Assignee
	Comment 4 • 9 years ago

I changed the log prefix value.

	Alessio Placitelli [:Dexter] Assignee
	Comment 5 • 9 years ago

Vladan, the document in [1] outlines the behaviour that the we should have when handling the datachoices infobar ourselves in Telemetry. Does it sound sane to you?

Benjamin, do you have any concern with what's written in [1]?

[1] - https://docs.google.com/document/d/1j341F8HFpl9ol0emgrXTOVXMbN4ZInKnjJniZKER_YQ/pub

	Benjamin Smedberg
	Comment 6 • 9 years ago

Email sent; I don't know the legal requirements here but I'd like to avoid the complexity if we can. Hope to have this resolved today.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 7 • 9 years ago

Some implementation notes are here: https://etherpad.mozilla.org/5q5YzioJjy

	Vladan Djeric (:vladan)
	Comment 8 • 9 years ago

I hate the desktop Telemetry infobar with a passion :( It's ugly, the animation often-times stutters, the explanation of user benefit is out-of-date and unconvincing, it hogs screen space but it's easy to miss, it's one of a half-dozen things screaming for the user's attention on fresh install, etc etc. I'm pretty sure we're getting almost no benefit from displaying it (we should analyze Telemetry to get a reliable answer).

I'd really like to have this prompt re-designed into something more sane with help from UX soon. For example, a prompt to opt-in inside about:home or about:newtab, and shown to new + existing users, but only twice in a year.

I know we're tight for time on unified Telemetry, so let's not invest a ton of time into this info bar right now. Let's commit to properly fixing it later in Q3.

Alessio, is the bheaviour in that document basically a copy of FHR's behaviour?
Benjamin, forward the legal-requirements reply when you get a chance. It will affect the decisions in this bug.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 9 • 9 years ago

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #8)
> Alessio, is the bheaviour in that document basically a copy of FHR's
> behaviour?

Yes, we need to replicate the data-choices logic from FHR here.
If we can cut anything out, great, but otherwise this is just straight-forward rebuilding of the FHR/datareportingservice logic (with less abstraction layers).

	Alessio Placitelli [:Dexter] Assignee
	Comment 10 • 9 years ago

This changed a bit to support infobar visualisation in multiple windows (even private ones).

	Alessio Placitelli [:Dexter] Assignee
	Comment 11 • 9 years ago

This pretty much does what policy.jsm [1] does. With a few differences:

- The minimumPolicyVersion is now used, it didn't seem to be used in policy.jsm (except in tests)
- There's no loop for checking if the user is notified or not.

Test coverage will come in the next patch.

[1] - https://hg.mozilla.org/mozilla-central/annotate/bbc26cc168c7/services/datareporting/policy.jsm

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 12 • 9 years ago

Results of talking over this today:
* use the existing datachoices bar UI implementation in browser/
* make the existing implementation always notify Telemetry of when it can start sending
* TelemetrySend should always block sending on whether the datachoices bar was accepted/shown
  * that Telemetry currently doesn't respect the datachoices acceptance is actually a long-standing, although a corner-case
* on Android, the datachoices implementation differs - we should hook into that to achieve the same behavior

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 13 • 9 years ago

Comment on attachment 8626550 [details] [diff] [review]
part 2 - Enforce the notification policy through TelemetryReportingPolicy.jsm

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

::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
@@ +55,5 @@
> +this.TelemetryReportingPolicy = {
> +  // The current policy version number. If the version number stored in the prefs
> +  // is smaller than this, data upload will be disabled until the user is re-notified
> +  // about the policy changes.
> +  DATAREPORTING_POLICY_VERSION: 1,

DEFAULT_DATAREPORTING_POLICY_VERSION?

@@ +155,5 @@
> +   * The minimum policy version which for dataSubmissionPolicyAccepted to
> +   * to be valid.
> +   */
> +  get minimumPolicyVersion() {
> +    // First check if the current channel has an ove

"has an ove"?

@@ +156,5 @@
> +   * to be valid.
> +   */
> +  get minimumPolicyVersion() {
> +    // First check if the current channel has an ove
> +    let channel = UpdateChannel.get(false);

try-catch?

@@ +174,5 @@
> +  /**
> +   * Checks to see if the user has been notified about data submission
> +   * @return {bool}
> +   */
> +  get userNotifiedOfCurrentPolicy() {

Can we name this more descriptively, e.g. is...Notified... or something?

@@ +175,5 @@
> +   * Checks to see if the user has been notified about data submission
> +   * @return {bool}
> +   */
> +  get userNotifiedOfCurrentPolicy() {
> +    // If we don't have a sane notification date, user is not notified.

... the user was not notified yet

@@ +187,5 @@
> +      return false;
> +    }
> +
> +    // If the accepted version is older than the current one, user is not notified.
> +    if (this.dataSubmissionPolicyAcceptedVersion < this.currentPolicyVersion) {

By re-introducing the minimumPolicyVersion this check is now redundant.

@@ +232,5 @@
> +    Services.obs.removeObserver(this, TelemetryInfobar.NOTIFICATION_OFFERED_EVENT);
> +  },
> +
> +  /**
> +   * Check if the we are allowed to upload data.

if the we?

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +715,5 @@
>      // The Telemetry pref enables sending extended data sets instead.
>      if (IS_UNIFIED_TELEMETRY) {
> +      const DRS_ENABLED = Preferences.get(PREF_DRS_ENABLED, true);
> +      if (!DRS_ENABLED && !TelemetryReportingPolicy.canUpload()) {
> +        return false;

As mentioned above, we can simplify this a lot.

	Alessio Placitelli [:Dexter] Assignee
	Comment 17 • 9 years ago

I will rebase this on bug 1156712 once part 2 of that bug is reviewed: this patch is missing the notification hook in TelemetrySend.jsm

	Alessio Placitelli [:Dexter] Assignee
	Comment 18 • 9 years ago

This patch was updated to fix the bc2/bc3 test fallout from [1]

The tests are basically a port of the original tests.

[1] - https://treeherder.mozilla.org/#/jobs?repo=try&revision=c97d32f089f3

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 20 • 9 years ago

Comment on attachment 8627222 [details] [diff] [review]
part 2 - Enforce the notification policy through TelemetryReportingPolicy.jsm - v3

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

::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
@@ +29,5 @@
> +// 2012 and no dates older than that should be encountered.
> +const OLDEST_ALLOWED_ACCEPTANCE_YEAR = 2012;
> +
> +const PREF_BRANCH = "datareporting.policy.";
> +const PREF_FIRST_RUN = "toolkit.telemetry.firstRun";

This is set pretty late in startup, so we should name it less general to avoid this being understood as a "telemetry was already initialized" pref.
Also, "firstRun" being true seems to imply "this is the first run".

"toolkit.telemetry.reportingpolicy.firstRun", default to true, flip it to false on init below?

Finally, can we add comments on what the prefs do?

@@ +81,5 @@
> +   * @param error
> +   *        (Error) Explains what went wrong.
> +   */
> +  onUserNotifyFailed: function (error) {
> +

This should at least log?

@@ +106,5 @@
> +    return TelemetryReportingPolicyImpl.shutdown();
> +  },
> +
> +  /**
> +   * Check if the we are allowed to upload data.

Nit: "if we are".
This should probably mention the display of the infobar as the condition.

@@ +189,5 @@
> +   * The minimum policy version which for dataSubmissionPolicyAccepted to
> +   * to be valid.
> +   */
> +  get minimumPolicyVersion() {
> +    const MIN_POLICY_VERSION = Preferences.get(PREF_MINIMUM_POLICY_VERSION, 1);

This is not really constant over the browser session, so i wouldn't use all-caps for the var name.

@@ +199,5 @@
> +      channel = UpdateChannel.get(false);
> +    } catch(e) {
> +      this._log.error("minimumPolicyVersion - Unable to retrieve the current channel.");
> +      return MIN_POLICY_VERSION;
> +    };

Nit: remove semicolon.

@@ +200,5 @@
> +    } catch(e) {
> +      this._log.error("minimumPolicyVersion - Unable to retrieve the current channel.");
> +      return MIN_POLICY_VERSION;
> +    };
> +    let channelPref = Preferences.get(PREF_MINIMUM_POLICY_VERSION + ".channel-" + channel);

Can just:
const channelPref = PREF_MINIMUM... + ...;
return Preferences.get(channelPref, minPolicyVersion);

@@ +224,5 @@
> +        this.dataSubmissionPolicyNotifiedDate.getTime() <= 0) {
> +      return false;
> +    }
> +
> +    // The accepted policy version should be greater than the minimum policy version.

"should not be less than"

@@ +229,5 @@
> +    if (this.dataSubmissionPolicyAcceptedVersion < this.minimumPolicyVersion) {
> +      return false;
> +    }
> +
> +    // Otherwise, it's notified.

"it's notified"?
-> "Otherwise the user was already notified."

@@ +312,5 @@
> +      return false;
> +    }
> +
> +    this._notificationInProgress = true;
> +    let request = new NotifyPolicyRequest();

What do we need this observer for? Why can't we just call back directly?
Can't we just call, say, TelemetryReportingPolicy.onUserNotifyComplete() directly?

@@ +313,5 @@
> +    }
> +
> +    this._notificationInProgress = true;
> +    let request = new NotifyPolicyRequest();
> +    Observers.notify("datareporting:notify-data-policy:request", request);

This was using notifications because it needs to broadcast to all windows? Let's comment on what this does and why.

Is there a nicer way to do this - i.e. calling on the windows directly - as a follow-up or is this still a proper Fx pattern?

@@ +334,5 @@
> +  _recordNotificationData: function() {
> +    this._log.trace("_recordNotificationData");
> +    this.dataSubmissionPolicyNotifiedDate = new Date();
> +    this.dataSubmissionPolicyAcceptedVersion = this.currentPolicyVersion;
> +    // Notified, finally.

Nit: What does that comment mean? Maybe we can write it clearer?

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +400,5 @@
> +  /**
> +   * Notify that we can start submitting data to the servers.
> +   */
> +  notifyCanUpload: function() {
> +    // TODO.

Are you planning to do this as part of this or an upcoming patch?

	Alessio Placitelli [:Dexter] Assignee
	Comment 21 • 9 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #20)
> @@ +312,5 @@
> > +      return false;
> > +    }
> > +
> > +    this._notificationInProgress = true;
> > +    let request = new NotifyPolicyRequest();
> 
> What do we need this observer for? Why can't we just call back directly?
> Can't we just call, say, TelemetryReportingPolicy.onUserNotifyComplete()
> directly?
> 
> @@ +313,5 @@
> > +    }
> > +
> > +    this._notificationInProgress = true;
> > +    let request = new NotifyPolicyRequest();
> > +    Observers.notify("datareporting:notify-data-policy:request", request);
> 
> This was using notifications because it needs to broadcast to all windows?
> Let's comment on what this does and why.
> 
> Is there a nicer way to do this - i.e. calling on the windows directly - as
> a follow-up or is this still a proper Fx pattern?

As discussed over IRC, both these will be addressed by bug 1178501.

> ::: toolkit/components/telemetry/TelemetrySend.jsm
> @@ +400,5 @@
> > +  /**
> > +   * Notify that we can start submitting data to the servers.
> > +   */
> > +  notifyCanUpload: function() {
> > +    // TODO.
> 
> Are you planning to do this as part of this or an upcoming patch?

As discussed, I'll update this patch as soon as part 2 in 1156712 is stable enough.

	Alessio Placitelli [:Dexter] Assignee
	Comment 23 • 9 years ago

Modified to to the changed preference name.

	Alessio Placitelli [:Dexter] Assignee
	Comment 24 • 9 years ago

Changed due a changed preference name.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 25 • 9 years ago

Comment on attachment 8627406 [details] [diff] [review]
part 2 - Enforce the notification policy through TelemetryReportingPolicy.jsm - v4

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

Just noticed that the shutdown ordering doesn't seem right here.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +651,5 @@
>        this._sessionRecorder = new SessionRecorder(PREF_SESSIONS_BRANCH);
>        this._sessionRecorder.onStartup();
>      }
>  
> +    // Setup the reporting policy.

Nit: Let's comment on what effect this has, i.e. triggering the datachoices infobar.

@@ +741,5 @@
>  
>        // Perform final shutdown operations.
>        yield TelemetryStorage.shutdown();
> +
> +      // Shutdown the reporting policy.

Nit: Let's comment on what this intents to do, not what it calls.
"Stop the datachoices infobar display."?

@@ +742,5 @@
>        // Perform final shutdown operations.
>        yield TelemetryStorage.shutdown();
> +
> +      // Shutdown the reporting policy.
> +      TelemetryReportingPolicy.shutdown();

We should shut this down first before TelemetrySend.
We don't need this to come up during shutdown and wouldn't want to risk triggering sending of pings here late in shutdown.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 26 • 9 years ago

Comment on attachment 8627407 [details] [diff] [review]
part 3 - Add test coverage (xpcshell + browser) - v2

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

I don't really like that we have two tests now for the same thing.
If we add a new test, we really should:
* file a follow-up on killing the old one
* write the new one properly:
  * taskify it using add_task() etc.
  * using BrowserTest.jsm utils etc.
  ...

::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
@@ +65,5 @@
> + * This is a policy object used to override behavior within this module.
> + * Tests override properties on this object to allow for control of behavior
> + * that would otherwise be very hard to cover.
> + */
> +let Policy = {

The policy to inject into the policy, which implements notifying about the policy.
I guess this wasn't the best naming after all.

@@ +67,5 @@
> + * that would otherwise be very hard to cover.
> + */
> +let Policy = {
> +  now: () => new Date(),
> +  setShowPolicyTimeout: (callback, delayMs) => setTimeout(callback, delayMs),

"ShowInfobarTimeout"?
Dito below.

@@ +143,5 @@
> +   * Test only method, invalidates the accepted policy and run setup again.
> +   */
> +  reset: function() {
> +    Preferences.reset(PREF_ACCEPTED_POLICY_DATE);
> +    Preferences.reset(PREF_ACCEPTED_POLICY_VERSION);

You don't want reset() to invalidate acceptance.
reset() should fake a restart, not starting from a fresh profile.

@@ +144,5 @@
> +   */
> +  reset: function() {
> +    Preferences.reset(PREF_ACCEPTED_POLICY_DATE);
> +    Preferences.reset(PREF_ACCEPTED_POLICY_VERSION);
> +    return this.setup();

You also need to clear the timeout.

::: toolkit/components/telemetry/tests/browser/browser.ini
@@ +1,5 @@
> +[DEFAULT]
> +support-files =
> +    head.js
> +
> +[browser_infobar_notification.js]

browser/base/content/test/general/browser_datachoices_notification.js

::: toolkit/components/telemetry/tests/browser/browser_infobar_notification.js
@@ +31,5 @@
> +  let button = buttons[0];
> +
> +  // Add an observer to ensure the "advanced" pane opened (but don't bother
> +  // closing it - we close the entire window when done.)
> +  Services.obs.addObserver(function observer(prefWin, topic, data) {

Can also unnest this using a Deferred or so.

@@ +34,5 @@
> +  // closing it - we close the entire window when done.)
> +  Services.obs.addObserver(function observer(prefWin, topic, data) {
> +    Services.obs.removeObserver(observer, "advanced-pane-loaded");
> +    Assert.ok(true, "Advanced preferences opened on info bar button press.");
> +    executeSoon(() => {

yield promiseNextTick() or so?

@@ +66,5 @@
> +}
> +
> +function test_single_window() {
> +  // Disable Healthreport/Data reporting service.
> +  Preferences.set(PREF_DRS_ENABLED, false);

This looks like it should be in test()?

@@ +75,5 @@
> +
> +  resetAcceptedPolicy();
> +
> +  // Close all the notifications, then try to trigger the data choices infobar.
> +  closeAllNotifications().then(() => {

yield closeAllNotifications()

@@ +79,5 @@
> +  closeAllNotifications().then(() => {
> +    let notificationBox = document.getElementById("global-notificationbox");
> +
> +    // Wait for it to show and check that we only get one notification.
> +    notificationBox.addEventListener("AlertActive", function active() {

yield promiseWaitForAlertActive()

@@ +88,5 @@
> +      Assert.ok(!TelemetryReportingPolicy.canUpload());
> +      Assert.equal(notificationBox.allNotifications.length, 1, "Only one notification displayed.");
> +
> +      // Cleanup and run the next test.
> +      executeSoon(() => {

yield promiseNextTick() or something?

@@ +89,5 @@
> +      Assert.equal(notificationBox.allNotifications.length, 1, "Only one notification displayed.");
> +
> +      // Cleanup and run the next test.
> +      executeSoon(() => {
> +        waitForNotificationClose(notificationBox.currentNotification, () => {

yield promiseWaitForNotificationClose(...);

@@ +92,5 @@
> +      executeSoon(() => {
> +        waitForNotificationClose(notificationBox.currentNotification, () => {
> +          Assert.equal(notificationBox.allNotifications.length, 0, "No notifications remain.");
> +
> +          // Check that we are clear to upload and that the policy data us saved.

Nit: "is saved"

@@ +103,5 @@
> +                         "Date pref set.");
> +          test_multiple_windows();
> +        });
> +        // Make sure clicking on the infobar button works and then close the notification.
> +        test_infobar_button(notificationBox.currentNotification);

yield test_infobar_button(...)

@@ +130,5 @@
> +
> +  // Ensure we see the notification on all windows and that action on one window
> +  // results in dismiss on every window.
> +  let otherWindow = OpenBrowserWindow();
> +  whenDelayedStartupFinished(otherWindow, () => {

other = yield promiseOpenAndLoadWindow({}, true)

@@ +190,5 @@
> +    Assert.equal(Preferences.get(PREF_ACCEPTED_POLICY_DATE, 0), 0, "No date should be set on init.");
> +    Assert.ok(!TelemetryReportingPolicy.testUserNotified(), "User not notified about datareporting policy.");
> +
> +    // This should be false and trigger the Infobar.
> +    Assert.ok(!TelemetryReportingPolicy.canUpload());

We can just promiseWaitForCondition() for the notifications being shown and closed, so we are able to move the Asserts out of the callbacks.

::: toolkit/components/telemetry/tests/unit/test_TelemetryReportingPolicy.js
@@ +35,5 @@
> +
> +function fakeShowInfobar() {
> +  let reportingPolicyImpl =
> +    Cu.import("resource://gre/modules/TelemetryReportingPolicy.jsm", {}).TelemetryReportingPolicyImpl;
> +  reportingPolicyImpl._infobarShownCallback();

Can't we just have a test-only function exposed properly?
We want to refactor things later to call back directly to, say, TelemetryReporting.onInfobarShownCallback() anyway.

Then we can just get rid of this helper and call the proper function directly in the tests below.

@@ +52,5 @@
> +
> +  run_next_test();
> +}
> +
> +add_task(function test_firstRun () {

function*, dito for the other tests below.

@@ +58,5 @@
> +  const FIRST_RUN_TIMEOUT_MSEC = 60 * 1000; // 60s
> +  const OTHER_RUNS_TIMEOUT_MSEC = 10 * 1000; // 10s
> +
> +  // Set that pref to indicate this is a first run.
> +  Preferences.set(PREF_FIRST_RUN, true);

You don't need to set the pref - the pref not being present should mean "first run behavior".

@@ +66,5 @@
> +  TelemetryReportingPolicy.reset();
> +
> +  Services.obs.notifyObservers(null, "sessionstore-windows-restored", null);
> +  Assert.equal(startupTimeout, FIRST_RUN_TIMEOUT_MSEC,
> +               "The infobar must be shown after 60s on the first run.");

Nit: "The infobar display timeout should be ..."

@@ +74,5 @@
> +  // Run again, and check that we actually wait only 10 seconds.
> +  TelemetryReportingPolicy.reset();
> +  Services.obs.notifyObservers(null, "sessionstore-windows-restored", null);
> +  Assert.equal(startupTimeout, OTHER_RUNS_TIMEOUT_MSEC,
> +               "The infobar must be shown after 10s on other runs.");

Nit: "The infobar display timeout should be ..."

@@ +83,5 @@
> +  TelemetryReportingPolicy.reset();
> +
> +  let now = fakeNow(2009, 11, 18);
> +
> +  // If the date is not valid (earlier than 2012), user should not be set as notified.

"should not be set as notified"?
"we don't regard the policy as accepted"?

@@ +86,5 @@
> +
> +  // If the date is not valid (earlier than 2012), user should not be set as notified.
> +  fakeShowInfobar()
> +  Assert.ok(!TelemetryReportingPolicy.testUserNotified());
> +  Assert.equal(Preferences.get(PREF_ACCEPTED_POLICY_DATE, null), 0);

Add messages for the asserts, dito the others below.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 27 • 9 years ago

Comment on attachment 8627408 [details] [diff] [review]
part 4 - Update the preferences docs

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

::: toolkit/components/telemetry/docs/preferences.rst
@@ +39,5 @@
>  ``toolkit.telemetry.log.dump``
>  
>    Sets whether to dump Telemetry log messages to ``stdout`` too.
> +
> +``toolkit.telemetry.reportingpolicy.firstRun``

Maybe we can have headings categorizing things?
The following ones could all go under "data-choices notification" or so.

@@ +41,5 @@
>    Sets whether to dump Telemetry log messages to ``stdout`` too.
> +
> +``toolkit.telemetry.reportingpolicy.firstRun``
> +
> +  If true, this is the first Telemetry run. This is used to decide when to show the infobar.

I don't think we ever need to store it as true.
This pref should not be present until we had the first run. Afterwards it should be false.

Add that this will be used to show the infobar with a more agressive timeout if it wasnt shown yet?

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 31 • 9 years ago

Comment on attachment 8628203 [details] [diff] [review]
part 4 - Update the preferences docs - v2

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

r=me with the question cleared up

::: toolkit/components/telemetry/docs/preferences.rst
@@ +68,5 @@
> +  Stores the current policy version, overrides the default value defined in TelemetryReportingPolicy.jsm.
> +
> +``datareporting.policy.minimumPolicyVersion``
> +
> +  The minimum policy version which for dataSubmissionPolicyAccepted to to be valid. This can be set per channel.

s/which for/for which/
s/to to/is/

Isn't this more like the "minimum policy version that is accepted for the current policy."?

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 32 • 9 years ago

Comment on attachment 8628202 [details] [diff] [review]
part 2 - Enforce the notification policy through TelemetryReportingPolicy.jsm - v5

Outdated patch.

	Alessio Placitelli [:Dexter] Assignee
	Comment 34 • 9 years ago

Rebased

	Alessio Placitelli [:Dexter] Assignee
	Comment 37 • 9 years ago

This is the interdiff from the last part 2 review from comment 25.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 38 • 9 years ago

Comment on attachment 8631593 [details] [diff] [review]
part 2 - Enforce the notification policy through TelemetryReportingPolicy.jsm - v5

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

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +648,5 @@
>        this._sessionRecorder = new SessionRecorder(PREF_SESSIONS_BRANCH);
>        this._sessionRecorder.onStartup();
>      }
>  
> +    // Setup the triggering of the datachoices infobar.

Nit: "This will trigger displaying the datachoices infobar." ?

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 39 • 9 years ago

Comment on attachment 8628227 [details] [diff] [review]
part 3 - Add test coverage (xpcshell + browser) - v3

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

The browser test is much clearer now, thanks.
r=me with the below resolved.

::: browser/base/content/test/general/browser_datachoices_notification.js
@@ +29,5 @@
> + */
> +function promiseNextTick() {
> +  let deferred = PromiseUtils.defer();
> +  executeSoon(deferred.resolve);
> +  return deferred.promise;

Nit: This is trivial enough to not use defer(), i.e.:
  return new Promise(resolve => executeSoon(resolve));

@@ +60,5 @@
> +
> +/**
> + * Wait for a <notification> to be closed then call the specified callback.
> + */
> +function waitForNotificationClose(notification, cb) {

Just fold this code into promiseWaitForNotificationClose()?
It's not used anywhere else.

@@ +78,5 @@
> +  });
> +  observer.observe(parent, {childList: true});
> +}
> +
> +let test_infobar_button = Task.async(function* (aNotification) {

checkInfobarButton?

@@ +87,5 @@
> +
> +  // Add an observer to ensure the "advanced" pane opened (but don't bother
> +  // closing it - we close the entire window when done.)
> +  let paneLoadedPromise = new Promise(resolve => {
> +    Services.obs.addObserver(function observer(prefWin, topic, data) {

let paneLoadedPromise = promiseTopicObserved(...);
...
let [preferenceWindow,] = yield paneLoadedPromise;

::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
@@ +151,5 @@
> +
> +  /**
> +   * Test only method, used to check if user is notified of the policy in tests.
> +   */
> +  testUserNotified: function() {

testIsUserNotified?

::: toolkit/components/telemetry/tests/unit/test_TelemetryReportingPolicy.js
@@ +48,5 @@
> +
> +  Services.prefs.setBoolPref(PREF_ENABLED, true);
> +  // We need to disable FHR in order to use the policy from telemetry.
> +  Services.prefs.setBoolPref(PREF_DRS_ENABLED, false);
> +  // Don't bypass the notifications in this tests, we'll fake it.

Nit: "this test"

@@ +52,5 @@
> +  // Don't bypass the notifications in this tests, we'll fake it.
> +  Services.prefs.setBoolPref(PREF_BYPASS_NOTIFICATION, false);
> +
> +  // Enable test-mode for TelemetrySend, otherwise we won't store pending pings
> +  // before the module is fully initialized later.

If sending is not off we should always store pending pings?
Or is there some discrepancy here that we should work on?

@@ +234,5 @@
> +
> +  // Get the ping and check its type.
> +  ping = yield PingServer.promiseNextPings(1);
> +  Assert.equal(ping.length, 1, "We should have received one ping.");
> +  Assert.equal(ping[0].type, TEST_PING_TYPE, "We should have received the new ping.");

Lets add one more test-case:
* fake a restart (TelemetryController.reset()) with a ping pending
* now we should be immediately sending the ping out
* a newly submitted ping should still be sent immediately

	Alessio Placitelli [:Dexter] Assignee
	Comment 42 • 9 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bbf345177c9

	Pulsebot
	Comment 43 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/b24454f4cd0f
https://hg.mozilla.org/integration/fx-team/rev/993b61d0a537
https://hg.mozilla.org/integration/fx-team/rev/ad42a3ee45bd

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 44 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/b24454f4cd0f
https://hg.mozilla.org/mozilla-central/rev/993b61d0a537
https://hg.mozilla.org/mozilla-central/rev/ad42a3ee45bd

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 45 • 9 years ago

Comment on attachment 8637986 [details] [diff] [review]
part 1 - Enforce the notification policy through TelemetryReportingPolicy.jsm - v6

Approval Request Comment
[Feature/regressing bug #]:
This is part of the uplift3 batch for the unified Telemetry project: http://bit.ly/1TCl4r8
This is targetting 41 now and these are the last minor "feature" patches blocking shipping - any further patches will be fixes for data quality etc. with very limited scope & impact.
[User impact if declined]:
Unified Telemetry can't ship - this patch allows us to respect the users data choices even with classic FHR turned off, triggering the notifications from Telemetry instead. 
[Describe test coverage new/current, TreeHerder]:
Automated test coverage present, fine on m-c for some days.
[Risks and why]:
Low-risk - manually tested and basically just triggering the same functionality from another place.
[String/UUID change made/needed]:
No string changes.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 46 • 9 years ago

Comment on attachment 8637987 [details] [diff] [review]
part 2 - Add test coverage (xpcshell + browser) - v4

Approval Request Comment
[Feature/regressing bug #]:
This is part of the uplift3 batch for the unified Telemetry project: http://bit.ly/1TCl4r8
This is targetting 41 now and these are the last minor "feature" patches blocking shipping - any further patches will be fixes for data quality etc. with very limited scope & impact.
[User impact if declined]:
Unified Telemetry can't ship - this patch allows us to respect the users data choices even with classic FHR turned off, triggering the notifications from Telemetry instead. 
[Describe test coverage new/current, TreeHerder]:
This is the test coverage.
[Risks and why]:
No risk, this is test-changes only.
[String/UUID change made/needed]:
No string changes.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 47 • 9 years ago

Comment on attachment 8631589 [details] [diff] [review]
part 3 - Update the preferences docs - v3

Approval Request Comment
[Feature/regressing bug #]:
This is part of the uplift3 batch for the unified Telemetry project: http://bit.ly/1TCl4r8
This is targetting 41 now and these are the last minor "feature" patches blocking shipping - any further patches will be fixes for data quality etc. with very limited scope & impact.
[User impact if declined]:
Unified Telemetry can't ship - this patch allows us to respect the users data choices even with classic FHR turned off, triggering the notifications from Telemetry instead. 
[Describe test coverage new/current, TreeHerder]:
This changes only documentation.
[Risks and why]:
No risk, documentation-only change.
[String/UUID change made/needed]:
No string changes.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 48 • 9 years ago

Tracked as this is a new feature shipping in FF41.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 49 • 9 years ago

Comment on attachment 8637986 [details] [diff] [review]
part 1 - Enforce the notification policy through TelemetryReportingPolicy.jsm - v6

This has been in m-c for a few days and since this feature is planned for FF41, the sooner we uplift to Aurora the better.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 50 • 9 years ago

Comment on attachment 8637987 [details] [diff] [review]
part 2 - Add test coverage (xpcshell + browser) - v4

Approved.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 51 • 9 years ago

Comment on attachment 8631589 [details] [diff] [review]
part 3 - Update the preferences docs - v3

Approved.

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/8250be3475fd
https://hg.mozilla.org/releases/mozilla-aurora/rev/ac8f2adbf320
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd42e0a4fa8f

	Ryan VanderMeulen [:RyanVM]
	Comment 53 • 9 years ago

Somehow the test changes in rev ac8f2adbf320 caused Linux32 PGO e10s mochitest-bc permacrashes like the log below:
https://treeherder.mozilla.org/logviewer.html#?job_id=1001159&repo=mozilla-aurora

I verified it was the test changes by backing out every individual commit from the push the permafail started on and it went green with just rev ac8f2adbf320 backed out.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c8dff23c85c

Backed out until it can be investigated.
https://hg.mozilla.org/releases/mozilla-aurora/rev/250b3d59502d

	Benjamin Smedberg
	Comment 54 • 9 years ago

Looking at the stack, I think there is an xlib error or warning triggering a stackwalk which is crashing.

{"thread": "ProcessReader", "level": "info", "pid": 1801, "source": "mochitest", "time": 1438298106400, "action": "log", "exc_info": false, "message": "Xlib:  extension \"RANDR\" missing on display \":0\"."}
{"thread": "ProcessReader", "level": "info", "pid": 1801, "source": "mochitest", "time": 1438298108809, "action": "log", "exc_info": false, "message": "\u0007[NPAPI 2197] ###!!! ASSERTION: plug removed: 'glib assertion', file /builds/slave/m-aurora-lx-000000000000000000/build/src/toolkit/xre/nsSigHandlers.cpp, line 138"}

I don't know what this assertion means in practice.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 55 • 9 years ago

Per bug 1113930, comment 40 ff, the failure doesn't seem related to this bug.
Lets land this with the test disabled for e10s on Aurora.

	Alessio Placitelli [:Dexter] Assignee
	Comment 56 • 9 years ago

Landed on Aurora again:

 View your changes here:
   https://hg.mozilla.org/releases/mozilla-aurora/rev/566655d13698
   https://hg.mozilla.org/releases/mozilla-aurora/rev/ae7e8a5be107
   https://hg.mozilla.org/releases/mozilla-aurora/rev/510a87909ff5

 Follow the progress of your build on Treeherder:
   https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=510a87909ff5

The try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=839f4989676b

	Alessio Placitelli [:Dexter] Assignee
	Comment 57 • 9 years ago

Due to [1], as discussed in bug 1113930, we need to skip the datachoices test on e10s on m-c as well, otherwise it will fail on Aurora.

[1] - https://treeherder.mozilla.org/logviewer.html#?job_id=9858073&repo=try

	Ada [:adalucinet]
	Comment 58 • 9 years ago

Verified with 41.0b8 across platforms [1], by launching Fx with a clean profile with prefs mentioned in the document from comment 5 and FHR turned off (datareporting.healthreport.service.enabled; false). 
The datachoices infobar is displayed in the required 60s/10s limit and the correct data is saved in the following prefs:
- datareporting.policy.dataSubmissionPolicyAcceptedVersion
- datareporting.policy.dataSubmissionPolicyNotifiedTime

[1] Mac OS X 10.10.4, Windows 7 64-bit and Ubuntu 14.04 32-bit

	Ada [:adalucinet]
	Comment 59 • 9 years ago

Also confirming this fix with 42.0b1 (Build ID: 20150921151815), across platforms [1].

[1] Windows 7 64-bit, Mac OS X 10.11 Beta and Ubuntu 14.04 32-bit