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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file)

      No description provided.
Component: General → Performance Monitoring
Product: Firefox → Toolkit
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/
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
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.
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
https://hg.mozilla.org/mozilla-central/rev/5d4dafb2877b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1246277
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: