1142937 - Add-on watcher should broadcast its findings through nsIObserverService

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: 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/

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 3

[:Felipe Gomes (needinfo for replies!)]

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?)

Comment 4

[David Teller [:Yoric] - still alive but not very active]

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 5

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 6

[:Felipe Gomes (needinfo for replies!)]

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

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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 8

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d91ead2872fb

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5d4dafb2877b

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5d4dafb2877b