Java: Implements union generation.

Java: Implements union generation.

R=ppi@chromium.org, azani@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/c366b98fcc8e8ba63e092682d75364977e8afb5b

[qsr, 2015-05-19 16:05:45]

The tests are not complete, but everything else is, so could you start
reviewing? Thanks.

[azani, 2015-05-19 21:13:24]

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:131: if
(result.elementsOrVersion != 0) {
The design doc doesn't specify an error when the tag is not equal to 0 for a
null union. Do you think that is an oversight? I think that we should enforce
the same rules for the tag and the data field. Either both have to be 0 for null
unions or neither has to be.

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:138:
"Unexpected size of an union. The size must be 0 for a null union, and 16 for "
nit: "or 16 for..."

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java
(right):

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:456:
public Encoder encoderForUnionPointer(int offset) {
Maybe document whether this encodes a pointer to a union or a union that
contains a pointer.

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/java_templates/data_types_definition.tmpl
(right):

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/data_types_definition.tmpl:41:
{% if kind|is_pointer_array_kind or kind|is_union_array_kind%}
nit: add space before %}

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/data_types_definition.tmpl:61:
if ({{variable}} == null) {
Shouldn't you look at "check_for_null" as on line 43? Maybe it would be better
to put the "encodeNullPointer" call at the top of the encode macro?

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom_bindings_generator.py (right):

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom_bindings_generator.py:198: default="c++,java",
That shouldn't be necessary anymore given your last CL regarding unions.

[qsr, 2015-05-20 09:22:37]

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:131: if
(result.elementsOrVersion != 0) {
On 2015/05/19 21:13:23, azani wrote:
> The design doc doesn't specify an error when the tag is not equal to 0 for a
> null union. Do you think that is an oversight? I think that we should enforce
> the same rules for the tag and the data field. Either both have to be 0 for
null
> unions or neither has to be.

That's not correct though. 0 is a valid value for a non-null union, as this will
be the tag value.

But I indeed think that we should have 0 for the tag when the union is null.

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:138:
"Unexpected size of an union. The size must be 0 for a null union, and 16 for "
On 2015/05/19 21:13:23, azani wrote:
> nit: "or 16 for..."

Done.

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java
(right):

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:456:
public Encoder encoderForUnionPointer(int offset) {
On 2015/05/19 21:13:23, azani wrote:
> Maybe document whether this encodes a pointer to a union or a union that
> contains a pointer.

Done.

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/java_templates/data_types_definition.tmpl
(right):

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/data_types_definition.tmpl:41:
{% if kind|is_pointer_array_kind or kind|is_union_array_kind%}
On 2015/05/19 21:13:23, azani wrote:
> nit: add space before %}

Done.

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/data_types_definition.tmpl:61:
if ({{variable}} == null) {
On 2015/05/19 21:13:23, azani wrote:
> Shouldn't you look at "check_for_null" as on line 43? Maybe it would be better
> to put the "encodeNullPointer" call at the top of the encode macro?

 This code is just moved. And I do not need to look at "check_for_null", because
there is no case where maps are known not to be nullable, while for arrays, we
know that if we are encoding a map, the array are not null.

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom_bindings_generator.py (right):

https://codereview.chromium.org/1125183007/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom_bindings_generator.py:198: default="c++,java",
On 2015/05/19 21:13:23, azani wrote:
> That shouldn't be necessary anymore given your last CL regarding unions.

 Right. Sorry, this was not intended to be here. Thanks.

[ppi, 2015-05-20 11:27:57]

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
File mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java
(right):

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java:25: //
Check that all tag are different.
s/tag/tags/

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java:122:
assertEquals(PodUnion.Tag.FInt8, pod2.which());
should we check that pod2 equals pod1 here?

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java:122:
assertEquals(PodUnion.Tag.FInt8, pod2.which());
how about checking serialization of uninitialized union too (one that we haven't
assigned to yet)?

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java:168: }
can we have a test for (de)seralization of union arrays, given that this is a
separate case in the code?

https://codereview.chromium.org/1125183007/diff/40001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://codereview.chromium.org/1125183007/diff/40001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:133:
"Unexpected tag for a null union. Expecting 0, found: "
"Tag" might be confused with the union tag. Maybe say "version tag"?

https://codereview.chromium.org/1125183007/diff/40001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Union.java
(right):

https://codereview.chromium.org/1125183007/diff/40001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Union.java:27: * Use
the given encoder to serialize this data structure.
s/Use/Uses/ or "Serializes this data structure using the given encoder."

[qsr, 2015-05-20 15:42:15]

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
File mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java
(right):

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/05/20 11:27:57, ppi wrote:
> 2015

Done.

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java:25: //
Check that all tag are different.
On 2015/05/20 11:27:57, ppi wrote:
> s/tag/tags/

Done.

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java:122:
assertEquals(PodUnion.Tag.FInt8, pod2.which());
On 2015/05/20 11:27:57, ppi wrote:
> should we check that pod2 equals pod1 here?

We already have a test for union equality.

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java:122:
assertEquals(PodUnion.Tag.FInt8, pod2.which());
On 2015/05/20 11:27:57, ppi wrote:
> how about checking serialization of uninitialized union too (one that we
haven't
> assigned to yet)?

Done.

https://codereview.chromium.org/1125183007/diff/40001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/UnionTest.java:168: }
On 2015/05/20 11:27:57, ppi wrote:
> can we have a test for (de)seralization of union arrays, given that this is a
> separate case in the code?

Yes, as said on the comment, the tests were not finished yet. You can look at it
now.

https://codereview.chromium.org/1125183007/diff/40001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://codereview.chromium.org/1125183007/diff/40001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:133:
"Unexpected tag for a null union. Expecting 0, found: "
On 2015/05/20 11:27:57, ppi wrote:
> "Tag" might be confused with the union tag. Maybe say "version tag"?

Done.

https://codereview.chromium.org/1125183007/diff/40001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Union.java
(right):

https://codereview.chromium.org/1125183007/diff/40001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Union.java:27: * Use
the given encoder to serialize this data structure.
On 2015/05/20 11:27:57, ppi wrote:
> s/Use/Uses/ or "Serializes this data structure using the given encoder."

Done.

[ppi, 2015-05-20 17:00:20]

lgtm

[qsr, 2015-05-21 09:50:49]

Committed patchset #7 (id:120001) manually as
c366b98fcc8e8ba63e092682d75364977e8afb5b (presubmit successful).