Mojo JS Bindings: add support for associative arrays (Mojo map type)

Mojo JS Bindings: add support for associative arrays (Mojo map type)

Currently there's only a limited test for the new type. More complete testing to come in a followup patch.

BUG=423017
Committed: https://crrev.com/d1fd54c8da40c9e7c1e7821259c39af947529f77
Cr-Commit-Position: refs/heads/master@{#299992}

[hansmuller, 2014-10-14 20:28:56]

hansmuller@chromium.org changed reviewers:
+ abarth@chromium.org, yzshen@chromium.org

[hansmuller, 2014-10-14 20:30:40]

Adam I'd appreciate if you'd look at the extension I made to expect.js

Yuzhu - Please review the bindings support for Mojo map.

[abarth-chromium, 2014-10-15 04:53:26]

LGTM

https://codereview.chromium.org/654843005/diff/1/mojo/public/js/bindings/code...
File mojo/public/js/bindings/codec.js (right):

https://codereview.chromium.org/654843005/diff/1/mojo/public/js/bindings/code...
mojo/public/js/bindings/codec.js:380: });
for (var [key, value] of val) {
  ...
}

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

Not sure if that works in V8, but it's worth a try.  :)

[yzshen1, 2014-10-15 05:58:02]

I didn't the code for map validation. The key/value arrays are of the same size;
and they should also pass array validation.  What do you think?

https://codereview.chromium.org/654843005/diff/1/mojo/public/interfaces/bindi...
File mojo/public/interfaces/bindings/tests/test_structs.mojom (right):

https://codereview.chromium.org/654843005/diff/1/mojo/public/interfaces/bindi...
mojo/public/interfaces/bindings/tests/test_structs.mojom:111: // Used to verify
that all possible Map key field types can be encoded and
int64 and uint64? :)

https://codereview.chromium.org/654843005/diff/1/mojo/public/js/bindings/code...
File mojo/public/js/bindings/codec.js (right):

https://codereview.chromium.org/654843005/diff/1/mojo/public/js/bindings/code...
mojo/public/js/bindings/codec.js:30: var kMapStructSize = 16;
It doesn't include the struct header itself, right? It seems
kMapStructPayloadSize is more suitable.

https://codereview.chromium.org/654843005/diff/1/mojo/public/js/bindings/code...
mojo/public/js/bindings/codec.js:381: this.writeUint32(kMapStructSize);
num_bytes should also include the header size.

[yzshen1, 2014-10-15 05:58:03]

I didn't the code for map validation. The key/value arrays are of the same size;
and they should also pass array validation.  What do you think?

[qsr, 2014-10-15 13:03:39]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2014-10-15 13:03:40]

https://chromiumcodereview.appspot.com/654843005/diff/1/mojo/public/interface...
File mojo/public/interfaces/bindings/tests/test_structs.mojom (right):

https://chromiumcodereview.appspot.com/654843005/diff/1/mojo/public/interface...
mojo/public/interfaces/bindings/tests/test_structs.mojom:124: map<string,
string> f9;
Do you want to test with structs, arrays and map as values?

https://chromiumcodereview.appspot.com/654843005/diff/1/mojo/public/js/bindin...
File mojo/public/js/bindings/codec.js (right):

https://chromiumcodereview.appspot.com/654843005/diff/1/mojo/public/js/bindin...
mojo/public/js/bindings/codec.js:186: this.skip(4); // numberOfFields
Don't you want to check that the header is the one you expect?

[hansmuller, 2014-10-15 21:45:46]

I'd originally planned to wait until the new conformance tests were ready,
before adding validation support.

I've added support for validating maps and I've incorporated validation into the
unit tests. I'll update the tests again, after the new conformance suite lands.

https://codereview.chromium.org/654843005/diff/1/mojo/public/interfaces/bindi...
File mojo/public/interfaces/bindings/tests/test_structs.mojom (right):

https://codereview.chromium.org/654843005/diff/1/mojo/public/interfaces/bindi...
mojo/public/interfaces/bindings/tests/test_structs.mojom:111: // Used to verify
that all possible Map key field types can be encoded and
On 2014/10/15 05:58:02, yzshen1 wrote:
> int64 and uint64? :)

