Closed
Bug 1051850
Opened 10 years ago
Closed 10 years ago
Localizing because of mutations should wait for the "ready" event
Categories
(Firefox OS Graveyard :: Gaia::L10n, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: julienw, Assigned: zbraniecki)
References
Details
Attachments
(4 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-github-pull-request
|
Details | |
(deleted),
patch
|
stas
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/x-github-pull-request
|
Details |
While optimizing the SMS startup sequence, I noticed that sometimes we get a L10nError just because l10n is not ready when we insert an element with an attribute data-l10n-id. Here is the stack: L10nError: L10nError@app://sms.gaiamobile.org/shared/js/l10n.js:11:17 getWithFallback@app://sms.gaiamobile.org/shared/js/l10n.js:1136:1 getEntity@app://sms.gaiamobile.org/shared/js/l10n.js:1172:19|translateElement@app://sms.gaiamobile.org/shared/js/l10n.js:1687:18 translateFragment@app://sms.gaiamobile.org/shared/js/l10n.js:1634:7 localizeMutations@app://sms.gaiamobile.org/shared/js/l10n.js:1495:13 onMutations@app://sms.gaiamobile.org/shared/js/l10n.js:1510:5 (I added a log to L10nError constructor to see the stack at that moment). We clearly see that there is no client code involved at all.
Reporter | ||
Updated•10 years ago
|
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee | ||
Comment 1•10 years ago
|
||
The only idea that comes to my mind is that it's because here: https://github.com/mozilla-b2g/gaia/blob/2beedaf2b9557dc951be5ed1aa430168f70b018e/shared/js/l10n.js#L1671 we test for isPretranslated. So in no-pretranslated case we attempt to getEntity with ctx.isReady is false. The code has been introduced very recently in bug 1030334. Stas, do you think we can just remove the isPretranslated test on translateElement pending case?
Flags: needinfo?(stas)
Assignee | ||
Comment 2•10 years ago
|
||
Ah, no. We should just have two cases here, I think. If we're in isPretranslated case, we should collect pending elements and translate them onReady. If we're in !isPretranslated case, we don't need to collect the nodes because we will translateDocument in onReady anyway. In both cases, we should exit translateElement early.
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
alternative approach: - delay MO initialization in non-pretranslated mode until document has been translated. This covers the same conditions, but instead of having MO initialized early and return on each translateElement, we just delay initializing MO and translate the document with the injected nodes.
Attachment #8478414 -
Flags: review?(stas)
Assignee | ||
Comment 5•10 years ago
|
||
Comment 6•10 years ago
|
||
Comment on attachment 8471055 [details] [diff] [review] patch Review of attachment 8471055 [details] [diff] [review]: ----------------------------------------------------------------- Canceling the review request on this b/c I prefer the approach from the other patch.
Attachment #8471055 -
Flags: review?(stas)
Updated•10 years ago
|
Flags: needinfo?(stas)
Comment 7•10 years ago
|
||
Comment on attachment 8478414 [details] [diff] [review] bug1051850.diff Review of attachment 8478414 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments below, thanks! ::: bindings/l20n/runtime.js @@ +124,4 @@ > waitFor('interactive', init.bind(navigator.mozL10n, pretranslate)); > } > > +function initMO() { Rename to initObserver? @@ +126,5 @@ > > +function initMO() { > + if (nodeObserver) { > + return; > + } Probably not needed if you check for nodeObserver in onReady (see the last comment of my review). @@ +139,5 @@ > } else { > + // if we are in pretranslated mode, we want to initialize MO > + // early, to collect nodes injected between now and when resources > + // are loaded. > + initMO(); The comment here might need some rephrasing. We end up in the else block if pretranslate is false, which can happen when the document is pretranslated or when data-no-complete-bug is set. @@ +264,4 @@ > pendingElements = null; > } > > + if (!isPretranslated) { I think it will be safer to check for the existence of nodeObserver. See my comment above about data-no-complete-bug.
Attachment #8478414 -
Flags: review?(stas) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Commit: https://github.com/mozilla-b2g/gaia/commit/9f55b98098b7d28ae2062ea41fc5e5c3ebd24c9b Merge: https://github.com/mozilla-b2g/gaia/commit/5f85fa9ee7a07e2d29ea7c0fb7b47c9901a24f99 L20n: https://github.com/l20n/l20n.js/commit/3f7e68a97df760ae5d76c6232c044e39c076c587 Thanks Stas!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•