903694 - Skip initial translation for pre-translated documents

Status: RESOLVED FIXED

(L20n :: HTML Bindings, defect)

Description

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

In case when the document is pre-translated (for example at build time) it would be good to recognize it (by html lang attribute) and skip the initialization past initial drawing.

This is still an intermediate step, since it'll push all JS l10n past firstPaint, but since we don't really have a reliable way to block painting on JS l10n anyway, I'd say it's a good logic.

Comment 1

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

Attachment: patch

I'd like to land it since it doesn't cost us, plays well with HTML spec and will be required by any BTO (including planned Gaia dropin proposal)

Comment 2

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

Attachment: patch v2

Actually, moving requires and context creation into bootstrap gives us even better perf win on BTO enabled builds.

Comment 3

[Staś Małolepszy :stas]

Comment on attachment 788471 [details] [diff] [review]
patch v2

Review of attachment 788471 [details] [diff] [review]:
-----------------------------------------------------------------

r- because I think this introduces a risk of a race condition.  I'll be happy to change it to r+ (with nits) if I'm wrong.

::: bindings/l20n/gaia.js
@@ +11,4 @@
>    var rtlLocales = ['ar', 'fa', 'he', 'ps', 'ur'];
>  
>    var documentLocalized = false;
> +  var documentPretranslated = document.documentElement.lang || null;

Shouldn't we check here as well what the value of mozSetting's language.current is?  What if the document had been pretranslated into English, but the user chose Polish as the system language in Gaia?

::: bindings/l20n/html.js
@@ +15,4 @@
>  
> +  if (documentPretranslated && document.readyState != 'complete') {
> +    documentLocalized = true;
> +    window.addEventListener('load', function() {

If you're checking readyState above, maybe it will be more consistent to listen to readystatechange event here?

Also, isn't 'load' too late?  We still want to initialize the context and localize dynamic nodes.

@@ +21,5 @@
> +    });
> +  } else {
> +    bootstrap();
> +  }
> +  bindPublicAPI();

If 'load' fires quickly and bootstrap is called via setTimeout , it will be queued for execution after the main thread finishes. bindPublicAPI will be called before in this case, and ctx might still be undefined then, potentially causing race condition when trying to call ctx.addEventListener et al.

@@ +64,5 @@
>  
>    function collectNodes() {
>      var nodes = getNodes(document);
>      localizeHandler = ctx.localize(nodes.ids, function localizeHandler(l10n) {
> +      if (nodes && documentPretranslated) {

AFAIR, you check for nodes to see if this is the first run or not.  Instead, could you maybe set documentPretranslated = false?  Would the following work?

if (documentPretranslated) {
  // this will happen only on the first run
  fireLocalizedEvent();
  documentPretranslated = false;
  return;
}

Comment 4

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

Thanks for the review Stas!

(In reply to Staś Małolepszy :stas from comment #3)
> Shouldn't we check here as well what the value of mozSetting's
> language.current is?  What if the document had been pretranslated into
> English, but the user chose Polish as the system language in Gaia?

Actually, we can just check for navigator.language, then if it doesn't match do langneg and identify if the highest available locale is the same as pretranslated language and then decide if we want to delay l10n or not.

> If you're checking readyState above, maybe it will be more consistent to
> listen to readystatechange event here?

Well no, because we don't know if it'll at all fire. One of the things we will do in prelocalization is change the <script> that loads L20n to defer in order to minimize its impact on first time load.
And then it's possible that this code will run after readyState is complete. That's why we can't rely on window.load event either.
 
> Also, isn't 'load' too late?  We still want to initialize the context and
> localize dynamic nodes.

> If 'load' fires quickly and bootstrap is called via setTimeout , it will be
> queued for execution after the main thread finishes. bindPublicAPI will be
> called before in this case, and ctx might still be undefined then,
> potentially causing race condition when trying to call ctx.addEventListener
> et al.


So, here's the question.

In the perfect scenario we want to fire bootstrap when the screen is painted. This will give us best performance.

The challenges are:

1) There may be JS code that will try to run before firstPaint that uses L20n. I would prefer that not to happen, but not sure how to prevent that. Hopefully "defer" scripts launch in order, but I'm not sure.

We can also do a getter on public API that created context the first time someones ask for it, but that's pretty expensive.

2) There may be dynamic nodes. I believe that we have to assume that their content will show post firstPaint.

Ultimately I'd love us to consider two stages - prefirstPaint and postfirstPaint for both HTML and JS l10n.
And let developers somehow let us know how to act. So far we can only do one, either do everything prefirstPaint or delay post firstPaint to get good performance.

At BTO we know if we have dynamic nodes so we can adjust the runtime information (by, for example setting "async" instead of "defer" and some meta info for bindings to alter the decisions to delay bootstrap post firstPaint or not.

But we don't have a way for a developer to say "this JS code should fire before firstPaint and block it, and this can wait".

I'd love to tackle this conversation not only in the context of this bug but also our API proposal for WebAPI.

Comment 5

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #4)

> So, here's the question.

Don't keep me in waiting;  what is the question? :)

> 1) There may be JS code that will try to run before firstPaint that uses
> L20n. I would prefer that not to happen, but not sure how to prevent that.
> Hopefully "defer" scripts launch in order, but I'm not sure.

That's the theory, however bug 688580 seems to be about this not being the case.

> 2) There may be dynamic nodes. I believe that we have to assume that their
> content will show post firstPaint.

I agree.  

I'm a little bit afraid of over-optimizing for performance here, but… would it be possible to have a ProxyContext class which only has the public methods of the Context which don't do anything except for 1) wait for the actual Context to be created, 2) forward the methods called on ProxyContext to the Context instance when it's created.

Creating ProxyContext instances would be cheap (no Resources, Locales, no get* logic, no retranslation logic), but would still allow people to add event listeners etc.

Comment 6

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

I'm making nice progress with ProxyContent.

I'm currently working on gaia-bto branch of l20n:
 - https://github.com/zbraniecki/l20n.js/tree/gaia-bto

and l20n-master-bto branch of gaia:
 - https://github.com/zbraniecki/gaia/tree/l20n-master-bto

In particular:
 - I'm using proxyctx, it works well, especially with js aggregation in Gaia
 - I load l20n using LazyLoader
 - I split bindings localize() wrapper into two groups - dynamic and static (identified at build time by gaia-build bindings)
 - thanks to that, when values change, I only retranslate dynamic group

The results are amazing - I'm gettig 15% startup win (892ms vs 760ms) and no flickering, good UI paradigms etc.

Comment 7

[Staś Małolepszy :stas]

Two notes from the triage call:

 - checking the value of the 'lang' attribute defined on <html> requires doing language negotiation in advance in order to know what to check it against;  we don't want to hard-code navigator.language,

 - checking for existence of the 'lang' attribute goes against the good practice of defining it when you want to keep your original content in the HTML.

Comment 8

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

We're doing this in v2.x and v3.x