Closed Bug 847662 Opened 7 years ago Closed 7 years ago

HealthReporter and providers refactoring

Categories

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

defect
Not set

Tracking

(firefox20 unaffected, firefox21 fixed)

RESOLVED FIXED
Firefox 22
Tracking Status
firefox20 --- unaffected
firefox21 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(3 files)

A lot of the code in healthreporter.jsm related to provider management. The goal is to more loosely couple this from the high-level service code to better facilitate Android, readable code, etc.

Goals of this bug:

* Rename Metrics.Collector -> Metrics.ProviderManager
* Move all provider management code from HealthReporter into Metrics.ProviderManager
* Have ProviderManager be more intelligent about automatically instantiating pull-only providers, etc.
This is largely mechanical. I'm pretty sure I got all the references.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #721369 - Flags: review?(rnewman)
While I was here, I noticed that tests could hang if there was an error in the test. The reason is the health reporter not shutting down will result in the process waiting for a shutdown signal which it never receives (at least in tests).

I added finally blocks to each test susceptible to this. This may also help us track down some intermittent test hangs in these tests!

There is a larger problem: why doesn't FHR shut down gracefully in xpcshell tests? My guess is the xpcshell runner doesn't broadcast the expected app shutdown notifications if a test fails. Hmmm.
Attachment #721372 - Flags: review?(rnewman)
Comment on attachment 721372 [details] [diff] [review]
Part 2: Add finalizers to tests, v1

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

Did you reproduce a test hang, even informally, and verify that this fixed the hang?
Attachment #721372 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #3)
> Did you reproduce a test hang, even informally, and verify that this fixed
> the hang?

Yes.
Attachment #721369 - Flags: review?(rnewman) → review+
This is a largely mechanical move of provider management function from healthreporter.jsm into providermanager.jsm. I tried to not change semantics except where necessary. A future patch will rework ProviderManager's internals to be more robust so things like pull-only provider management is transparent to the outside world. It will also lock down the public API so providers can only be registered with their constructor function, etc.
Attachment #721478 - Flags: review?(rnewman)
Comment on attachment 721478 [details] [diff] [review]
Part 3: Move provider code into providermanager, v1

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

Largely a mechanical review, too :D

Looking forward to seeing this keep moving.
Attachment #721478 - Flags: review?(rnewman) → review+
This'll do for now.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla22
Comment on attachment 721369 [details] [diff] [review]
Part 1: Rename Collector -> ProviderManager, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None
Testing completed (on m-c, etc.): Code has been baking for a while. FHR would be significantly broken if this patch were broken.
Risk to taking this patch (and alternatives if risky): Little
String or UUID changes made by this patch: None

This falls into the same bucket as bug 841074. The uplift isn't required but it will make applying future tracked uplifts much easier and will result in a more unified code configuration. If it's not accepted, we can work around it.

The patches in this bug are mostly a movement of code between files and a renaming of symbols. The risk should be very low.
Attachment #721369 - Flags: approval-mozilla-aurora?
Comment on attachment 721372 [details] [diff] [review]
Part 2: Add finalizers to tests, v1

[Approval Request Comment]
See initial uplift request.
Attachment #721372 - Flags: approval-mozilla-aurora?
Comment on attachment 721478 [details] [diff] [review]
Part 3: Move provider code into providermanager, v1

[Approval Request Comment]
See initial uplift request.
Attachment #721478 - Flags: approval-mozilla-aurora?
Attachment #721369 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #721372 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #721478 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22
Blocks: 866253
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.