Closed
Bug 1142937
Opened 9 years ago
Closed 8 years ago
Add-on watcher should broadcast its findings through nsIObserverService
Categories
(Toolkit :: Performance Monitoring, defect)
Toolkit
Performance Monitoring
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: Yoric, Assigned: Yoric)
References
Details
Attachments
(1 file)
No description provided.
Updated•8 years ago
|
Component: General → Performance Monitoring
Product: Firefox → Toolkit
Assignee | ||
Comment 1•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8712646 -
Flags: review?(felipc)
Assignee | ||
Comment 2•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → dteller
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8712646 -
Flags: review?(felipc)
Assignee | ||
Comment 5•8 years ago
|
||
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/
Updated•8 years ago
|
Attachment #8712646 -
Flags: review?(felipc) → review+
Comment 6•8 years ago
|
||
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
Assignee | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 9•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d91ead2872fb
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5d4dafb2877b
Keywords: checkin-needed
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5d4dafb2877b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•