Validate incoming JS Message Headers: test message parser

Validate incoming JS Message Headers: test message parser

This is the first part of the "Validate incoming JS Message Headers" CL - https://codereview.chromium.org/406993002/

It's just the test message file parser and its sanity check.

TBR=jochen@chromium.org

BUG=395801
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285632

[hansmuller, 2014-07-22 16:23:10]

First subset of the https://codereview.chromium.org/406993002/ CL. I've asked to
defer two changes Adam asked for, please see the description.

[abarth-chromium, 2014-07-22 17:04:25]

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
File mojo/bindings/js/message_file_parser.js (right):

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Is this file used in production or for testing?  If it's just for testing, it
should be in a "test" directory.

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:8: ], function (console, expect) {
You shouldn't need to import either of these modules.

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:18: })();
Can you refactor the code so we don't have to copy/paste this code?

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:24: }
This is also duplicated from codec.js

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:540: }
This word should be done in validation_unittests.js

[abarth-chromium, 2014-07-22 17:05:36]

I don't think we should make those changes in a subsequent CL.  Copy/pasting
code isn't good engineering.

[hansmuller, 2014-07-22 17:30:32]

On 2014/07/22 17:05:36, abarth wrote:
> I don't think we should make those changes in a subsequent CL.  Copy/pasting
> code isn't good engineering.

Right. I will do the refactoring now. I wish I had git skills.

[Matt Perry, 2014-07-22 20:38:28]

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
File mojo/bindings/js/message_file_parser.js (right):

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
2014

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:172: // The text for Each whitespace
delimited binary data "item" loaded
each*

[hansmuller, 2014-07-23 00:14:07]

I've factored a common Buffer class from codec and the message_file_parser
MessageData class.

I've moved all of the JS tests from mojo/bindings/js to
mojo/public/js/bindings/tests. I created a DEPS file in the latter enabled the
includes the code already depended on. I hope I got that right.

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
File mojo/bindings/js/message_file_parser.js (right):

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2014/07/22 20:38:28, Matt Perry wrote:
> 2014

Done.

https://codereview.chromium.org/411553003/diff/1/mojo/bindings/js/message_fil...
mojo/bindings/js/message_file_parser.js:172: // The text for Each whitespace
delimited binary data "item" loaded
On 2014/07/22 20:38:28, Matt Perry wrote:
> each*

Done.

[Matt Perry, 2014-07-23 00:39:39]

handful of nits

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/buffer.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:168: return Buffer;
I think you should return a dictionary object that contains a Buffer value,
rather than the Buffer object directly.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/message_file_parser.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/message_file_parser.js:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/validation_unittests.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_unittests.js:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2014

[abarth-chromium, 2014-07-23 02:13:52]

LGTM with the comments below addressed.

