Closed Bug 992473 Opened 10 years ago Closed 10 years ago

Switch to use MutationObservers for DOM localization

Categories

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

defect

Tracking

(b2g-v2.0 wontfix, b2g-v2.1 fixed)

RESOLVED FIXED
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.1 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

()

Details

Attachments

(2 files, 7 obsolete files)

We currently use a manual DOM traversing to localize DOM at runtime and developers have to refer to specific API to translate new nodes that they inject into DOM.

We should aim to switch to something like this https://github.com/zbraniecki/l20n.js/blob/prototype/bindings/l20n/html.js

One obstacle may be performance, but let's create POC and measure.
Depends on: 914414
Assignee: nobody → gandalf
Attached patch WIP (obsolete) — Splinter Review
initial patch. I ported L20nMutationObserver from prototype branch and just enabled it in runtime bindings. It seems to work really well! Very promising :)
I was able to successfully remove the use of mozL10n.translate to translate panels injected in Settings.

There's no performance impact for pretranslation case.

Next, I want to test performance impact without pretranslation.
Blocks: 999779
Andrew, can you help me CC the right people to this bug, who can help me evaluate if there's any cost associated with the use of MutationObserver for l10n?
Flags: needinfo?(bugmail)
Vivien: same question. Any people that we should work with on this bug to make sure that we're keeping the perf/mem characteristic in shape?
Flags: needinfo?(21)
(In reply to Zibi Braniecki [:gandalf] from comment #4)
> Vivien: same question. Any people that we should work with on this bug to
> make sure that we're keeping the perf/mem characteristic in shape?

IIRC smaug has implemented Mutation Observers. You may want to explain it a bit more precisely what you are trying to do exactly as he has no idea of how l10n.js works AFAIK.
Flags: needinfo?(21)
Olli: we're currently support HTML runtime localization using data-l10n-id attribute on nodes.

We need to be able to react to:
 - nodes with data-l10n-id being inserted into DOM
 - nodes with data-l10n-id modifying this attribute

One other optional idea is to enable MutationObserver before DOM is interactive which means that nodes that are written in HTML will be reported into MO.

If we don't do that, then I need to querySelectorAll('data-l10n-id') before firstPaint to make sure that all nodes initially present in the document are translated.

You can see my WIP to create L20nMutationObserver here: https://github.com/zbraniecki/l20n.js/blob/gaia-mutationObserver/bindings/l20n/mutation_observer.js

and the way I use it here: https://github.com/zbraniecki/l20n.js/blob/gaia-mutationObserver/bindings/l20n/runtime.js#L213-L240

My questions are:

1) Do you see any obvious serious overhead of using MO for Gaia's performance/memory characteristic that would make this a no-go?

2) Do you think that it's better to start MO early, or use querySelectorAll for nodes from HTML? I'm trying to mitigate the risk of firstPaint happening before the nodes from HTML are translated, and sicking suggested that MO should give me that. [1]


[1] https://groups.google.com/d/msg/mozilla.dev.platform/elGa2NnRKa4/7NOUP5ljcqoJ
Flags: needinfo?(bugs)
CCing Ben Kelly. Any idea on how to measure the overhead of the MutationObserver or who would on the DOM team? Thanks!
Flags: needinfo?(bkelly)
I would be the right person to ask about MutationObserver API and performance ;)

What does "before DOM is interactive" mean?

(1) MutationObserver shouldn't affect to memory usage too much. Performance might get some hit.
    It really depends on what kind of DOM mutations the app does.
    But I'll profile attribute changes some more tomorrow.

(2) MutationObserver doesn't guarantee anything related to painting.
You need to use some script to add the mutation observer, and it is in theory possible
to get the app painted before any script has run... But sure, if mutation observer is created
in a <script> inside <head>, then it is there before any real visible content is painted.
But does MutationObserver affect too much to parsing during page load - hard to say without doing some testing
and profiling

btw, I hope I can land bug 902618 soonish. It makes certain things a bit slower, but the API becomes
consistent, and without it you effectively have to use querySelectorAll-like approach while the page
is still being parsed.
> (2) MutationObserver doesn't guarantee anything related to painting.

Doesn't MutationObservers guarantee that the observer gets a chance to react before painting happens? With the only exception to that rule being various sync APIs (which we're trying to get rid of, and likely will be able to get rid of once e10s is turned on)
As I said, you need to have MutationObserver in place early enough. So the page might get painted
before MutationObserver callback is called. But usually one creates MutationObserver inside
<head> and the visual content is inside <body>, so in that case mutation observer callback gets
called before any real visual content is painted.
And for dynamic changes, yes, mutation observer callback is called before painting.
Flags: needinfo?(bugs)
Thanks Olli!

Yes, the idea is to get MutationObserver initialized before we during document.readyState 'loading', which will give us all the nodes passed through the MO.

Right now we're doing this with querySelectorAll, and my initial tests show little to no performance hit from the switch, but I need to test more.

Good to hear that memory should not be a concern.

I'll report back when I have more code to show and play with, but I still would appreciate help with designing tests for performance impact.
Flags: needinfo?(bkelly)
(In reply to Zibi Braniecki [:gandalf] from comment #11)
> I'll report back when I have more code to show and play with, but I still
> would appreciate help with designing tests for performance impact.
Good tests would be realistic tests. Something which behaves like a complicated, yet realistic b2g app. 
It would be good to profile also simple apps.

Artificially complicated tests are probably less useful.
Blocks: 1006359
Blocks: 994519
Priority: -- → P2
Blocks: 993193
Flags: needinfo?(bugmail)
Attached file pull request
I got it working on Tarako without FOUC's, which sounds like a good step toward landing it.

We currently have 52 places where we do mozL10n.translate. I flashed gaia with this change and removed some of them and it seems to work great.

I'm not sure how we want to land it. I see two strategies:

a) We may want to remove all uses of translate and land it as one big patch

or

b) land this as is and remove uses of mozL10n.translate in the followups.

Stas: can you take a look at this code and tell me how it looks to you?

I'll be doing perf/memory tests tomorrow.
Attachment #8404324 - Attachment is obsolete: true
Attachment #8421053 - Flags: review?(stas)
wrt. bootstrapping the code works like that:

============

1) Check if the document is pretranslated (build-time translation)

1.1) If 'no' check if the document is in readyState 'loading'

1.1.1) If 'no' iterate over DOM to find localizable nodes and translate them

2) Initialize MutationObserver

============

In result, if the code is fired before 'interactive', we only initialize MO and the document reports the nodes.
If the code is fired after 'interactive', we "manually" localize DOM nodes.

Olli: can you look at my patch and verify that:

a) the MO config is optimal for translating nodes with data-l10n-id 
b) fireCallback and onNodeModified functions are well written to react to MO callbacks
c) the code does work as designed for DOMFragment injections (where a node with data-l10n-id may be injected with a child node with another data-l10n-id). I'm not sure if I understand how MO reports that to its callback and I'm worried that I may be redundantly localizing it, first when the parent is reported (via translateFragment) and then when the child is reported.
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)
oh, and:

