1347800 - Create Localization API for Gecko

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

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

We need a layer on top of Fluent that will bind Fluent, L10nRegistry and other Gecko APIs together.

Sort of a StringBundle 2.0.

Comment 1

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

Attachment: Bug 1347800 - Add Localization API for Gecko.

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

Comment 2

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

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

Comment 3

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

And this is also ready for feedback round.

Together with L10nRegistry and MessageContext, those three will allow us to replace the StringBundle, localize any raw JS/JSM resources and pave the way to DOMLocalization (bug 1347799).

Comment 4

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

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

Comment 5

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

Updated the patch with first set of tests

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

https://reviewboard.mozilla.org/r/120908/#review144686

It's difficult to review this without additional explanations on the API so I'll wait for that first.

::: intl/l10n/Localization.jsm:1
(Diff revision 3)
> +const Cu = Components.utils;

This whole file needs more comments to explain the API better.

::: intl/l10n/test/test_localization.js:1
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.

Please use add_task and async functions in this test.

Comment 7

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

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

Comment 8

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120908/diff/4-5/

Comment 9

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120908/diff/5-6/

Comment 10

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

I think this is ready for review.

This is the first high-level API that combines L10nRegistry, MessageContext and LocaleService.

It is intended to replace some uses of StringBundle that have to remain in JS, although majority of l10n should happen via the subclass of this - DOMLocalization and declarative API.

The division between 5 pieces stays as follows:

MessageContext.jsm - low level API shared between vanilla JS APIs, react APIs etc. (may move to Rust in the future using fluent-rs)
L10nRegistry.jsm - low level I/O for localization
Localization.jsm - high level API for JS contexts
DOMLocalization.jsm - high level API for HTML/XUL (may move to DOM as WebL10n API?)  (bug 1347799)
l10n.js - polyfill .js file to be included into HTML/XUL to get DOMLocalization going. (bug 1347798)

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

https://reviewboard.mozilla.org/r/120908/#review168506

::: intl/l10n/Localization.jsm:27
(Diff revision 6)
> +const { L10nRegistry } = Cu.import("resource://gre/modules/L10nRegistry.jsm", {});
> +const LocaleService = Cc["@mozilla.org/intl/localeservice;1"].getService(Ci.mozILocaleService);

These are never used

::: intl/l10n/Localization.jsm:43
(Diff revision 6)
> + * do not prevent the message from being used.
> + *
> + * An example of an L10nError is a missing entry.
> + */
> +class L10nError extends Error {
> +  constructor(message, id, lang) {

The arguments id and lang are never used. What are they for?

::: intl/l10n/Localization.jsm:60
(Diff revision 6)
> + * It combines language negotiation, MessageContext and I/O to
> + * provide a scriptable API to format translations.
> + */
> +class Localization {
> +  /**
> +   * @returns {Localization}

What are the parameters here?

::: intl/l10n/Localization.jsm:125
(Diff revision 6)
> +
> +  /**
> +   * Retrieve translations corresponding to the passed keys.
> +   *
> +   * A generalized version of `DOMLocalization.formatValue`. Keys can
> +   * either be simple string identifiers or `[id, args]` arrays.

It doesn't look like the case for simple string identifiers is handled or tested.

::: intl/l10n/Localization.jsm:132
(Diff revision 6)
> +   *     docL10n.formatValues([
> +   *       ['hello', { who: 'Mary' }],
> +   *       ['hello', { who: 'John' }]
> +   *     ]).then(console.log);
> +   *
> +   *     // ['Hello, Mary!', 'Hello, John!', 'Welcome!']

Where does the third result come from?

::: intl/l10n/Localization.jsm:307
(Diff revision 6)
> + *
> + * The idea here is that if the previous `MessageContext` did not resolve
> + * all keys, we're calling this function with the next context to resolve
> + * the remaining ones.
> + *
> + * In the function, we loop oer `keys` and check if we have the `prev`

s/oer/over/

Comment 12

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120908/diff/6-7/

Comment 13

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

Thanks Dave!

I updated the patch and in the process, included a functional change.

We're drafting a vision for allowing us to use differnet localization strategies depending on some division of modules/packages. You can read more about it in bug 1365433 and bug 1280690, but the gist is: We see a scenario where the user has Firefox in de, but devtools in en-US.

This is going to be something we'll work on later, but for now, the very base of it is the function that is passed to the constructor of the Localization class - `generateMessages`.

I moved in from the later patch (bug 1347798) to this one the default strategy that uses L10nRegistry and LocaleService.

In result, users can now create new Localization objects without having to define their own strategy. I'm still allowing users to override it, which may be used by the special-case modules (like devtools) and is useful for tests.

The result API looks more like this:

```
let l10n = new Localization(['browser/menu.ftl', 'toolkit/brand.ftl']);

let msg = await l10n.formatValue('file-open');
console.log(msg);
```

As we discussed before, the sync version of the call can be added with a couple methods, and the async will be used by the declarative API which will be next in line for the review (bug 1347799).

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

https://reviewboard.mozilla.org/r/120908/#review171324

::: intl/l10n/Localization.jsm:50
(Diff revisions 6 - 7)
> + * A wrapper for an Iterable that allows to reply the iteration many
> + * times without regenerating the Iterable.

This sentence needs rephrasing to make sense. Do you mean replay rather then reply?

::: intl/l10n/Localization.jsm:53
(Diff revisions 6 - 7)
> + * This is used by the Localization class to wrap the MessageContext generator
> + * and reply the loop on each call to format messages.

Same here

::: intl/l10n/Localization.jsm:117
(Diff revisions 6 - 7)
>     * @returns {Localization}
>     */
> -  constructor(resIds, generateMessages) {
> -    this.resIds = resIds;
> +  constructor(resourceIds, generateMessages = defaultGenerateMessages) {
> +    this.resourceIds = resourceIds;
>      this.generateMessages = generateMessages;
> -    this.ctxs = this.generateMessages(this.resIds);
> +    this.ctxs = this.generateMessages(this.resourceIds);

Why wouldn't you always wrap the result in cachedIterable rather than relying on generateMessages to do it?

Comment 15

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120908/diff/7-8/

Comment 16

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

https://reviewboard.mozilla.org/r/120908/#review171324

> Why wouldn't you always wrap the result in cachedIterable rather than relying on generateMessages to do it?

that makes sense! Changed :)

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

https://reviewboard.mozilla.org/r/120908/#review171338

Fine as a start. Please file a bug to add more tests though.

Comment 18

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

Thanks! Added bug 1388458 and made it block bug 1365426.

Comment 19

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120908/diff/8-9/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/251857f1fb4b
Add Localization API for Gecko. r=mossop

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because I needed to back out bug 1333980

https://hg.mozilla.org/integration/autoland/rev/eb6df0971ede

Comment 22

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120908/diff/9-10/

Comment 23

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

Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120908/diff/10-11/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/963a623e68ce
Add Localization API for Gecko. r=mossop

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/963a623e68ce