1280689 - Create about:localization

Status: RESOLVED FIXED

(L20n :: General, defect)

Comment 1

[Staś Małolepszy :stas]

The initial version of about:localization landed on the l20n branch in:

    https://github.com/stasm/gecko-dev/commit/0e5b338b85553649017778ff8dcfc5d848dbf105

It used the old experimental L10nService which is being phased out by the L10nRegistry (bug 1280671).

I updated about:localization on the branch to use the registry in:

    https://github.com/stasm/gecko-dev/commit/35fa08dd001143c378f9dd3ba44fa75592530adf

Comment 2

[Axel Hecht [:Pike]]

Mass change dependency tree for bug 1279002 into a whiteboard keyword.

Comment 3

[Staś Małolepszy :stas]

I'm working on landing l20n-chrome-html.js in bug 1303883 for Android and I'd like to have a Desktop counterpart.  I'd like to land about:localization on larch.

Comment 4

[Staś Małolepszy :stas]

Attachment: Bug 1280689 - Create about:localization.

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

Comment 5

[Staś Małolepszy :stas]

Because of how the L10nRegistry creates (resource, source) permutations and the order in which the sources are currently registered, about:localization first looks for /developer/aboutLocalization.ftl in the 'app' source, and only then in the 'platform' source.  This generates a lot of "Unknown entity" errors which we currently print to the console.

I think we should start talking about how we're going to optimize the empty/non-viable bundles out of the array returned by L10nRegistry.getResources.  I think there are three solutions:

 - use an index which is built on the buildtime,
 - stat files for existence before returning the array of lazy bundles,
 - change L10n.getResources to return a generator object which can stat files only when they're actually requested;  we could also remove the fetchResource method in this case:  the laziness is already guaranteed by the generator and each bundle it returns can already be prefetched.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 8799372 [details]
Bug 1280689 - Create about:localization.

https://reviewboard.mozilla.org/r/84564/#review83126

We talked about the packaging issues on irc, that needs to be folded in.

I'm also wondering if we can use an html instead of an xhtml document? Might be an interesting additional step for perf testing, though.

I'd also think that some of the <div> elements could be more semantic, like <header> ? Also, no idea how a11y works here.

Last but not least, can you make sure that this file is covered by our js linter and that the linter is happy with code formatting etc?

::: toolkit/content/aboutLocalization.js:16
(Diff revision 1)
> +const gInfoRequests = {
> +  sources: () => displaySources(L10nRegistry.requestSourceInfo()),
> +  cache: () => displayCache(L10nRegistry.requestCacheInfo()),
> +};
> +
> +function col(element, rowspan = 1) {

The naming of "element" is weird, maybe this is content? and content is something else?

::: toolkit/content/aboutLocalization.js:28
(Diff revision 1)
> +
> +function displaySources(data) {
> +  const cont = document.getElementById('sources-content');
> +  const parent = cont.parentNode;
> +  const new_cont = document.createElement('tbody');
> +  new_cont.setAttribute('id', 'sources-content');

Use cont.cloneNode(false) instead?

::: toolkit/content/aboutLocalization.js:51
(Diff revision 1)
> +
> +function displayCache(data) {
> +  const cont = document.getElementById('cache-content');
> +  const parent = cont.parentNode;
> +  const new_cont = document.createElement('dl');
> +  new_cont.setAttribute('id', 'cache-content');

... same here.

Comment 7

[Staś Małolepszy :stas]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53aeadfb2d51

Comment 8

[Staś Małolepszy :stas]

Comment on attachment 8799372 [details]
Bug 1280689 - Create about:localization.

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

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 8799372 [details]
Bug 1280689 - Create about:localization.

https://reviewboard.mozilla.org/r/84564/#review83152

Much nicer :-), just a nit about the explicit creation of text nodes, I think .textContent = "foo" is nicer.

Also, seems that my hopes for eslint aren't covering all of our styleguides, let's use explicit braces for single-line ifs.

You, I, and the style guide approves.

r+ with that on the actual about:localization patch.

I'm not sure we should land this patch without a fix to the console.log flooding, though.

::: toolkit/content/aboutLocalization.js:20
(Diff revision 2)
> +
> +function col(content, rowspan = 1) {
> +  const col = document.createElement('td');
> +  col.setAttribute('rowspan', rowspan);
> +  const text = document.createTextNode(content);
> +  col.appendChild(text);

I think this is easier written as

col.textContent = content;

Same below with the other text nodes.

Comment 10

[Staś Małolepszy :stas]

https://hg.mozilla.org/projects/larch/rev/090c736d36d255900ca32c37fe33dc9446c994da
Bug 1280689 - Create about:localization. r=Pike

Comment 11

[Staś Małolepszy :stas]

Thanks for the review, Pike!