1880909 - Move gBrowserInit to its own file

Status: RESOLVED FIXED

(Firefox :: General, task, P3)

Description

[:Gijs (he/him)]

gBrowserInit lives here. The entire object and its direct dependencies (_resolveDelayedStartup and delayedStartupPromise and the content window paint bit) should move to their own file - probably unoriginally called something like browser-init.js. It should be loaded only from browser windows, so immediately after https://searchfox.org/mozilla-esr78/rev/3c633b1a0994f380032a616eced632398354d83e/browser/base/content/browser.xhtml#83-86 . We shouldn't need it in other windows, though we'll want to run automated tests to make sure that moving it doesn't break anything.

Comment 1

[Steve P]

Hi Gijs, I'm a new contributor and will be working on getting a patch for this submitted ASAP.

I think your description is enough to get started on this, I'll reach out with any questions, thanks!

Comment 2

[Steve P]

Hi GIjs,

I wanted to double check a couple of things with you. From what I'm seeing in browser.js, the modules are loaded in via a .sys.mjs file. Will I need to create this file as well to import in browser-init.js? If so, should I be copying a certain .sys.mjs file or template and swapping out the module name to the new one? Also when importing it, should it be at the top where XPCOMUtils and AppConstants are imported, or will it be part of the lazy module getters below?

Next question is, I had initially created a BrowserInit variable to try be consistent with the other I see here, however when I did that the eslint was not happy and had a message saying new global variables are not permitted. Should I keep this variable as gBrowserInit then?

Thanks!

Comment 3

[:Gijs (he/him)]

