Issue 468713002: JavaScript bindings for Mojo message validation

Issue 468713002: JavaScript bindings for Mojo message validation (Closed)

Created:
6 years, 4 months ago by hansmuller

Modified:
6 years, 4 months ago

Reviewers:
Tom Sepez, mpcomplete, yzshen1, Matt Perry

CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Project:
chromium

Visibility:
Public.

More Reviews

Description

JavaScript bindings for Mojo message validation For each interface Foo, the Mojo JavaScript bindings now generate a pair of functions: |validateFooRequest(validator)| and |validateFooResponse(messageValidator)|. For each Mojo struct type, an additional |validate(validator)| method is generated. All of the validate methods return a validationError; validationError.NONE if the message is valid or not recognized. The JS validator class has been extended to validate structs, arrays, and handles. The generated methods are unit tested, in the same way as their C++ counterparts, by reading test messages and expected results from /mojo/public/interfaces/bindings/tests/data/validation/. Currently the two pointer overflow test cases are skipped because they depend on reading 64 uints, which the test message file parser can't handle at the moment. JS doesn't support 64 bit integers. 53 bits is the limit for integer Numbers. The message router still only checks the validity of incoming message headers. I'd like to integrate the router with the new validation code in a separate commit. BUG=386808 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290104

Patch Set 1 #

Patch Set 2 : Merged #

Patch Set 3 : Removed a console import #

Total comments: 14

Patch Set 4 : Simplified the interface definition template #

Total comments: 15

Patch Set 5 : Updates to the unit test, bindings generator, and codec.js #

Patch Set 6 : Binding generator simplification for Arrays #

Total comments: 33

Patch Set 7 : Updates per yzshen's second round of feedback #

Total comments: 1

Patch Set 8 : Restored a blank link #

Patch Set 9 : Skip unexpected null tests" #

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+352 lines, -46 lines)			Patch
M	mojo/public/js/bindings/codec.js	View	1 2 3 4	6 chunks	+10 lines, -3 lines	0 comments	Download
M	mojo/public/js/bindings/router.js	View		1 chunk	+1 line, -1 line	0 comments	Download
M	mojo/public/js/bindings/validation_unittests.js	View	1 2 3 4 5 6 7 8	5 chunks	+54 lines, -19 lines	0 comments	Download
M	mojo/public/js/bindings/validator.js	View	1 2 3 4 5 6	5 chunks	+150 lines, -6 lines	0 comments	Download
M	mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl	View	1 2 3 4 5 6	1 chunk	+49 lines, -0 lines	0 comments	Download
M	mojo/public/tools/bindings/generators/js_templates/module.js.tmpl	View		2 chunks	+4 lines, -1 line	0 comments	Download
M	mojo/public/tools/bindings/generators/js_templates/struct_definition.tmpl	View	1 2 3 4 5 6	1 chunk	+30 lines, -0 lines	0 comments	Download
M	mojo/public/tools/bindings/generators/mojom_cpp_generator.py	View	1 2 3 4 5 6 7 8	2 chunks	+1 line, -7 lines	0 comments	Download
M	mojo/public/tools/bindings/generators/mojom_js_generator.py	View	1 2 3 4 5 6	7 chunks	+45 lines, -9 lines	0 comments	Download
M	mojo/public/tools/bindings/pylib/mojom/generate/module.py	View	1 2 3 4 5 6 7	1 chunk	+8 lines, -0 lines	0 comments	Download

Messages

Total messages: 19 (0 generated)

Expand Messages | Collapse Messages

Matt Perry

https://codereview.chromium.org/468713002/diff/40001/mojo/public/js/bindings/validator.js File mojo/public/js/bindings/validator.js (right): https://codereview.chromium.org/468713002/diff/40001/mojo/public/js/bindings/validator.js#newcode223 mojo/public/js/bindings/validator.js:223: var elementSize = codec.PointerTo.prototype.encodedSize; I'm confused about this one. ...

