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

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

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'

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

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