JS can't really encode 64 bit integers properly but testing 64 bit keys with
smaller values is OK. I will add that.

https://codereview.chromium.org/654843005/diff/1/mojo/public/interfaces/bindi...
mojo/public/interfaces/bindings/tests/test_structs.mojom:124: map<string,
string> f9;
On 2014/10/15 13:03:40, qsr wrote:
> Do you want to test with structs, arrays and map as values?

This test was just intended to verify that all possible key types worked. I've
renamed the mojom type and etc to MapKeyTypes to make it a teeny bit clearer.

I've also added a unit test for the value types you suggested. I had to leave
the following fields out for now, since the C++ bindings don't generate code
that will compile for them yet:

  // map<string, map<string, string>> f3;
  // map<string, array<map<string, string>>> f4;
  // map<string, Rect?> f5;

https://codereview.chromium.org/654843005/diff/1/mojo/public/js/bindings/code...
File mojo/public/js/bindings/codec.js (right):

https://codereview.chromium.org/654843005/diff/1/mojo/public/js/bindings/code...
mojo/public/js/bindings/codec.js:30: var kMapStructSize = 16;
On 2014/10/15 05:58:02, yzshen1 wrote:
> It doesn't include the struct header itself, right? It seems
> kMapStructPayloadSize is more suitable.

Done.

https://codereview.chromium.org/654843005/diff/1/mojo/public/js/bindings/code...
mojo/public/js/bindings/codec.js:186: this.skip(4); // numberOfFields
On 2014/10/15 13:03:40, qsr wrote:
> Don't you want to check that the header is the one you expect?

Yes. That will happen when the incoming message is validated.

https://codereview.chromium.org/654843005/diff/1/mojo/public/js/bindings/code...
mojo/public/js/bindings/codec.js:380: });
On 2014/10/15 04:53:26, abarth wrote:
> for (var [key, value] of val) {
>   ...
> }
> 
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
> 
> Not sure if that works in V8, but it's worth a try.  :)

That would be nicer. I gave it a try; it doesn't appear to supported yet.

https://codereview.chromium.org/654843005/diff/1/mojo/public/js/bindings/code...
mojo/public/js/bindings/codec.js:381: this.writeUint32(kMapStructSize);
On 2014/10/15 05:58:02, yzshen1 wrote:
> num_bytes should also include the header size.

Yes it should! Sorry about that.

[yzshen1, 2014-10-15 22:22:13]

https://codereview.chromium.org/654843005/diff/1/mojo/public/interfaces/bindi...
File mojo/public/interfaces/bindings/tests/test_structs.mojom (right):

https://codereview.chromium.org/654843005/diff/1/mojo/public/interfaces/bindi...
mojo/public/interfaces/bindings/tests/test_structs.mojom:124: map<string,
string> f9;
On 2014/10/15 21:45:46, hansmuller wrote:
> On 2014/10/15 13:03:40, qsr wrote:
> > Do you want to test with structs, arrays and map as values?
> 
> This test was just intended to verify that all possible key types worked. I've
> renamed the mojom type and etc to MapKeyTypes to make it a teeny bit clearer.
> 
> I've also added a unit test for the value types you suggested. I had to leave
> the following fields out for now, since the C++ bindings don't generate code
> that will compile for them yet:
> 
>   // map<string, map<string, string>> f3;
>   // map<string, array<map<string, string>>> f4;
>   // map<string, Rect?> f5;
>  
(Out of curiosity) I thought the C++ bindings should work? what are the
problems?

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