Please make sure mpcomplete is happy too.  :)

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/buffer.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:5: define([], function () {
I think you can just omit [] when it's empty.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:168: return Buffer;
+1

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/codec.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:8: ], function(unicode, Buffer) {
s/Buffer/buffer/

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/codec_unittests.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/codec_unittests.js:5: define([
Sorry, I was unclear.  I think it's fine for the _unittests.js files to be
adjacent to the files they test.  That's the normal convention in Chrome for
_unittests.cc files.  What I meant is that supporting code for unit tests (e.g.,
JavaScript modules or C++ code) should be in the test subdirectory.  For
example, moving run_js_tests.cc into this directory is great.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/message_file_parser.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/message_file_parser.js:7: ], function (Buffer) {
No space between function and (

[yzshen1, 2014-07-23 07:26:55]

First part of comments. I will send you comments for the parser (if any) soon.

Thanks for working on this!

https://codereview.chromium.org/411553003/diff/20001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/mojo.gyp#newcode748
mojo/mojo.gyp:748: 'target_name': 'mojo_js_unittests',
shall we move this target into mojo_public_tests.gypi?

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/buffer.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:14: this.byteLength =
this.arrayBuffer.byteLength;
Maybe we could make a getter which simply reads the byteLength of arrayBuffer,
so that we don't have to worry about keeping them in sync (which is error
prone)?

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:18: var kHostIsLittleEndian = (function () {
(Not familiar with JS style.)

Maybe we should order the code so that the helpers are at the top and Buffer
constructor/methods stay together?

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:112: return getUint64(this.dataView, offset,
kHostIsLittleEndian);
Does it make sense to merge the impl of getUint64() into this method? It seems
that function is only used once.

(And do similar things for [gs]et(Ui|I)nt64)

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/connector.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/connector.js:10: ], function(Buffer, codec, core,
support) {
buffer?

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/message_file_parser.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/message_file_parser.js:9: // Files and Lines
represent the raw text from a ".data" file.
This is not a very useful comment. You could probably point to the comments in
mojo/public/cpp/bindings/tests/validation_test_input_parser.h which explains the
format of validation test input.

Besides, message_file_parser is not very accurate: it doesn't have to be in a
"file". Maybe it is more clear and consistent to call it
validation_test_input_parser.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/run_js_tests.cc (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/run_js_tests.cc:50: static bool
MojoBindingFilesExist() {
I think this whole check is not necessary anymore.
(You could remove some includes and DEPS.)

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/validation_unittests.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_unittests.js:11:
expect(parser.checkMessageFileParser()).toBeNull();
Why putting the testing code in message_file_parser.js instead of here?

[hansmuller, 2014-07-23 18:06:36]

I've made all of the suggested changes.

https://codereview.chromium.org/411553003/diff/20001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/mojo.gyp#newcode748
mojo/mojo.gyp:748: 'target_name': 'mojo_js_unittests',
On 2014/07/23 07:26:55, yzshen1 wrote:
> shall we move this target into mojo_public_tests.gypi?

Done.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/buffer.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:5: define([], function () {
On 2014/07/23 02:13:51, abarth wrote:
> I think you can just omit [] when it's empty.

Done.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:14: this.byteLength =
this.arrayBuffer.byteLength;
On 2014/07/23 07:26:55, yzshen1 wrote:
> Maybe we could make a getter which simply reads the byteLength of arrayBuffer,
> so that we don't have to worry about keeping them in sync (which is error
> prone)?
Good point.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:18: var kHostIsLittleEndian = (function () {
On 2014/07/23 07:26:55, yzshen1 wrote:
> (Not familiar with JS style.)
> 
> Maybe we should order the code so that the helpers are at the top and Buffer
> constructor/methods stay together?

OK.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:112: return getUint64(this.dataView, offset,
kHostIsLittleEndian);
On 2014/07/23 07:26:55, yzshen1 wrote:
> Does it make sense to merge the impl of getUint64() into this method? It seems
> that function is only used once.
> 
> (And do similar things for [gs]et(Ui|I)nt64)

Yes, that makes sense.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/buffer.js:168: return Buffer;
On 2014/07/23 00:39:38, Matt Perry wrote:
> I think you should return a dictionary object that contains a Buffer value,
> rather than the Buffer object directly.

I didn't want user s of this module to have to write |new buffer.Buffer(...)| or
|var Buffer = buffer.Buffer; new Buffer(...)|. That said, I'll yield to the
consensus on this point and return a dictionary.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/codec.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:8: ], function(unicode, Buffer) {
On 2014/07/23 02:13:51, abarth wrote:
> s/Buffer/buffer/

Done.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/connector.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/connector.js:10: ], function(Buffer, codec, core,
support) {
On 2014/07/23 07:26:55, yzshen1 wrote:
> buffer?

Done.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/codec_unittests.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/codec_unittests.js:5: define([
On 2014/07/23 02:13:52, abarth wrote:
> Sorry, I was unclear.  I think it's fine for the _unittests.js files to be
> adjacent to the files they test.  That's the normal convention in Chrome for
> _unittests.cc files.  What I meant is that supporting code for unit tests
(e.g.,
> JavaScript modules or C++ code) should be in the test subdirectory.  For
> example, moving run_js_tests.cc into this directory is great.

OK, I've moved the unit tests up the parent directory, where the corresponding
implementation files are.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/message_file_parser.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/message_file_parser.js:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/07/23 00:39:38, Matt Perry wrote:
> 2014

Sorry about that, I think I fixed the date problems on wrong branch.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/message_file_parser.js:7: ], function (Buffer) {
On 2014/07/23 02:13:52, abarth wrote:
> No space between function and (

Done.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/message_file_parser.js:9: // Files and Lines
represent the raw text from a ".data" file.
On 2014/07/23 07:26:55, yzshen1 wrote:
> This is not a very useful comment. You could probably point to the comments in
> mojo/public/cpp/bindings/tests/validation_test_input_parser.h which explains
the
> format of validation test input.
> 
> Besides, message_file_parser is not very accurate: it doesn't have to be in a
> "file". Maybe it is more clear and consistent to call it
> validation_test_input_parser.

Done.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/run_js_tests.cc (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/run_js_tests.cc:50: static bool
MojoBindingFilesExist() {
On 2014/07/23 07:26:55, yzshen1 wrote:
> I think this whole check is not necessary anymore.
> (You could remove some includes and DEPS.)

OK. I've removed it from here and from /mojo/apps/js/test/run_apps_js_tests.cc

I've also removed the corresponding includes and updated the DEPS file.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/validation_unittests.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_unittests.js:11:
expect(parser.checkMessageFileParser()).toBeNull();
On 2014/07/23 07:26:55, yzshen1 wrote:
> Why putting the testing code in message_file_parser.js instead of here?

I didn't wanted to export parser internals like File, parseTestMessage(), etc.

The real validation tests will only depend on one additional parser function
(which I've taken out for now, since the original CL has split into parts). The
rest of the parser infrastructure will remain hidden.

[yzshen1, 2014-07-23 19:53:38]

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/run_js_tests.cc (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/run_js_tests.cc:6: #include
"base/files/file_path.h"
nit: this is still needed.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/run_js_tests.cc:50: static bool
MojoBindingFilesExist() {
Thanks! :)

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/validation_unittests.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_unittests.js:11:
expect(parser.checkMessageFileParser()).toBeNull();
I am not sure I understand:
The parser will need to expose a public method (say, Parse) anyway, right?
And I imagine Parse() takes a string which contains input data, and output
succeeded, Buffer, and handle_count.

We could test the parser by calling this Parse() function. Why do we need to
expose the internal of parser?

Do you mean you would like to put the file reading code in the parser? (Because
I saw things like File in the parser code.)
I think it is better to avoid that. File reading is not part of the parsing
functionality.

https://codereview.chromium.org/411553003/diff/60001/mojo/apps/js/test/run_ap...
File mojo/apps/js/test/run_apps_js_tests.cc (left):

https://codereview.chromium.org/411553003/diff/60001/mojo/apps/js/test/run_ap...
mojo/apps/js/test/run_apps_js_tests.cc:6: #include "base/files/file_path.h"
It seems this one is still needed.

https://codereview.chromium.org/411553003/diff/60001/mojo/mojo_public_tests.gypi
File mojo/mojo_public_tests.gypi (right):

https://codereview.chromium.org/411553003/diff/60001/mojo/mojo_public_tests.g...
mojo/mojo_public_tests.gypi:199: 'mojo_run_all_unittests',
Now that you are touching the code, please order the list alphabetically.

[Matt Perry, 2014-07-23 20:26:02]

lgtm

[yzshen1, 2014-07-23 20:31:10]

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/validation_test_input_parser.js (right):

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:35:
File.prototype.isEmpty = function() {
nit: maybe "reachEnd" is more clear. (and similar comment for Line)

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:43: var end =
this.contents.indexOf('\n', start);
Does \r matter here?

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:63: var
itemTypeSizes = {
Shall we name it kItemTypeSizes because it is constant? (But I am not familiar
with JS style guide.)

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:96: // file is
represented by an Item.
Is this line correct?

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:156: case 'u1':
2 more spaces of indent, please.

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:159: this.index +=
1;
We could consider advancing this.index with item.size and moving this line out
of the switch (or even out of addX() methods).

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:319: "expected
length: " + expectedData.byteLength;
wrong indent.

[hansmuller, 2014-07-23 21:43:44]

I've made the requested changes.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/run_js_tests.cc (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/run_js_tests.cc:6: #include
"base/files/file_path.h"
On 2014/07/23 19:53:38, yzshen1 wrote:
> nit: this is still needed.

OK.  I've restored it in run_apps_js_tests.cc as well.

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/validation_unittests.js (right):

https://codereview.chromium.org/411553003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_unittests.js:11:
expect(parser.checkMessageFileParser()).toBeNull();
On 2014/07/23 19:53:38, yzshen1 wrote:
> I am not sure I understand:
> The parser will need to expose a public method (say, Parse) anyway, right?
> And I imagine Parse() takes a string which contains input data, and output
> succeeded, Buffer, and handle_count.

> We could test the parser by calling this Parse() function. Why do we need to
> expose the internal of parser?
> 
> Do you mean you would like to put the file reading code in the parser?
(Because
> I saw things like File in the parser code.)
> I think it is better to avoid that. File reading is not part of the parsing
> functionality.

What I'd written for the original patch was to include
|readTestMessage(filename)| which would read the ".data" and "expected" files
and return an object that contained the parsed message, and the expected result.

I've moved |checkMessageTests()| to validation_unittests.js and will only export
a parser function for an input string.

https://codereview.chromium.org/411553003/diff/60001/mojo/mojo_public_tests.gypi
File mojo/mojo_public_tests.gypi (right):

https://codereview.chromium.org/411553003/diff/60001/mojo/mojo_public_tests.g...
mojo/mojo_public_tests.gypi:199: 'mojo_run_all_unittests',
On 2014/07/23 19:53:38, yzshen1 wrote:
> Now that you are touching the code, please order the list alphabetically.

Done.

[hansmuller, 2014-07-24 00:36:37]

Made the set of changes from the feedback that I'd missed! Sorry about that.

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/validation_test_input_parser.js (right):

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:35:
File.prototype.isEmpty = function() {
On 2014/07/23 20:31:10, yzshen1 wrote:
> nit: maybe "reachEnd" is more clear. (and similar comment for Line)

OK, I used "endReached()".

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:43: var end =
this.contents.indexOf('\n', start);
On 2014/07/23 20:31:10, yzshen1 wrote:
> Does \r matter here?
Not really, since breaking the input up into "lines" is just to make it possible
to report input error line numbers.

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:63: var
itemTypeSizes = {
On 2014/07/23 20:31:10, yzshen1 wrote:
> Shall we name it kItemTypeSizes because it is constant? (But I am not familiar
> with JS style guide.)

According to the style guide I should write:
var ITEM_TYPE_SIZES = {...};

The mojo JS code doesn't appear to consistently follow the style guide on this
point, but I can do so.

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:96: // file is
represented by an Item.
On 2014/07/23 20:31:10, yzshen1 wrote:
> Is this line correct?

That "...file is" was a typo, thanks for pointing it out.

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:156: case 'u1':
On 2014/07/23 20:31:10, yzshen1 wrote:
> 2 more spaces of indent, please.

Done.

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:159: this.index +=
1;
On 2014/07/23 20:31:10, yzshen1 wrote:
> We could consider advancing this.index with item.size and moving this line out
> of the switch (or even out of addX() methods).

Good point. I moved it to addItem().

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:319: "expected
length: " + expectedData.byteLength;
On 2014/07/23 20:31:10, yzshen1 wrote:
> wrong indent.

Done.

[yzshen1, 2014-07-24 01:15:11]

LGTM with two nits.

Thanks for the great work!

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
File mojo/public/js/bindings/tests/validation_test_input_parser.js (right):

https://codereview.chromium.org/411553003/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/tests/validation_test_input_parser.js:43: var end =
this.contents.indexOf('\n', start);
It also matters where to end a comment, right?

On 2014/07/24 00:36:37, hansmuller wrote:
> On 2014/07/23 20:31:10, yzshen1 wrote:
> > Does \r matter here?
> Not really, since breaking the input up into "lines" is just to make it
possible
> to report input error line numbers.

https://codereview.chromium.org/411553003/diff/100001/mojo/public/js/bindings...
File mojo/public/js/bindings/tests/validation_test_input_parser.js (right):

https://codereview.chromium.org/411553003/diff/100001/mojo/public/js/bindings...
mojo/public/js/bindings/tests/validation_test_input_parser.js:95: // The text
for each whitespace delimited binary data "item"
nit: please fill the line as close to 80 chars as possible.

[hansmuller, 2014-07-24 01:36:33]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-07-24 01:40:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/411553003/120001

[commit-bot: I haz the power, 2014-07-24 05:48:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-24 05:52:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-24 05:52:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[hansmuller, 2014-07-24 17:01:37]

Darin - I added mojo/public/js/bindings/tests/DEPS and apparently that requires
your approval.

[hansmuller, 2014-07-24 21:37:51]

Darin, Jochen: I added mojo/public/js/bindings/tests/DEPS which contains the
following. It looks like I need an LGTM from a base and v8 owner. I'd appreciate
it if you'd take a look.

include_rules = [
  "+base",
  "+gin",
  "+v8",
  "+mojo/bindings/js/core.h",
]

[darin (slow to review), 2014-07-25 04:29:35]

On 2014/07/24 21:37:51, hansmuller wrote:
> Darin, Jochen: I added mojo/public/js/bindings/tests/DEPS which contains the
> following. It looks like I need an LGTM from a base and v8 owner. I'd
appreciate
> it if you'd take a look.
> 
> include_rules = [
>   "+base",
>   "+gin",
>   "+v8",
>   "+mojo/bindings/js/core.h",
> ]

LGTM

[hansmuller, 2014-07-25 15:34:31]

Jochen - I've TBR'd you since the JS mojo tests have already depended on V8.

[hansmuller, 2014-07-25 15:34:45]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-07-25 15:35:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/411553003/140001

[commit-bot: I haz the power, 2014-07-25 18:49:02]

Change committed as 285632