(In reply to Steve P from comment #2)

Hi GIjs,

I wanted to double check a couple of things with you. From what I'm seeing in browser.js, the modules are loaded in via a .sys.mjs file. Will I need to create this file as well to import in browser-init.js? If so, should I be copying a certain .sys.mjs file or template and swapping out the module name to the new one?

sys.mjs modules are singletons (loaded once - any subsequent imports just get a pointer to the same exported object - there's only ever 1), and browser scripts are loaded (and run) once per window (so there can be more than one). Although it could be interesting to switch this code to being a singleton module, because the gBrowserInit code depends on so many other things that live inside the window it probably isn't a good idea to try to do it at the same time as splitting it off from the main window code.

So, I think in this bug this should stay an "old fashioned" normal script file (not a module) and loaded here:

https://searchfox.org/mozilla-central/rev/1f5e1875cbfd5d4b1bfa27ca54832f62dd19589e/browser/base/content/browser.xhtml#102-106

To actually do the load, you can use the approach from global-scripts.inc:

Services.scriptloader.loadSubScript("chrome://browser/content/browser-init.js", this);

inside a <script> tag, directly in browser.xhtml (because we only want this object in browser windows, not in other ones). Note that you'll need to add the new file to jar.mn, in this alphabetically sorted list

Also when importing it, should it be at the top where XPCOMUtils and AppConstants are imported, or will it be part of the lazy module getters below?

See above. For future reference, the "lazy" scripts and modules are only loaded when accessed. But because gBrowserInit is used when the window is loaded, that wouldn't really be very lazy! It would be dereferenced immediately. So there's no point in loading it lazily - so we'll load it immediately (synchronously) using loadSubScript.

Next question is, I had initially created a BrowserInit variable to try be consistent with the other I see here, however when I did that the eslint was not happy and had a message saying new global variables are not permitted. Should I keep this variable as gBrowserInit then?

Yes, keep it as gBrowserInit. If you look at https://searchfox.org/mozilla-central/search?q=gBrowserInit you'll see there's quite some code that depends on being able to access it this way. If you add it in the script file as suggested, the linter should be happy - though once you remove it from browser.js it will tell you (if it hasn't already) that there was an expected symbol in browser.js for gBrowserInit that will not be there anymore (because we're moving it to browser-init.js instead). You can remove it from the .globals file at that point.

Comment 4

[Steve P]

Thanks Gijs, I've made most of the changes I think, and when running ./mach run, I'm currently getting a ReferenceError that gBrowserInit is not defined. I'll walk through what I've done so far, and hopefully you can catch where I'm missing something.

I first moved the entire definition of gBrowserInit, as well as the declarations of _resolveDelayedStartup and delayedStartupPromise to teh file browser-init.js, and removed them from browser.js. Like you mentioned, the linter was not happy about the global variable not being defined there, so I removed those three from the browser.js.globals file. I then added Services.scriptloader.loadSubScript("chrome://browser/content/browser-init.js", this); as the first line within <script>, assuming our init file should be the first file loaded, since there are dependencies requiring it. I also added to jar.mn this line: content/browser/browser-init.js (content/browser-init.js) between browser-gestureSupport.js and browser-pageActions.js.

The one thing I see that you have, that I haven't done yet, is 'the content window paint bit' that you mentioned should also be moved, however I'm not sure what exactly I'm moving here. In browser.js, "_firstContentWindowPaintDeferred" is just a string argument to ChromeUtils.defineLazyGetter(), which seems to be its own separate thing. Within browser-init.js, I see a function _setupFirstContentWindowPaintPromise() with some event listeners and resolving the promise of this._firstContentWindowPaintDeferred, but I'm really not sure if I should be doing something with this or not.

Please let me know when you have time if I've missed something here, thank you!

Comment 5

[Steve P]

Attachment: Bug 1880909 - Move gBrowserInit to its own file.r?Gijs

Comment 6

[Steve P]

Here is a WIP patch for you to view the changes: https://phabricator.services.mozilla.com/D208681

Comment 7

[:Gijs (he/him)]

Thanks for the patch! Unfortunately, it looks like you haven't git add/hg add-ed (depending on how you're working with mozilla-central) the new file (browser-init.js) in the patch, so I can't really try out and see what problems you're hitting.

At a guess, right now your patch loads the file like so:

  <title data-l10n-id="browser-main-window-title"></title>

# All JS files which are needed by browser.xhtml and other top level windows to
# support MacOS specific features *must* go into the global-scripts.inc file so
# that they can be shared with macWindow.inc.xhtml.
#include global-scripts.inc

<script>
  /* eslint-env mozilla/browser-window */
  Services.scriptloader.loadSubScript("chrome://browser/content/browser-init.js", this);

Inside global-scripts.inc, we load browser.js - before we load browser-init.js. So when browser.js runs, gBrowserInit isn't yet defined.

It looks like as you said, you haven't yet yet moved the _firstContentWindowPaint stuff - and so the post-patch version of browser.js still has:

ChromeUtils.defineLazyGetter(
  gBrowserInit,
  "_firstContentWindowPaintDeferred",
  () => Promise.withResolvers()
);

this tries to access gBrowserInit - but it doesn't exist yet because it's not being loaded until after browser.js has been loaded.

That line of JS, although it passes a string argument for _firstContentWindowPaintDeferred, creates that as a property on gBrowserInit.
It's lazy (so the promise isn't created until the property is accessed) but I think that's not actually needed here (to create the "deferred" object that has a promise and resolve property we used to have to load a module but we don't anymore because Promise.withResolvers() is now a language feature - and it looks like we never de-lazified the code). You could remove it from browser.js and in your new browser-init.js file, you could simply define a property with that name on the gBrowserInit object that as its value has Promise.withResolvers() - no lazy getter required.

Hopefully that makes sense?

The idleTasksFinishedPromise property is also accessed from browser.js still, so adding that onto gBrowserInit will also want to move into browser-init.js.

After that I think you should be OK! But if not, and you still see JS errors, please include the full stack - that'll help me work out what is going wrong. :-)

Comment 8

[Steve P]

Hi Gijs, that did make sense, and it seems to all be working now! One question I had was if there was any conventional way for where I should be naming those new properties? I added them right after the _tabToAdopt but I can move them if need be. Also is there any tests I should be running to make sure everything is functioning properly?

Thanks.

Comment 9

[:Gijs (he/him)]

Hey Steve, this is looking good - I have left a review on phab, let's continue the conversation there. :-)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9be3252c1b82
Move gBrowserInit to its own file.r=Gijs

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/9be3252c1b82