Closed Bug 1347800 Opened 3 years ago Closed 3 years ago

Create Localization API for Gecko

Categories

(Core :: Internationalization, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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

Sort of a StringBundle 2.0.
Depends on: 1347801
Depends on: 1333980
Priority: -- → P3
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).
Attachment #8847966 - Flags: feedback?(dtownsend)
Comment on attachment 8847966 [details]
Bug 1347800 - Add Localization API for Gecko.

Updated the patch with first set of tests
Attachment #8847966 - Flags: feedback?(dtownsend)
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
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.
Attachment #8847966 - Flags: feedback?(dtownsend)
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 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/
Attachment #8847966 - Flags: review?(dtownsend)
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 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?
Attachment #8847966 - Flags: review?(dtownsend)
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 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.
Attachment #8847966 - Flags: review?(dtownsend) → review+
Thanks! Added bug 1388458 and made it block bug 1365426.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/251857f1fb4b
Add Localization API for Gecko. r=mossop
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/963a623e68ce
Add Localization API for Gecko. r=mossop
https://hg.mozilla.org/mozilla-central/rev/963a623e68ce
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Flags: needinfo?(gandalf)
You need to log in before you can comment on or make changes to this bug.