1439766 - Remove editMenuOverlay.xul

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, enhancement)

Description

[Brendan Dahl [:bdahl]]

Part of the XUL overlay removal project:

This overlay is the most commonly used overlay in Firefox. It helps share code for the edit commands for the top menu, context menu and shortcut keys. The overlay can reduce the amount copying/pasting needed for this functionality, however, in practice not much is saved over using the preprocessor and inling the required elements.

The current plan to remove the overlay is:
1) Move all <command> / <commandset> into an include file or create them from within JS and include that in the master document
2) Add the DTD to the master document
3) Add the JS needed to the master document
4) Copy over used elements and attributes from the keyset/top menu/context menu portions.

Comment 1

[Brendan Dahl [:bdahl]]

Uses of elements from the overlay:

ELEMENT               COUNT USED BY
id="editMenuKeys"         6 contentAreaDownloadsView.xul, history-panel.xul, places.xul, toolbox.xul, scratchpad.xul, webconsole.xul

id="editMenuCommands"    14 browser-sets.inc, web-panels.xul, webext-panels.xul, contentAreaDownloadsView.xul, bookmarksPanel.xul, history-panel.xul, places.xul, debugger.xul, toolbox.xul, scratchpad.xul, storage.xul, styleeditor.xul, webconsole.xul, webide.xul

id="menu_edit"            0

id="menu_undo"            2 scatchpad.xul and browser-menubar.inc(has attributes)
id="menu_redo"            2 scatchpad.xul and browser-menubar.inc(has attributes)
id="menu_cut"             2 scatchpad.xul and browser-menubar.inc(has attributes)
id="menu_copy"            3 scratchpad, pageinfo.xul(has attributes) and browser-menubar.inc(has attributes)
id="menu_paste"           2 scatchpad.xul and browser-menubar.inc(has attributes)
id="menu_delete"          1 browser-menubar.inc(has attributes)
id="menu_selectAll"       3 scratchpad, pageinfo.xul(has attributes) and browser-menubar.inc(has attributes)
id="menu_find"            2 scatchpad.xul and browser-menubar.inc(has attributes)
id="menu_findAgain"       2 scatchpad.xul and browser-menubar.inc(has attributes)
id="menu_findPrevious"    0

id="cMenu_undo"           2 toolbox.xul, styleeditor.xul
id="cMenu_redo"           0
id="cMenu_cut"            3 toolbox.xul, scratchpad.xul, styleeditor.xul
id="cMenu_copy"           5 debugger.xul, toolbox.xul, scratchpad.xul, styleeditor.xul, webconsole.xul
id="cMenu_paste"          3 toolbox.xul, scratchpad.xul, styleeditor.xul
id="cMenu_delete"         3 toolbox.xul, scratchpad.xul, styleeditor.xul
id="cMenu_selectAll"      5 debugger.xul, toolbox.xul, scratchpad.xul, styleeditor.xul, webconsole.xul
id="cMenu_find"           0
id="cMenu_findAgain"      1 styleeditor.xul
id="cMenu_findPrevious"   0

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Brendan Dahl [:bdahl] from comment #0)
> 3) Add the JS needed to the master document

Just a note from previous discussions we had about this: we have multiple global-ish js files that get loaded in many XUL docs (and most docs in the case of OSX where we load macBrowserOverlay.xul). In particular:

* https://searchfox.org/mozilla-central/source/toolkit/content/globalOverlay.js
* https://searchfox.org/mozilla-central/source/toolkit/content/editMenuOverlay.js
* https://searchfox.org/mozilla-central/source/browser/base/content/utilityOverlay.js