d) that I do the right thing by disabling MO in fireCallback so that injected node's localization does not trigger redundant MO call. (I'm not sure if MO has some inner system to prevent from cyclic firing in that case or maybe the node I'm translating is not injected yet so I don't need such check?)
Attached patch bug992473.diff (obsolete) — Splinter Review
Attachment #8421107 - Flags: review?(stas)
addedNode.nodeType === 1 looks odd.
addedNode.nodeType === Node.ELEMENT_NODE perhaps

When a fragment is added, its children are removed to be children of a new parent.
So all the child nodes are in the addedNodes list. The patch seems to deal with that.

and d) all the changes made to the dom subtree you observe are notified. If you modify that dom subtree
during mo callback, your callback will be called later. But since you disconnect() and observe()
again, that should be fine.


I would land the patch first, and then convert gaia stuff to use it, and do some perf testing with each
less-than-trivial app.
Flags: needinfo?(bugs)
How do we handle localization these days in gaia? What does the API look like, and how is it implemented?
Comment on attachment 8421107 [details] [diff] [review]
bug992473.diff

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

This looks very promising.  I'm going to change your review request to a feedback request.  The patch needs some more work before it can get an r+. For now, it's an f=me :)

Before we move forward with the landing, I'd like us to give some thought to the following questions:

- testing strategy - how do we test this?  currently, the l20n.js repo is not capable of running tests which involve DOM manipulation;  should we land tests in Gaia only?  Either way, we need tests in the patch.

- transition strategy - you hinted at this question already, but what should we do with all the current uses of mozL10n.translate() and mozL10n.localize in Gaia?  A potential solution is to no-op mozL10n.translate (the element will be localized when inserted anyways), and change mozL10n.localize so that it doesn't call translateElement.  (In the longer run, mozL10n.localize should be deprecated in favor of node.setAttribute('data-l10n-id').)

- cross-browser compatibility strategy - we should keep l10n.js relevant for cross-browser webapps, not only Firefox OS.  According to http://caniuse.com/#feat=mutationobserver, IE 10 (desktop and mobile) don't support MO yet, but there's a polyfill available: https://github.com/Polymer/MutationObservers.  I see two options for us:  recommend the polyfill if you target IE, or provide a special transitional API (mozL10n.localizeNode?) which developers targeting IE can use.  I'd lean toward the former;  what are your thoughts on this?

Generally the code looks good.  My biggest doubt is about the necessity of creating an abstraction in the form of L20nMutationObserver.  It seems like the only thing that you want to be able to keep a state of is the _rootNode, so that you can disconnect and reconnect the observer inside the callback.  Do you expect _rootNode to be something else than document in the near future?  Do you think we'll need more than one observer per document?

Depending on the memory footprint of the patch it might be worth experimenting with a simpler implementation which doesn't use the L20nMutationObserver abstraction.  Something like this:

var nodeObserver = new MutationObserver(function(mutations, self) {
  self.disconnect();
  var nodes = mutationsToNodes(mutations);  // this is what fireCallback currently does
  onNodeModified(nodes);                    // this iterates again;  maybe do everything in a single loop?
  self.observe(document, config);
});
nodeObserver.observe(document, config);

