mojo: Fixing array serialization for java bindings.

mojo: Fixing array serialization for java bindings.

The num_bytes header for an array must contain the real size of the
array, not the 8 bytes aligned one.

R=ppi@chromium.org

Committed: https://crrev.com/bc48a481990a51564c4f0208b09627a00b2523eb
Cr-Commit-Position: refs/heads/master@{#292656}

[ppi, 2014-08-29 09:09:24]

Thanks, a few questions below.

https://codereview.chromium.org/519673002/diff/1/mojo/public/java/bindings/sr...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://codereview.chromium.org/519673002/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:124: //
The memory for the header has already been claimed.
Please excuse an unrelated (to the CL) question, but what's the reason to claim
this memory in constructor, as opposed to here?

https://codereview.chromium.org/519673002/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:126:
DataHeader res = new DataHeader(BindingsHelper.align(size), numFields);
It feels weird to have a DataHeader that represents the data header encoded on
the wire, but has different |size|.

- are we sure the |size| encoded in the archive is supposed not to include the
padding after the last member? (I left a question on the Mojo Archive doc)
- if yes, does it matter if we align this here?

https://codereview.chromium.org/519673002/diff/1/mojo/public/java/bindings/sr...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java
(right):

https://codereview.chromium.org/519673002/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:441: new
DataHeader(DataHeader.HEADER_SIZE + byteSize, length));
Why here we set the size of DataHeader without alignment, while we align it in
readDataHeader()?

[qsr, 2014-08-29 09:51:45]

https://codereview.chromium.org/519673002/diff/1/mojo/public/java/bindings/sr...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://codereview.chromium.org/519673002/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:124: //
The memory for the header has already been claimed.
On 2014/08/29 09:09:24, ppi wrote:
> Please excuse an unrelated (to the CL) question, but what's the reason to
claim
> this memory in constructor, as opposed to here?

 Need to be done before reading the 2 ints, but you are correct. Changed.

https://codereview.chromium.org/519673002/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:126:
DataHeader res = new DataHeader(BindingsHelper.align(size), numFields);
On 2014/08/29 09:09:24, ppi wrote:
> It feels weird to have a DataHeader that represents the data header encoded on
> the wire, but has different |size|.
> 
> - are we sure the |size| encoded in the archive is supposed not to include the
> padding after the last member? (I left a question on the Mojo Archive doc)
> - if yes, does it matter if we align this here?

 Hum... It was not necessary -> Removed the alignment. Claiming with alignment
is sufficient. We do not need to change the size.

https://codereview.chromium.org/519673002/diff/1/mojo/public/java/bindings/sr...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java
(right):

https://codereview.chromium.org/519673002/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:441: new
DataHeader(DataHeader.HEADER_SIZE + byteSize, length));
On 2014/08/29 09:09:24, ppi wrote:
> Why here we set the size of DataHeader without alignment, while we align it in
> readDataHeader()?

 Changed readDataHeader.

 We do set the size without alignment because we are following the C++
implementation. See discussion on mojo-dev, I'd like us to change that in the
end and forcing the size to always be 8 aligned, whatever the type of data.

[ppi, 2014-08-29 15:56:49]

lgtm, thanks!

[qsr, 2014-08-29 15:58:06]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-08-29 15:59:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/519673002/20001

[commit-bot: I haz the power, 2014-08-29 16:53:50]

Committed patchset #2 (id:20001) as 09ed4f43a5d093e4af7c955b8e3ef7e2a8ba06b3

[commit-bot: I haz the power, 2014-09-10 03:09:25]

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