1462839 - [meta] L10nRegistry v2

Status: NEW

(Core :: Internationalization, enhancement, P3)

Description

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

L10nRegistry successfully replaced ChromeRegistry in handling l10n resources and we now have the first version of Firefox (60) in release and zero known bugs or regressions, the code is well documented and has ~90% test coverage [0].

It's a great time to start looking into the second revision of the API.

In particular I'd like to target several high level concepts:

 - content/chrome process separation
 - separating extensions locales from main product locales
 - feasability to use rust parser for FTL resource parsing
 - telemetry
 - locale fallback chains
 - get code coverage above 90%

I'm going to use this bug as a meta for the work and will likely span the work across 62/63/64 releases.

[0] https://codecov.io/gh/marco-c/gecko-dev/src/master/intl/l10n/L10nRegistry.jsm

Comment 1

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

I think we'll need to address an update to this in context of bug 1455649. I started toying with this and I think I can get several improvements together:

 - switch caching to FTLResource based (bug 1384236 and https://github.com/projectfluent/fluent.js/issues/217)
 - introduce IPC via FTLResource (bug 1462841)
 - reorganize the generator to accommodate for missing files (bug 1464156)
 - add ability to prefetch resources to improve our ability to hook into DOM


The core idea is to replace the current model of generating all possible permutations per locale, to generate permutations per resource.

I'll try to get a workable patch now.

Pike - do you have any conceptual feedback on the proposed changes?

Comment 2

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

Attachment: Bug 1462839 - Switch L10nRegistry to cache FTLResource objects, prefetch resources and return incomplete contexts.

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

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8983615 [details]
Bug 1462839 - Switch L10nRegistry to cache FTLResource objects, prefetch resources and return incomplete contexts.

https://reviewboard.mozilla.org/r/249470/#review255642


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: intl/l10n/MessageContext.jsm:31
(Diff revision 1)
>  const identifierRe = /[a-zA-Z][a-zA-Z0-9_-]*/y;
>  const functionIdentifierRe = /^[A-Z][A-Z_?-]*$/;
>  
> +class FTLResource {
> +  constructor(source) {
> +    const [entries, errors] = parse(source);

Error: 'errors' is assigned a value but never used. [eslint: no-unused-vars]

::: intl/l10n/l10n.js:53
(Diff revision 1)
>  
>    document.l10n = new DOMLocalization(window, resourceIds);
>  
>    // Trigger the first two contexts to be loaded eagerly.
> -  document.l10n.ctxs.touchNext(2);
> +  const appLocales = Services.locale.getAppLocalesAsBCP47();
> +  return L10nRegistry.prefetch(appLocales, resourceIds, 2);

Error: Parsing error: 'return' outside of function [eslint]

Comment 4

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

The patch may, hopefully, shed a bit of light onto what kind of changes we're talking about. I don't know if it'll make sense to split it into smaller patches and keep this as a meta later, but for now, I think a single meta patch to visualize the concepts should work better since those changes work together.

In the patch I am:

1) turning *off* skipping contexts that have missing files. This is a request from Flod that I'm very torn about.
On one hand, I see the value of not rejecting a localization with 11/12 files present (Preferences in ab-CD with just one minor file missing), on the other I'm afraid of scaling this model to accepting a context with 1/12 files present.

But it seems that for now, the former is more realistic.

2) Switching caching from per-ctx to per-ftlresource

This means that parsing is done once-per resource, and initializing new MessageContext is very cheap.
This also lays a nice foundation for IPC (transferring FTLResource, potentially serialized, should be easier than MessageContext), and would make it easier to plug a Rust parser.

3) Adds a prefetch step, which replaces CachedIterator.touchNext.

This would fit much better into DOM model where we could trigger it whenever <link> is parsed, rather than having to collect all resources and only kick start prefetching by creating the new context.

I hope it'll help making sense out of the NI :)

p.s. of course the code review bot feedback is useless since the patch is only a POC to visualize the concepts (although it launches :))

