Closed Bug 1462673 Opened 2 years ago Closed 2 years ago

Only load LightWeightThemeWebInstallListener when necessary

Categories

(Firefox :: Theme, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p1])

Attachments

(2 files)

We can convert the code to a singleton in a jsm, and only load it when the related events happen
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!
Attachment #8977002 - Flags: review?(mconley) → review+
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?
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
Flags: needinfo?(continuation)
I don't know. Maybe Olli can answer this question.
Flags: needinfo?(continuation) → needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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!
Attachment #8977003 - Flags: review?(mconley) → review+
Priority: -- → P3
Pushed by felipc@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/5327b22f40b0
https://hg.mozilla.org/mozilla-central/rev/37f13f4b96e2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Whiteboard: [fxperf:p1]
You need to log in before you can comment on or make changes to this bug.