1473160 - Set up nonBrowserWindowStartup / nonBrowserWindowDelayedStartup / nonBrowserWindowShutdown somewhere outside of browser.js

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1473160 - Move non browser window startup and shutdown functionality into a separate JS file;r=Gijs

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 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1473160 - Move non browser window startup and shutdown functionality into a separate JS file;

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.

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

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1473160 - Move the startup and shutdown functions out of gBrowserInit;

They don't rely on anything from that object, so simplify things and remove the
cross-file reference by making them normal functions.

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

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8991049 [details]
Bug 1473160 - Move non browser window startup and shutdown functionality into a separate JS file;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256036/diff/1-2/

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8991050 [details]
Bug 1473160 - Move the startup and shutdown functions out of gBrowserInit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256038/diff/1-2/

Comment 6

[:Gijs (he/him)]

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 7

[:Gijs (he/him)]

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!

Comment 8

[:Gijs (he/him)]

Oh, and the PB bug turns out to be on file!

https://bugzilla.mozilla.org/show_bug.cgi?id=1332391

Comment 9

[Brian Grinstead [:bgrins]]

(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.

Comment 10

[:Gijs (he/him)]

(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.

Comment 11

[Brian Grinstead [:bgrins]]

(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.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8991049 [details]
Bug 1473160 - Move non browser window startup and shutdown functionality into a separate JS file;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256036/diff/2-3/

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8991050 [details]
Bug 1473160 - Move the startup and shutdown functions out of gBrowserInit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256038/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Raul Gurzau (:RaulG)]

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