Create a small per-document bindings for Localization API

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

Most of the new localization logic will live in a JS module, but the minimal live bindings will be per-document.
(Assignee)

Updated

2 years ago
Blocks: 1347797
(Assignee)

Updated

2 years ago
Depends on: 1347799
(Assignee)

Updated

2 years ago
Blocks: 1347825
(Assignee)

Updated

2 years ago
No longer blocks: 1347797

Comment 2

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

Updated

2 years ago
Blocks: 1363864
(Assignee)

Updated

2 years ago
No longer blocks: 1363864
Depends on: 1363864
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Depends on: 1394977
(Assignee)

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

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

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

Comment 11

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

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

Comment 13

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

Updated

2 years ago
Blocks: 1402061
(Assignee)

Updated

2 years ago
No longer blocks: 1347825
(Assignee)

Updated

2 years ago
No longer depends on: 1363864

Comment 14

2 years ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1ae02c8d697
Create a small per-document bindings for DOMLocalization. r=mossop
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
Ugh, thank you and apologies for the hassle!
Flags: needinfo?(gandalf)

Comment 18

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

Comment 21

2 years ago
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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Updated

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