Add-on watcher should broadcast its findings through nsIObserverService

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
Performance Monitoring
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)

Updated

2 years ago
Component: General → Performance Monitoring
Product: Firefox → Toolkit
Created attachment 8712646 [details]
MozReview Request: Bug 1142937 - AddonWatcher now communicates through nsIObserverService;r?felipe

The current API of AddonWatcher only supports a single callback. That's pretty unfriendly to testing, debugging, add-ons, etc.

This patch replaces the mechanism with a notification through the nsIObserverService.

Review commit: https://reviewboard.mozilla.org/r/32593/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32593/
Attachment #8712646 - Flags: review?(felipc)
Comment on attachment 8712646 [details]
MozReview Request: Bug 1142937 - AddonWatcher now communicates through nsIObserverService;r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32593/diff/1-2/
Assignee: nobody → dteller
Blocks: 1243706
Comment on attachment 8712646 [details]
MozReview Request: Bug 1142937 - AddonWatcher now communicates through nsIObserverService;r?felipe

https://reviewboard.mozilla.org/r/32593/#review29793

::: browser/components/nsBrowserGlue.js:799
(Diff revision 2)
> -
> +    AddonWatcher.init();

why does init() needs to be called outside the conditional now?

::: browser/components/nsBrowserGlue.js:801
(Diff revision 2)
> -      AddonWatcher.init(this._notifySlowAddon);
> +      Services.obs.addObserver((subject, topic, value) => this._notifySlowAddon(value),

it would be nice if you could change \`\_notifySlowAddon\` to be \`observe\`, and then just use AddonWatcher as the observer here, in order to be able to add/remove this observer together with the other ones in nsBrowserGlue.  (Since this is a closure with no refs, you can't remove it later)

::: toolkit/components/perfmonitoring/tests/browser/browser_addonPerformanceAlerts_2.js:8
(Diff revision 2)
> -  AddonWatcher.uninit();
> +  AddonWatcher.paused = true;

is this just a semantic change?

if you do the observer removal in nsBrowserGlue as suggested above, can AddonWatcher.uninit be gone, and thus avoiding the removePerformanceListeners call during shutdown? (I'm guessing that would be a good thing?)
Attachment #8712646 - Flags: review?(felipc)
https://reviewboard.mozilla.org/r/32593/#review29793

> why does init() needs to be called outside the conditional now?

Right, I should probably keep this to the next bug.

> it would be nice if you could change \`\_notifySlowAddon\` to be \`observe\`, and then just use AddonWatcher as the observer here, in order to be able to add/remove this observer together with the other ones in nsBrowserGlue.  (Since this is a closure with no refs, you can't remove it later)

Ok, that makes sense.

> is this just a semantic change?
> 
> if you do the observer removal in nsBrowserGlue as suggested above, can AddonWatcher.uninit be gone, and thus avoiding the removePerformanceListeners call during shutdown? (I'm guessing that would be a good thing?)

Actually, it's more that `uninit()` isn't really needed for that test.
Attachment #8712646 - Flags: review?(felipc)
Comment on attachment 8712646 [details]
MozReview Request: Bug 1142937 - AddonWatcher now communicates through nsIObserverService;r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32593/diff/2-3/
Attachment #8712646 - Flags: review?(felipc) → review+
Comment on attachment 8712646 [details]
MozReview Request: Bug 1142937 - AddonWatcher now communicates through nsIObserverService;r?felipe

https://reviewboard.mozilla.org/r/32593/#review29989

::: browser/components/nsBrowserGlue.js:804
(Diff revision 3)
> +      Services.obs.addObserver(this, AddonWatcher.TOPIC_SLOW_ADDON_DETECTED);

I thought this observer could be added/removed together with all other ones in `_init`/`_dispose`. If there was a good reason to keep it here, fine, otherwise let's move it there just for consistency
https://reviewboard.mozilla.org/r/32593/#review29989

> I thought this observer could be added/removed together with all other ones in `_init`/`_dispose`. If there was a good reason to keep it here, fine, otherwise let's move it there just for consistency

Good point, fixed.
Comment on attachment 8712646 [details]
MozReview Request: Bug 1142937 - AddonWatcher now communicates through nsIObserverService;r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32593/diff/3-4/
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d91ead2872fb

Comment 10

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/5d4dafb2877b
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5d4dafb2877b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1246277
You need to log in before you can comment on or make changes to this bug.