1051850 - Localizing because of mutations should wait for the "ready" event

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect)

Description

[Julien Wajsberg [:julienw]]

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.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch

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.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: bug1051850.diff

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.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request, delay MO approach

Comment 6

[Staś Małolepszy :stas]

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.

Comment 7

[Staś Małolepszy :stas]

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.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

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!