Closed Bug 1347798 Opened 3 years ago Closed 3 years ago

Create a small per-document bindings for Localization API

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Most of the new localization logic will live in a JS module, but the minimal live bindings will be per-document.
Depends on: 1347799
No longer blocks: 1347797
Comment on attachment 8847968 [details]
Bug 1347798 - Create a small per-document bindings for DOMLocalization.

https://reviewboard.mozilla.org/r/120916/#review122932

::: intl/l10n/l20n.js:18
(Diff revision 1)
> +    )
> +  );
> +}
> +
> +function getResourceLinks(elem) {
> +  return Array.prototype.map.call(

I like Array.from with a map function, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from.
No longer blocks: 1363864
Depends on: 1363864
Depends on: 1394977
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
This is the final of the six main patches for the new L10n API! :)

The role of this file is to be loaded into every document that is to be localized using DOMLocalization. This code will polyfill the future WebL10n API by:

 - collecting links with rel="localization"
 - creating the initial DOMLocalization attached on `document.l10n`
 - performing the initial localization of the DOM
 - registering observers for the initial DOMLocalization

The one thing it does on top of the list is performing the initial registration of L10nRegistry sources. I don't know where this code should live. In a perfect world I'd imagine that each of the two - browser and toolkit would at some point register its own FileSource and the toolkit's one would be registered first (so that the browser's is newer).

Any other Gecko app like Fennec, Thunderbird, Seamonkey etc. would then register the "toolkit" FileSource and potentially other FileSources per component.

I'm open to suggestions as to where this code should live.

Also, currently, it just takes the locale from AppConstants, but once we land bug 1362617 and start being able to produce multilocale builds, we'll want to switch to this code fetching via async I/O multilocale.json.

Bug 1394977 also would allow me to move that into L10nRegistry because then L10nRegistry.generateContexts would be an async function that could wait for L10nRegistry to be ready.
Comment on attachment 8847968 [details]
Bug 1347798 - Create a small per-document bindings for DOMLocalization.

https://reviewboard.mozilla.org/r/120916/#review183454

::: intl/l10n/l10n.js:42
(Diff revision 6)
> +    );
> +  }
> +
> +  const resIds = getResourceLinks(document.head || document);
> +
> +  document.l10n = new DOMLocalization(MutationObserver, resIds);

This needs updating for the chnges we made to DOMLocalization.jsm

::: intl/l10n/l10n.js:44
(Diff revision 6)
> +  // This is a one-time bootstrapping code for L10nRegistry.
> +  if (L10nRegistry.ready === undefined) {
> +    const {AppConstants} =
> +      Components.utils.import("resource://gre/modules/AppConstants.jsm", {});
> +
> +    L10nRegistry.ready = async function() {
> +      let locales = [AppConstants.INSTALL_LOCALE];
> +
> +      const toolkitSource = new FileSource('toolkit', locales, 'resource://gre/localization/{locale}/');
> +      L10nRegistry.registerSource(toolkitSource);
> +      const appSource = new FileSource('app', locales, 'resource://app/localization/{locale}/');
> +      L10nRegistry.registerSource(appSource);
> +    }();
> +  }

nsBrowserGlue.js is where I'd put most or all of this.
Attachment #8847968 - Flags: review?(dtownsend)
> nsBrowserGlue.js is where I'd put most or all of this.

Excellent! Thank you!

I can see us in the future making it more declarative and store some JSON with build time info about which sources we have in the package, because the `toolkit` one should be initialized by toolkit, not browser, but for now I think it's a good start.
Comment on attachment 8847968 [details]
Bug 1347798 - Create a small per-document bindings for DOMLocalization.

https://reviewboard.mozilla.org/r/120916/#review185110

Still not convinced about touchNext but I'm not sure the alternative is any better so lets go with it.
Attachment #8847968 - Flags: review?(dtownsend) → review+
Thanks Dave!

I'm going to wait with this patch until 58. It adds a piece of code on the startup path (in nsBrowserGlue) that we're not using in 57, so I think it makes sense to wait :)
No longer blocks: 1347825
No longer depends on: 1363864
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1ae02c8d697
Create a small per-document bindings for DOMLocalization. r=mossop
Ugh, thank you and apologies for the hassle!
Flags: needinfo?(gandalf)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e46cfa3b45ff
Create a small per-document bindings for DOMLocalization. r=mossop
Backed out because it will fail browser-chrome's browser_all_files_referenced.js:

https://hg.mozilla.org/integration/autoland/rev/45a8457d012e1a51531981e88adae9b1cdc7cba1

Previous push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f1ae02c8d697c6ca0c1a24cdadcca304b8a30638&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132549644&repo=autoland

> TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: chrome://global/content/l10n.js -
> TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unused whitelist entry: resource://gre/modules/DOMLocalization.jsm -
Flags: needinfo?(gandalf)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/587c58121d45
Create a small per-document bindings for DOMLocalization. r=mossop
https://hg.mozilla.org/mozilla-central/rev/587c58121d45
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: needinfo?(gandalf)
You need to log in before you can comment on or make changes to this bug.