Closed Bug 1945290 Opened 16 days ago Closed 10 days ago

Make `TelemetryReportingPolicy` more async

Categories

(Toolkit :: Telemetry, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
137 Branch
Tracking Status
firefox136 --- fixed
firefox137 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(4 files)

Right now, TelemetryReportingPolicy.sys.mjs is a mix of old-style callbacks and new-style async code. This ticket tracks moving some of the callbacks to async/Promise style code.

This partially reverts the changes of an earlier commit, which made
some things asynchronous. The NimbusFeatures.getAllVariables() API
is not, in fact, asynchronous, so opening the internals is not
strictly necessary. This simplifies the expression and testing for
later.

N.b.: the existing implementation did not correctly test
_openFirstRunPage: that function was async and there was no await.
Therefore existing tests were asserting things about the function
definitions and not the function behaviours.

The goal here is to separate the display of policy (tabs or infobars)
from marking the user notified. This is mostly moving the deck chairs
around.

For testing this:

  • a new profile will show a background tab;
  • a new profile with datareporting.policy.firstRunURL="" will show an infobar after 60s;
  • quitting before seeing that first infobar will show an infobar after 10s on the next invocation.

N.b.: the data reporting policy state machine determines "New profile" by toolkit.telemetry.reportingpolicy.firstRun=true, so you can do all the testing in one profile with a little effort.

Blocks: 1945672
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/caf8581c6983 Part 1: Hide `TelemetryReportingPolicyImpl`. r=chutten,mviar https://hg.mozilla.org/integration/autoland/rev/7b360633f06b Part 2: Make `TelemetryReportingPolicy` more async. r=chutten

Backed out for causing xpcshell failures @ test_TelemetryReportingPolicy.js

TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryReportingPolicy.js | xpcshell return code: 0
Flags: needinfo?(nalexander)
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/636011fb8288 Part 1: Hide `TelemetryReportingPolicyImpl`. r=chutten,mviar https://hg.mozilla.org/integration/autoland/rev/2d16ee415c72 Part 2: Make `TelemetryReportingPolicy` more async. r=chutten
Status: ASSIGNED → RESOLVED
Closed: 10 days ago
Resolution: --- → FIXED
Target Milestone: --- → 137 Branch

Clearing NI: this landed!

Flags: needinfo?(nalexander)

This partially reverts the changes of an earlier commit, which made
some things asynchronous. The NimbusFeatures.getAllVariables() API
is not, in fact, asynchronous, so opening the internals is not
strictly necessary. This simplifies the expression and testing for
later.

N.b.: the existing implementation did not correctly test
_openFirstRunPage: that function was async and there was no await.
Therefore existing tests were asserting things about the function
definitions and not the function behaviours.

Original Revision: https://phabricator.services.mozilla.com/D235886

Attachment #9464485 - Flags: approval-mozilla-beta?

The goal here is to separate the display of policy (tabs or infobars)
from marking the user notified. This is mostly moving the deck chairs
around.

Original Revision: https://phabricator.services.mozilla.com/D236443

Attachment #9464486 - Flags: approval-mozilla-beta?
Attachment #9464486 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9464485 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: