Closed Bug 1441378 Opened 2 years ago Closed 2 years ago

Remove baseMenuOverlay.xul

Categories

(Toolkit :: Toolbars and Toolbar Customization, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bdahl, Assigned: bdahl)

References

Details

Attachments

(3 files)

I think we can do something similar to bug 1439766, by just inlining and using the preprocessor.

How everything is used:

OVERLAY                             USED BY
baseMenuOverlay.xul                 browser.xul, macBrowserOverlay.xul, places.xul

OVERLAY ELEMENT                     USED IN
helpMenu                            browser-menubar.inc, window_menubar.xul (test file)
baseMenuKeyset (empty non-mac)      browser-sets.inc, browserMountPoints.inc, macWindowMenu.inc
(macOnly):
  menu_ToolsPopup                   browser-menubar.inc
  (from macWindowMenu.inc):
    baseMenuCommandSet              browser-sets.inc, browserMountPoints.inc
    baseMenuKeyset                  baseMenuOverlay.xul, browser-sets.inc, browserMountPoints.inc
    windowMenu                      browser-menubar.inc, layoutdebug.xul (not used anymore?)

INCLUDE FILES                       USED IN
browser-menubar.inc                 browser.xul, macBrowserOverlay.xul
browserMountPoints.inc              aboutDialog.xul, hiddenWindow.xul, pageInfo.xul, places.xul, setDesktopBackground.xul, updates.xul
browser-sets.inc                    browser.xul, macBrowserOverlay.xul
Assignee: nobody → bdahl
Attached image before.svg
I tried to make sense of this by tracing the flow with dot file. Here's the before file. 

red=MacOS only
green=non-MacOS
Attached image after.svg
And here's after the changes.
For the removal of macWindowMenu.inc I filed bug 1442045 for thunderbird.
Comment on attachment 8954949 [details]
Bug 1441378 - Replace baseMenuOverlay.xul with inlining and preprocessing.

https://reviewboard.mozilla.org/r/224132/#review230320

This looks great, but I spotted some more stuff that we can avoid doing (and perhaps shipping) on macOS, plus we can remove a `<keyset>` now that we don't need it to overlay into. It probably makes sense for me to look at it again after that, or perhaps I'm wildly misunderstanding how we use this overlay / the associated l10n bits, so clearing review.

::: browser/base/content/aboutDialog.xul:16
(Diff revision 1)
> +<!ENTITY % baseMenuOverlayDTD SYSTEM "chrome://browser/locale/baseMenuOverlay.dtd">
> +%baseMenuOverlayDTD;

Here and in all the other files that don't have a menubar visible on Linux/Windows (which I think is all the xul files you're touching besides browser.xul ?), I think we only need this dtd on macOS, so we can just put this in an ifdef as well, right? In fact, perhaps we can avoid shipping that dtd file on non-mac in the relevant jar.mn file?

::: browser/base/content/browser-menubar.inc:543
(Diff revision 1)
> +<!-- nsMenuBarX hides these and uses them to build the Application menu.
> +     When using Carbon widgets for Mac OS X widgets, some of these are not
> +     used as they only apply to Cocoa widget builds. All version of Firefox
> +     through Firefox 2 will use Carbon widgets. -->

Please kill the "Carbon" bit of this comment which hasn't been relevant since Carbon died, at least 6 years ago.

::: browser/base/content/browser-menubar.inc:547
(Diff revision 1)
> +#ifdef XP_MACOSX
> +<!-- nsMenuBarX hides these and uses them to build the Application menu.
> +     When using Carbon widgets for Mac OS X widgets, some of these are not
> +     used as they only apply to Cocoa widget builds. All version of Firefox
> +     through Firefox 2 will use Carbon widgets. -->
> +                <menuitem id="menu_preferences" label="&preferencesCmdMac.label;" key="key_preferencesCmdMac" oncommand="openPreferences(undefined, {origin: 'commandLineLegacy'});"/>

While I agree with you that these kids should be 2-space indented from their `<menupopup>` parent, the other kids aren't, so now this menuitem looks like a kid of the previous one. So please either also indent all the other kids, or just stick to the pre-existing weird indent.

::: browser/base/content/browser-menubar.inc:571
(Diff revision 1)
> -          <menu id="helpMenu" />
> +#ifdef XP_WIN
> +            <menu id="helpMenu"
> +                  label="&helpMenuWin.label;"
> +                  accesskey="&helpMenuWin.accesskey;">
> +#else
> +            <menu id="helpMenu"
> +                  label="&helpMenu.label;"
> +                  accesskey="&helpMenu.accesskey;">
> +#endif

Nit: can you put the ifdefs only around the actual attribute differences rather than the `<menu>` node? Just put the closing `>` on its own line.