But what gets loaded where gets pretty complicated (I've started to document this here: https://docs.google.com/document/d/1FOB6Qp_qhxMw4R2h6P81yWFWnsM9LfNPEOIWroqN4r8/edit).

Removing overlays in general is going to help a lot, but I also think we could also simplify this from the JS side by putting all of these global functions into a single file and ensure that it gets loaded in all XUL docs. This could be done by putting a <script> tag in all docs or through the subscript loader and running in response to an observer notification when a XUL doc gets created.

Comment 3

[Brian Grinstead [:bgrins]]

We don't necessarily have to tackle that in this bug, though.

Comment 4

[Brendan Dahl [:bdahl]]

Attachment: Bug 1439766 - Replace editMenuOverlay.xul by inlining and preprocessing.

The overlay elements with children of editMenuOverlay.xul are moved into
include files (editMenuCommands.inc.xul and editMenuKeys.inc.xul). For
the other single elements in the overlay, the attributes are inlined
wherever they are used.

Review commit: https://reviewboard.mozilla.org/r/223356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223356/

Comment 5

[Brendan Dahl [:bdahl]]

Comment on attachment 8954198 [details]
Bug 1439766 - Replace editMenuOverlay.xul by inlining and preprocessing.

https://reviewboard.mozilla.org/r/223356/#review229412

::: toolkit/content/tests/chrome/bug366992_window.xul:16
(Diff revision 1)
>          onload="onLoad();"
>          width="600"
>          height="600"
>          title="366992 test">
>  
> -  <commandset id="editMenuCommands"/>
> +  <commandset id="editMenuCommands">

I wasn't quite sure what to do here, for now I've just copied in edit menu commands since there's no preprocessor for test files. Also, this test really seems like it should have been a pure xul test without pulling in browser components.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8954198 [details]
Bug 1439766 - Replace editMenuOverlay.xul by inlining and preprocessing.

https://reviewboard.mozilla.org/r/223356/#review229528

::: browser/base/content/browser-doctype.inc:22
(Diff revision 1)
> +<!ENTITY % editMenuDTD SYSTEM "chrome://global/locale/editMenuOverlay.dtd">
> +%editMenuDTD;

The removal of the overlay actually removes this in a few other places (besides browser.xul and macBrowserOverlay.xul, which are the only includers of browser-doctype.inc) as well, but it's covered up by the fact that we're still loading it from the placesOverlay.xul file as well. Can we omit it from here and just ensure that we remember to add it separately to all the consumers when removing placesOverlay.xul ?

::: browser/components/places/content/history-panel.xul:39
(Diff revision 1)
> +
>    <commandset id="placesCommands"/>
>  
> -  <keyset id="editMenuKeys">
> +#include ../../../../toolkit/content/editMenuKeys.inc.xul
> +  <keyset id="editMenuKeysExtra">
>  #ifdef XP_MACOSX

Could make the entire keyset (of 1 key) XP_MACOSX (here and in the other files where we have to have an 'extra' bit)

::: browser/components/places/content/places.xul:133
(Diff revision 1)
>  #endif
>    </keyset>
>  
> -  <keyset id="editMenuKeys">
> +#include ../../../../toolkit/content/editMenuKeys.inc.xul
> +
> +  <keyset id="editMenuKeysExtra">

If it does not strike you as a crazy idea, can you file a follow-up to see if we should just add the VK_BACK macOS key to the editMenuKeys generally? 3 of the includes now add this themselves, and I expect that in many cases we'll just want it available anyway, though I could be wrong.

::: toolkit/content/jar.mn
(Diff revision 1)
>     content/global/buildconfig.css
>     content/global/contentAreaUtils.js
>     content/global/datepicker.xhtml
>  #ifndef MOZ_FENNEC
>     content/global/editMenuOverlay.js
> -*  content/global/editMenuOverlay.xul

Ah, this is toolkit. If you haven't yet, it would be courteous to file a bug each for Thunderbird and SeaMonkey that they'll have to restore this file / apply similar-ish changes to their files.

::: toolkit/content/tests/chrome/bug366992_window.xul:16
(Diff revision 1)
>          onload="onLoad();"
>          width="600"
>          height="600"
>          title="366992 test">
>  
> -  <commandset id="editMenuCommands"/>
> +  <commandset id="editMenuCommands">

I think for now this is fine, and yes the test probably ought to have done something else, but at this point, meh.

Comment 7

[Richard Marti (:Paenglab)]

(In reply to :Gijs from comment #6)
> 
> Ah, this is toolkit. If you haven't yet, it would be courteous to file a bug
> each for Thunderbird and SeaMonkey that they'll have to restore this file /
> apply similar-ish changes to their files.

Thank you. For TB it's not needed as it does not use editMenuOverlay.xul. Soon we use it in the forked viewSource.xul, but I'm looking that it gets the needed changes.

SM has some hits of editMenuOverlay.xul and they would surely appreciate a info.

Comment 8

[Brendan Dahl [:bdahl]]

Comment on attachment 8954198 [details]
Bug 1439766 - Replace editMenuOverlay.xul by inlining and preprocessing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223356/diff/1-2/

Comment 9

[Brendan Dahl [:bdahl]]

Comment on attachment 8954198 [details]
Bug 1439766 - Replace editMenuOverlay.xul by inlining and preprocessing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223356/diff/2-3/

Comment 10

[Brendan Dahl [:bdahl]]

Comment on attachment 8954198 [details]
Bug 1439766 - Replace editMenuOverlay.xul by inlining and preprocessing.

https://reviewboard.mozilla.org/r/223356/#review229528

> If it does not strike you as a crazy idea, can you file a follow-up to see if we should just add the VK_BACK macOS key to the editMenuKeys generally? 3 of the includes now add this themselves, and I expect that in many cases we'll just want it available anyway, though I could be wrong.

That sounds resonable, I guess we'd need to make sure that the binding doesn't conflict with anywhere that just uses editMenuKeys.inc.xul.

> Ah, this is toolkit. If you haven't yet, it would be courteous to file a bug each for Thunderbird and SeaMonkey that they'll have to restore this file / apply similar-ish changes to their files.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1441668 for seamonkey.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba1808744ced
Replace editMenuOverlay.xul by inlining and preprocessing. r=Gijs

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/ba1808744ced