https://codereview.chromium.org/654843005/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:191: Validator.prototype.validateMapPointer
= function(
This probably misses fixed-size array dimensions, right?
map<uint8, array<int8, 5>>

https://codereview.chromium.org/654843005/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:218: keysArrayLength,
This will detect the validation failure, but with a different error code.

[hansmuller, 2014-10-16 20:11:04]

I've corrected the validation support for map array values and added some test
cases to check it.

https://codereview.chromium.org/654843005/diff/1/mojo/public/interfaces/bindi...
File mojo/public/interfaces/bindings/tests/test_structs.mojom (right):

https://codereview.chromium.org/654843005/diff/1/mojo/public/interfaces/bindi...
mojo/public/interfaces/bindings/tests/test_structs.mojom:124: map<string,
string> f9;
On 2014/10/15 22:22:13, yzshen1 wrote:
> On 2014/10/15 21:45:46, hansmuller wrote:
> > On 2014/10/15 13:03:40, qsr wrote:
> > > Do you want to test with structs, arrays and map as values?
> > 
> > This test was just intended to verify that all possible key types worked.
I've
> > renamed the mojom type and etc to MapKeyTypes to make it a teeny bit
clearer.
> > 
> > I've also added a unit test for the value types you suggested. I had to
leave
> > the following fields out for now, since the C++ bindings don't generate code
> > that will compile for them yet:
> > 
> >   // map<string, map<string, string>> f3;
> >   // map<string, array<map<string, string>>> f4;
> >   // map<string, Rect?> f5;
> >  
> (Out of curiosity) I thought the C++ bindings should work? what are the
> problems?

-  The first two fail in the cpp parser:
  File
"/builds/dev/src/mojo/public/tools/bindings/generators/mojom_cpp_generator.py",
line 117, in GetCppArrayArgWrapperType
    return _kind_to_cpp_type[kind]
KeyError: <mojom.generate.module.Kind object at 0x2d641d0>


- The third one generates code that doesn't compile:
struct MapSerializer;
       ^
../../mojo/public/cpp/bindings/lib/map_serialization.h:99:19: error: implicit
instantiation of undefined template
'mojo::internal::MapSerializer<mojo::InlinedStructPtr<mojo::test::Rect>,
mojo::test::internal::Rect_Data *, true>'
        internal::MapSerializer<MapValue, DataValue>::GetItemSize(

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

https://codereview.chromium.org/654843005/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:191: Validator.prototype.validateMapPointer
= function(
On 2014/10/15 22:22:13, yzshen1 wrote:
> This probably misses fixed-size array dimensions, right?
> map<uint8, array<int8, 5>>

I've integrated the patch with the fixed-size array dimensions changes and fixed
the support for map array values.

In a subsequent patch I'd like to change the way that arrays are represented to
the validator. We should be able to just pass a codec ArrayOf type that
characterizes an array completely.

https://codereview.chromium.org/654843005/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:218: keysArrayLength,
On 2014/10/15 22:22:13, yzshen1 wrote:
> This will detect the validation failure, but with a different error code.

I've changed this to first validate the keys and values arrays, and then verify
that their lengths are the same.

[yzshen1, 2014-10-16 20:22:47]

LGTM

Thanks!

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

https://codereview.chromium.org/654843005/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/struct_unittests.js:149: f3: new Map([["a", ["1", "2"]],
["b", ["1", "2"]]]), // array<string,2>>
nit: two spaces before //

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

https://codereview.chromium.org/654843005/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:194: Validator.prototype.arrayLength =
function(arrayPointerOffset) {
nit: please consider commenting that |arrayPointerOffset| has to be validated
before using this method.

[hansmuller, 2014-10-16 20:43:41]

Thanks for the digging through this change so carefully.

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

https://codereview.chromium.org/654843005/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/struct_unittests.js:149: f3: new Map([["a", ["1", "2"]],
["b", ["1", "2"]]]), // array<string,2>>
On 2014/10/16 20:22:47, yzshen1 wrote:
> nit: two spaces before //

Done.

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

https://codereview.chromium.org/654843005/diff/60001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:194: Validator.prototype.arrayLength =
function(arrayPointerOffset) {
On 2014/10/16 20:22:47, yzshen1 wrote:
> nit: please consider commenting that |arrayPointerOffset| has to be validated
> before using this method.

Done.

[hansmuller, 2014-10-16 20:44:35]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-10-16 20:46:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/654843005/80001

[commit-bot: I haz the power, 2014-10-16 22:21:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-16 22:22:14]

Patchset 5 (id:??) landed as
https://crrev.com/d1fd54c8da40c9e7c1e7821259c39af947529f77
Cr-Commit-Position: refs/heads/master@{#299992}