::: bindings/l20n/mutation_observer.js
@@ +4,5 @@
> +  this._callback = cb;
> +  this._rootNode = null;
> +  this._observer = null;
> +  this._l10nIdName = 'data-l10n-id';
> +  this._config = { attributes: true,

Nit: linebreak after {.

@@ +12,5 @@
> +    attributeFilter: [this._l10nIdName]};
> +}
> +
> +
> +L20nMutationObserver.prototype.harvestDocument = function harvestDocument() {

Rename this to something without 'document' in it?  Maybe harvest(node)?

@@ +13,5 @@
> +}
> +
> +
> +L20nMutationObserver.prototype.harvestDocument = function harvestDocument() {
> +  var nodes = this._rootNode.querySelectorAll('['+this._l10nIdName+']');

nodes should also include the root node itself.

@@ +25,5 @@
> +  var observerActive = false;
> +
> +  if (this._observer) {
> +    this._observer.disconnect();
> +    observerActive = true;

Nit:  reorder these two lines, or rename observerActive to observerPaused.

@@ +30,5 @@
> +  }
> +
> +  var affectedNodes = {
> +    'added': null,
> +    'modified': null,

Nit: single quotes are not needed here.

@@ +58,5 @@
> +    if (mutation.type === 'attributes') {
> +      if (!affectedNodes.modified) {
> +        affectedNodes.modified = [];
> +      }
> +      affectedNodes.modified.push(mutation.target);

Should we push to affectedNodes.modified when the mutation consisted of removing a data-l10n-id attribute?  AFAICT, right now we do.

@@ +83,5 @@
> +      case 'loading':
> +        break;
> +      case 'interactive':
> +      case 'complete':
> +        this.harvestDocument();

Does this belong in L20nMO, or in runtime.js?  Also, see my general comment regarding the need of having the L20nMO abstraction.

@@ +93,5 @@
> +  this._observer.observe(this._rootNode, this._config);
> +};
> +
> +L20nMutationObserver.prototype.disconnect = function() {
> +  this.observer.disconnect();

Spelling: this._observer

::: bindings/l20n/runtime.js
@@ +199,4 @@
>  }
>  
>  function onReady() {
> +  var nodeObserver = new L20nMutationObserver(onNodeModified);

onReady can be called multiple times, when the language changes.  I don't think we need multiple L20nMO in such case?

@@ +199,5 @@
>  }
>  
>  function onReady() {
> +  var nodeObserver = new L20nMutationObserver(onNodeModified);
> +  nodeObserver.observe(null, null, !isPretranslated);

Pass document (or document.body?) here instead of null?
Attachment #8421107 - Flags: review?(stas) → feedback+
Attachment #8421053 - Flags: review?(stas)
> @@ +83,5 @@
> > +      case 'loading':
> > +        break;
> > +      case 'interactive':
> > +      case 'complete':
> > +        this.harvestDocument();
> 
> Does this belong in L20nMO, or in runtime.js?  Also, see my general comment
> regarding the need of having the L20nMO abstraction.

One more thing: if we switch to MO entirely (and recommend the polyfill for supporting IE), I think we could greatly simplify the bootstrapping part in runtime.js:

https://github.com/mozilla-b2g/gaia/blob/7800803e41c3e9a598955b5ef8b8b87fc8f07cd5/shared/js/l10n.js#L1213-L1256
(In reply to Staś Małolepszy :stas from comment #19)
> - testing strategy - how do we test this?  currently, the l20n.js repo is
> not capable of running tests which involve DOM manipulation;  should we land
> tests in Gaia only?  Either way, we need tests in the patch.

I have no idea. We do need tests, but first we need to prove that the concept works and that mem/perf characteristics are in line.
 
> - transition strategy - you hinted at this question already, but what should
> we do with all the current uses of mozL10n.translate() and mozL10n.localize
> in Gaia?  A potential solution is to no-op mozL10n.translate (the element
> will be localized when inserted anyways), and change mozL10n.localize so
> that it doesn't call translateElement.  (In the longer run, mozL10n.localize
> should be deprecated in favor of node.setAttribute('data-l10n-id').)

no-op may work.
 
> - cross-browser compatibility strategy - we should keep l10n.js relevant for
> cross-browser webapps, not only Firefox OS.  According to
> http://caniuse.com/#feat=mutationobserver, IE 10 (desktop and mobile) don't
> support MO yet, but there's a polyfill available:
> https://github.com/Polymer/MutationObservers.  I see two options for us: 
> recommend the polyfill if you target IE, or provide a special transitional
> API (mozL10n.localizeNode?) which developers targeting IE can use.  I'd lean
> toward the former;  what are your thoughts on this?

I'm in favor of using polyfills for old versions of browsers.
 
> Generally the code looks good.  My biggest doubt is about the necessity of
> creating an abstraction in the form of L20nMutationObserver.  It seems like
> the only thing that you want to be able to keep a state of is the _rootNode,
> so that you can disconnect and reconnect the observer inside the callback. 

There are three things I achieve with wrapping MO in my own class:
 - I transition away from reporting mutations to reporting affected nodes
 - I gain ability to retroactively cover nodes from DOM *with the same translation API*
 - I preserve the same Observer API to MO which makes it easy to plug it into bindings

I can get rid of the wrapper, or turn it into a literal (it's a singleton anyway), but it seems valuable to keep interaction between bindings and the Observer clean.

> Do you expect _rootNode to be something else than document in the near
> future?  Do you think we'll need more than one observer per document?

I don't know how we'll work with iframes, but except of that, no, I don't expect it to be anything else than a document.
 
> @@ +83,5 @@
> > +      case 'loading':
> > +        break;
> > +      case 'interactive':
> > +      case 'complete':
> > +        this.harvestDocument();
> 
> Does this belong in L20nMO, or in runtime.js?  Also, see my general comment
> regarding the need of having the L20nMO abstraction.

Yeah, I like it here, because it uses the same API as MO.

> onReady can be called multiple times, when the language changes.  I don't
> think we need multiple L20nMO in such case?

Good catch.

I'll work on testing memory/perf footprint and report back here.
Startup performance seems to be unaffected:

Keon, master, b2gperf with 31 iterations, delay 10, median:

	 master   MO    delta
browser    585    571   -14
calendar	   941    945     4
camera	  1225   1208   -17
clock	   847    818   -29
contacts	  1205   1235    30
email	   604    584   -20
messages	  1201   1212    11
music	   867    875     8
phone	   960    957    -3
settings	  1376   1378     2
usage	  1096   1102     6
==============================
                   AVG:  -2

I also confirmed no FOUC's on Tarako, Keon and Flame.

I'll measure memory next.
(In reply to Olli Pettay [:smaug] from comment #18)
> How do we handle localization these days in gaia? What does the API look
> like, and how is it implemented?

I'm not sure what are you asking about specifically, but here are some links:

* Current API: https://developer.mozilla.org/en-US/docs/Web/API/L10n
* Target API: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/L20n/Javascript_API

Let me know if that's what you're looking for.
(In reply to Staś Małolepszy :stas from comment #19)
> @@ +13,5 @@
> > +}
> > +
> > +
> > +L20nMutationObserver.prototype.harvestDocument = function harvestDocument() {
> > +  var nodes = this._rootNode.querySelectorAll('['+this._l10nIdName+']');
> 
> nodes should also include the root node itself.

Interestingly enough, I don't think so anymore.

See, currently we do have the code to localize the root node when we localize DOMFragment, but it all works only because the root node is never localizable ;)

Consider this:

<div data-l10n-id="foo">
  <h1 data-l10n-id="foo2"></h1>
</div>

foo=Title
foo2=Header

What should be the result DOM?

Currently, the result DOM will be:

<div data-l10n-id="foo">Title</div>

So, until we have something more sophisticated, I'd say we want to check if the DOM has firstElementChild and if it does, we will launch translateFragment and *not* localize it.

> Should we push to affectedNodes.modified when the mutation consisted of
> removing a data-l10n-id attribute?  AFAICT, right now we do.

In my updated code I actually remove the content of the node if:
 - l10n-id is removed
 - l10n-id is changed to an ID that does not exist

I'm still testing memory and the initial results are very good for Settings and Contacts. The memory footprint is within the stdev.

I'll try to wrap it up on Monday and start writing tests.

One interesting note is that I was able to no-op mozL10n.translate, but mozL10n.localize is still needed. Cleaning the use cases of localize will be tricky because it's been used in Clock, Dialer, Contacts, FTU, Email, Settings, SMS and System, and in many cases it is misused instead of mozL10n.get :(
Where is the implementation for ctx.localize ?
Olli: it's here:

https://github.com/mozilla-b2g/gaia/blob/83f622db256dad92e8c97cccf9c71611d0e5a737/shared/js/l10n.js#L1459-L1481

It essentially sets data-l10n-id and data-l10n-args on a node so that it can be used later on to localize the node.  data-l10n-args is a stringified JSON with program data which will be interpolated in place of {{ variables }} found in translations.  We need this data to persist somehow between the moment we set it and the moment it is used to localize the node.

With mutation observers we can at least get rid of the |if (this.ctx.isReady)| part of the implementation, since the observer will take care of that.  In talking to Gandalf, we agreed that we also don't like the |if (!id)| path;  I'm planning on changing it in bug 994519.
Olli: long story short, we want to get rid of the current incarnation of 'localize'.

Instead, we want devs to write:

  node.setAttribute('data-l10n-id', 'hello');
  node.setAttribute('data-l10n-args', JSON.stringify({ username: 'Mary' }));
  document.body.appendChild(node);

( see bug 994357 comment 7 )

and we think about providing a wrapper for that:

mozL10n.setNodeLocalizable(node, l10nId, l10nArgs);
(In reply to Staś Małolepszy :stas from comment #26)

> With mutation observers we can at least get rid of the |if
> (this.ctx.isReady)| part of the implementation, since the observer will take
> care of that.

Actually, there is a use-case which I thought of that made me reconsider this statement.

See https://github.com/mozilla-b2g/gaia/blob/df05327a7fdb517ead5923f6b81e8284e440bc7d/apps/settings/js/battery.js

The element is already in the DOM and it already has data-l10n-id (so there is no mutation per se), and yet we need a method to insert new translation into it.
(In reply to Staś Małolepszy :stas from comment #28)
> (In reply to Staś Małolepszy :stas from comment #26)
> 
> > With mutation observers we can at least get rid of the |if
> > (this.ctx.isReady)| part of the implementation, since the observer will take
> > care of that.
> 
> Actually, there is a use-case which I thought of that made me reconsider
> this statement.
> 
> See
> https://github.com/mozilla-b2g/gaia/blob/
> df05327a7fdb517ead5923f6b81e8284e440bc7d/apps/settings/js/battery.js
> 
> The element is already in the DOM and it already has data-l10n-id (so there
> is no mutation per se), and yet we need a method to insert new translation
> into it.

Let's move the conversation about the future of mozL10n.localize to bug 994519
Can you test apps with heavy DOM insertions like the Call log panel in Dialer, SMS and Contacts? You can use the various |make reference-workload-*| to test this.
(In reply to Anthony Ricaud (:rik) from comment #30)
> Can you test apps with heavy DOM insertions like the Call log panel in
> Dialer, SMS and Contacts? You can use the various |make
> reference-workload-*| to test this.

Can I use b2gperf to test that? Because that's what I'm using now and I'm testing Messages, Contacts and Phone apps (which sounds like it?)
Attached patch patch against master (obsolete) — Splinter Review
I completed performance and memory tests and fixed all bugs I found that collided with this patch. Below is the summary:


== Performance ==

Performance tests have been done using b2gperf and test-perf using
Device: Keon
Gecko: 4595a49 (32)
Gaia: 57ad1b3

Iterations: 31
Delay: 10

Default locale (pretranslation):

	 master   MO    delta
browser    464    467     3
calendar	   944    947     3
camera	  1249   1249     0
clock	   806    805    -1
contacts	  1202   1201    -1
email	   562    559    -3
messages	  1250   1251     1
music	   829    834     5
phone	   908    907    -1
settings	   796    798     2
usage	  1082   1073    -9
==============================
                   AVG:   0

Non-default locale:


	 master   MO    delta
browser    480    484     4
calendar	   962    961    -1
camera	  1247   1244    -3
clock	  1085   1088     3
contacts	  1173   1169    -4
email	   559    561     2
messages	  1281   1292    11
music	   848    851     3
phone	  1186   1185    -1
settings	   814    821     7
usage	  1090   1093     3
==============================
                   AVG:   2

Stdevs were around 20-30ms.

On Settings I was also able to measure the new perf states:

moz-chrome-dom-loaded: 638 -> 645
moz-chrome-interactive: 639 -> 645
moz-content-interactive: 4824 -> 4383
moz-app-loaded: 5110 -> 4928

and older tests:

startup time: 1037 -> 985
startup-path-done: 1576 -> 1394

Although I was able to reproduce some perf win over several attempts, I only got 4 runs each, so I would not put too much faith in test-perf numbers yet ( with more than 4 runs on wifi test I crashed the device).

== Memory ==

Settings, mozPerfMemoryAverage:

 uss: 13.181 -> 12.970
 pss: 17.031 -> 16.921
 rss: 31.625 -> 31.695
 vsize: 153.216 -> 149.608

== Visible bugs ==

There were a couple of minor changes I had to make:

1) Email app wants to cache translated DOM, so it needed a method to do so. I named it translateFragment.

2) Music app had a small bug in the code responsible for settings data-l10n-id. The bug was only visible when the language change happened (because the l10n-id didn't exist and the UI would not be retranslated).

3) Lockscreen had a a small bug in the code related to looping over status lines.

== Landing risk ==

I expect there to be a few more minor bugs in code that has been used to:
 - set the node value
 - set the node l10n-id

Those bugs, until MO, have been only exposed when user changed language. With this patch, they will be exposed also in default locale.

Thanks to a lot of testing that QA has been doing on non-default locales, I expect the number of those undiscovered to be low and with this patch, we make them more exposed, and easier to find and fix.

Stas: I want to work on tests for this patch next week, in parallel on getting all necessary reviews and sr's.

I'd like to start with a review from you, followed by sr from vivien and then r's from affected apps authors.
Attachment #8421107 - Attachment is obsolete: true
Attachment #8432021 - Flags: review?(stas)
Comment on attachment 8432021 [details] [diff] [review]
patch against master

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

Per bug 1005760 comment 11: Jonas, it seems that I cannot reproduce any performance regression with the switch. Are you ok with this change in such case?
Attachment #8432021 - Flags: feedback?(jonas)
Attached patch patch (obsolete) — Splinter Review
updated patch.

I made the unit tests pass. It actually required only two changes:

1) l10n_test.js was expecting mozL10n.localize to also translate, I added explicit mozL10n.translateFragment calls

2) lockscreen tests were testing state lines using textContent value instead of testing presence of data-l10n-id.

Because I had to fix that, I went a bit forward and actually cleaned a small portion of settings the state lines in lockscreen code, and then updated the tests to use it.

Some of the tests are still testing textContent because until we fix bug 1004973, it would be much more complex to test the output properly.
I think for now, cleaning the tests the way I did in the patch is enough. And once bug 1004973 is fixed, I'll update the remaining pieces of that test.

Wrt. lockscreen code - I could file a separate bug and submit the patch there, but the fix that I'm using (to unify state lines to always rely on data-l10n-id and toggle it) relies on MO, so fixing it without MO and then updating to MO feels redundant.

I still have 4 ui tests failing, I'll be working on that tomorrow.

Stas: can you review the l10n portion of the code and tell me if that looks good?

Sicking: can you look at how we introduce MO to react to DOM and tell me if that looks good to you?
Attachment #8432021 - Attachment is obsolete: true
Attachment #8432021 - Flags: review?(stas)
Attachment #8432021 - Flags: feedback?(jonas)
Attachment #8432353 - Flags: review?(stas)
Attachment #8432353 - Flags: feedback?(jonas)
Hubert: Do we currently have performance results for Call log, Contacts and SMS  measuring how long it takes to render a reference-workload? Can you point us to a datazilla URL that show these results?
Hub: See comment 30, 31 and 35.
Flags: needinfo?(hub)
On datazilla we run with "reference-workload-light"

Here are various results on datazilla.


Dialer call log
https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=7&test=Dialer/callLog_rendering_time_%3E_first-chunk-ready&app_list=communications/dialer&app=communications/dialer&gaia_rev=e98fe1e94d3d80ad&gecko_rev=0e10c8151654&plot=avg
https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=7&test=Dialer/callLog_rendering_time_%3E_call-log-ready&app_list=communications/dialer&app=communications/dialer&gaia_rev=e98fe1e94d3d80ad&gecko_rev=0e10c8151654&plot=avg

Contacts startup path
https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=7&test=startup_%3E_init-finished&app_list=communications/contacts,communications/dialer,sms&app=communications/contacts&gaia_rev=e98fe1e94d3d80ad&gecko_rev=0e10c8151654&plot=avg
https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=7&test=startup_%3E_startup-path-done&app_list=communications/contacts,communications/dialer&app=communications/contacts&gaia_rev=e98fe1e94d3d80ad&gecko_rev=0e10c8151654&plot=avg

SMS startup path
https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=7&test=startup_%3E_load&app_list=communications/contacts,communications/dialer,sms&app=communications/contacts&gaia_rev=e98fe1e94d3d80ad&gecko_rev=0e10c8151654&plot=avg
https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=7&test=startup_%3E_objects-init-finished&app_list=communications/contacts,communications/dialer,sms&app=communications/contacts&gaia_rev=e98fe1e94d3d80ad&gecko_rev=0e10c8151654&plot=avg
https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=7&test=startup_%3E_will-render-threads&app_list=communications/contacts,communications/dialer,sms&app=communications/contacts&gaia_rev=e98fe1e94d3d80ad&gecko_rev=0e10c8151654&plot=avg
https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=7&test=startup_%3E_startup-path-done&app_list=communications/dialer,sms&app=communications/dialer&gaia_rev=e98fe1e94d3d80ad&gecko_rev=0e10c8151654&plot=avg


Ideally if you want to compare your changes, I'd recommend running the test yourself with the right workload.
Flags: needinfo?(hub)
Comment on attachment 8432353 [details] [diff] [review]
patch

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

Great work, Gandalf.  The implementation looks great and lean, and it's cool that you also took time to fix bugs in Gaia apps.

Before I give it an r+, I'd like to discuss the questions about the bootstrapping logic which you'll find inline below.

::: shared/js/l10n.js
@@ +1174,1 @@
>      },

Maybe put a comment here explaining why it's no-op and the bug number?

@@ +1174,2 @@
>      },
> +    translateFragment: function translate(element) {

Nit: Either rename the function translateFragment or remove the name after the 'function' keyword. JS engines will figure it out from the context anyways.

Also, I'm not a big fan of how sometimes we use 'translate' and sometimes 'localize.'  Should we unify this here and rename the method to localizeFragment?

The argument should be called fragment, too.

@@ +1367,5 @@
> +
> +          if (addedNode.nodeType === Node.ELEMENT_NODE &&
> +              (addedNode.firstElementChild ||
> +               addedNode.hasAttribute('data-l10n-id'))) {
> +             if (addedNode.firstElementChild) {

It looks like this could be simplified by removing the addedNode.firstElementChild from the outer if, and moving the addedNode.hasAttribute('data-l10n-id') check into an inner else-if.

  if (addedNode.nodeType === Node.ELEMENT_NODE) {
    if (addedNode.firstElementChild) {
      translateFragment.call(navigator.mozL10n, addedNode);
    } else if (addedNode.hasAttribute('data-l10n-id')) {
      translateElement.call(navigator.mozL10n, addedNode);
    }
  }

@@ +1388,5 @@
> +  function onMutations(mutations, self) {
> +    self.disconnect();
> +
> +    localizeMutations(mutations);
> +

nit: remove newlines

@@ +1398,5 @@
> +      switch (document.readyState) {
> +        case 'loading':
> +          break;
> +        case 'interactive':
> +        case 'complete':

How does this relate to the bootstrap logic in https://github.com/l20n/l20n.js/blob/c7d12ed9f1113efc15754bad7455a498ae03be61/bindings/l20n/runtime.js#L107-L117 ?  Should we remove it?

Right now, it seems that the loading case will never be reached, because onReady is called only when the context is ready, and that doesn't happen too early thanks to the waitFor calls.  Also, we should think how to hook in pretranslate() here.

Another note about the loading case.  I was curious about how it worked so I set up a test document with a simple MutationObserver which I activated very early on.  I saw two mutation records when document became interactive:  one was for the insertion of <body> into <html>, and the other one was about adding all children of <body>.  This surprised me because I though the first one was enough.  This might be specific to document.body.  Have you seen this too?
Comment on attachment 8432353 [details] [diff] [review]
patch

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

::: shared/js/l10n.js
@@ +1174,2 @@
>      },
> +    translateFragment: function translate(element) {

They do serve different purposes. localize sets id/args, translateFragment translates fragment.

I'd prefer not to add this conversation to the scope of this bug.

@@ +1398,5 @@
> +      switch (document.readyState) {
> +        case 'loading':
> +          break;
> +        case 'interactive':
> +        case 'complete':

That's bug 902618. It doesn't affect our code.
Attached patch patch (obsolete) — Splinter Review
updated patch.

I applied reviewer's comments.
Attachment #8432353 - Attachment is obsolete: true
Attachment #8432353 - Flags: review?(stas)
Attachment #8432353 - Flags: feedback?(jonas)
Attachment #8433115 - Flags: review?(stas)
(In reply to Anthony Ricaud (:rik) from comment #30)
> Can you test apps with heavy DOM insertions like the Call log panel in
> Dialer, SMS and Contacts? You can use the various |make
> reference-workload-*| to test this.

I did run this with three workloads and three apps. I was only able to use RUNS=5 since anything higher was giving me errors in at least one test for each app.

SMS:
                        ‖        light         ‖        medium        ‖        heavy         ‖
------------------------‖----------------------‖----------------------‖----------------------‖
                        ‖ master | MO   |   Δ  ‖ master | MO   |   Δ  ‖ master | MO   |   Δ  ‖
------------------------‖----------------------‖----------------------‖----------------------‖
load                    ‖  1223  | 1195 | -28  ‖  1243  | 1205 | -38  ‖  1214  | 1220 |   6  ‖
will-render-threads     ‖  1204  | 1196 |  -8  ‖  1243  | 1208 | -35  ‖  1214  | 1220 |   6  ‖
objects-init-finished   ‖  1204  | 1197 |  -7  ‖  1243  | 1210 | -33  ‖  1215  | 1221 |   6  ‖
above-the-fold-ready    ‖  3258  | 3215 | -43  ‖  3268  | 3310 |  42  ‖  3163  | 3247 |  84  ‖
startup-path-done       ‖  6363  | 6169 |-194  ‖  9882  | 9729 |-153  ‖ 16061  |16127 |  66  ‖
startup time            ‖  1653  | 1650 |  -3  ‖  1585  | 1575 | -10  ‖  1598  | 1622 |  24  ‖


Contacts:
                        ‖        light         ‖        medium        ‖        heavy         ‖
------------------------‖----------------------‖----------------------‖----------------------‖
                        ‖ master | MO   |   Δ  ‖ master | MO   |   Δ  ‖ master | MO   |   Δ  ‖
------------------------‖----------------------‖----------------------‖----------------------‖
init-finished           ‖  1203  | 1237 |  34  ‖  1129  | 1112 | -17  ‖  1137  | 1194 |  57  ‖
above-the-fold-read     ‖  1801  | 1811 |  10  ‖  1616  | 1597 | -19  ‖  1661  | 1688 |  27  ‖
startup-path-done       ‖  7185  | 6903 |-282  ‖ 11664  |12005 | 341  ‖ 20094  |20058 | -36  ‖
startup time            ‖  1512  | 1507 |  -5  ‖  1542  | 1549 |   7  ‖  1534  | 1568 |  34  ‖


Dialer:
                        ‖        light         ‖        medium        ‖        heavy         ‖
------------------------‖----------------------‖----------------------‖----------------------‖
                        ‖ master | MO   |   Δ  ‖ master | MO   |   Δ  ‖ master | MO   |   Δ  ‖
------------------------‖----------------------‖----------------------‖----------------------‖
startup time            ‖  1116  | 1072 | -44  ‖  1100  | 1111 |  11  ‖  1168  | 1103 | -65  ‖
first-chunk-ready       ‖  4416  | 4353 | -63  ‖  4568  | 4489 | -79  ‖  4671  | 4503 |-168  ‖
call-log-ready          ‖  5515  | 5523 |   8  ‖  7130  | 7021 |-109  ‖  9942  | 9319 |-623  ‖


stdev was usually much higher than delta, but it seems that there are no major differences that would indicate that MO is significantly slower.

One theory I have for why it may be the case is that currently everything we inject and translate is translated synchronously before injection (using mozL10n.translate). With this change, the translation happens asynchronously in another event loop spin.

I also don't see any UX difference when loading master and MO side by side on Keon.
I think the results of perf tests done so far indicate that the switch should not carry any performance or memory cost and it will significantly improve our localizability and the quality of our code.

Jonas, Anthony, Julien: do you have any remaining concerns with regards to this change or any other tests your like me to run before I ask for sr from Vivien?
Flags: needinfo?(jonas)
Flags: needinfo?(felash)
Flags: needinfo?(anthony)
This looks great but I think this will fail in the SMS app's iframes, so you'll need to change the translate calls to translateFragment where needed (I'd say, in attachment.js and startup.js, where we call translate on the iframe's body).

So, for the startup, it's all fine, but I wonder what happens when we asynchronously add some content to the DOM and display it: I wonder if we'll first see a blank element, and _then_ a element with content (because observers are synchronous), and thus we can have an additional reflow because of this... Maybe you can have a look to the panels and menus we have in SMS :
* long press a SMS, does the menu look fine or does it have a reflow?
* long press a SMS or MMS, press "view report"
* attach an image, then tap that image: does the menu look fine?

There is _also_ a mock_l10n.js file in the sms app's unit tests, by the way (yeah, I know...)
Flags: needinfo?(felash)
MutationObserver callbacks are called at the end of a microtask or at the end of a task, whichever
happens first. Usually it is microtask. Painting should happen in a different task.
(In reply to Julien Wajsberg [:julienw] from comment #43)

> There is _also_ a mock_l10n.js file in the sms app's unit tests, by the way
> (yeah, I know...)

Yeah, looking at the failures in https://tbpl.mozilla.org/php/getParsedLog.php?id=40941657&tree=Gaia-Try, we'll need to fix MockL10n in SMS and Video.

(In the future, we should be able to solve this once and for all with bug 1004973, btw.)
Comment on attachment 8433115 [details] [diff] [review]
patch

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

Thanks for updating the patch, r=me.  I have one question below which doesn't affect my r+.  Great work overall!

Let's fix the existing tests and think how to add more unit test explicitly testing the mutation observer.

::: shared/js/l10n.js
@@ +1557,5 @@
>      if (!entity) {
> +      if (!element.firstElementChild) {
> +        element.textContent = '';
> +      }
> +      return true;

Can you explain the reason for this change?
Attachment #8433115 - Flags: review?(stas) → review+
(In reply to Staś Małolepszy :stas from comment #46)
> Can you explain the reason for this change?

Yes. The goal here is to ensure that if we encounter a bug, we don't leave the old value. An example is code that cycles through a list of various data-l10n-id's for one node.

If it attempts to switch to an ID that we don't have an entity for, I'd prefer to show it as missing.

I'm not overly attached to this approach, so if you think it's better not to do that (and throw a warning? error?), I'm happy to change that.
Attachment #8433115 - Flags: superreview?(21)
(In reply to Julien Wajsberg [:julienw] from comment #43)
> This looks great but I think this will fail in the SMS app's iframes, so
> you'll need to change the translate calls to translateFragment where needed
> (I'd say, in attachment.js and startup.js, where we call translate on the
> iframe's body).

Yeah. That's what I'll do for now. In the future, I'd like to automatically register MO on iframes as well.

> So, for the startup, it's all fine, but I wonder what happens when we
> asynchronously add some content to the DOM and display it: I wonder if we'll
> first see a blank element, and _then_ a element with content (because
> observers are synchronous), and thus we can have an additional reflow
> because of this... 

As Olli wrote, Gecko guards against it pretty well. I trust Gecko more :)

> Maybe you can have a look to the panels and menus we have
> in SMS :
> * long press a SMS, does the menu look fine or does it have a reflow?
> * long press a SMS or MMS, press "view report"
> * attach an image, then tap that image: does the menu look fine?

That looks good.
Blocks: 1020130
Blocks: 1020132
Blocks: 1020136
Blocks: 1020137
Blocks: 1020138
(In reply to Zibi Braniecki [:gandalf] from comment #34)
> Wrt. lockscreen code - I could file a separate bug and submit the patch
> there, but the fix that I'm using (to unify state lines to always rely on
> data-l10n-id and toggle it) relies on MO, so fixing it without MO and then
> updating to MO feels redundant.

I actually decided to file bug 1019970 and move this part of the patch away from this bug. It should make reviewing changes cleaner.

Also, I'm down to one gaia-ui test failure.
Depends on: 1019970
Julien: The data-l10n-id in this line https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/index.html#L465 seems to be redundant. With the switch to MO it brings a re-translation flash for messages while they're loaded. Can we remove it?

On a more general note, I think I'm down to zero test failures. I'm going to work on new tests for MO.
Flags: needinfo?(felash)
Hey Gandalf,

The "data-l10n-id" in this place is not redundant: it's wrong :)

It works on master because we don't call "translate" and as a result it's not used at all (and it's eventually changed when we get the contact information (or not) in `setContact`.

So, yeah, feel free to just remove it.
Flags: needinfo?(felash)
It's great to actually see a small perf improvement for Call log!

(Complete sidenote: Please paste links to github that contain a commit hash and not a branch name. They will become irrelevant (comment 50 already has). To quickly do that, you can press "y" while on Github to get redirected to the canonical version of that page.)
Flags: needinfo?(anthony)
great! I updated the patch to add the change in SMS.

I played with two Keons side by side with all HUD turned on. FPS, memory, jank seems comparable. On reflows I always end up having better numbers with this patch.

I also expect to have unit tests by tomorrow, and then I'll ask :stas for r? again.

Vivien: can we land this as soon as the tree opens for 2.1?
Flags: needinfo?(21)
Attached patch patch + tests (obsolete) — Splinter Review
Final patch with tests. Changes:

 - added separate bindings tests
 - moved lockscreen patch to a separate bug (bug 1019970)
 - reduced amount of changes in localizeElement (we'll deal with that in bug 994519)
 - added a fix for SMS templating bug
 - manually translate SMS's iframes
 - fix for net_error l10n
 - guard against creating duplicate MO on each onReady call
 - moved setting dir/lang away from translateFragment into proper bindings calls

Vivien confirmed on IRC that he'll look into this right after we branch.

Stas: I'd like to ask you to take a look again at this patch
Attachment #8433115 - Attachment is obsolete: true
Attachment #8433115 - Flags: superreview?(21)
Attachment #8436053 - Flags: superreview?(21)
Attachment #8436053 - Flags: review?(stas)
Flags: needinfo?(21)
Comment on attachment 8436053 [details] [diff] [review]
patch + tests

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

This looks relaly good, with small nits below, but I have to give it an r- because of a regression I'm seeing in setting the dir attribute of document.documentElement for a non-default RTL locale.  See inline comments for details.

::: apps/sms/js/attachment.js
@@ +267,4 @@
>      var clickOnFrame = iframe.click.bind(iframe);
>  
>      iframe.removeEventListener('load', iframeLoad);
> +    navigator.mozL10n.translateFragment(iframe.contentDocument.body);

This will translate the iframe, but do we also want to register a new Mutation Observer?  Do you want to file a follow-up for this?

::: apps/sms/test/unit/mock_l10n.js
@@ +24,3 @@
>        setTimeout(handler);
>      },
>      translate: function translate(element) {},

Can we remove this from the mock?

::: shared/js/l10n.js
@@ +1379,5 @@
> +    }
> +  }
> +
> +
> +

nit: remove blank lines.

@@ -1442,5 @@
>    function translateFragment(element) {
> -    if (!element) {
> -      element = document.documentElement;
> -      document.documentElement.lang = this.language.code;
> -      document.documentElement.dir = this.language.direction;

Please don't remove document.documentElement.{lang,dir} from here, or at least put them in inlineLocalization as well.  These lines are meant to be run earlier than onReady in order to make sure the content doesn't jump from one side of the screen to the other when the HTML has been pretranslated to an LTR locale and the current language is an RTL one.

I tested the patch on a device and it seems that there's a regression when switching to an RTL language - the 'dir' is not set at all.  Can you look into it?

@@ +1553,5 @@
>      if (!entity) {
> +      if (!element.firstElementChild) {
> +        element.textContent = '';
> +      }
> +      return true;

I think this is inconsistent.  We'll reset the textContent of the element in case the entity is falsy, but only if the element doesn't have children.  If it has children, will we leave the previous translation?

Maybe something like this would be better?

if (entity === null ||          // the translation is missing
    entity === undefined) {     // or the translation contains errors
  element.textContent = '';
}

The amount of HTML-enabled strings is currently minimal, and bug 994357 will likely change these lines anyways.
Attachment #8436053 - Flags: review?(stas) → review-
(In reply to Staś Małolepszy :stas from comment #55)
> Comment on attachment 8436053 [details] [diff] [review]
> patch + tests
> 
> Review of attachment 8436053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks relaly good, with small nits below, but I have to give it an r-
> because of a regression I'm seeing in setting the dir attribute of
> document.documentElement for a non-default RTL locale.  See inline comments
> for details.
> 
> ::: apps/sms/js/attachment.js
> @@ +267,4 @@
> >      var clickOnFrame = iframe.click.bind(iframe);
> >  
> >      iframe.removeEventListener('load', iframeLoad);
> > +    navigator.mozL10n.translateFragment(iframe.contentDocument.body);
> 
> This will translate the iframe, but do we also want to register a new
> Mutation Observer?  Do you want to file a follow-up for this?

That's bug 1020130, but I don't think they need this. So far they didn't retranslate.

> ::: shared/js/l10n.js
> @@ +1379,5 @@
> > +    }
> > +  }
> > +
> > +
> > +
> 
> nit: remove blank lines.
> 
> @@ -1442,5 @@
> >    function translateFragment(element) {
> > -    if (!element) {
> > -      element = document.documentElement;
> > -      document.documentElement.lang = this.language.code;
> > -      document.documentElement.dir = this.language.direction;
> 
> Please don't remove document.documentElement.{lang,dir} from here, or at
> least put them in inlineLocalization as well.  These lines are meant to be
> run earlier than onReady in order to make sure the content doesn't jump from
> one side of the screen to the other when the HTML has been pretranslated to
> an LTR locale and the current language is an RTL one.
> 
> I tested the patch on a device and it seems that there's a regression when
> switching to an RTL language - the 'dir' is not set at all.  Can you look
> into it?

Sure, how about I just introduce internal translateDocument next to translateFragment. I don't like the polymorphism of translateFragment and the fact that we expose it.

Also, your comment made me realize that in cases where we fire translateFragment on iframe's body, we actually should also set dir/lang, but we don't.

I believe that bug 1020130 will solve this.
(In reply to Staś Małolepszy :stas from comment #55)
> ::: apps/sms/test/unit/mock_l10n.js
> @@ +24,3 @@
> >        setTimeout(handler);
> >      },
> >      translate: function translate(element) {},
> 
> Can we remove this from the mock?

Not in this patch. SMS tests rely on this. When we'll be cleaning up use cases of mozL10n.translate (bug 1020136), we'll remove that.
(In reply to Staś Małolepszy :stas from comment #55)
> Maybe something like this would be better?
> 
> if (entity === null ||          // the translation is missing
>     entity === undefined) {     // or the translation contains errors
>   element.textContent = '';
> }
> 
> The amount of HTML-enabled strings is currently minimal, and bug 994357 will
> likely change these lines anyways.

I ended up having to remove that because it broke Custom Tag's templates in Contacts. See bug 1023603 and its dependencies.
Attached patch patch v3 + tests (obsolete) — Splinter Review
Updated test.

Three changes:

 - I bound mo handler to mozL10n to avoid conflicts when mozL10n is replaced (tests)
 - I added l10n-args tests
 - I reverted change of behavior in translateElement - if entity is not found, we'll keep the node content intact for now. (bug 1023603)

Travis and tbpl are green.
Attachment #8436053 - Attachment is obsolete: true
Attachment #8436053 - Flags: superreview?(21)
Attachment #8438106 - Flags: superreview?(21)
Attachment #8438106 - Flags: review?(stas)
Comment on attachment 8438106 [details] [diff] [review]
patch v3 + tests

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

Beautiful! r=me.

Two more comments, not affecting my r+:

 - it would be great to figure out a way to test bindings_test.js in l20n.js repo.  Can you file a bug about this?

 - as evidenced in bug 1004973 comment 25, some tests explicitly load shared/js/l10n.js instead of using MockL10n.  I think this creates a risk of intermittent failures where the real mozL10n sets up an observer, and then the mock one localizes something, and the result actually comes from the real one (and is likely an empty string, since the real mozL10n doesn't have any resources configured).

There are only 6 places where we require shared/js/l10n.js in tests:

$ ag "require(App)?\('.*shared/js/l10n.js'\);" -- apps shared

apps/sharedtest/test/unit/l10n/l10n_date_test.js
3:require('/shared/js/l10n.js');

apps/calendar/test/unit/setup.js
296:  requireApp('calendar/shared/js/l10n.js');

apps/video/test/unit/thumbnail_item_test.js
6:require('/shared/js/l10n.js');

apps/video/test/unit/thumbnail_date_group_test.js
7:require('/shared/js/l10n.js');

apps/video/test/unit/video_test.js
7:require('/shared/js/l10n.js');

apps/sms/test/unit/sms_test.js
12:require('/shared/js/l10n.js');

Should we switch them to MockL10n before landing this patch?  I hope to have bug 1004973 ready for review soon, so it might be an easy change.
Attachment #8438106 - Flags: review?(stas) → review+
Vivien, I think we're ready to land this right after bug 1004973 lands.

Can you sr this?
Flags: needinfo?(21)
We landed some rather ugly hacks in bug 1023714 for localization. Just wanted to confirm whether or not this would work for the shadow dom, or if we should file a follow-up. Thanks.
MutationObserver can't observe changes cross shadow dom boundaries.
(I'm pretty sure our preliminary implementation of shadow dom hasn't be updated to handle
MutationObserver correctly)
So one would need to observe changes under the ShadowRoots too.
Blocks: 1026236
I filed bug 992473 to discuss how to extend it to ShadowRoots.
Comment on attachment 8438106 [details] [diff] [review]
patch v3 + tests

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

I'm tempted to sr+ as I believed it will be very useful but before I would like to ensure it is not going to regress the load time of big lists.

Can you try to load some of the workloads and measure the times taken to load the relative list with and without this patch ?

::: apps/system/js/net_error.js
@@ +70,1 @@
>    }

Any reason to not just replace the content of the localizeElement function ?

::: shared/js/l10n.js
@@ +1151,5 @@
>        return localizeElement.call(navigator.mozL10n, element, id, args);
>      },
> +    translate: function () {
> +      // XXX: Remove after removing obsolete calls. Bug 992473
> +    },

Is it not this bug ? :)

@@ +1335,5 @@
>  
> +  function localizeMutations(mutations) {
> +    var mutation, i;
> +
> +    for (i = 0; i < mutations.length; i++) {

nit: for (var i... instead of declaring |i| separately.

@@ +1340,5 @@
> +      mutation = mutations[i];
> +      if (mutation.type === 'childList') {
> +        var addedNode, j;
> +
> +        for (j = 0; j < mutation.addedNodes.length; j++) {

nit: for (var j...

@@ +1343,5 @@
> +
> +        for (j = 0; j < mutation.addedNodes.length; j++) {
> +          addedNode = mutation.addedNodes[j];
> +
> +          if (addedNode.nodeType === Node.ELEMENT_NODE) {

nit:

if (addedNode.nodeType !== Node.ELEMENT_NODE) {
  continue;
}

Let's avoid nesting.

@@ +1344,5 @@
> +        for (j = 0; j < mutation.addedNodes.length; j++) {
> +          addedNode = mutation.addedNodes[j];
> +
> +          if (addedNode.nodeType === Node.ELEMENT_NODE) {
> +             if (addedNode.firstElementChild) {

s/firstElementChild/childElementCount ?

@@ +1372,2 @@
>      }
> +

Extra space warning.
Clearing the needinfo as I answered directly by reading at the code.
Flags: needinfo?(21)
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #65)
> I'm tempted to sr+ as I believed it will be very useful but before I would
> like to ensure it is not going to regress the load time of big lists.

Awesome!
 
> Can you try to load some of the workloads and measure the times taken to
> load the relative list with and without this patch ?

Is that different than comment 41?

> ::: apps/system/js/net_error.js
> @@ +70,1 @@
> >    }
> 
> Any reason to not just replace the content of the localizeElement function ?

I always thought that a function that's only line is to call another function is pure overhead. Do you want me to bring it back?

> ::: shared/js/l10n.js
> @@ +1151,5 @@
> >        return localizeElement.call(navigator.mozL10n, element, id, args);
> >      },
> > +    translate: function () {
> > +      // XXX: Remove after removing obsolete calls. Bug 992473
> > +    },
> 
> Is it not this bug ? :)

So, that's a great question!

Initially, I wanted to remove all use cases of mozL10n.translate in this patch and land it all together. Then, reading about how master -> 2.0 merging works, I figured that drifting apart from 2.0 will make backporting all patches to it harder so I wanted to land this without removing use cases of translate and then remove the no-op calls in a few weeks when the work on master really begin.

Does it make sense or do you think I should remove the calls and the method here?
Flags: needinfo?(21)
FWIW I think this patch is already big and I like the 2-step approach from removing "translate" calls in a next patch.
(In reply to Zibi Braniecki [:gandalf] from comment #67)
> > ::: shared/js/l10n.js
> > @@ +1151,5 @@
> > >        return localizeElement.call(navigator.mozL10n, element, id, args);
> > >      },
> > > +    translate: function () {
> > > +      // XXX: Remove after removing obsolete calls. Bug 992473
> > > +    },
> > 
> > Is it not this bug ? :)
> 
> So, that's a great question!
> 
> Initially, I wanted to remove all use cases of mozL10n.translate in this
> patch and land it all together. Then, reading about how master -> 2.0
> merging works, I figured that drifting apart from 2.0 will make backporting
> all patches to it harder so I wanted to land this without removing use cases
> of translate and then remove the no-op calls in a few weeks when the work on
> master really begin.
> 
> Does it make sense or do you think I should remove the calls and the method
> here?

I think the comment would be clearer if we referenced both this bug (bug 992473) and bug 1020136, which is specifically about removing mozL10n.translate.
(In reply to Zibi Braniecki [:gandalf] from comment #67)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,  
> > Can you try to load some of the workloads and measure the times taken to
> > load the relative list with and without this patch ?
> 
> Is that different than comment 41?

It's not :)

> 
> > ::: apps/system/js/net_error.js
> > @@ +70,1 @@
> > >    }
> > 
> > Any reason to not just replace the content of the localizeElement function ?
> 
> I always thought that a function that's only line is to call another
> function is pure overhead. Do you want me to bring it back?
>

Yep please. I don't think the overhead bring by this really matters in this specific case.
 
> > ::: shared/js/l10n.js
> > @@ +1151,5 @@
> > >        return localizeElement.call(navigator.mozL10n, element, id, args);
> > >      },
> > > +    translate: function () {
> > > +      // XXX: Remove after removing obsolete calls. Bug 992473
> > > +    },
> > 
> > Is it not this bug ? :)
> 
> So, that's a great question!
> 
> Initially, I wanted to remove all use cases of mozL10n.translate in this
> patch and land it all together. Then, reading about how master -> 2.0
> merging works, I figured that drifting apart from 2.0 will make backporting
> all patches to it harder so I wanted to land this without removing use cases
> of translate and then remove the no-op calls in a few weeks when the work on
> master really begin.
> 
> Does it make sense or do you think I should remove the calls and the method
> here?


What stas says with adding the 2 bug numbers. It is confusing otherwise.
Flags: needinfo?(21)
Comment on attachment 8438106 [details] [diff] [review]
patch v3 + tests

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

You don't need to fix my nits to land but I would appreciate it.

Let's go MutationObserver!
Attachment #8438106 - Flags: superreview?(21) → superreview+
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #71)
> You don't need to fix my nits to land but I would appreciate it.

I applied all your feedback and also fixed gaia-menu to use translateFragment for now until we fix bug 1026236.

> Let's go MutationObserver!

Yay! :)
Attached patch patch v4 + testsSplinter Review
carrying over r/sr from the previous patch.
Attachment #8438106 - Attachment is obsolete: true
Attachment #8442035 - Flags: superreview+
Attachment #8442035 - Flags: review+
Depends on: 1037637
(marking as landed in v2.1)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: