Closed Bug 732896 Opened 12 years ago Closed 1 month ago

Refactor confusing "nonBrowserWindow" code in browser.js

Categories

(Firefox :: General, defect)

x86
macOS
defect

Tracking

()

RESOLVED DUPLICATE of bug 1473160

People

(Reporter: Dolske, Unassigned)

References

Details

Attachments

(1 file)

Attached patch Patch v.1Splinter Review
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.
Attachment #602825 - Flags: review?(gavin.sharp)
(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.
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 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.
Attachment #602825 - Flags: review?(gavin.sharp) → review-
Severity: normal → S3

This was fixed in bug 1473160 . 🎉

Status: NEW → RESOLVED
Closed: 1 month ago
Duplicate of bug: 1473160
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: