Closed Bug 1442006 Opened 6 years ago Closed 6 years ago

Provide a way to run a JS script every time a chrome global gets created

Categories

(Core :: XUL, enhancement, P5)

Product:
Core
Component:
XUL
Type:
enhancement

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1411707

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1442006 - Rename globalOverlay.js to chromeGlobals.js and load it in all chrome documents;
59 bytes, text/x-review-board-request
Details 
There are three recent cases I'm aware of where we've discussed having global script(s) that load in all XUL documents, so I'm filing a new bug to figure out if/how we should do this.

1: When defining Cc/Cu/Ci/Cr in chrome globals. From https://bugzilla.mozilla.org/show_bug.cgi?id=767640#c26:

(In reply to Andrew McCreight [:mccr8] from comment #26)
> This seems to work, at least with what part of my try run has run so far.
> 
> I just noticed the part of comment 9 about also wanting Services, XPCOMUtils
> and AppConstants, and I wonder if maybe a better approach would be to set up
> a JS script to be run any time we create a chrome JS global. I'm not sure
> how that would work, but it would be nice because then it could be updated
> by people who are writing chrome JS, rather than requiring some gross
> XPConnect C++ code. It might be slower, but at least we create less globals
> now due to the shared JSM loading.

2: To register Custom Elements instead of XBL bindings. From https://bugzilla.mozilla.org/show_bug.cgi?id=1411707#c10:

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Smaug, in this patch I'm trying to get the Custom Element registrations
> loaded in every chrome document. Right now the way I'm doing that is to bind
> to "chrome-document-global-created" in the parent process and then using the
> subscript loader to load the JS files that do the registrations. I wonder if
> we should add (or already have) some mechanism to load scripts inside of
> each document that has AllowXULXBL set to true.
> 
> This would allow us to handle the "remote XUL" case in the content process
> without needing to set up a new observer for chrome documents in that
> process, and maybe would allow for performance optimization for when/how we
> load the script. Any ideas on if/how we should do this?

3: When moving away from XUL overlays. From https://bugzilla.mozilla.org/show_bug.cgi?id=1439766#c2:

(In reply to Brian Grinstead [:bgrins] from comment #2)
> (In reply to Brendan Dahl [:bdahl] from comment #0)
> > 3) Add the JS needed to the master document
> 
> Just a note from previous discussions we had about this: we have multiple
> global-ish js files that get loaded in many XUL docs (and most docs in the
> case of OSX where we load macBrowserOverlay.xul). In particular:
> 
> *
> https://searchfox.org/mozilla-central/source/toolkit/content/globalOverlay.js
> *
> https://searchfox.org/mozilla-central/source/toolkit/content/editMenuOverlay.
> js
> *
> https://searchfox.org/mozilla-central/source/browser/base/content/
> utilityOverlay.js
> 
> But what gets loaded where gets pretty complicated (I've started to document
> this here:
> https://docs.google.com/document/d/
> 1FOB6Qp_qhxMw4R2h6P81yWFWnsM9LfNPEOIWroqN4r8/edit).
> 
> Removing overlays in general is going to help a lot, but I also think we
> could also simplify this from the JS side by putting all of these global
> functions into a single file and ensure that it gets loaded in all XUL docs.
> This could be done by putting a <script> tag in all docs or through the
> subscript loader and running in response to an observer notification when a
> XUL doc gets created.
I think Enn had some ideas on this - ni?ing.
Flags: needinfo?(enndeakin)
In Bug 1411707 / https://reviewboard.mozilla.org/r/193106/ there's an example of how we could do this in frontend code. Basically: Set up a "chrome-document-global-created" observer in a place that gets loaded before other windows like nsBrowserGlue or MainProcessSingleton, and then use the subscript loader to inject a script.

It's quite likely this would be better handled in the platform, though. As smaug mentioned in that bug:

(In reply to Olli Pettay [:smaug] from comment #11)
> As far as I know, we don't have such thing yet, but probably should add.
> Or, for now, at least for non-remote case, coulnd't we just always have some
> <script> element included in XUL documents? The scripts are cached and just
> cloned for each new instance of the document.
> 
> But perhaps we should do that implicitly. Each non-overlay XUL document
> would basically have some default script loaded. I guess easiest would be to
> reuse the existing XUL prototype stuff. When a new prototype document is
> created, we'd ensure the default script has been loaded. And when actual XUL
> document is created, we clone the script and run it. I wonder at which
> point. Right after creating the document, but before adding document element?
Bah! Mid-aired and lost comment...

(In reply to Brian Grinstead [:bgrins] from comment #0)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > Smaug, in this patch I'm trying to get the Custom Element registrations
> > loaded in every chrome document. Right now the way I'm doing that is to bind
> > to "chrome-document-global-created" in the parent process and then using the
> > subscript loader to load the JS files that do the registrations. I wonder if
> > we should add (or already have) some mechanism to load scripts inside of
> > each document that has AllowXULXBL set to true.

This is the kind of thing chrome-document-global-created exists for. Is there a particular reason we need another API?

> > This would allow us to handle the "remote XUL" case in the content process
> > without needing to set up a new observer for chrome documents in that
> > process, and maybe would allow for performance optimization for when/how we
> > load the script. Any ideas on if/how we should do this?

Remote XUL is no longer actually supported, even though it still mostly works. Are there places where we're still using XUL[1] in content processes? If so, they should be fixed.

The subscript loader already optimizes these types of loads. The scripts are background pre-compiled at startup amd cloned into the target global when they're executed. We're probably not going to get much better than that.

[1]: Actual XUL. Not the content XBL bindings that we use for things like <video>.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #3)
> > > This would allow us to handle the "remote XUL" case in the content process
> > > without needing to set up a new observer for chrome documents in that
> > > process, and maybe would allow for performance optimization for when/how we
> > > load the script. Any ideas on if/how we should do this?
> 
> Remote XUL is no longer actually supported, even though it still mostly
> works. Are there places where we're still using XUL[1] in content processes?
> If so, they should be fixed.
>
> [1]: Actual XUL. Not the content XBL bindings that we use for things like
> <video>.

I seem to remember :bz mentioning a case where we load XUL docs in the content process which led to my initial comment in Bug 1411707, but unfortunately I can't find the logs of that conversation so ni? him. I see some comments / special handling for remote xul in quite a few places so if we don't support it anymore there's some cleanup that can be done.
Flags: needinfo?(bzbarsky)
> Remote XUL is no longer actually supported

We still support both remote XUL and the "XUL and XBL from file://" pref, last I checked.
Flags: needinfo?(bzbarsky)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #0)
> > (In reply to Brian Grinstead [:bgrins] from comment #10)
> > > Smaug, in this patch I'm trying to get the Custom Element registrations
> > > loaded in every chrome document. Right now the way I'm doing that is to bind
> > > to "chrome-document-global-created" in the parent process and then using the
> > > subscript loader to load the JS files that do the registrations. I wonder if
> > > we should add (or already have) some mechanism to load scripts inside of
> > > each document that has AllowXULXBL set to true.
> 
> This is the kind of thing chrome-document-global-created exists for. Is
> there a particular reason we need another API?

The two main reasons I had:

* Thought it could not run on remote xul or other cases I wasn't aware of.
* Thought we could get better performance by keeping a list of scripts in the platform instead of loading them via JS.

But sounds like perf may not be an issue after all. And maybe we can retire remote XUL (or not load global scripts in them which would eventually break things as we retire XBL). I guess the main issue with doing that would be breaking test cases attached to bugs?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> > Remote XUL is no longer actually supported
> 
> We still support both remote XUL and the "XUL and XBL from file://" pref,
> last I checked.

It's still possible to enable remote XUL via the permission manager, but that was only ever meant as a transitional step to removing support altogether. It's been officially unsupported for several years, and has been degrading ever since (especially when OOP, where things like popups/tooltips/dropdowns don't work at all). And since legacy extension support, there isn't even a way to enable it anymore.

So while the code still exists, its not really supported.
(In reply to Brian Grinstead [:bgrins] from comment #6) 
> * Thought it could not run on remote xul or other cases I wasn't aware of.

We could always do this from a process script if necessary, but I'd rather not make any concessions for remote XUL support, at this point. Besides, it doesn't run in chrome globals anyway, and the de-XBL process will probably break whatever remote XUL is left in the wild.

> * Thought we could get better performance by keeping a list of scripts in
> the platform instead of loading them via JS.

I doubt the performance difference would be measurable. At some point we should probably make a project of reducing the number of JS observers that we register, but even then, this will probably be one of the lower-impact ones.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #8)
> (In reply to Brian Grinstead [:bgrins] from comment #6) 
> > * Thought it could not run on remote xul or other cases I wasn't aware of.
> 
> We could always do this from a process script if necessary, but I'd rather
> not make any concessions for remote XUL support, at this point. Besides, it
> doesn't run in chrome globals anyway, and the de-XBL process will probably
> break whatever remote XUL is left in the wild.
> 
> > * Thought we could get better performance by keeping a list of scripts in
> > the platform instead of loading them via JS.
> 
> I doubt the performance difference would be measurable. At some point we
> should probably make a project of reducing the number of JS observers that
> we register, but even then, this will probably be one of the lower-impact
> ones.

OK works for me. Unless if others here want to try something else, we can use this bug to try out the observer / subscript loader approach on a script like globalOverlay.js and see how it works.
(In reply to Brian Grinstead [:bgrins] from comment #9)

> OK works for me. Unless if others here want to try something else, we can
> use this bug to try out the observer / subscript loader approach on a script
> like globalOverlay.js and see how it works.

From looking at the patch, I'm afraid this would load globalOverlay.js into the about:blank window I'm opening during early startup with the code from bug 1336227.
(In reply to Florian Quèze [:florian] from comment #12)
> (In reply to Brian Grinstead [:bgrins] from comment #9)
> 
> > OK works for me. Unless if others here want to try something else, we can
> > use this bug to try out the observer / subscript loader approach on a script
> > like globalOverlay.js and see how it works.
> 
> From looking at the patch, I'm afraid this would load globalOverlay.js into
> the about:blank window I'm opening during early startup with the code from
> bug 1336227.

I'm not sure if there's some case where this would break, but if I skip loading the script in about:blank (https://hg.mozilla.org/try/rev/754ad960adb146985695cca75ad8b3a735817297#l16.24) the try push seems pretty much OK.
(In reply to Brian Grinstead [:bgrins] from comment #13)
> I'm not sure if there's some case where this would break, but if I skip
> loading the script in about:blank
> (https://hg.mozilla.org/try/rev/754ad960adb146985695cca75ad8b3a735817297#l16.
> 24) the try push seems pretty much OK.

Can you try checking `document.contentType === "application/vnd.mozilla.xul+xml"`? I'm not sure that will always be set by the time we get to chrome-document-global-created, but hopefully it should.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #14)
> (In reply to Brian Grinstead [:bgrins] from comment #13)
> > I'm not sure if there's some case where this would break, but if I skip
> > loading the script in about:blank
> > (https://hg.mozilla.org/try/rev/754ad960adb146985695cca75ad8b3a735817297#l16.
> > 24) the try push seems pretty much OK.
> 
> Can you try checking `document.contentType ===
> "application/vnd.mozilla.xul+xml"`? I'm not sure that will always be set by
> the time we get to chrome-document-global-created, but hopefully it should.

Unfortunately that doesn't seem to work. The only document that has the content type matching is "chrome://extensions/content/dummy.xul". The "about:blank" loads have "text/html" and I what I presume are the browser.xul/hiddenWindow.xul loads show an empty location string and "text/html" for contentType.
I haven't heard any complaints about the plan here, so I'll make sure try is green and we don't have any talos issues and then request review. We can move the 'load XPCOMUtils / Services in every global' into a follow-up bug, since that would require rewriting JS across the tree.
There were some eslint errors around NS_ASSERT on the previous push but those have been fixed now that it's removed in Bug 1431050.
Depends on: 1431050
Attachment #8955195 - Attachment is obsolete: true
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment on attachment 8955194 [details]
Bug 1442006 - Rename globalOverlay.js to chromeGlobals.js and load it in all chrome documents;

https://reviewboard.mozilla.org/r/224356/#review233312

FWIW, I think the approach makes sense but I think it's somewhat backwards to add more Mozilla magic if part of the point of de-XUL/de-XBL was to make the codebase more like web development so it's easier for contributors. Removing the <script> tags is a step backwards from that IMO and AFAICT the specific change to globalOverlay.js doesn't help with de-XUL/de-XBL.

Instead we could add the other things we want to load in Chrome in this existing file (probably after renaming).
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #20)
> Comment on attachment 8955194 [details]
> Bug 1442006 - Rename globalOverlay.js to chromeGlobals.js and load it in all
> chrome documents;
> 
> https://reviewboard.mozilla.org/r/224356/#review233312
> 
> FWIW, I think the approach makes sense but I think it's somewhat backwards
> to add more Mozilla magic if part of the point of de-XUL/de-XBL was to make
> the codebase more like web development so it's easier for contributors.

Yeah, I'm a bit worried about that as well. But at least for XBL->Custom Element migration, requiring every XUL document (including tests) to add a script tag to get the 'built-in' XUL widgets to work seems like a step backwards for development ergonomics compared with XBL (where the bindings are auto loaded in all documents with UA stylesheets).  I think we'd want to have pretty strict guidelines about what gets loaded / imported in this file.

> Removing the <script> tags is a step backwards from that IMO and AFAICT the
> specific change to globalOverlay.js doesn't help with de-XUL/de-XBL.
> 
> Instead we could add the other things we want to load in Chrome in this
> existing file (probably after renaming).

I picked this file based on the fact that it's intended to be 'global' and is used in lots of places, but the way it gets loaded is pretty complicated based on overlays in some places and script tags in others. I do think it's a cleanup but maybe the stuff in here shouldn't actually be considered global.
(In reply to Brian Grinstead [:bgrins] from comment #21)
> > Removing the <script> tags is a step backwards from that IMO and AFAICT the
> > specific change to globalOverlay.js doesn't help with de-XUL/de-XBL.
> > 
> > Instead we could add the other things we want to load in Chrome in this
> > existing file (probably after renaming).
> 
> I picked this file based on the fact that it's intended to be 'global' and
> is used in lots of places, but the way it gets loaded is pretty complicated
> based on overlays in some places and script tags in others. I do think it's
> a cleanup but maybe the stuff in here shouldn't actually be considered
> global.

It's not completely clear to me how 'global' globalOverlay.js is intended to be. Any opinions on if we should make globalOverlay.js load in all chrome documents automagically or if we should save the magic for Custom Element registrations and/or importing XPCOMUtils, Services, etc? See Comment 0 and Comment 21 for the reasons we are attempting to do this in the first place.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dtownsend)
I think this is difficult in part because there are a number of concerns, some of which may conflict. From the Cc/Cu/Ci bug's perspective, we had:

- performance. If we can make XPCOMUtils/AppConstants/Services and various other bits fast and not have per-window/global overhead for loading them, that is good.
- developer ergonomics in terms of DRY. It's annoying to have to start each new file with a bunch of boilerplate, just like it's annoying to start tests with a license header, "use strict", a doc comment and "add_task(async function() {" before you actually get to write anything that does useful stuff.

For custom elements / XUL stuff, there's:

- performance. Manually loading the same script for every global is something that obviously ought to be highly optimizable (should be able to share/clone bytecode!)
- developer ergonomics in terms of DRY. Don't want to have to remember to include all the scripts "just" to have things work.
- similarly but not quite the same: developer ergonomics during the changeover in terms of having less change/churn/confusion and patch conflicts as we change paradigms because changes happen "under the hood", rather than having to update Every Single XUL Document Ever with a bunch of script tags to have "normal" XUL stuff work.
- developer ergonomics in terms of familiarity. If you add a tag, not knowing how to add it because it's added via mozilla-specific magic-sauce is unfortunate (compared to having a regular <script> tag which is more understandable, and similar to CE in HTML, AIUI)


On the whole, I think it's useful enough to have "run this in every window" script that we should make the architecture change. The advantages in terms of performance and ergonomics make it worth it. I think they outweigh the concern that it's a Mozilla-ism. The current stuff we have (XUL overlays, DTDs for l10n) and even what replaces them (our own python-based C-style preprocessing, Fluent) are also all Mozilla-isms (though obviously for Fluent the long-term goal may be that it gathers wider adoption), not off-the-shelf replacements (webpack or other more widely-used stuff, po files, etc.). And we had good reasons for those choices, and I think we have reasonable reasons here too, especially because what we're doing with this stuff (OS-widget-level UI code) is not something for which off-the-shelf markup-y stuff is available to which we could switch with reasonable cost.

In terms of the specifics of the patch and globalOverlay, I don't think globalOverlay itself is particularly valuable here. It looks appealing because of the name and because it lives in toolkit. It might be that in terms of the bikeshedding it's not worth using something else. Equally, in terms of disentangling how it gets used right now, maybe that *is* worth it. I don't think some of the stuff in it (closeWindow, goUpdateCommand) needs to be in every window. So it may be worth having a separate file for the purposes of this bug, rather than repurposing globalOverlay.js

So, TL;DR: I still think we should have a globally loaded thing rather than <script> tags for all the bindings, I think having it be globalOverlay.js may not be the best choice, and a new file might be better.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8955194 [details]
Bug 1442006 - Rename globalOverlay.js to chromeGlobals.js and load it in all chrome documents;

Clearing review while we decide what to include
Attachment #8955194 - Flags: review?(florian)
(In reply to :Gijs from comment #23)
> So, TL;DR: I still think we should have a globally loaded thing rather than
> <script> tags for all the bindings, I think having it be globalOverlay.js
> may not be the best choice, and a new file might be better.

Yeah I think I agree with this. Let's just do this for the custom element bindings.
Flags: needinfo?(dtownsend)
OK, we can put this on hold then until we are ready to migrate the first binding. We'll still have to decide how we want to load the individual files and how to set up the CE registrations on the page.

As for how to load individual files, I expect there will be quite a number of them as we begin to migrate bindings and we will want keep them in separate files so I could see:

1) Have a single chromeGlobals.js, and then use subscriptloader to pull custom element js file in
2) Have a single chromeGlobals.js, and then use preprocessor to pull each custom element js file in
3) Pull each custom element JS file in using the subscriptloader in response to "chrome-document-global-created" (i.e. instead of loading chromeGlobals as the patch here does, loop over an array and load them)
4) Somehow use ES6 modules / import (not sure how that would interact with loading scripts in response to "chrome-document-global-created").

One benefit to (1), (2), and (4) is that we'd have a place to put other globals (like XPCOMUtils, Services, etc). I think we should drop (2) unless if we absolutely need to do that for perf reasons because it makes debugging worse. I'm not sure what (4) would look like in this case - is there a way to import modules from a script that isn't loaded as a script tag (I haven't tested this, so maybe it "just works").

As for how to register the custom elements, the main question I'd have is if they should be registered in the same file where the class is set up.

So should it be (a):

  deck.js:
    class MozDeck extends XULElement { }
    customElements.define("moz-deck", MozDeck);

  button.js:
    class MozButton extends XULElement { }
    customElements.define("moz-button", MozDeck);

  chromeGlobals.js
    // Somehow import deck.js and button.js

Or (b):

  deck.js:
    class MozDeck extends XULElement { }

  button.js:
    class MozButton extends XULElement { }

  chromeGlobals.js
    // First, somehow import deck.js and button.js. Then:
    customElements.define("moz-deck", MozDeck);
    customElements.define("moz-button", MozButton);
Flags: needinfo?(enndeakin)
Triage: P5 - Feature request for non-primary UI that is not on the roadmap; Code cleanup.
Priority: -- → P5
Thanks everyone for the input. I don't think there's any more action needed in this bug, so going to dupe it to Bug 1411707 where patches should land.
No longer blocks: 1411707
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: