1409784 - Remove mStringBundle from tabbrowser binding and use gNavigatorBundle instead

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

The less anonymous content we have inside of <tabbrowser>, the easier it'll be to migrate to a JS object. One child that looks removable is mStringBundle:  https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#79.

This is a <stringbundle> reference to https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/tabbrowser.properties.

We already have a global reference to browser.properties with gNavigatorBundle.  So we could potentially remove the stringbundle by merging the strings from tabbrowser.properties into browser.properties, and reference gNavigatorBundle instead of tabbrowser.mStringBundle

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1409784 - Remove mStringBundle from tabbrowser binding and expose gTabBrowserBundle instead;

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

Comment 2

[Brian Grinstead [:bgrins]]

I think this makes sense since gNavigatorBundle is already global and I don't see a particular benefit to keeping the strings in separate files, but let me know if you don't think it does. If do we land this, we'll have to make a note to localizers that the strings were just copied over.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

flod, does comment #2 sound good to you?

Comment 4

[Francesco Lodolo [:flod]]

> I think this makes sense since gNavigatorBundle is already global and I
> don't see a particular benefit to keeping the strings in separate files, but
> let me know if you don't think it does. If do we land this, we'll have to
> make a note to localizers that the strings were just copied over.

Unfortunately the note is not going to help: those strings will show up as brand new, require localization, and fall back to English until worked on.

The other concern is that we want to move away from monolithic huge files like browser.*. where everyone puts strings – from obscure error messages to very prominent UI – just because that bundle is already loaded. And this goes into the opposite direction.

I would really suggest to talk with pike and zibi, because the long term goal is to move away from the current localization infrastructure, and I'm not sure about the overlapping with migrating to a JS object.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Francesco Lodolo [:flod] from comment #4)
> > I think this makes sense since gNavigatorBundle is already global and I
> > don't see a particular benefit to keeping the strings in separate files, but
> > let me know if you don't think it does. If do we land this, we'll have to
> > make a note to localizers that the strings were just copied over.
> 
> Unfortunately the note is not going to help: those strings will show up as
> brand new, require localization, and fall back to English until worked on.
> 
> The other concern is that we want to move away from monolithic huge files
> like browser.*. where everyone puts strings – from obscure error messages to
> very prominent UI – just because that bundle is already loaded. And this
> goes into the opposite direction.

Another option that would accomplish the goal in Comment 0 without moving strings around could be to move the <stringbundle> for tabbrowser.properties out of the xbl content and as a sibling of gNavigatorBundle (i.e. gTabBrowserBundle).

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> (In reply to Francesco Lodolo [:flod] from comment #4)
> Another option that would accomplish the goal in Comment 0 without moving
> strings around could be to move the <stringbundle> for tabbrowser.properties
> out of the xbl content and as a sibling of gNavigatorBundle (i.e.
> gTabBrowserBundle).

Or maybe better: moving away from a <stringbundle> element entirely and changing the consumers to use the Services.strings.createBundle APIs directly (like gBrowserBundle but for tabbrowser.properties).

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8919815 [details]
Bug 1409784 - Remove mStringBundle from tabbrowser binding and expose gTabBrowserBundle instead;

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

Comment 8

[Brian Grinstead [:bgrins]]

Updated the patch to work as in Comment 6 so that no string changes would be needed

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8919815 [details]
Bug 1409784 - Remove mStringBundle from tabbrowser binding and expose gTabBrowserBundle instead;

https://reviewboard.mozilla.org/r/190744/#review196262

::: browser/base/content/tabbrowser.xml:5521
(Diff revision 2)
>  
>            var label;
>            if (tab.mOverCloseButton) {
>              label = tab.selected ?
>                      stringWithShortcut("tabs.closeSelectedTab.tooltip", "key_close") :
> -                    this.mStringBundle.getString("tabs.closeTab.tooltip");
> +                    gTabBrowserBundle.GetStringFromName("tabs.closeTab.tooltip");

formatStringFromName vs. GetStringFromName *sigh*
Would be nice to get this API cleaned up.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #9)
> ::: browser/base/content/tabbrowser.xml:5521
> (Diff revision 2)
> >  
> >            var label;
> >            if (tab.mOverCloseButton) {
> >              label = tab.selected ?
> >                      stringWithShortcut("tabs.closeSelectedTab.tooltip", "key_close") :
> > -                    this.mStringBundle.getString("tabs.closeTab.tooltip");
> > +                    gTabBrowserBundle.GetStringFromName("tabs.closeTab.tooltip");
> 
> formatStringFromName vs. GetStringFromName *sigh*
> Would be nice to get this API cleaned up.

Yeah, I agree. Zibi, in the future are we planning to continue using Services.strings.createBundle and the associated nsIStringBundle APIs? If so, we could make them easier to use by emulating the <stringbundle> methods (not requiring an extra `length` parameter in formatStringFromName / formatStringFromId and renaming the functions).

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

We do plan to move away from this, toward a declarative API. In the case quoted above, you'll be doing:

```
let l10nId;
if (...) {
  l10nId = 'one-id';
} else if (...) {
  l10nId = 'another-id';
} else {
  l10nId = 'yet-another-id';
}

document.l10n.setAttributes(event.target, l10nId);
```

which is the most common approach (localize the UI by annotating it for localization).

If you'll need to get a string out of the document's l10n context, you'll do:

```
let title = await document.l10n.formatValue('one-id');
```

and if you'll want to create a separate l10n context and get a string out of it:

```
let l10n = new Localization(['browser/main.ftl', 'browser/menu.ftl']);
let title = await l10n.formatValue('one-id');
```

I expect to expose this new API by All Hands.

Not sure if it means it's worth refactoring the old one for consistency or not.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #11)
> I expect to expose this new API by All Hands.
> 
> Not sure if it means it's worth refactoring the old one for consistency or
> not.

OK, thanks! Given that timeline, I don't think it's worth refactoring the old one.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74b50e659210
Remove mStringBundle from tabbrowser binding and expose gTabBrowserBundle instead;r=dao

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/74b50e659210