Closed
Bug 923461
Opened 11 years ago
Closed 11 years ago
Clean up the start-up path (bootstrap, collectNodes)
Categories
(L20n :: HTML Bindings, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: stas, Unassigned)
References
Details
Attachments
(1 file, 2 obsolete files)
15.90 KB,
patch
|
zbraniecki
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
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)
Comment 2•11 years ago
|
||
Can you attach performance impact on gaia/l20n-master?
Reporter | ||
Updated•11 years ago
|
Attachment #813531 -
Attachment is obsolete: true
Attachment #813531 -
Flags: feedback?(gandalf)
Reporter | ||
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
median on Keon, l20n-master: 1108 -> 1081.
Comment 5•11 years ago
|
||
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+
Reporter | ||
Comment 6•11 years ago
|
||
(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.
Reporter | ||
Updated•11 years ago
|
Attachment #813669 -
Attachment is obsolete: true
Reporter | ||
Comment 7•11 years ago
|
||
Here's a patch with the logic changed removed and review comments addressed.
Attachment #813762 -
Flags: review?(gandalf)
Updated•11 years ago
|
Attachment #813762 -
Flags: review?(gandalf) → review+
Reporter | ||
Comment 8•11 years ago
|
||
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.
Description
•