Support packed arrays of booleans in Mojo messages

Support packed arrays of booleans in Mojo messages

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

[hansmuller, 2014-07-07 21:43:00]

A boolean array is encoded with its logical size (number of bool elements) since
that implies its packed size.

There may be a more elegant way to add this feature to the code.

If there are tests that verify that cross-language communication works then I
haven't updated them.

[Matt Perry, 2014-07-07 22:45:45]

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

https://codereview.chromium.org/377713004/diff/1/mojo/public/interfaces/bindi...
mojo/public/interfaces/bindings/tests/sample_service.mojom:41: bool[][]
array_of_array_of_bools@12;
What do we do for this case? Is it encoded as an array of bit-vectors, or 1-byte
per bool?

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

https://codereview.chromium.org/377713004/diff/1/mojo/public/js/bindings/code...
mojo/public/js/bindings/codec.js:395: var valLength = val.length;
nit: why not just use val.length everywhere?

https://codereview.chromium.org/377713004/diff/1/mojo/public/js/bindings/code...
mojo/public/js/bindings/codec.js:433: var encodedSize = kArrayHeaderSize +
Math.ceil(numberOfElements / 8);
do you also need to add cls.encodedSize? That reserves space for the pointer, I
thought.

[hansmuller, 2014-07-07 23:59:40]

On 2014/07/07 22:45:45, Matt Perry wrote:
>
https://codereview.chromium.org/377713004/diff/1/mojo/public/interfaces/bindi...
> File mojo/public/interfaces/bindings/tests/sample_service.mojom (right):
> 
>
https://codereview.chromium.org/377713004/diff/1/mojo/public/interfaces/bindi...
> mojo/public/interfaces/bindings/tests/sample_service.mojom:41: bool[][]
> array_of_array_of_bools@12;
> What do we do for this case? Is it encoded as an array of bit-vectors, or
1-byte
> per bool?

It's encoded as an array of bit-vectors. The name of the codec field for this
case was misleading. I changed it to ArrayOfBoolArrayPointers.

So for this mojom definition, 

struct TestStruct {
  bool[] boolArray1;
  bool[][] boolArray2;
  bool[][][] boolArray3;
};

the generate encode/decode for the three fields is hopefully a little easier to
read:

val.boolArray1 = decoder.decodeBoolArrayPointer();
val.boolArray2 = decoder.decodeArrayPointer(new
codec.ArrayOfBoolArrayPointers());
val.boolArray3 = decoder.decodeArrayPointer(new codec.ArrayOf(new
codec.ArrayOfBoolArrayPointers()));

encoder.encodeBoolArrayPointer(val.boolArray1);
encoder.encodeArrayPointer(new codec.ArrayOfBoolArrayPointers(),
val.boolArray2);
encoder.encodeArrayPointer(new codec.ArrayOf(new
codec.ArrayOfBoolArrayPointers()), val.boolArray3);

> 
>
https://codereview.chromium.org/377713004/diff/1/mojo/public/js/bindings/code...
> File mojo/public/js/bindings/codec.js (right):
> 
>
https://codereview.chromium.org/377713004/diff/1/mojo/public/js/bindings/code...
> mojo/public/js/bindings/codec.js:395: var valLength = val.length;
> nit: why not just use val.length everywhere?

Good point, I'll do that.

> 
>
https://codereview.chromium.org/377713004/diff/1/mojo/public/js/bindings/code...
> mojo/public/js/bindings/codec.js:433: var encodedSize = kArrayHeaderSize +
> Math.ceil(numberOfElements / 8);
> do you also need to add cls.encodedSize? That reserves space for the pointer,
I
> thought.

I thought that createAndEncodeEncoder(encodedSize) (the next line) was
allocating space for the array itself, after writing the 8 byte pointer.
EncodeArrayPointer(), which is very similar to this function appears to work
that way.

[Matt Perry, 2014-07-08 00:48:27]