6 years, 4 months ago (2014-08-13 19:31:01 UTC) #2

hansmuller

Thanks for the review! https://codereview.chromium.org/468713002/diff/40001/mojo/public/js/bindings/validator.js File mojo/public/js/bindings/validator.js (right): https://codereview.chromium.org/468713002/diff/40001/mojo/public/js/bindings/validator.js#newcode223 mojo/public/js/bindings/validator.js:223: var elementSize = codec.PointerTo.prototype.encodedSize; On ...

6 years, 4 months ago (2014-08-13 21:23:41 UTC) #3

Thanks for the review!

https://codereview.chromium.org/468713002/diff/40001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/468713002/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:223: var elementSize =
codec.PointerTo.prototype.encodedSize;
On 2014/08/13 19:31:01, Matt Perry wrote:
> I'm confused about this one. How do you know the array contains pointers? What
> about int[] or bool[]? Am I misreading this?

I apologize, this is a little confusing. We're validating an array's elements
and we know that the elements are PointerTo(ArrayOf(Foo)). However CodecType()
(from
mojo/public/tools/bindings/generators/mojom_js_generator.py) just reports the
array's element type as ArrayOf(Foo); in this case the PointerTo() is implicit.

So what we've got is an array of pointers to arrays, which is why elementSize is
essentially 8.

