1462673 - Only load LightWeightThemeWebInstallListener when necessary

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement, P3)

Description

[:Felipe Gomes (needinfo for replies!)]

We can convert the code to a singleton in a jsm, and only load it when the related events happen

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1462673 - Part 0. Copy LightWeightThemeWebInstallListener code from content.js to a jsm.

Review commit: https://reviewboard.mozilla.org/r/245132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245132/

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1462673 - Only load LightWeightThemeWebInstallListener when necessary.

Convert LightWeightThemeWebInstallListener to a singleton, and add it as a lazy proxy listener.

Review commit: https://reviewboard.mozilla.org/r/245134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245134/

Comment 3

[Mike Conley (:mconley) (:⚙️) (PTO: May 24 - June 2nd)]

Comment on attachment 8977002 [details]
Bug 1462673 - Part 0. Copy LightWeightThemeWebInstallListener code from content.js to a jsm.

https://reviewboard.mozilla.org/r/245132/#review251078

Thanks for doing the copy!

Comment 4

[Mike Conley (:mconley) (:⚙️) (PTO: May 24 - June 2nd)]

Comment on attachment 8977003 [details]
Bug 1462673 - Only load LightWeightThemeWebInstallListener when necessary.

https://reviewboard.mozilla.org/r/245134/#review251082

Holding on r+ until I hear back about whether or not the JSM event listener is going to end up keeping the content window alive.

::: browser/base/content/content.js:1016
(Diff revision 1)
> +addEventListener("InstallBrowserTheme", LightWeightThemeWebInstallListener, false, true);
> +addEventListener("PreviewBrowserTheme", LightWeightThemeWebInstallListener, false, true);
> +addEventListener("ResetBrowserThemePreview", LightWeightThemeWebInstallListener, false, true);

Remind me again... when a JSM object as added as an event listener on a content window, will it end up keeping that content window alive?

If so, we'll need to remove these event listeners in an unload event handler probably to avoid leaking, right?

Comment 5

[:Felipe Gomes (needinfo for replies!)]

No, I don't think that's needed. It's content.js that keeps a reference to the JSM, not the other way around. The EventTarget for this tab has the same lifetime as the tab itself, so they are cleaned up together.

(It's usually necessary in case of observers because it's the observer service that keeps the reference)


Let me needinfo mccr8 to double check

Comment 6

[Andrew McCreight [:mccr8]]

I don't know. Maybe Olli can answer this question.

Comment 7

[Olli Pettay [:smaug]]

So event listener is added to TabChildGlobal and the listener doesn't capture .content in any way, as far as I see, so nothing should keep the window alive.

Comment 8

[Mike Conley (:mconley) (:⚙️) (PTO: May 24 - June 2nd)]

Comment on attachment 8977003 [details]
Bug 1462673 - Only load LightWeightThemeWebInstallListener when necessary.

https://reviewboard.mozilla.org/r/245134/#review251170

Alright, fears settled! Let's do it!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5327b22f40b0
Part 0. Copy LightWeightThemeWebInstallListener code from content.js to a jsm. r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f13f4b96e2
Only load LightWeightThemeWebInstallListener when necessary. r=mconley

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/5327b22f40b0
https://hg.mozilla.org/mozilla-central/rev/37f13f4b96e2