On 2014/07/07 23:59:40, hansmuller wrote:
> On 2014/07/07 22:45:45, Matt Perry wrote:
> > 
> >
>
https://codereview.chromium.org/377713004/diff/1/mojo/public/js/bindings/code...
> > mojo/public/js/bindings/codec.js:433: var encodedSize = kArrayHeaderSize +
> > Math.ceil(numberOfElements / 8);
> > do you also need to add cls.encodedSize? That reserves space for the
pointer,
> I
> > thought.
> 
> I thought that createAndEncodeEncoder(encodedSize) (the next line) was
> allocating space for the array itself, after writing the 8 byte pointer.
> EncodeArrayPointer(), which is very similar to this function appears to work
> that way.

Looking at encodeArrayPointer, it manually includes cls.encodedSize:

  var encodedSize = kArrayHeaderSize + cls.encodedSize * val.length;

[hansmuller, 2014-07-08 01:12:06]

On 2014/07/08 00:48:27, Matt Perry wrote:
> On 2014/07/07 23:59:40, hansmuller wrote:
> > On 2014/07/07 22:45:45, Matt Perry wrote:
> > > 
> > >
> >
>
https://codereview.chromium.org/377713004/diff/1/mojo/public/js/bindings/code...
> > > mojo/public/js/bindings/codec.js:433: var encodedSize = kArrayHeaderSize +
> > > Math.ceil(numberOfElements / 8);
> > > do you also need to add cls.encodedSize? That reserves space for the
> pointer,
> > I
> > > thought.
> > 
> > I thought that createAndEncodeEncoder(encodedSize) (the next line) was
> > allocating space for the array itself, after writing the 8 byte pointer.
> > EncodeArrayPointer(), which is very similar to this function appears to work
> > that way.
> 
> Looking at encodeArrayPointer, it manually includes cls.encodedSize:
> 
>   var encodedSize = kArrayHeaderSize + cls.encodedSize * val.length;

Right, but isn't that: header-size + array-element-size * array-length?

[abarth-chromium, 2014-07-08 02:00:33]

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

https://codereview.chromium.org/377713004/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:237: var bits = new Uint8Array(bitsLength);
You should be able to do this work without allocating a new Uint8Array.  Can't
you just call readInt8 in a loop?

[hansmuller, 2014-07-08 15:30:49]

On 2014/07/08 02:00:33, abarth wrote:
>
https://codereview.chromium.org/377713004/diff/20001/mojo/public/js/bindings/...
> File mojo/public/js/bindings/codec.js (right):
> 
>
https://codereview.chromium.org/377713004/diff/20001/mojo/public/js/bindings/...
> mojo/public/js/bindings/codec.js:237: var bits = new Uint8Array(bitsLength);
> You should be able to do this work without allocating a new Uint8Array.  Can't
> you just call readInt8 in a loop?

Good point, that would be much simpler.

[Matt Perry, 2014-07-08 18:04:00]

On 2014/07/08 01:12:06, hansmuller wrote:
> On 2014/07/08 00:48:27, Matt Perry wrote:
> > On 2014/07/07 23:59:40, hansmuller wrote:
> > > On 2014/07/07 22:45:45, Matt Perry wrote:
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/377713004/diff/1/mojo/public/js/bindings/code...
> > > > mojo/public/js/bindings/codec.js:433: var encodedSize = kArrayHeaderSize
+
> > > > Math.ceil(numberOfElements / 8);
> > > > do you also need to add cls.encodedSize? That reserves space for the
> > pointer,
> > > I
> > > > thought.
> > > 
> > > I thought that createAndEncodeEncoder(encodedSize) (the next line) was
> > > allocating space for the array itself, after writing the 8 byte pointer.
> > > EncodeArrayPointer(), which is very similar to this function appears to
work
> > > that way.
> > 
> > Looking at encodeArrayPointer, it manually includes cls.encodedSize:
> > 
> >   var encodedSize = kArrayHeaderSize + cls.encodedSize * val.length;
> 
> Right, but isn't that: header-size + array-element-size * array-length?

You're right, I was looking at it wrong. Sorry for the distraction.

[Matt Perry, 2014-07-09 19:32:35]

lgtm

[hansmuller, 2014-07-09 20:14:50]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-07-09 20:17:50]

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

[commit-bot: I haz the power, 2014-07-10 00:06:24]

Change committed as 282182