Closed
Bug 1022859
Opened 10 years ago
Closed 10 years ago
Factorize parser into a class
Categories
(Firefox OS Graveyard :: Gaia::L10n, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: zbraniecki, Assigned: zbraniecki)
References
Details
Attachments
(3 files, 5 obsolete files)
(deleted),
patch
|
stas
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/x-github-pull-request
|
Details | |
(deleted),
patch
|
stas
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•10 years ago
|
Priority: -- → P4
Assignee | ||
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
Oh, I also moved the AST returned by the parser to be Object.create(null) and in result removed hasOwnProperty checks.
Comment 3•10 years ago
|
||
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?
Attachment #8450418 -
Flags: feedback?(stas) → feedback+
Assignee | ||
Comment 4•10 years ago
|
||
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
Attachment #8450418 -
Attachment is obsolete: true
Attachment #8451861 -
Flags: review?(stas)
Assignee | ||
Comment 5•10 years ago
|
||
Comment 6•10 years ago
|
||
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 :)
Attachment #8451861 -
Flags: review?(stas) → review-
Comment 7•10 years ago
|
||
(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!)
Assignee | ||
Comment 8•10 years ago
|
||
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.
Attachment #8451861 -
Attachment is obsolete: true
Attachment #8452446 -
Flags: review?(stas)
Assignee | ||
Comment 9•10 years ago
|
||
(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•10 years ago
|
||
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.
Attachment #8452446 -
Flags: review?(stas) → review+
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Priority: P4 → P2
Summary: Use L20n's file format instead of quasi-properties → Factorize parser into a class
Assignee | ||
Comment 12•10 years ago
|
||
updated pull request with addressed review comments
Attachment #8451864 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
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
Assignee | ||
Comment 14•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 15•10 years ago
|
||
With this landing, the work on introducing l20n format moves to bug 1027684.
Comment 16•10 years ago
|
||
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
Flags: needinfo?(gandalf)
Assignee | ||
Comment 17•10 years ago
|
||
Marina: feel free to file a bug, I'm about to investigate it.
Flags: needinfo?(gandalf)
Assignee | ||
Comment 18•10 years ago
|
||
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?
Flags: needinfo?(mri)
Comment 19•10 years ago
|
||
Hi Zibi, I've sent you info about the bug by email, Regards
Flags: needinfo?(mri)
Assignee | ||
Comment 20•10 years ago
|
||
I think I found the bug
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 21•10 years ago
|
||
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?
Flags: needinfo?(stas)
Assignee | ||
Comment 22•10 years ago
|
||
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.
Attachment #8454647 -
Attachment is obsolete: true
Attachment #8454844 -
Flags: review?(stas)
Comment 23•10 years ago
|
||
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 :)
Attachment #8454844 -
Flags: review?(stas) → review-
Updated•10 years ago
|
Flags: needinfo?(stas)
Assignee | ||
Comment 24•10 years ago
|
||
(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.
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8454844 -
Attachment is obsolete: true
Attachment #8455359 -
Flags: review?(stas)
Comment 26•10 years ago
|
||
(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•10 years ago
|
||
Comment on attachment 8455359 [details] [diff] [review] follow up patch, v2 Review of attachment 8455359 [details] [diff] [review]: ----------------------------------------------------------------- r=me.
Attachment #8455359 -
Flags: review?(stas) → review+
Assignee | ||
Comment 28•10 years ago
|
||
(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
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•