https://codereview.chromium.org/468713002/diff/40001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/468713002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:113:
function validate{{interface.name}}Request(messageValidator) {
On 2014/08/13 19:31:01, Matt Perry wrote:
> I wonder if we should add an {{interface.name}} object, with validateRequest
and
> validateResponse. We could also move constants and enums there.

That seems reasonable. Currently we put interface scoped constant/enums in both
the Proxy and Stub classes. Are you suggesting that we also put them in an
{{interface.name}} object?

https://codereview.chromium.org/468713002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:114:
{%- if interface.methods %}
On 2014/08/13 19:31:01, Matt Perry wrote:
> nit: this would be slightly clearer to me if you inverted the if expression,
> since the else block is so much shorter.
Good point, I'll swap them.

https://codereview.chromium.org/468713002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:124:
if (message.expectsResponse())
On 2014/08/13 19:31:01, Matt Perry wrote:
> is it not an error if the message expects a response but has no response
params?
> or vice versa?
I believe that the previous clause covers the first case (no response parameters
defined, but the message expects a response). And this clause covers the other
case (response parameters defined but the message doesn't expect a response). 

As I understand it, we don't generate a validation error in either case since
the message will be ignored by the router anyway. I believe the logic for the
C++ case is that same.

https://codereview.chromium.org/468713002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:139:
{%- if interface|has_callbacks %}
On 2014/08/13 19:31:01, Matt Perry wrote:
> ditto line 114

Done.

Matt Perry

6 years, 4 months ago (2014-08-13 21:30:44 UTC) #4

https://codereview.chromium.org/468713002/diff/40001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/468713002/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:223: var elementSize =
codec.PointerTo.prototype.encodedSize;
On 2014/08/13 21:23:40, hansmuller wrote:
> On 2014/08/13 19:31:01, Matt Perry wrote:
> > I'm confused about this one. How do you know the array contains pointers?
What
> > about int[] or bool[]? Am I misreading this?
> 
> I apologize, this is a little confusing. We're validating an array's elements
> and we know that the elements are PointerTo(ArrayOf(Foo)). However CodecType()
> (from
> mojo/public/tools/bindings/generators/mojom_js_generator.py) just reports the
> array's element type as ArrayOf(Foo); in this case the PointerTo() is
implicit.
> 
> So what we've got is an array of pointers to arrays, which is why elementSize
is
> essentially 8.

Just to make sure I understand you correctly, you're saying that this function
is only called in the case of an array of arrays?

https://codereview.chromium.org/468713002/diff/40001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/468713002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:113:
function validate{{interface.name}}Request(messageValidator) {
On 2014/08/13 21:23:40, hansmuller wrote:
> On 2014/08/13 19:31:01, Matt Perry wrote:
> > I wonder if we should add an {{interface.name}} object, with validateRequest
> and
> > validateResponse. We could also move constants and enums there.
> 
> That seems reasonable. Currently we put interface scoped constant/enums in
both
> the Proxy and Stub classes. Are you suggesting that we also put them in an
> {{interface.name}} object?

That's right. (Though maybe they need to be on the Proxy/Stub objects for other
reasons?)

hansmuller

6 years, 4 months ago (2014-08-13 21:53:21 UTC) #5

https://codereview.chromium.org/468713002/diff/40001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/468713002/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:223: var elementSize =
codec.PointerTo.prototype.encodedSize;
On 2014/08/13 21:30:44, Matt Perry wrote:
> On 2014/08/13 21:23:40, hansmuller wrote:
> > On 2014/08/13 19:31:01, Matt Perry wrote:
> > > I'm confused about this one. How do you know the array contains pointers?
> What
> > > about int[] or bool[]? Am I misreading this?
> > 
> > I apologize, this is a little confusing. We're validating an array's
elements
> > and we know that the elements are PointerTo(ArrayOf(Foo)). However
CodecType()
> > (from
> > mojo/public/tools/bindings/generators/mojom_js_generator.py) just reports
the
> > array's element type as ArrayOf(Foo); in this case the PointerTo() is
> implicit.
> > 
> > So what we've got is an array of pointers to arrays, which is why
elementSize
> is
> > essentially 8.
> 
> Just to make sure I understand you correctly, you're saying that this function
> is only called in the case of an array of arrays?

Yes. We only validate array elements if they're handles, pointers to arrays, or
pointers to structs.

https://codereview.chromium.org/468713002/diff/40001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/468713002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:113:
function validate{{interface.name}}Request(messageValidator) {
On 2014/08/13 21:30:44, Matt Perry wrote:
> On 2014/08/13 21:23:40, hansmuller wrote:
> > On 2014/08/13 19:31:01, Matt Perry wrote:
> > > I wonder if we should add an {{interface.name}} object, with
validateRequest
> > and
> > > validateResponse. We could also move constants and enums there.
> > 
> > That seems reasonable. Currently we put interface scoped constant/enums in
> both
> > the Proxy and Stub classes. Are you suggesting that we also put them in an
> > {{interface.name}} object?
> 
> That's right. (Though maybe they need to be on the Proxy/Stub objects for
other
> reasons?)

I assume so however I haven't seen a JS use case. Applications wouldn't need
access to the validate methods but they would need to use the Proxy/Stub
classes. So maybe leaving things they way they are would be OK.

yzshen1

first part of comments. https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/validation_unittests.js File mojo/public/js/bindings/validation_unittests.js (right): https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/validation_unittests.js#newcode204 mojo/public/js/bindings/validation_unittests.js:204: if (testFiles[i].indexOf("overflow") != -1) It ...

6 years, 4 months ago (2014-08-14 00:06:32 UTC) #6

first part of comments.

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validation_unittests.js (right):

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:204: if
(testFiles[i].indexOf("overflow") != -1)
It may be nice to output a warning with the file name saying that the test is
skipped.

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:209: var message =  new
codec.Message(testMessage.buffer, handles);
extra space after =

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:212: var err =
messageValidator.validateMessageHeader();
Something to think about:
It seems a little inconsistent:
messageValidator.validateMessageHeader() and
XXX.validateXXXXRequest(messageValidator).

Maybe we could have a ValidationContext object, and then do:
validateMessageHeader(validationContext)
XXX.validateXXXRequest(validationContext)
?

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:218:
expect(actualResult).toEqual(expectedResult);
It seems nice if we print out the file name on failure.

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:225:
testInterface.validateConformanceTestInterfaceResponse
All the conformance test cases are for ConformanceTestInterface*Request*.

It is incorrect to include the Response validator here.

(And the goal is to test our validation logic, not the ConformanceTestInterface
itself. We don't have to test its both Validate functions. Besides, the Response
validator is empty anyway.)

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:230:
testMessageValidation("integration_", [
The goal of the integration tests is to make sure validators are hooked up with
the router correctly. This test is not doing that. Maybe we can do the
integration tests in the next CL, when the router actually uses the payload
validators?

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:231:
testInterface.validateIntegrationTestInterface1Request,
The tests are designed for a message pipe endpoint receiving Interface1 request
and Interface2 response.

It doesn't make sense to add all the four payload validators.

hansmuller

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/validation_unittests.js File mojo/public/js/bindings/validation_unittests.js (right): https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/validation_unittests.js#newcode204 mojo/public/js/bindings/validation_unittests.js:204: if (testFiles[i].indexOf("overflow") != -1) On 2014/08/14 00:06:32, yzshen1 wrote: ...

6 years, 4 months ago (2014-08-14 00:57:57 UTC) #7

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validation_unittests.js (right):

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:204: if
(testFiles[i].indexOf("overflow") != -1)
On 2014/08/14 00:06:32, yzshen1 wrote:
> It may be nice to output a warning with the file name saying that the test is
> skipped.
Good point, I've done that.

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:209: var message =  new
codec.Message(testMessage.buffer, handles);
On 2014/08/14 00:06:32, yzshen1 wrote:
> extra space after =

Done.

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:212: var err =
messageValidator.validateMessageHeader();
On 2014/08/14 00:06:32, yzshen1 wrote:
> Something to think about:
> It seems a little inconsistent:
> messageValidator.validateMessageHeader() and
> XXX.validateXXXXRequest(messageValidator).
> 
> Maybe we could have a ValidationContext object, and then do:
> validateMessageHeader(validationContext)
> XXX.validateXXXRequest(validationContext)
> ?
> 

Acknowledged.

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:218:
expect(actualResult).toEqual(expectedResult);
On 2014/08/14 00:06:32, yzshen1 wrote:
> It seems nice if we print out the file name on failure.

I agree. The simple JS expect framework doesn't appear to provide a way to
inject a failure message, so I've added an extra (trivial) test as well.

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:225:
testInterface.validateConformanceTestInterfaceResponse
On 2014/08/14 00:06:32, yzshen1 wrote:
> All the conformance test cases are for ConformanceTestInterface*Request*.
> 
> It is incorrect to include the Response validator here.
> 
> (And the goal is to test our validation logic, not the
ConformanceTestInterface
> itself. We don't have to test its both Validate functions. Besides, the
Response
> validator is empty anyway.)
> 

Done.

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validation_unittests.js:231:
testInterface.validateIntegrationTestInterface1Request,
On 2014/08/14 00:06:32, yzshen1 wrote:
> The tests are designed for a message pipe endpoint receiving Interface1
request
> and Interface2 response.
> 
> It doesn't make sense to add all the four payload validators.

I hadn't completely understood the scope of message names, thanks for clearing
that up.  I've just included the two filter methods you suggested.

yzshen1

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/validator.js File mojo/public/js/bindings/validator.js (right): https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/validator.js#newcode71 mojo/public/js/bindings/validator.js:71: this.handleIndex = index + 1; // safe because handle ...

6 years, 4 months ago (2014-08-14 18:02:22 UTC) #8

chromium-reviews

And +tsepez for security review. Thanks Tom! On Thu, Aug 14, 2014 at 11:02 AM, ...

6 years, 4 months ago (2014-08-14 18:06:50 UTC) #9

And +tsepez for security review.

Thanks Tom!


On Thu, Aug 14, 2014 at 11:02 AM, <yzshen@chromium.org> wrote:

>
> https://codereview.chromium.org/468713002/diff/60001/mojo/
> public/js/bindings/validator.js
> File mojo/public/js/bindings/validator.js (right):
>
> https://codereview.chromium.org/468713002/diff/60001/mojo/
> public/js/bindings/validator.js#newcode71
> mojo/public/js/bindings/validator.js:71: this.handleIndex = index + 1;
> // safe because handle indices are uint32
> - Two spaces before //
> - Please use complete sentence, with capital initial and trailing '.'.
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/js/bindings/validator.js
> File mojo/public/js/bindings/validator.js (right):
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/js/bindings/validator.js#newcode103
> mojo/public/js/bindings/validator.js:103: var err =
> this.validateStructHeader(0, codec.kStructHeaderSize, 2);
> Should be codec.kMessageHeaderSize?
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/js/bindings/validator.js#newcode163
> mojo/public/js/bindings/validator.js:163: return
> this.validateArrayPointer(offset, codec.Uint8.encodedSize, 0);
> Is it intentional to omit the last parameter?
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/js/bindings/validator.js#newcode170
> mojo/public/js/bindings/validator.js:170: function (offset, elementSize,
> elementCount, elementType) {
> nit: it seems better to name it expectedElementCount.
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/js/bindings/validator.js#newcode203
> mojo/public/js/bindings/validator.js:203:
> Is validating string elements covered by this if block?
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/js/bindings/validator.js#newcode227
> mojo/public/js/bindings/validator.js:227: elementOffset,
> elementClass.encodedSize, 0, elementClass);
> The last argument: should it be element type of the element arrays?
> (I.e., one level deeper)
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/
> interface_definition.tmpl
> File
> mojo/public/tools/bindings/generators/js_templates/
> interface_definition.tmpl
> (right):
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/
> interface_definition.tmpl#newcode120
> mojo/public/tools/bindings/generators/js_templates/
> interface_definition.tmpl:120:
> {%- for method in interface.methods %}
> Please consider the following way of indentation:
>
> {%- if...
> {%-   for...
> {%-   endfor...
> {%- endif
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/
> interface_definition.tmpl#newcode125
> mojo/public/tools/bindings/generators/js_templates/
> interface_definition.tmpl:125:
> {%-     else %}
> Inconsistent indent, this else corresponds to line 122.
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/
> interface_definition.tmpl#newcode134
> mojo/public/tools/bindings/generators/js_templates/
> interface_definition.tmpl:134:
> return paramsClass.validate(messageValidator, codec.kMessageHeaderSize);
> This is incorrect:
> The message header may contain fields that our version doesn't
> understand. The offset should be the num_bytes field of the message
> header struct.
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/
> interface_definition.tmpl#newcode156
> mojo/public/tools/bindings/generators/js_templates/
> interface_definition.tmpl:156:
> return paramsClass.validate(messageValidator,
> codec.kMessageWithRequestIDHeaderSize);
> - 80 chars?
> - The offset is incorrect, please see line 134.
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/module.js.tmpl
> File mojo/public/tools/bindings/generators/js_templates/module.js.tmpl
> (right):
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/
> module.js.tmpl#newcode52
> mojo/public/tools/bindings/generators/js_templates/module.js.tmpl:52:
> exports.validate{{interface.name}}Response =
> validate{{interface.name}}Response;
> 80 chars?
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/struct_definition.tmpl
> File
> mojo/public/tools/bindings/generators/js_templates/struct_definition.tmpl
> (right):
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/struct_definition.tmpl#
> newcode29
> mojo/public/tools/bindings/generators/js_templates/
> struct_definition.tmpl:29:
> err = messageValidator.validateStructHeader(offset,
> {{struct.name}}.encodedSize, {{struct.packed.packed_fields|length}});
> 80 chars, please. (here and elsewhere)
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/struct_definition.tmpl#
> newcode33
> mojo/public/tools/bindings/generators/js_templates/
> struct_definition.tmpl:33:
> {%-   set fieldName = packed_field.field.name %}
> nit: I think the naming convention is field_name (this is not part of
> the JS code)
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/js_templates/struct_definition.tmpl#
> newcode37
> mojo/public/tools/bindings/generators/js_templates/
> struct_definition.tmpl:37:
> if (err !== validator.validationError.NONE)
> I think line 37-38 can be moved outside of the {%if%} block.
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/mojom_js_generator.py
> File mojo/public/tools/bindings/generators/mojom_js_generator.py
> (right):
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/mojom_js_generator.py#newcode49
> mojo/public/tools/bindings/generators/mojom_js_generator.py:49: if
> mojom.IsArrayKind(field.kind):
> I think this should also be IsAnyArrayKind right?
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/mojom_js_generator.py#newcode183
> mojo/public/tools/bindings/generators/mojom_js_generator.py:183: def
> HasCallbacks(interface):
> Shall we move this method into generator.py so that both
> mojom_cpp_generator and mojom_js_generate can use it?
>
> https://codereview.chromium.org/468713002/diff/100001/
> mojo/public/tools/bindings/generators/mojom_js_generator.py#newcode189
> mojo/public/tools/bindings/generators/mojom_js_generator.py:189: def
> IsArrayPointerField(field):
> [optional] If you want, you could consider directly export
> mojom.IsXXXKind() in js_filters. The consequence is that you need to do
> field.kind|is_any_array_kind, for example. But you don't need to define
> these one-line functions.
>
> https://codereview.chromium.org/468713002/
>



-- 
Best regards,
Yuzhu Shen.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

hansmuller

I believe I've addressed all of Yuzhu's feedback. https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/validator.js File mojo/public/js/bindings/validator.js (right): https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/validator.js#newcode71 mojo/public/js/bindings/validator.js:71: this.handleIndex ...

6 years, 4 months ago (2014-08-14 23:51:46 UTC) #10

I believe I've addressed all of Yuzhu's feedback.

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/468713002/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:71: this.handleIndex = index + 1; // safe
because handle indices are uint32
On 2014/08/14 18:02:21, yzshen1 wrote:
> - Two spaces before //
> - Please use complete sentence, with capital initial and trailing '.'.

Done.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/js/bindings...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/468713002/diff/100001/mojo/public/js/bindings...
mojo/public/js/bindings/validator.js:103: var err = this.validateStructHeader(0,
codec.kStructHeaderSize, 2);
On 2014/08/14 18:02:21, yzshen1 wrote:
> Should be codec.kMessageHeaderSize?
Yes it should.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/js/bindings...
mojo/public/js/bindings/validator.js:163: return
this.validateArrayPointer(offset, codec.Uint8.encodedSize, 0);
On 2014/08/14 18:02:21, yzshen1 wrote:
> Is it intentional to omit the last parameter?
No, that's my mistake. It didn't matter in this case, but it should have been
there.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/js/bindings...
mojo/public/js/bindings/validator.js:170: function (offset, elementSize,
elementCount, elementType) {
On 2014/08/14 18:02:21, yzshen1 wrote:
> nit: it seems better to name it expectedElementCount.
Yes.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/js/bindings...
mojo/public/js/bindings/validator.js:203: 
On 2014/08/14 18:02:21, yzshen1 wrote:
> Is validating string elements covered by this if block?
No, but it should be. I will add that.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/js/bindings...
mojo/public/js/bindings/validator.js:227: elementOffset,
elementClass.encodedSize, 0, elementClass);
On 2014/08/14 18:02:21, yzshen1 wrote:
> The last argument: should it be element type of the element arrays? (I.e., one
> level deeper)

I believe this is correct because the caller is already providing the element
type of the element arrays - elementType.cls does that. I think the codec
ArrayOf type would be somewhat less confusing if its "cls" field was called
elementClass.

I've added a comment above validateArrayElements() that explains what the
elementClass parameter means.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:120:
{%- for method in interface.methods %}
On 2014/08/14 18:02:21, yzshen1 wrote:
> Please consider the following way of indentation:
> 
> {%- if...
> {%-   for...
> {%-   endfor...
> {%- endif

Done.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:125:
{%-     else %}
On 2014/08/14 18:02:21, yzshen1 wrote:
> Inconsistent indent, this else corresponds to line 122.

Done.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:134:
return paramsClass.validate(messageValidator, codec.kMessageHeaderSize);
On 2014/08/14 18:02:21, yzshen1 wrote:
> This is incorrect:
> The message header may contain fields that our version doesn't understand. The
> offset should be the num_bytes field of the message header struct.

Done.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:156:
return paramsClass.validate(messageValidator,
codec.kMessageWithRequestIDHeaderSize);
On 2014/08/14 18:02:21, yzshen1 wrote:
> - 80 chars?
The existing templates don't strictly adhere to the 80 character limit.

> - The offset is incorrect, please see line 134.

Done.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/js_templates/module.js.tmpl (right):

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/module.js.tmpl:52:
exports.validate{{interface.name}}Response = validate{{interface.name}}Response;
On 2014/08/14 18:02:22, yzshen1 wrote:
> 80 chars?

The existing templates don't strictly adhere to 80 characters and generating a
line break here will not make the generated code easier to read.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/js_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/struct_definition.tmpl:29:
err = messageValidator.validateStructHeader(offset, {{struct.name}}.encodedSize,
{{struct.packed.packed_fields|length}});
On 2014/08/14 18:02:22, yzshen1 wrote:
> 80 chars, please. (here and elsewhere)
See earlier comments

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/struct_definition.tmpl:33:
{%-   set fieldName = packed_field.field.name %}
On 2014/08/14 18:02:22, yzshen1 wrote:
> nit: I think the naming convention is field_name (this is not part of the JS
> code)

Done.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/struct_definition.tmpl:37: if
(err !== validator.validationError.NONE)
On 2014/08/14 18:02:22, yzshen1 wrote:
> I think line 37-38 can be moved outside of the {%if%} block.
Yes! That makes this template much easier to read.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_js_generator.py (right):

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_js_generator.py:49: if
mojom.IsArrayKind(field.kind):
On 2014/08/14 18:02:22, yzshen1 wrote:
> I think this should also be IsAnyArrayKind right?
Yes. I've changed it.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_js_generator.py:183: def
HasCallbacks(interface):
On 2014/08/14 18:02:22, yzshen1 wrote:
> Shall we move this method into generator.py so that both mojom_cpp_generator
and
> mojom_js_generate can use it?

Yes. I moved it into module.py, since the Interface class is defined there. The
C++ bindings generator now uses it as well.

https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_js_generator.py:189: def
IsArrayPointerField(field):
On 2014/08/14 18:02:22, yzshen1 wrote:
> [optional] If you want, you could consider directly export mojom.IsXXXKind()
in
> js_filters. The consequence is that you need to do
field.kind|is_any_array_kind,
> for example. But you don't need to define these one-line functions.

That's a reasonable suggestion however if you don't mind I will leave  this
particular detail alone for the moment. I liked the fact that the both the test,
like is_struct_pointer_field, and the corresponding method parameter generate,
like validate_struct_params, both just applied to the field object.

yzshen1

LGTM, Thanks! (Please wait for others' LGs.) https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindings/generators/mojom_js_generator.py File mojo/public/tools/bindings/generators/mojom_js_generator.py (right): https://codereview.chromium.org/468713002/diff/100001/mojo/public/tools/bindings/generators/mojom_js_generator.py#newcode189 mojo/public/tools/bindings/generators/mojom_js_generator.py:189: def IsArrayPointerField(field): ...

6 years, 4 months ago (2014-08-15 16:26:37 UTC) #11

commit-bot: I haz the power

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

6 years, 4 months ago (2014-08-15 21:43:01 UTC) #15

commit-bot: I haz the power

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

6 years, 4 months ago (2014-08-15 21:55:33 UTC) #18

commit-bot: I haz the power

6 years, 4 months ago (2014-08-16 06:09:28 UTC) #19

Message was sent while issue was closed.

Committed patchset #9 (160001) as 290104

Expand Messages | Collapse Messages