1435437 - Remove bundle_browser and bundle_browser_region elements from baseMenuOverlay.xul

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Brian Grinstead [:bgrins]]

This markup gets added into any document that loads the overlay baseMenuOverlay.xul [0]:

    <stringbundleset id="stringbundleset">
        <stringbundle id="bundle_browser" src="chrome://browser/locale/browser.properties"/>
        <stringbundle id="bundle_browser_region" src="chrome://browser-region/locale/region.properties"/>
    </stringbundleset>

bundle_browser_region appears unreferenced, and bundle_browser is referenced in two places from script:

1) browser.js (mapped onto the gNavigatorBundle variable)
2) utilityOverlay.js (loaded by baseMenuOverlay.xul)

We could lesson the dependence on the overlay system if we didn't use a <stringbundle> here but rather Services.strings.createBundle. Or, continue using a <stringbundle> but move it out into the markup in place that currently uses baseMenuOverlay.xul.

After that I believe we could remove <stringbundleset id="stringbundleset"> throughout the codebase [1].

[0]: https://searchfox.org/mozilla-central/search?q=bundle_browser&path=
[1]: https://searchfox.org/mozilla-central/search?q=id%3D%22stringbundleset%22&path=

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1435437 - Expose gNavigatorBundle as a plain JS object with a stringbundle-like API;

This will allow us to not rely on an actual <stringbundle> while still avoiding a mass rewrite
of code that accesses gNavigatorBundle with the more awkward API exposed by gBrowserBundle.

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

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1435437 - Don't append bundle_browser using an overlay;

- bundle_browser_region is already unused so that's just removed.
- After the previous changeset, bundle_browser is referenced only in utilityOverlay.js,
  where it's used for the context menu's "Open Link In New Container Tab" submenu.

By updating that one consumer to use Services.strings.createBundle directly we can
remove all instances of the bundle_browser node (overlay and otherwise), and chip
away at the baseMenuOverlay file.

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

Comment 3

[Brian Grinstead [:bgrins]]

Gijs, for context: I've started documenting the various includes / overlays that get loaded in each window configuration (browser.xul windows, non-browser windows on OSX, non-browser windows on Win/Linux): https://docs.google.com/document/d/1FOB6Qp_qhxMw4R2h6P81yWFWnsM9LfNPEOIWroqN4r8/edit.

In addition to wanting to ultimately remove baseMenuOverlay.xul, the way the browser_bundle element gets loaded was a bit confusing, at least to me.

There's a dependence on callers that load baseMenuOverlay.xul on having a <stringbundleset id="stringbundleset"> in their DOM. That's guaranteed if they `#include browser-sets.inc` or `#include browserMountPoints.inc` but there are two cases I've found worth noting:

1) There's one case where we do include baseMenuOverlay but don't include one of those two places that define the stringbundleset: viewSource.xul via viewSourceOverlay.xul (https://searchfox.org/mozilla-central/source/browser/base/content/viewSourceOverlay.xul). This window will be removed in Bug 1418403 anyway.
2) In places.xul we include browser-sets.inc in OSX via macBrowserOverlay.xul, and we include browserMountPoints.inc in non-osx so the overlay has a place to put the stringbundle. But in the latter case I don't think there's any way that the bundle ever gets accessed (as browser.js doesn't get loaded and the utilityOverlay.js->createUserContextMenu won't get called due to browser-menubar.inc not being included).

Comment 4

[:Gijs (he/him)]

Comment on attachment 8948048 [details]
Bug 1435437 - Expose gNavigatorBundle as a plain JS object with a stringbundle-like API;

https://reviewboard.mozilla.org/r/217686/#review223662

Curious how this will impact fluent later, but this looks great for now. Thanks!

Comment 5

[:Gijs (he/him)]

Comment on attachment 8948049 [details]
Bug 1435437 - Don't append bundle_browser using an overlay;

https://reviewboard.mozilla.org/r/217688/#review223670

Comment 6

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bad0380ddc8f305c18f1ad94deb076a795ed101d

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to :Gijs from comment #4)
> Comment on attachment 8948048 [details]
> Bug 1435437 - Expose gNavigatorBundle as a plain JS object with a
> stringbundle-like API;
> 
> https://reviewboard.mozilla.org/r/217686/#review223662
> 
> Curious how this will impact fluent later, but this looks great for now.
> Thanks!

AIUI this will neither help nor hurt Fluent migration. A lot of the gBrowserBundle / gNavigatorBundle accesses are going to need refactoring to fit a "wait to localize a string until it's time to change the DOM" pattern, or to make callers async if we actually need to hold onto the string in JS for some reason. And that refactoring will have to happen whether we access strings through <stringbundle> or through Services.strings.createBundle.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5eb50c5047f
Expose gNavigatorBundle as a plain JS object with a stringbundle-like API;r=Gijs
https://hg.mozilla.org/integration/autoland/rev/384ba949a896
Don't append bundle_browser using an overlay;r=Gijs

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/a5eb50c5047f
https://hg.mozilla.org/mozilla-central/rev/384ba949a896