::: browser/base/content/browser-sets.inc:414
(Diff revision 1)
> +             label="&zoomWindow.label;"
> +             oncommand="zoomWindow();" />
> +  </commandset>
> +
> +  <keyset id="baseMenuKeyset">
> +    <key id="key_minimizeWindow"

Nothing relies on the keys in this keyset being in this specific keyset. All of the hits in DXR except baseMenuOverlay and macWindowMenu (which are being nuked in this patch) are self-closing ( https://dxr.mozilla.org/mozilla-central/search?q=basemenukeyset&=mozilla-central ). So I don't think we need this anymore. Can you include the keys themselves in the main keyset above here and remove the `baseMenuKeyset` element here and in browserMountPoints.inc as well please?

::: browser/base/content/browser-sets.inc:422
(Diff revision 1)
> +         modifiers="accel"/>
> +    <key id="key_openHelpMac"
> +         oncommand="openHelpLink('firefox-osxkey');"
> +         key="&helpMac.commandkey;"
> +         modifiers="accel"/>
> +    <!-- These are used to build the Application menu under Cocoa widgets -->

Nit: remove 'under cocoa widgets'
Attachment #8954949 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8954949 [details]
Bug 1441378 - Replace baseMenuOverlay.xul with inlining and preprocessing.

https://reviewboard.mozilla.org/r/224132/#review230320

> Here and in all the other files that don't have a menubar visible on Linux/Windows (which I think is all the xul files you're touching besides browser.xul ?), I think we only need this dtd on macOS, so we can just put this in an ifdef as well, right? In fact, perhaps we can avoid shipping that dtd file on non-mac in the relevant jar.mn file?

I'll add ifdefs, but we can't avoid shipping baseMenuOverlay.dtd on win/linux since it's still needed in browser.xul for the main menu.
Comment on attachment 8954949 [details]
Bug 1441378 - Replace baseMenuOverlay.xul with inlining and preprocessing.

https://reviewboard.mozilla.org/r/224132/#review230320

> Nothing relies on the keys in this keyset being in this specific keyset. All of the hits in DXR except baseMenuOverlay and macWindowMenu (which are being nuked in this patch) are self-closing ( https://dxr.mozilla.org/mozilla-central/search?q=basemenukeyset&=mozilla-central ). So I don't think we need this anymore. Can you include the keys themselves in the main keyset above here and remove the `baseMenuKeyset` element here and in browserMountPoints.inc as well please?

Looks like I can also do this with baseMenuCommandSet too?
Comment on attachment 8954949 [details]
Bug 1441378 - Replace baseMenuOverlay.xul with inlining and preprocessing.

https://reviewboard.mozilla.org/r/224132/#review230652

Just two small nits, thanks!

::: browser/base/content/browser-sets.inc:110
(Diff revisions 2 - 3)
>  
>  #ifdef NIGHTLY_BUILD
>      <command id="wrCaptureCmd" oncommand="gWebRender.capture();"/>
>  #endif
> +#ifdef XP_MACOSX
> +    <script type="application/javascript" src="chrome://global/content/macWindowMenu.js"/>

Can you put the script include in global-scripts.inc ? Sorry for not thinking of this for the first review... :-(

::: browser/base/content/browserMountPoints.inc:2
(Diff revision 3)
>  <commandset id="mainCommandSet"/>
>  <commandset id="baseMenuCommandSet"/>

Good catch on the commandset, this means you can remove this one, too, I think?

I wonder when we can just ditch this whole file. I guess it's when there's 1 or 0 overlays relying on the elements in it? (If there's just 1, I would assume any new elements just get appended into the document, though I could be wrong on that...)
Attachment #8954949 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954949 [details]
Bug 1441378 - Replace baseMenuOverlay.xul with inlining and preprocessing.

https://reviewboard.mozilla.org/r/224132/#review230320

> I'll add ifdefs, but we can't avoid shipping baseMenuOverlay.dtd on win/linux since it's still needed in browser.xul for the main menu.

Good point!
Comment on attachment 8954949 [details]
Bug 1441378 - Replace baseMenuOverlay.xul with inlining and preprocessing.

https://reviewboard.mozilla.org/r/224132/#review230652

> Good catch on the commandset, this means you can remove this one, too, I think?
> 
> I wonder when we can just ditch this whole file. I guess it's when there's 1 or 0 overlays relying on the elements in it? (If there's just 1, I would assume any new elements just get appended into the document, though I could be wrong on that...)

When I remove macBrowserOverlay.xul I think I'll be able to remove this file then.
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d301d4626305
Replace baseMenuOverlay.xul with inlining and preprocessing. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/d301d4626305
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.