Closed Bug 1473160 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: General, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Summary: Set up nonBrowserWindowStartup somewhere outside of browser.js → Set up nonBrowserWindowStartup / nonBrowserWindowDelayedStartup / nonBrowserWindowShutdown somewhere outside of browser.js
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.
Attachment #8991041 - Attachment is obsolete: true
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Priority: -- → P1
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? :-)
Attachment #8991049 - Flags: review?(gijskruitbosch+bugs) → review+
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!
Attachment #8991050 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Blocks: 1332391
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.