Closed Bug 828829 Opened 11 years ago Closed 11 years ago

Refactor Health Report policy and service

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set
normal

Tracking

(firefox20 fixed)

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(2 files, 3 obsolete files)

Per discussion with rnewman earlier today:

* We want policy.jsm to drive triggering of the data submission info bar
* Because the info bar is shared between Telemetry and Firefox Health Report, we can't have policy.jsm invocation from an XPCOM service hidden behind MOZ_SERVICES_HEALTHREPORT

What I've done in this patch is create a new, always on XPCOM service tentatively called MetricsCollectionService. We should bikeshed about the name and the location of files in the tree.

Anyway, this XPCOM service initializes a policy instance and starts it polling. The XPCOM service also exposes a HealthReporter instance iff FHR is enabled. Note that previously policy.jsm was bound to a HealthReporter instance.

Because policy.jsm could some day drive Telemetry as well, we should probably reflect that in the API. Currently, things are FHR centric. Even the prefs still live in the "healthreport" branch! We could say follow-up on all this. Although, that would involve writing and maintaining migration code, which I don't care much for. I think we should do it right from the start.

Anyway, this is something to look at.

Splinter will likely fail with the patch because of file moving/renaming.
Attachment #700214 - Flags: feedback?(rnewman)
Blocks: 804745
Blocks: 829184
This patch actually works.
Assignee: nobody → gps
Attachment #700214 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #700214 - Flags: feedback?(rnewman)
Attachment #700587 - Flags: feedback?(rnewman)
Comment on attachment 700587 [details] [diff] [review]
Make data collection service generic, v2

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

Keep goin'.
Attachment #700587 - Flags: feedback?(rnewman) → feedback+
Blocks: 718066
Disclaimer: I haven't ran unit tests with this version of the patch. Waiting for build to finish.

* XPCOM service and policy moved to /services/datareporting/
* This code is loaded if MOZ_DATA_REPORTING build variable is set. This variable is set if 1 of {Telemetry, FHR, Crash Reports} are enabled. It will presumably be enabled for all official Mozilla builds.
* Policy is now instantiated very early in startup and isn't bound to HealthReporter instance.
* Renamed HealthReporterPolicy to DataReportingPolicy.
* Preferences moved under "datareporting." branch.
* Split preferences into "datareporting.policy." for generic data reporting policy tracking and "datareporting.healthreport." for Health Report state.
* DataReportingPolicy and the other types in policy.jsm need to be split because there is now a clear line between policy and FHR. This can be follow-up.
Attachment #700587 - Attachment is obsolete: true
Attachment #701217 - Flags: review?(rnewman)
Fixed two very minor issues with patch that aren't worth commenting on.
Attachment #701217 - Attachment is obsolete: true
Attachment #701217 - Flags: review?(rnewman)
Attachment #701233 - Flags: review?(rnewman)
Previously things were stored in the policy branch instead of healthreport. This didn't make any sense. There might be more of these prefs lingering. Please audit the list!
Attachment #701250 - Flags: review?(rnewman)
Comment on attachment 701233 [details] [diff] [review]
Make data reporting service generic, v4

Combined review of both parts. Not using Splinter, sorry.


* Fix removed-files/manifest.

+const BRANCH = "datareporting.";

-- ROOT_BRANCH, please.


* Re policy/DataReportingService observer design: noting that this is half-way through a refactor. We really want several loosely coupled components:

** A policy object that *only* decides whether the user has made an election, and broadcasts accordingly
** A set of data upload services that listen for this and decide whether to start timers etc.
** A set of data uploaders that are driven by the upload services.


Otherwise, looks good enough. Apologies in advance that a fair amount of this was reviewed through pattern recognition/instinct, rather than opening 20 editor buffers and spending five hours!
Attachment #701233 - Flags: review?(rnewman) → review+
Attachment #701250 - Flags: review?(rnewman) → review+
Comment on attachment 701233 [details] [diff] [review]
Make data reporting service generic, v4

[Approval Request Comment]
See uplift comment in bug 804745.
Attachment #701233 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f2c240863a0c
https://hg.mozilla.org/mozilla-central/rev/1761f4a9081c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla21
Comment on attachment 701233 [details] [diff] [review]
Make data reporting service generic, v4

https://bugzilla.mozilla.org/show_bug.cgi?id=804745#c37
Attachment #701233 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 831404
Blocks: 837459
No longer blocks: 837459
Depends on: 837459
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla21 → Firefox 21
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.