732896 - Refactor confusing "nonBrowserWindow" code in browser.js

Status: RESOLVED DUPLICATE of bug 1473160

(Firefox :: General, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

While working on bug 731926 I stumbled on this confusing code. Decided not to clean it up there to keep things simple.

3 main weird things with the current code:

 1) browser.js contains nonBrowserWindowStartup / nonBrowserWindowShutdown.
    Normally one expects code in browser.js to be for, well, browser windows!

 2) This code is only ever referenced by macBrowserOverlay.xul, so it seems
    better to just live there.

 3) A chunk of this code is only relevant to hiddenWindow.xul, so it also seems
    appropriate to have that code live in hiddenWindow.xul.

I'd like to find a way to avoid the (now) macBrowserOverlay code from calling into browser.js globals, but that's a much bigger ball of twine to unravel. Ditto for not requiring various xul windows to remember to include macBrowserOverlay. Seems like this ought to be a window focus watcher in browser.js, to automagically twiddle stuff when switching to a non-browser window.

Note that this is built on top of the patches in bug 731926, though strictly speaking they're independent.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to Justin Dolske [:Dolske] from comment #0)
> I'd like to find a way to avoid the (now) macBrowserOverlay code from
> calling into browser.js globals, but that's a much bigger ball of twine to
> unravel. Ditto for not requiring various xul windows to remember to include
> macBrowserOverlay. Seems like this ought to be a window focus watcher in
> browser.js, to automagically twiddle stuff when switching to a non-browser
> window.

The relevant "browser.js globals" are all of the functions needed for the menu bar commands. I don't think our Cocoa menubar implementation supports easily "twiddling stuff" dynamically, at least not in a way that would be performant. I don't doubt that we can clean some stuff up, but I think that the kind of re-working you're talking about would involve significant architectural work.

Comment 2

[Justin Dolske [:Dolske]]

No, I just meant the stuff being called in nonBrowserWindowDelayedStartup et al, so that OSX and non-OSX startup paths are more similar.

The "twiddling" would just be a simpler way to ensure non-browser windows do the right thing without having to include the overlay (think: addons). For example, have some browser component fire an "macMenusSetup" even at new windows during creation. If code in that window cancels the event, it's responsible for controlling the menubar itself. Otherwise the component does the fixup that's currently being done by macBrowserOverlay.xul.

I haven't thought about either extensively, so I'm sure there are some catches. And certain neither will be fixed here. :)

Comment 3

[Asaf Romano (gone)]

Comment on attachment 602825 [details] [diff] [review]
Patch v.1

What Gavin said.

Keep in mind:
1) Our cocoa implementation for menus only fakes app-menu. That's the way it is. There really is a per-window-menu
2) The mac browser overlay is also used for non-browser windows (e.g. the download manager), not just the hidden window.

As for this patch, I don't think we'll benefit from moving js code (some of it very-similar to the regular startup path) to a xul file. Nor do I think we'd benefit from introducing more js files in this area. The later option is, however, better than the former. If you & Gavin think the current approach is confusing more-than-it-could-be (given 1+2), please move the code to a new js file.

One more thing you could do is to disable the various item by actually overlaying them. I considered doing so back when teenager-me wrote this code, and I cannot recall why I chose not to do so after all.

Comment 4

[:Gijs (he/him)]

This was fixed in bug 1473160 . 🎉