mojo: Validate fixed size array for the mojo java bindings.

mojo: Validate fixed size array for the mojo java bindings.

R=ppi@chromium.org

Committed: https://crrev.com/1e342dff6f2d0c330423a29a26576665daf33b57
Cr-Commit-Position: refs/heads/master@{#292862}

[ppi, 2014-08-29 15:23:45]

lgtm % nits below

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/BindingsHelper.java
(right):

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/BindingsHelper.java:34:
public static final int UNDEFINED_ARRAY_LENTH = -1;
s/LENTH/LENGTH, also I'd go for UNSPECIFIED ("specify" for a value, "define" for
a type).

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:131: *
Deserializes a {@link DataHeader} at the given offset for an array.
nit: reorder -> "Deserializes a {@link DataHeader} of an array at the given
offset."

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:139: throw
new DeserializationException("Incorrect array length. Expecting: " +
nit: align tenses -> s/Expecting/Expected/

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:139: throw
new DeserializationException("Incorrect array length. Expecting: " +
nit: maybe "Incorrect length of a received array. (..)", to indicate this is
upon deserialization?

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/SerializationException.java
(right):

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/SerializationException.java:8:
* Error when serializing a mojo message.
nit: "Error that can be thrown when..."

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/tools/bin...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/tools/bin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:155: if
mojom.IsArrayKind(kind):
nit: how about checking if mojom.IsAnyArrayKind at the top level, and then
putting if else clause inside? That would make it explicit, that for any array
there will be exactly one length parameter added into the list.

[qsr, 2014-08-29 16:24:12]

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/BindingsHelper.java
(right):

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/BindingsHelper.java:34:
public static final int UNDEFINED_ARRAY_LENTH = -1;
On 2014/08/29 15:23:45, ppi wrote:
> s/LENTH/LENGTH, also I'd go for UNSPECIFIED ("specify" for a value, "define"
for
> a type).

Done.

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:131: *
Deserializes a {@link DataHeader} at the given offset for an array.
On 2014/08/29 15:23:45, ppi wrote:
> nit: reorder -> "Deserializes a {@link DataHeader} of an array at the given
> offset."

Done.

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:139: throw
new DeserializationException("Incorrect array length. Expecting: " +
On 2014/08/29 15:23:45, ppi wrote:
> nit: align tenses -> s/Expecting/Expected/

Done.

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:139: throw
new DeserializationException("Incorrect array length. Expecting: " +
On 2014/08/29 15:23:45, ppi wrote:
> nit: maybe "Incorrect length of a received array. (..)", to indicate this is
> upon deserialization?

 That's a deserialization exception. Isn't the information already in the
exception type?

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/SerializationException.java
(right):

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/SerializationException.java:8:
* Error when serializing a mojo message.
On 2014/08/29 15:23:45, ppi wrote:
> nit: "Error that can be thrown when..."

Done.

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/tools/bin...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/tools/bin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:155: if
mojom.IsArrayKind(kind):
On 2014/08/29 15:23:45, ppi wrote:
> nit: how about checking if mojom.IsAnyArrayKind at the top level, and then
> putting if else clause inside? That would make it explicit, that for any array
> there will be exactly one length parameter added into the list.

Done.

[ppi, 2014-08-29 16:27:28]

Thanks, still lgtm.

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java
(right):

https://chromiumcodereview.appspot.com/522653004/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:139: throw
new DeserializationException("Incorrect array length. Expecting: " +
On 2014/08/29 16:24:11, qsr wrote:
> On 2014/08/29 15:23:45, ppi wrote:
> > nit: maybe "Incorrect length of a received array. (..)", to indicate this is
> > upon deserialization?
> 
>  That's a deserialization exception. Isn't the information already in the
> exception type?

Acknowledged.

[yzshen1, 2014-08-29 16:32:24]

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

[yzshen1, 2014-08-29 16:32:24]

https://chromiumcodereview.appspot.com/522653004/diff/20001/mojo/public/tools...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://chromiumcodereview.appspot.com/522653004/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/mojom_java_generator.py:177:
"org.chromium.mojo.bindings.BindingsHelper.UNSPECIFIED_ARRAY_LENGTH");
One nit: Other bindings use 0 as "unspecified length". I understand that they
don't affect each other, but it may worth considering to unify them:
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/tools/...

[qsr, 2014-08-29 21:18:15]

On Aug 29, 2014 6:32 PM, <yzshen@chromium.org> wrote:
>
>
>
https://chromiumcodereview.appspot.com/522653004/diff/20001/mojo/public/tools...
> File mojo/public/tools/bindings/generators/mojom_java_generator.py
> (right):
>
>
https://chromiumcodereview.appspot.com/522653004/diff/20001/mojo/public/tools...
> mojo/public/tools/bindings/generators/mojom_java_generator.py:177:
> "org.chromium.mojo.bindings.BindingsHelper.UNSPECIFIED_ARRAY_LENGTH");
> One nit: Other bindings use 0 as "unspecified length".

I wanted to ask you about this. Why 0? This is a valid array length, even
if not really useful...

I understand that
> they don't affect each other, but it may worth considering to unify
> them:
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/tools/...
>
> https://chromiumcodereview.appspot.com/522653004/

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

[yzshen1, 2014-08-29 21:26:32]

I don't have a strong reason. :) (+CC jamesr)

I think that is because we pass expected_num_elements (as an argument at
the beginning, now inside a template argument) to Array_Data's Validate()
method. If we want to use -1, then it needs to be something that can cover
the whole range of uint32_t (num_elements value range), say, int64_t.

In any case, consistency seems nice to me. If we want to use -1 (or 0),
let's use it everywhere. :)

On Fri, Aug 29, 2014 at 2:18 PM, Benjamin Lerman <qsr@chromium.org> wrote:

>
> On Aug 29, 2014 6:32 PM, <yzshen@chromium.org> wrote:
> >
> >
> >
>
https://chromiumcodereview.appspot.com/522653004/diff/20001/mojo/public/tools...
> > File mojo/public/tools/bindings/generators/mojom_java_generator.py
> > (right):
> >
> >
>
https://chromiumcodereview.appspot.com/522653004/diff/20001/mojo/public/tools...
> > mojo/public/tools/bindings/generators/mojom_java_generator.py:177:
> > "org.chromium.mojo.bindings.BindingsHelper.UNSPECIFIED_ARRAY_LENGTH");
> > One nit: Other bindings use 0 as "unspecified length".
>
> I wanted to ask you about this. Why 0? This is a valid array length, even
> if not really useful...
>
> I understand that
> > they don't affect each other, but it may worth considering to unify
> > them:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/tools/...
> >
> > https://chromiumcodereview.appspot.com/522653004/
>

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

[jamesr, 2014-08-29 21:34:07]

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

[jamesr, 2014-08-29 21:34:08]

Is the question about how what a mojom declaration like

  int[0] foo;

should do?  I think that should be rejected as invalid syntax, personally.

[yzshen1, 2014-08-29 22:08:03]

I think the question from Ben is why picking 0 as "any element number is
okay" (used in the bindings impl internally), instead of picking, say, -1.



On Fri, Aug 29, 2014 at 2:34 PM, <jamesr@chromium.org> wrote:

> Is the question about how what a mojom declaration like
>
>   int[0] foo;
>
> should do?  I think that should be rejected as invalid syntax, personally.
>
> https://chromiumcodereview.appspot.com/522653004/
>

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

[chromium-reviews, 2014-08-29 22:37:44]

Well, it's an unsigned isn't it?  -1 is not a valid value.  MAX_VALUE is
possibly in bounds.  0 is never a valid value for a fixed array length so
it seems like a good choice.


On Fri, Aug 29, 2014 at 3:08 PM, Yuzhu Shen <yzshen@chromium.org> wrote:

>
> I think the question from Ben is why picking 0 as "any element number is
> okay" (used in the bindings impl internally), instead of picking, say, -1.
>
>
>
> On Fri, Aug 29, 2014 at 2:34 PM, <jamesr@chromium.org> wrote:
>
>> Is the question about how what a mojom declaration like
>>
>>   int[0] foo;
>>
>> should do?  I think that should be rejected as invalid syntax, personally.
>>
>> https://chromiumcodereview.appspot.com/522653004/
>>
>
>

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

[qsr, 2014-08-30 09:22:43]

> Is the question about how what a mojom declaration like
>
>   int[0] foo;
>
> should do?  I think that should be rejected as invalid syntax, personally.

 Why would it be invalid? This declares a mandatory array which size
must be 0. I agree that I do not see a lot of use case for this (but
imagine that some code starts generating mojom file, I can imagine
this happening), but there is no reason for it to be a syntax error.
Otherwise, things like this do not make more sense:

Struct[1]
handle[1]



>
> https://chromiumcodereview.appspot.com/522653004/

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

[qsr, 2014-09-01 09:07:01]

https://codereview.chromium.org/522653004/diff/20001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://codereview.chromium.org/522653004/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:177:
"org.chromium.mojo.bindings.BindingsHelper.UNSPECIFIED_ARRAY_LENGTH");
On 2014/08/29 16:32:24, yzshen1 wrote:
> One nit: Other bindings use 0 as "unspecified length". I understand that they
> don't affect each other, but it may worth considering to unify them:
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/tools/...

 Not sure to understand exactly what to do here. The -1, 0 debate aside, I'm
using a constant here to make the code more readable. Do you want use to do this
in all bindings?

 For the -1, 0, I will push like this, and change to 0 if the discussion ends up
deciding we never want fixed size arrays of size 0.

[qsr, 2014-09-01 09:07:17]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-09-01 09:08:00]

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

[qsr, 2014-09-01 10:48:13]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-09-01 10:49:20]

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

[commit-bot: I haz the power, 2014-09-01 11:44:23]

Committed patchset #4 (id:60001) as 9fd89706b1ab272b734bf4ec13762d6d56f2d213

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

Patchset 4 (id:??) landed as
https://crrev.com/1e342dff6f2d0c330423a29a26576665daf33b57
Cr-Commit-Position: refs/heads/master@{#292862}