Closed Bug 1435437 Opened 3 years ago Closed 2 years ago

Remove bundle_browser and bundle_browser_region elements from baseMenuOverlay.xul

Categories

(Firefox :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(2 files)

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=
Product: Core → Firefox
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
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 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!
Attachment #8948048 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8948049 [details]
Bug 1435437 - Don't append bundle_browser using an overlay;

https://reviewboard.mozilla.org/r/217688/#review223670
Attachment #8948049 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
Pushed by bgrinstead@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/a5eb50c5047f
https://hg.mozilla.org/mozilla-central/rev/384ba949a896
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.