From https://bugzilla.mozilla.org/show_bug.cgi?id=1471734#c4: > Can we move the whole nonBrowserWindowStartup stuff out of browser.js into something > more obviously correct / related to where this stuff lives? Doesn't need to happen here. It seems reasonable to add a new script tag in macWindow.inc.xul that both defines the gBrowserInit.nonBrowserWindowStartup / gBrowserInit.nonBrowserWindowDelayedStartup / gBrowserInit.nonBrowserWindowShutdown functions and sets up event listeners for them. Then it would be included after `#include global-scripts.inc`. But maybe there's a different approach here that would make sense.

These are mac-only functions used to support the dock and application menu for non browser windows (anything that includes macWindow.inc.xul). Make this more straightforward by splitting the code out into a new script file that gets loaded directly by macWindow.inc.xul rather than unconditionally adding the functions and only calling them when needed. Bug 1473160 - Move the startup and shutdown functions out of gBrowserInit;r=Gijs They don't rely on anything from that object, so simplify things and remove the cross-file reference by making them normal functions.

Comment on attachment 8991049 [details] Bug 1473160 - Move non browser window startup and shutdown functionality into a separate JS file; https://reviewboard.mozilla.org/r/256036/#review263058 ::: browser/base/content/nonbrowser-mac.js:19 (Diff revision 2) > + }, { once: true }); > + > + return win; > +} > + > +gBrowserInit.nonBrowserWindowStartup = function() { Did you consider using `hg cp` to keep blame for this file? Either is fine by me, just wanted to check... ::: browser/base/content/nonbrowser-mac.js:90 (Diff revision 2) > + // initialize the private browsing UI > + gPrivateBrowsingUI.init(); Because it's less obvious now, can you add a comment to both the BrowserOffline and gPrivateBrowsingUI objects in browser.js indicating they also get called for non-browser windows? In that vein, having just looked at gPrivateBrowsingUI, I found a bug! The URL bar search param stuff errors out whenever we load a private non-browser window on mac today. So if the user is in permanent private browsing (set "never remember history" in prefs), for every non-browser window (otherwise, I doubt non-browser windows can be deemed private, though maybe it happens e.g. for popped-out devtools for private tabs? Haven't checked.). gURLBar is null, because it doesn't exist in non-browser windows. Can you either move that code into the conditional checking getBrowserURL(), or maybe just remove this call to gPrivateBrowsingUI.init() from the non-browser code, remove the check for getBrowserURL(), and copy the disabling of the Tools:Sanitize thing with a check for whether the window is private to this new file, and a comment referencing the existing bug about fixing 'clear recent history' to work with PB? :-)

Comment on attachment 8991050 [details] Bug 1473160 - Move the startup and shutdown functions out of gBrowserInit; https://reviewboard.mozilla.org/r/256038/#review263064 Woop, cleanup!

Oh, and the PB bug turns out to be on file! https://bugzilla.mozilla.org/show_bug.cgi?id=1332391

(In reply to :Gijs (he/him) from comment #6) > Comment on attachment 8991049 [details] > Bug 1473160 - Move non browser window startup and shutdown functionality > into a separate JS file; > > https://reviewboard.mozilla.org/r/256036/#review263058 > > ::: browser/base/content/nonbrowser-mac.js:19 > (Diff revision 2) > > + }, { once: true }); > > + > > + return win; > > +} > > + > > +gBrowserInit.nonBrowserWindowStartup = function() { > > Did you consider using `hg cp` to keep blame for this file? Either is fine > by me, just wanted to check... > > ::: browser/base/content/nonbrowser-mac.js:90 > (Diff revision 2) > > + // initialize the private browsing UI > > + gPrivateBrowsingUI.init(); > > Because it's less obvious now, can you add a comment to both the > BrowserOffline and gPrivateBrowsingUI objects in browser.js indicating they > also get called for non-browser windows? Will do. There's also a lot more surface area in browser.js that is indirectly and probably unexpectedly called by the nonbrowser windows through the menu / keys / commands. Longer term, I'd like to audit what gets called and think about how we can better test this case. For instance, I'm now wondering if other null-checks that got removed in https://hg.mozilla.org/mozilla-central/rev/dd5aaa1e47ad are possible to hit in nonbrowser windows. > In that vein, having just looked at gPrivateBrowsingUI, I found a bug! The > URL bar search param stuff errors out whenever we load a private non-browser > window on mac today. So if the user is in permanent private browsing (set > "never remember history" in prefs), for every non-browser window (otherwise, > I doubt non-browser windows can be deemed private, though maybe it happens > e.g. for popped-out devtools for private tabs? Haven't checked.). gURLBar is > null, because it doesn't exist in non-browser windows. > > Can you either move that code into the conditional checking getBrowserURL(), > or maybe just remove this call to gPrivateBrowsingUI.init() from the > non-browser code, remove the check for getBrowserURL(), and copy the > disabling of the Tools:Sanitize thing with a check for whether the window is > private to this new file, and a comment referencing the existing bug about > fixing 'clear recent history' to work with PB? :-) If you don't mind, I'd like to move that change out to Bug 1332391 since there's already a bug on file.

(In reply to Brian Grinstead [:bgrins] from comment #9) > If you don't mind, I'd like to move that change out to Bug 1332391 since > there's already a bug on file. Sure, that makes sense.

(In reply to :Gijs (he/him) from comment #6) > Comment on attachment 8991049 [details] > Bug 1473160 - Move non browser window startup and shutdown functionality > into a separate JS file; > > https://reviewboard.mozilla.org/r/256036/#review263058 > > ::: browser/base/content/nonbrowser-mac.js:19 > (Diff revision 2) > > + }, { once: true }); > > + > > + return win; > > +} > > + > > +gBrowserInit.nonBrowserWindowStartup = function() { > > Did you consider using `hg cp` to keep blame for this file? Either is fine > by me, just wanted to check... I did consider it but since browser.js is so huge, it felt like overkill. I also don't feel strongly one way or another though.

Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4c4c0eb58466 Move non browser window startup and shutdown functionality into a separate JS file;r=Gijs https://hg.mozilla.org/integration/autoland/rev/a2520436f2ae Move the startup and shutdown functions out of gBrowserInit;r=Gijs

https://hg.mozilla.org/mozilla-central/rev/4c4c0eb58466 https://hg.mozilla.org/mozilla-central/rev/a2520436f2ae