Closed Bug 923461 Opened 11 years ago Closed 11 years ago

Clean up the start-up path (bootstrap, collectNodes)

Categories

(L20n :: HTML Bindings, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

We learned a few things about avoiding race conditions on the Gaia branch. I'd like to port these changes to master for 1.0 to make our startup story sound. Things like waiting for specific ready state events, exposing the public API early enough etc.
Attached patch Clean up html.js (obsolete) — Splinter Review
This patch ports the startup logic from the Gaia branch to master. It specifically doesn't do any optimizations for pre-translated documents. Let's continue working on this in bug 903694. During the clean-up, I also made one logic change: I now allow both the manifest link *and* <script type="application/l20n"> to be present and they all work together fine. (Before, if scripts were present, the manifest link was completely ignored). The translations in the script are added to all locales (as per bug 897862), which may be useful for things like a shared brandName. I can file a separate bug for this, if you prefer; sorry for mixing this in here, but I was just generally cleaning html.js up and only after the fact did I realize that this should be a separate change.
Attachment #813531 - Flags: feedback?(gandalf)
Can you attach performance impact on gaia/l20n-master?
Attachment #813531 - Attachment is obsolete: true
Attachment #813531 - Flags: feedback?(gandalf)
Attached patch WIP 2 (with changes to gaia.js) (obsolete) — Splinter Review
I wish I could; I broked my Keon today :( I'll try to un-break it tomorrow. Do you think you could do some perf testing as well? In the meantime, I tested in Nightly (DEBUG=1 make) and it seems to be working fine. I'm still seeing a problem on desktop with multilingual-dev.html: this.entries is null. Not sure where it comes from, but I'll debug tomorrow.
Attachment #813669 - Flags: feedback?(gandalf)
median on Keon, l20n-master: 1108 -> 1081.
Comment on attachment 813669 [details] [diff] [review] WIP 2 (with changes to gaia.js) Review of attachment 813669 [details] [diff] [review]: ----------------------------------------------------------------- ::: bindings/l20n/gaia.js @@ +1,4 @@ > define(function (require) { > 'use strict'; > > + var DEBUG = true; debug should be false by default? @@ +72,5 @@ > + } > + > + function bindPublicAPI(ctx) { > + ctx.addEventListener('error', logMessage.bind(null, 'error')); > + ctx.addEventListener('warning', logMessage.bind(null, 'warn')); can we instead check for DEBUG here and not bind it at all if it's false? @@ +112,2 @@ > var scripts = headNode.querySelectorAll('script[type="application/l20n"]'); > + var manifestLink = headNode.querySelector('link[rel="localization"]'); so, each query here costs. I'd like to measure the cost before we start using both. Until we do, I'd prefer to have priority list - if manifest exists, use it, if not, check for scripts. (so yeah, file a new bug for this change pls)
Attachment #813669 - Flags: feedback?(gandalf) → feedback+
(In reply to Zbigniew Braniecki [:gandalf] from comment #5) > debug should be false by default? Yes, I left it there while testing. > can we instead check for DEBUG here and not bind it at all if it's false? Ah, of course! I copied this verbatim from the gaia branch. Good point. > > @@ +112,2 @@ > > var scripts = headNode.querySelectorAll('script[type="application/l20n"]'); > > + var manifestLink = headNode.querySelector('link[rel="localization"]'); > > so, each query here costs. I'd like to measure the cost before we start > using both. Last I checked, the perf impact of qSA was none. I'll redo the tests when I fix my Keon. > Until we do, I'd prefer to have priority list - if manifest exists, use it, > if not, check for scripts. > > (so yeah, file a new bug for this change pls) Sure, bug 923670 it is. Removing this fixed the errors I was getting in multilingual-dev.html so I'm happy to tackle it in a separate bug.
Attachment #813669 - Attachment is obsolete: true
Attached patch Atomic patchSplinter Review
Here's a patch with the logic changed removed and review comments addressed.
Attachment #813762 - Flags: review?(gandalf)
Attachment #813762 - Flags: review?(gandalf) → review+
Status: NEW → RESOLVED
Closed: 11 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: