Don't show the datachoices infobar when sending deletion pings

RESOLVED FIXED in Firefox 42

Status

()

Toolkit
Telemetry
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Dexter, Assigned: gfritzsche)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox42 fixed)

Details

(Whiteboard: [unifiedTelemetry])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

3 years ago
When sending a deletion ping on the first run, if the infobar wasn't shown already, disabling datareporting.healthreport.uploadEnabled immediately triggers the infobar.

This happens because disabling upload generates a deletion ping that needs to be sent to the servers.

FHR doesn't show the data choices infobar when sending deletion requests.
(Reporter)

Updated

3 years ago
Blocks: 1122482
status-firefox41: --- → affected
Whiteboard: [unifiedTelemetry]
(Reporter)

Comment 1

3 years ago
Created attachment 8644412 [details] [diff] [review]
bug1191336.patch
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8644412 - Flags: review?(gfritzsche)
(Reporter)

Updated

3 years ago
Attachment #8644412 - Flags: review?(gfritzsche)
(Reporter)

Comment 2

3 years ago
Created attachment 8644954 [details] [diff] [review]
part 2 - Don't show the infobar when checking if user can upload
(Reporter)

Comment 3

3 years ago
Created attachment 8644983 [details] [diff] [review]
part 1 - Allow deletion pings to be sent w/o prompting the datachoices infobar
Attachment #8644412 - Attachment is obsolete: true
Comment on attachment 8644954 [details] [diff] [review]
part 2 - Don't show the infobar when checking if user can upload

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

::: browser/base/content/test/general/browser_datachoices_notification.js
@@ +133,5 @@
>    Assert.ok(!TelemetryReportingPolicy.canUpload(),
> +            "User should not be allowed to upload.");
> +
> +  // Trigger the infobar.
> +  TelemetryReportingPolicy.testShowInfobar();

Lets trigger the timer this waits on instead using a policy object.
Ditto below.

::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
@@ +158,5 @@
>  
>    /**
> +   * Test only method, used to show the infobar.
> +   */
> +  testShowInfobar: function() {

We don't need this, we can just trigger the timers directly here similar to other Telemetry test.

@@ +345,5 @@
>      }
>  
> +    // Submission is enabled. We enable upload if user is notified or we need to bypass
> +    // the policy.
> +    const BYPASS_NOTIFICATION = Preferences.get(PREF_BYPASS_NOTIFICATION, false);

Nit: const bypass = ...

@@ +361,5 @@
>    },
>  
>    /**
>     * Make sure the user is notified about the policy before allowing upload.
>     * @return {Boolean} True if the user was notified, false otherwise.

Update this.
Attachment #8644954 - Flags: feedback+
Comment on attachment 8644983 [details] [diff] [review]
part 1 - Allow deletion pings to be sent w/o prompting the datachoices infobar

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

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +676,5 @@
> +      if (isDeletionPing(ping)) {
> +        return TelemetryStorage.saveDeletionPing(ping);
> +      } else {
> +        return TelemetryStorage.savePendingPing(ping);
> +      }

This is duplicated below and in sendPings(), let's move this to a helper function.
Attachment #8644983 - Flags: review+
(Reporter)

Comment 6

3 years ago
Created attachment 8645036 [details] [diff] [review]
part 1 - Allow deletion pings to be sent w/o prompting the datachoices infobar - v2
Attachment #8644983 - Attachment is obsolete: true
Attachment #8645036 - Flags: review+
(Reporter)

Comment 7

3 years ago
Created attachment 8645051 [details] [diff] [review]
part 2 - Don't show the infobar when checking if user can upload - v2
Attachment #8644954 - Attachment is obsolete: true
Attachment #8645051 - Flags: review?(gfritzsche)
Comment on attachment 8645051 [details] [diff] [review]
part 2 - Don't show the infobar when checking if user can upload - v2

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

::: browser/base/content/test/general/browser_datachoices_notification.js
@@ +146,4 @@
>    Assert.ok(!TelemetryReportingPolicy.canUpload(),
> +            "User should not be allowed to upload.");
> +
> +  // Trigger the infobar.

This section is duplicated below, lets move it to a helper function triggerInfoBar(expectedTimeout) or so.

@@ +149,5 @@
> +  // Trigger the infobar.
> +  let showInfobarCallback = null;
> +  fakeShowPolicyTimeout((callback, timeout) => showInfobarCallback = callback, () => {});
> +  sendSessionRestoredNotification();
> +  Assert.ok(!!showInfobarCallback, "Must have a timer callback.")

While we are here, we should also check the timeout being correct.
Attachment #8645051 - Flags: review?(gfritzsche) → review+
Created attachment 8645063 [details] [diff] [review]
Part 1 - Don't show the datachoices infobar when sending deletion pings
Assignee: alessio.placitelli → gfritzsche
Created attachment 8645064 [details] [diff] [review]
Part 2 - Don't trigger the datachoices infobar when checking if user is allowed to upload
Attachment #8645036 - Attachment is obsolete: true
Attachment #8645051 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/20565e58ac99
https://hg.mozilla.org/mozilla-central/rev/f63aa8d0a399
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.