Closed Bug 1409784 Opened 5 years ago Closed 5 years ago

Remove mStringBundle from tabbrowser binding and use gNavigatorBundle instead

Categories

(Firefox :: Tabbed Browser, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file)

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
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.
flod, does comment #2 sound good to you?
Flags: needinfo?(francesco.lodolo)
> 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.
Flags: needinfo?(francesco.lodolo)
(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).
(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).
Updated the patch to work as in Comment 6 so that no string changes would be needed
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.
Attachment #8919815 - Flags: review?(dao+bmo) → review+
Assignee: nobody → bgrinstead
Priority: -- → P1
(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).
Flags: needinfo?(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.
Flags: needinfo?(gandalf)
(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.
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74b50e659210
Remove mStringBundle from tabbrowser binding and expose gTabBrowserBundle instead;r=dao
https://hg.mozilla.org/mozilla-central/rev/74b50e659210
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.