Comment 5

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

Comment on attachment 8983615 [details]
Bug 1462839 - Switch L10nRegistry to cache FTLResource objects, prefetch resources and return incomplete contexts.

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

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8983615 [details]
Bug 1462839 - Switch L10nRegistry to cache FTLResource objects, prefetch resources and return incomplete contexts.

https://reviewboard.mozilla.org/r/249470/#review255658


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: intl/l10n/MessageContext.jsm:31
(Diff revision 2)
>  const identifierRe = /[a-zA-Z][a-zA-Z0-9_-]*/y;
>  const functionIdentifierRe = /^[A-Z][A-Z_?-]*$/;
>  
> +class FTLResource {
> +  constructor(source) {
> +    const [entries, errors] = parse(source);

Error: 'errors' is assigned a value but never used. [eslint: no-unused-vars]

Comment 7

[Axel Hecht [:Pike]]

The patch helps to see what you're hoping for.

To rephrase, you're switching the caching from source to parsed in the registry, and drop caching of contexts?

I don't have a strong opinion on that, mostly because I have no instinct on IPC impact of either solution. Also, I think this is mostly an implementation aspect.

I am a bit concerned about the intl cache, and that whether that keeps a strong reference to a locale list. Related question, do we truncate the list of languages we pass to the intl formatters as we fall back? I.e., with ['fy-NL', 'nl', 'en-US'], do we use a dutch number formatter when the context has dutch resources? 

Stas, I know that you've been concerned about Objects close to the AST. Is giving this an explicit name helping? Also, now that we have a use-case for dynamically changing contexts, does that change the balance between pros and cons of storing parsed objects and passing them around?

Comment 8

[Staś Małolepszy :stas]

(In reply to Axel Hecht (Back on June 28) [:Pike] from comment #7)

> Stas, I know that you've been concerned about Objects close to the AST. Is
> giving this an explicit name helping? Also, now that we have a use-case for
> dynamically changing contexts, does that change the balance between pros and
> cons of storing parsed objects and passing them around?

Yes, I think it helps. I'm also happy that we took some time to discuss the scope and the motivations behind this proposal in https://github.com/projectfluent/fluent.js/issues/217. Furthermore, the changes planned in https://github.com/projectfluent/fluent.js/issues/220 (Make the runtime parser output real Fluent AST) also align well with this proposal.

Comment 9

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

With rkv XPCOM FFI we will soon be able to start learning how many fallbacks were needed on the previous run to minimize FOUCs on longer fallback chains.

Comment 10

[Worcester12345]

Not sure where this should go, but I am getting this in my Error Console:
[Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIXPCComponents_Utils.readUTF8URI]" nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)" location: "JS frame :: resource://gre/modules/L10nRegistry.jsm :: L10nRegistry.loadSync :: line 658" data: no] L10nRegistry.jsm:658:19
loadSync resource://gre/modules/L10nRegistry.jsm:658
fetchFile resource://gre/modules/L10nRegistry.jsm:573
generateResourceSetSync resource://gre/modules/L10nRegistry.jsm:478
map self-hosted:240
generateResourceSetSync resource://gre/modules/L10nRegistry.jsm:473
generateResourceSetsForLocaleSync resource://gre/modules/L10nRegistry.jsm:415
InterpretGeneratorResume self-hosted:1151
next self-hosted:1099
generateBundlesSync resource://gre/modules/L10nRegistry.jsm:177
InterpretGeneratorResume self-hosted:1151
next self-hosted:1099
touchNext resource://gre/modules/Localization.jsm:167
regenerateBundles resource://gre/modules/Localization.jsm:552
activate resource://gre/modules/Localization.jsm:243
<anonymous> chrome://messenger/content/mailWidgets.js:37
<anonymous> chrome://messenger/content/customElements.js:34
<anonymous> chrome://messenger/content/customElements.js:37
observe resource://gre/modules/MailGlue.jsm:201

78.10.2 (64-bit)