Closed Bug 1280689 Opened 8 years ago Closed 8 years ago

Create about:localization

Categories

(L20n :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

(Whiteboard: [gecko-l20n])

User Story

Inspect the current state of the L10n Registry.

Attachments

(1 file)

No description provided.
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
Assignee: nobody → stas
Status: NEW → ASSIGNED
Mass change dependency tree for bug 1279002 into a whiteboard keyword.
No longer blocks: gecko-l20n
Whiteboard: [gecko-l20n]
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.
Depends on: 1280670
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 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.
Attachment #8799372 - Flags: review?(l10n) → review-
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.
Attachment #8799372 - Flags: review?(l10n) → review+
Thanks for the review, Pike!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: