1022859 - Factorize parser into a class

Status: RESOLVED FIXED

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

Description

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

Gaia currently uses proprietary format based on .properties

It limits data structures that we can handle, forces us to further dirtyhack .properties format whenever we need to add new data structures and is confusing for third parties because it pretends to be .properties.

Let's move to use l20n file format described here http://l20n.github.io/spec/grammar.html

Comment 1

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

Attachment: refactor patch

This patch is just an internal refactor of our code to make it ready for introduction of a second resource format.

Main things:
 - wrap up properties parser in PropertiesParser class.
 - introduce header.js in compiler tests to simplify test header

The PropertiesParser class is exactly the same code as the old parser, but it's wrapper up in a class. It should have no performance or memory impact on runtime since the class is just never initialized there when GAIA_CONCAT_LOCALES=1.

Comment 2

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

Oh, I also moved the AST returned by the parser to be Object.create(null) and in result removed hasOwnProperty checks.

Comment 3

[Staś Małolepszy :stas]

Comment on attachment 8450418 [details] [diff] [review]
refactor patch

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

This looks great.  One question though:  will header.js work when you copy test/lib to apps/sharedtest/test/unit/l10n/lib in Gaia?  IIRC, the reason we inlined that part of code was that it was the only way to make it work in Gaia tests.

