Create Localization API for Gecko

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

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

Sort of a StringBundle 2.0.
(Assignee)

Updated

2 years ago
Blocks: 1347799
(Assignee)

Updated

2 years ago
Depends on: 1347801
(Assignee)

Updated

2 years ago
Depends on: 1333980
Comment hidden (mozreview-request)
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
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 hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
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)

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Blocks: 1365426

Comment 6

2 years ago
mozreview-review
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
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

2 years ago
mozreview-review
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
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

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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+
(Assignee)

Updated

2 years ago
Blocks: 1388458
(Assignee)

Comment 18

2 years ago
Thanks! Added bug 1388458 and made it block bug 1365426.
Comment hidden (mozreview-request)

Comment 20

2 years ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/251857f1fb4b
Add Localization API for Gecko. r=mossop
I had to back this out because I needed to back out bug 1333980

https://hg.mozilla.org/integration/autoland/rev/eb6df0971ede
Flags: needinfo?(gandalf)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/963a623e68ce
Add Localization API for Gecko. r=mossop

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/963a623e68ce
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Updated

2 years ago
Flags: needinfo?(gandalf)
You need to log in before you can comment on or make changes to this bug.