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.

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

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?

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

I changed the log prefix value.

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

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.

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

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.

(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).

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

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

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

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.

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

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

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?

(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.

Modified to to the changed preference name.

Changed due a changed preference name.

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.

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.

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?

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."?

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

Rebased

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

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." ?

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

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

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

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

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.

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.

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.

Tracked as this is a new feature shipping in FF41.

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.

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

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

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

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

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.

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.

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

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

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

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