::: lib/l20n/format/properties/parser.js
@@ +15,5 @@
> +    controlChars: /\\([\\\n\r\t\b\f\{\}\"\'])/g
> +  };
> +
> +  this.parse = function (ctx, source) {
> +    var ast = Object.create(null);

This is cool.  Can you do the same for Locale.entries?

Comment 4

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

Attachment: patch, v2

Updated the patch:
 - moved locale.entries to null proto object
 - moved entity.attributes to null proto object
 - removed one test that should not work with null proto object (if I understand correctly :))
 - updated tests to work in gaia setup

Comment 5

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

Attachment: pull request

Comment 6

[Staś Małolepszy :stas]

Comment on attachment 8451861 [details] [diff] [review]
patch, v2

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

The test that you removed got me thinking, because I expected it to return the raw "{{ __proto__ }}" and instead I noticed it returned "undefined".  It turns out that __proto__ is an *own* non-enumerable key on objects created with Object.create(null) and our code does the wrong thing in subPlaceable.  See my comment inline.

Please also make necessary changes in tools/{parse,compile}.js.

::: lib/l20n/compiler.js
@@ +80,4 @@
>      return ctxdata[id];
>    }
>  
> +  if (id in env) {

This doesn't do the right thing for '__proto__'.  Previously, env.hasOwnProperty('__proto__') returned false, so we didn't go into this branch.  Now, there is no hasOwnProperty that we could use, and | '__proto__' in env | returns true.

I see a number of solutions:

 1. if (id in env && env[id] !== null),
 2. don't create env with Object.create(null) and use hasOwnProperty, like before (it was me who suggested to use Object.create(null) for env, so sorry if it was a wrong call),
 3. make identifiers starting with two underscores illegal in the parser (we already do this to some extent to allow for env.__plural),
 4. special-case '__proto__' here and return match,
 5. accept that for proto, we won't return match but something else, but preferable not create a new Entity.

::: tests/lib/compiler/attributes_test.js
@@ +4,3 @@
>  
>  if (typeof navigator !== 'undefined') {
> +  require('/apps/sharedtest/test/unit/l10n/lib/compiler/header.js');

I think the following should also work:

  requireApp('sharedtest/test/unit/l10n/lib/compiler/header.js');

and it looks like the preferred way of requiring files from the same app in Gaia.

::: tests/lib/compiler/ctxdata_test.js
@@ -114,5 @@
>      });
>  
> -    it('returns the raw string when __proto__ is referenced', function(){
> -      var value = env.protoReference.toString(ctxdata);
> -      assert.strictEqual(value, '{{ __proto__ }}');

Let's bring this test back.  The __proto__ is actually quite interesting :)

Comment 7

[Staś Małolepszy :stas]

(Also, an ask for the future:  when attaching patches which have renames in them, please use the -M option to gif show or git diff.  This makes git look for renames.  If the renamed file has many changes in it, you may need to specify the fuzziness threshold, like -M80. Thanks!)

Comment 8

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

Attachment: patch, v3

I brought back the test and special-cased the __proto__.

The reason I decided to go this route is that '__proto__' in Object.create(null) returns false for all major browsers (Fx, Chrome, Safari, Opera) and it only returns true for Node.

Once Node updates, we can remove the hack.

Comment 9

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

(In reply to Staś Małolepszy :stas from comment #7)
> (Also, an ask for the future:  when attaching patches which have renames in
> them, please use the -M option to gif show or git diff.  This makes git look
> for renames.  If the renamed file has many changes in it, you may need to
> specify the fuzziness threshold, like -M80. Thanks!)

I tried this here, with -M80 and -M120 and it still showed parser.js as removed/added.

Comment 10

[Staś Małolepszy :stas]

Comment on attachment 8452446 [details] [diff] [review]
patch, v3

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

Thanks, this makes sense.  I didn't realize it was only Node.js which manifested this weird behavior.  r=me, with a nit below and also please make necessary changes to tools/{parse,compile}.js.

::: lib/l20n/compiler.js
@@ +83,5 @@
> +  /**
> +   * special case for Node where still:
> +   * '__proto__' in Object.create(null) => true
> +   *
> +   * Remove it when Node updates to spec

Nit: can you use // comments here for consistency with the rest of the file, prefix the comment with 'XXX' and remove the "Remove it when" line?  Also, it's Node.js, not Node.

Comment 11

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf] from comment #9)
> (In reply to Staś Małolepszy :stas from comment #7)
> > (Also, an ask for the future:  when attaching patches which have renames in
> > them, please use the -M option to gif show or git diff.  This makes git look
> > for renames.  If the renamed file has many changes in it, you may need to
> > specify the fuzziness threshold, like -M80. Thanks!)
> 
> I tried this here, with -M80 and -M120 and it still showed parser.js as
> removed/added.

Yeah, I guess the file-wide indentation change makes git dizzy.  Thanks for trying anyways.

Comment 12

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

Attachment: pull request

updated pull request with addressed review comments

Comment 13

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

I tested perf/mem impact on Settings app, 31 runs, test-perf:

MemoryAvg: 15.006 -> 14.955
moz-content-interactive: 3024 -> 3008
moz-app-visually-complete: 3024 -> 3007
moz-app-loaded: 4004 -> 3997

Comment 14

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

Commit: https://github.com/mozilla-b2g/gaia/commit/4e2ad6d5e1069d4c36837330f20dfdb1c9e2e83d
Merge: https://github.com/mozilla-b2g/gaia/commit/c4248a8de5ed8336424a3a766fd5e67f40e2e7ef

L20n: https://github.com/l20n/l20n.js/commit/8993e54a807e8c6cd3269bf19bba695ad75249ef

Comment 15

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

With this landing, the work on introducing l20n format moves to bug 1027684.

Comment 16

[Marina Rodríguez [:mai]]

Hi, 
after this patch was landed on master branch, the costcontrol app with balance configuration is failing:
 E/GeckoConsole( 1842): [JavaScript Error: "too much recursion" {file: "app://costcontrol.gaiamobile.org/shared/js/l10n.js" line: 715}]

The error is produced by this change:
Before:
if (node.hasOwnProperty(key) && key[0] !== '_') {
After:
if (key[0] !== '_') {

would you mind restoring the assertion node.hasOwnProperty(key) on the line 715 of the l10n.js file?

Regards

Comment 17

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

Marina: feel free to file a bug, I'm about to investigate it.

Comment 18

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

I cannot reproduce the bug. Steps I tried:

1) Install master on the device
2) Open adb logcat
3) Open "Usage"
4) Switch to Accented English
5) Open "Usage" again

I currently see a few GeckoConsole errors, but not the one you mentioned:

E/GeckoConsole(19119): Content JS WARN at app://costcontrol.gaiamobile.org/js/sim_manager.js:86 in _requestService: SimManager is not ready, waiting for initialized custom event
E/GeckoConsole(19119): Content JS WARN at app://costcontrol.gaiamobile.org/js/sim_manager.js:52 in _onsuccesSlotId: The setting ril.data.defaultServiceId does not exists, using default Slot (0)
E/GeckoConsole(19119): Content JS ERROR at app://costcontrol.gaiamobile.org/js/sim_manager.js:107 in _requestServiceSIMIcc: The slot 0, configured as the data slot, is empty
E/GeckoConsole(19119): Content JS WARN at app://costcontrol.gaiamobile.org/gaia_build_defer_index.js:244 in _errorNoSim: Error when trying to get the ICC, SIM not detected.

Can you provide steps to reproduce for me?

Comment 19

[Marina Rodríguez [:mai]]

Hi Zibi,
I've sent you info about the bug by email,
Regards

Comment 20

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

I think I found the bug

Comment 21

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

Attachment: follow up patch

So, there is one scenario where we have no control over how prototype-less is our AST - when it comes from JSON.

Until we have Object.assign, the easiest way I see for now is just to test if an object has __proto__ and reset it. Alternatively I can return to what we did before and test if the object has hasOwnProperty and use it if it does, but I think it's less gentle workaround.

Stas, what do you think?

Comment 22

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

Attachment: follow up patch

I thought more about it and looked at some jsperf's.

The issue here is that as long as we use our parser/compiler Object.create(null) is beneficial from perf/memory and security standpoint, but once we use JSON.parse we loose that.

There are two loops in our code that loop over AST provided by JSON.parse. One in addAST and another in new Entity where we loop over attributes.

In this patch I switch to use Object.keys() for both.

I also identified why the problem exist: https://github.com/mozilla-b2g/gaia/blob/fb488e21f5236cd955c5f4be47c5ec2ef7333ec0/apps/costcontrol/js/utils/toolkit.js#L34 - I think we may want to file a bug to stop extending Object.prototype there.

Comment 23

[Staś Małolepszy :stas]

Comment on attachment 8454844 [details] [diff] [review]
follow up patch

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

r-, because I don't think we can remove Locale.prototype.addAST from build/l10n.js at this time.  If I'm wrong, please change my r- into r+.

::: bindings/l20n/buildtime.js
@@ -101,5 @@
> -    this.ast = {};
> -  }
> -  for (var id in ast) {
> -    this.ast[id] = ast[id];
> -    this.entries[id] = ast[id];

Is there a reason why you remove this here?  AFAIR, we needed to override this method to add the 'ast' member to Locales created on buildtime.  This, in turn, was done in order to make getDictionary safe;  it can't use locale.entries due to our lazy compilation approach in which members of entries can be raw AST nodes or compiled Entity objects.

::: lib/l20n/compiler.js
@@ +18,5 @@
>      // it's either a hash or it has attrs, or both
> +    var keys = Object.keys(node);
> +
> +    for (var i = 0; i < keys.length; i++) {
> +      var key = keys[i];

I have a slight aesthetic preference towards declaring the value var in the for-loop statement, if the semantics allow it:

  for (var i = 0, key; key = keys[i], i++)

keys are guaranteed to be truthy here, so you should be able to use this safely, if you like.

::: lib/l20n/locale.js
@@ +93,5 @@
> +  var keys = Object.keys(ast);
> +
> +  for (var i = 0; i < keys.length; i++) {
> +    var key = keys[i];
> +    this.entries[key] = ast[key];

Alternatively, you could check for existance of ast.hasOwnProperty, and use two different loops?  Not sure if that would be faster or nicer, though :)

Comment 24

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

(In reply to Staś Małolepszy :stas from comment #23)
> Comment on attachment 8454844 [details] [diff] [review]
> follow up patch
> 
> Review of attachment 8454844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r-, because I don't think we can remove Locale.prototype.addAST from
> build/l10n.js at this time.  If I'm wrong, please change my r- into r+.

No, you're right. It was added to the diff by mistake.

> ::: lib/l20n/locale.js
> @@ +93,5 @@
> > +  var keys = Object.keys(ast);
> > +
> > +  for (var i = 0; i < keys.length; i++) {
> > +    var key = keys[i];
> > +    this.entries[key] = ast[key];
> 
> Alternatively, you could check for existance of ast.hasOwnProperty, and use
> two different loops?  Not sure if that would be faster or nicer, though :)

In cases where there are more than one solution, but only one works in all scenarios, while the other requires if's, I have a strong aesthetic preference to go for the former :)

I'll attach an updated patch in a few.

Comment 25

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

Attachment: follow up patch, v2

Comment 26

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf] from comment #24)

> In cases where there are more than one solution, but only one works in all
> scenarios, while the other requires if's, I have a strong aesthetic
> preference to go for the former :)

Your training is now complete, my padawan of aesthetics :)

Comment 27

[Staś Małolepszy :stas]

Comment on attachment 8455359 [details] [diff] [review]
follow up patch, v2

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

r=me.

Comment 28

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

(In reply to Staś Małolepszy :stas from comment #26)
> (In reply to Zibi Braniecki [:gandalf] from comment #24)
> 
> > In cases where there are more than one solution, but only one works in all
> > scenarios, while the other requires if's, I have a strong aesthetic
> > preference to go for the former :)
> 
> Your training is now complete, my padawan of aesthetics :)

Hahaha! :) Osu!

Commit: https://github.com/mozilla-b2g/gaia/commit/deb82c903ca85ae47eea0e35543241f19667940a

L20n: https://github.com/l20n/l20n.js/commit/1b188bb52629611b126def0c36f2cdbc0061dec0