Generate java bindings for structs.

Generate java bindings for structs.

This CL introduces bindings for struct. It only generates fields and not
serialization/deserialization methods.

R=rmcilroy@chromium.org, viettrungluu@chromium.org

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

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

[rmcilroy, 2014-06-03 22:53:22]

https://codereview.chromium.org/306893010/diff/10001/mojo/android/javatests/s...
File mojo/android/javatests/src/org/chromium/mojo/HandleMock.java (right):

https://codereview.chromium.org/306893010/diff/10001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/HandleMock.java:39: return 0;
return MojoResult.OK

https://codereview.chromium.org/306893010/diff/10001/mojo/android/javatests/s...
File mojo/android/javatests/src/org/chromium/mojo/TestHelper.java (right):

https://codereview.chromium.org/306893010/diff/10001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/TestHelper.java:14: public class
TestHelper {
TestUtils is probably more appropriate name for this class. Make it final and
update javadoc too please.

https://codereview.chromium.org/306893010/diff/10001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/TestHelper.java:20: public static
void assertArrayEquals(Object a, Object b) {
Couldn't you just use Arrays.deepEquals()?

https://codereview.chromium.org/306893010/diff/10001/mojo/android/javatests/s...
File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java
(right):

https://codereview.chromium.org/306893010/diff/10001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:134:
private static void checkField(Field field, Class<?> expectedClass) {
Would it be possible to parameterize this by the type and do the check on the
field value here too? E.g., something like:
 private static <T> void checkField(Object obj, Field field, T expectedValue) {
  assertEquals(expectedValue, (T) field.get(obj));
 ...

Then both checks could be done as a:
  checkField<byte>(test, getField("foo"), 12);
WDYT?

https://codereview.chromium.org/306893010/diff/10001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:243:
assertFooEquals(typicalFoo, typicalFoo);
Not sure why you are checking the object is equal to itself, especially since
the equals check is test code and not something generated by the bindings.  I'm
also not entirely sure on the reasoning behind the createFoo code - this seem a
lot of complex code (which will break whenever the Foo object is modified in the
mojom file) and doesn't really exercise the generated binding code much.  I
think simply checking that Foo and Bar are generated with fields of the correct
type and have the correct initial default values is enough here.

https://codereview.chromium.org/306893010/diff/10001/mojo/not-to-commit/mojo_...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Thing.java
(right):

https://codereview.chromium.org/306893010/diff/10001/mojo/not-to-commit/mojo_...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Thing.java:18:
public int shape = (int)
org.chromium.mojo.bindings.test.imported.Shape.SHAPE_RECTANGLE;
A couple of things on formatting:
 - I think it would be better to set the defaults in the constructor (like the
C++ bindings). 
 - Fields first then constructor (and other methods).  
 - You don't need to output the full package for the field types or default
constant values since they are in the same package (by virtue of being in the
same mojo module).
 - Do you need the casts here?  Surely we want to generated bindings to break at
compile time if someone uses the wrong typed constant as the default to a
particular field.
 - I don't think you need a newline between each field declaration.

https://codereview.chromium.org/306893010/diff/10001/mojo/not-to-commit/mojo_...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Bar.java
(right):

https://codereview.chromium.org/306893010/diff/10001/mojo/not-to-commit/mojo_...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Bar.java:19:
public static final int TYPE_BOTH = (int)
(org.chromium.mojo.bindings.test.sample.Bar.Type.TYPE_VERTICAL|org.chromium.mojo.bindings.test.sample.Bar.Type.TYPE_HORIZONTAL);
Again, you shouldn't need the fully prefixed package on the constant or the cast
here - can't it just read:
  public static final int TYPE_BOTH = TYPE_HORIZONTAL | TYPE_VERTICAL;

https://codereview.chromium.org/306893010/diff/10001/mojo/not-to-commit/mojo_...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Response.java
(right):

https://codereview.chromium.org/306893010/diff/10001/mojo/not-to-commit/mojo_...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Response.java:20:
public org.chromium.mojo.system.MessagePipeHandle pipe;
I think this should be initialized with a non-null MessagePipeHandle instance
instead of being null by default.  This seems to be the approach the cpp
bindings take.  Maybe also have non-null defaults for inner structs, arrays and
strings?

https://codereview.chromium.org/306893010/diff/10001/mojo/not-to-commit/mojo_...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/StructWithHoleV2.java
(right):

https://codereview.chromium.org/306893010/diff/10001/mojo/not-to-commit/mojo_...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/StructWithHoleV2.java:22:
public long v2 = (long) 2L;
Why are these re-ordered compared to the way they are defined in the mojom file?
  In the cpp bindings the fields are in the same order they are defined in in
the mojom.

[qsr, 2014-06-04 08:42:13]

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
File mojo/android/javatests/src/org/chromium/mojo/HandleMock.java (right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/HandleMock.java:39: return 0;
On 2014/06/03 22:53:22, rmcilroy wrote:
> return MojoResult.OK

Done.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
File mojo/android/javatests/src/org/chromium/mojo/TestHelper.java (right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/TestHelper.java:14: public class
TestHelper {
On 2014/06/03 22:53:22, rmcilroy wrote:
> TestUtils is probably more appropriate name for this class. Make it final and
> update javadoc too please.

Done.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/TestHelper.java:20: public static
void assertArrayEquals(Object a, Object b) {
On 2014/06/03 22:53:22, rmcilroy wrote:
> Couldn't you just use Arrays.deepEquals()?

 Doesn't work on primary type, as it takes Object[] and not Object. Updated with
some small hack and using deepEquals, tell me if that's fine with you.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:134:
private static void checkField(Field field, Class<?> expectedClass) {
On 2014/06/03 22:53:22, rmcilroy wrote:
> Would it be possible to parameterize this by the type and do the check on the
> field value here too? E.g., something like:
>  private static <T> void checkField(Object obj, Field field, T expectedValue)
{
>   assertEquals(expectedValue, (T) field.get(obj));
>  ...
> 
> Then both checks could be done as a:
>   checkField<byte>(test, getField("foo"), 12);
> WDYT?

 This will transform primary type into boxed type. Also, will be a problem for
float and double as I won't be able to use an almostEquals. Should we have 2
methods to handle this case?

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:243:
assertFooEquals(typicalFoo, typicalFoo);
On 2014/06/03 22:53:22, rmcilroy wrote:
> Not sure why you are checking the object is equal to itself, especially since
> the equals check is test code and not something generated by the bindings. 
I'm
> also not entirely sure on the reasoning behind the createFoo code - this seem
a
> lot of complex code (which will break whenever the Foo object is modified in
the
> mojom file) and doesn't really exercise the generated binding code much.  I
> think simply checking that Foo and Bar are generated with fields of the
correct
> type and have the correct initial default values is enough here.

 Well, this will be necessary as soon as I add serialization/deserialization
which will come soon. If you prefer I remove this for now, no problem.

 Also this will not break anymore than any of the other test if we change the
name or type of a field. And this will not break if we add more fields. Mainly
it is as you wish if you prefer this now or with the serialization CL.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Thing.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Thing.java:18:
public int shape = (int)
org.chromium.mojo.bindings.test.imported.Shape.SHAPE_RECTANGLE;
On 2014/06/03 22:53:22, rmcilroy wrote:
> A couple of things on formatting:
>  - I think it would be better to set the defaults in the constructor (like the
> C++ bindings). 

 Ok. Any reason for this?

>  - Fields first then constructor (and other methods).  
>  - You don't need to output the full package for the field types or default
> constant values since they are in the same package (by virtue of being in the
> same mojo module).

 I do. mojom can import other mojom that can be in another package. I do not
think it is worth having special case in generated code for this. Always having
FQC simplified the templates.

>  - Do you need the casts here?  Surely we want to generated bindings to break
at
> compile time if someone uses the wrong typed constant as the default to a
> particular field.

 I need it because mojom have signed and unsigned data, while java doesn't have
any unsigned type. I convert unsigned value into signed one (which mean that a
uint8 of 128 will be a byte of -128). This is a little unfortunate, but I do not
have a good solution.

>  - I don't think you need a newline between each field declaration.

 Done

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Bar.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Bar.java:19:
public static final int TYPE_BOTH = (int)
(org.chromium.mojo.bindings.test.sample.Bar.Type.TYPE_VERTICAL|org.chromium.mojo.bindings.test.sample.Bar.Type.TYPE_HORIZONTAL);
On 2014/06/03 22:53:22, rmcilroy wrote:
> Again, you shouldn't need the fully prefixed package on the constant or the
cast
> here - can't it just read:
>   public static final int TYPE_BOTH = TYPE_HORIZONTAL | TYPE_VERTICAL;

 I can be values from another enum and or package.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Response.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Response.java:20:
public org.chromium.mojo.system.MessagePipeHandle pipe;
On 2014/06/03 22:53:22, rmcilroy wrote:
> I think this should be initialized with a non-null MessagePipeHandle instance
> instead of being null by default.  This seems to be the approach the cpp
> bindings take.  Maybe also have non-null defaults for inner structs, arrays
and
> strings?

 Ok for pipe (but will be an invalid one, C++ doesn't generate valid pipe, and
the only reason it is valid is that for C++ pipe is mostly a primary type). For
all other type, the default value is null by specification.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/StructWithHoleV2.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/StructWithHoleV2.java:22:
public long v2 = (long) 2L;
On 2014/06/03 22:53:22, rmcilroy wrote:
> Why are these re-ordered compared to the way they are defined in the mojom
file?
>   In the cpp bindings the fields are in the same order they are defined in in
> the mojom.

They were in packing order. Reordered in declaration order.

[rmcilroy, 2014-06-05 11:01:16]

Thanks, this is looking a lot cleaner IMO.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
File mojo/android/javatests/src/org/chromium/mojo/TestHelper.java (right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/TestHelper.java:20: public static
void assertArrayEquals(Object a, Object b) {
On 2014/06/04 08:42:13, qsr wrote:
> On 2014/06/03 22:53:22, rmcilroy wrote:
> > Couldn't you just use Arrays.deepEquals()?
> 
>  Doesn't work on primary type, as it takes Object[] and not Object. Updated
with
> some small hack and using deepEquals, tell me if that's fine with you.

But you only use it on fields which you already know have a Object[] type. I'm
fine with you having an assertArrayEquals to wrap an
assertTrue(Arrays.deepEquals(a, b)), but I think it should take Object[]
arguments unless you have a good reason otherwise.  Also, maybe rename
assertArrayDeepEquals to be clearer.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:134:
private static void checkField(Field field, Class<?> expectedClass) {
On 2014/06/04 08:42:13, qsr wrote:
> On 2014/06/03 22:53:22, rmcilroy wrote:
> > Would it be possible to parameterize this by the type and do the check on
the
> > field value here too? E.g., something like:
> >  private static <T> void checkField(Object obj, Field field, T
expectedValue)
> {
> >   assertEquals(expectedValue, (T) field.get(obj));
> >  ...
> > 
> > Then both checks could be done as a:
> >   checkField<byte>(test, getField("foo"), 12);
> > WDYT?
> 
>  This will transform primary type into boxed type. Also, will be a problem for
> float and double as I won't be able to use an almostEquals. Should we have 2
> methods to handle this case?

I think it should be fine just to have one - you aren't using almostEquals at
the moment and I don't think you would need to check a double with almostEqual
in the checkField method, since this will only really be being checked for
default values where an exact value can be used.  For other checks (e.g., when
serializing / deserializing) there is no need to check the field types /
modifiers only the values in the field, so we can use almostEquals directly in
those test cases.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:243:
assertFooEquals(typicalFoo, typicalFoo);
On 2014/06/04 08:42:13, qsr wrote:
> On 2014/06/03 22:53:22, rmcilroy wrote:
> > Not sure why you are checking the object is equal to itself, especially
since
> > the equals check is test code and not something generated by the bindings. 
> I'm
> > also not entirely sure on the reasoning behind the createFoo code - this
seem
> a
> > lot of complex code (which will break whenever the Foo object is modified in
> the
> > mojom file) and doesn't really exercise the generated binding code much.  I
> > think simply checking that Foo and Bar are generated with fields of the
> correct
> > type and have the correct initial default values is enough here.
> 
>  Well, this will be necessary as soon as I add serialization/deserialization
> which will come soon. If you prefer I remove this for now, no problem.
> 
>  Also this will not break anymore than any of the other test if we change the
> name or type of a field. And this will not break if we add more fields. Mainly
> it is as you wish if you prefer this now or with the serialization CL.

I would prefer this in the followup serialization CL please.  I think it would
still be worthwhile having a test in this CL of the Foo (and possibly Bar)
class's field layouts and their default values (e.g., lines 245 to 253), similar
to testStructDefaultValue above, since these are quite complex types and so a
good test on the bindings generator.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Thing.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Thing.java:18:
public int shape = (int)
org.chromium.mojo.bindings.test.imported.Shape.SHAPE_RECTANGLE;
On 2014/06/04 08:42:13, qsr wrote:
> On 2014/06/03 22:53:22, rmcilroy wrote:
> > A couple of things on formatting:
> >  - I think it would be better to set the defaults in the constructor (like
the
> > C++ bindings). 
> 
>  Ok. Any reason for this?

I think it's a bit more idiomatic, but also could be an optimization since field
initializations are always done before every constructor (even if the fields are
overwritten in the constructor), so doing it this way avoids setting the
defaults where they aren't needed (e.g., in a deserialization constructor?),
which could avoid GC pressure if we ever allow object defaults.

> >  - Fields first then constructor (and other methods).  
> >  - You don't need to output the full package for the field types or default
> > constant values since they are in the same package (by virtue of being in
the
> > same mojo module).
> 
>  I do. mojom can import other mojom that can be in another package. I do not
> think it is worth having special case in generated code for this. Always
having
> FQC simplified the templates.

I realize that you can't avoid the package name on types imported from another
package (due to potential for namespace collisions), but you can safely exclude
the package name for types in the current package / mojom.  Since this will be
the common case, I think it is worth the special case to improve readability of
the generated code, and it should only require an "if (kind.module == module)"
check in GetNameForKind() to do this special casing.  WDYT?

> >  - Do you need the casts here?  Surely we want to generated bindings to
break
> at
> > compile time if someone uses the wrong typed constant as the default to a
> > particular field.
> 
>  I need it because mojom have signed and unsigned data, while java doesn't
have
> any unsigned type. I convert unsigned value into signed one (which mean that a
> uint8 of 128 will be a byte of -128). This is a little unfortunate, but I do
not
> have a good solution.

As discussed offline, this is only necessary for literal values.  I think it's
worth special casing the literal values to have this cast, and avoid it on
non-literal values, both for safety and for code clarity in the generated code,
but if you strongly feel this is not worth the effort then I won't block the
review over this.
 
> >  - I don't think you need a newline between each field declaration.
> 
>  Done
>

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java:21:
}
An extra indentation block seems to have crept in here (from "public int x;" to
the end, and in all other struct types.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Response.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Response.java:19:
pipe = new org.chromium.mojo.system.InvalidHandle();
Assuming an InvalidHandle isn't really stateful, you might want to consider a
static singleton INVALID_HANDLE to avoid having to allocate a new one each time
(and causing GC pressure) - fine to do this in another CL if you agree.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
File mojo/public/tools/bindings/generators/java_templates/enum.java.tmpl
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/enum.java.tmpl:4:
{{enum_def(enum, true)}}
Could you remove the enum CL changes from this CL please.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
File mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:6:
{% set has_response = method.response_parameters %}
has_response doesn't seem to be used?

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:7:
{{ 'static ' if method else 'public '}}final class {{struct|name}} extends
{{method|super_class}} {
I'm assuming the super_class filter will be used by the interface CL?  If so,
please rename "method"->"interface_method" and
"super_class/GetSuperClass"->"struct_super_class/GetStructSuperClass".

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:26:
{{field|name}} = {{build_default(module, field.kind, field.default)|indent(8)}};
nit - I think it would be clearer to separate out InvalidHandle() from
build_defaults - e.g., revert the change to build_default, and just do:
  {%   if field.default %}
         {{field|name}} = {{build_default(module, field.kind,
field.default)|indent(8)}};
  {%   elif field.kind|is_handle %} 
         {{field|name}} = new org.chromium.mojo.system.InvalidHandle();
  {%   endif %}

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

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/mojom_java_generator.py:13: import
mojom.generate.pack as pack
nit - remove unneeded import.

[qsr, 2014-06-05 11:46:10]

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java:21:
}
On 2014/06/05 11:01:17, rmcilroy wrote:
> An extra indentation block seems to have crept in here (from "public int x;"
to
> the end, and in all other struct types.

Done.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Response.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Response.java:19:
pipe = new org.chromium.mojo.system.InvalidHandle();
On 2014/06/05 11:01:17, rmcilroy wrote:
> Assuming an InvalidHandle isn't really stateful, you might want to consider a
> static singleton INVALID_HANDLE to avoid having to allocate a new one each
time
> (and causing GC pressure) - fine to do this in another CL if you agree.

Done.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
File mojo/public/tools/bindings/generators/java_templates/enum.java.tmpl
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/enum.java.tmpl:4:
{{enum_def(enum, true)}}
On 2014/06/05 11:01:17, rmcilroy wrote:
> Could you remove the enum CL changes from this CL please.

 Forgot to use the right git cl upload parameters. Sorry.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
File mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:6:
{% set has_response = method.response_parameters %}
On 2014/06/05 11:01:17, rmcilroy wrote:
> has_response doesn't seem to be used?

Done.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:7:
{{ 'static ' if method else 'public '}}final class {{struct|name}} extends
{{method|super_class}} {
On 2014/06/05 11:01:17, rmcilroy wrote:
> I'm assuming the super_class filter will be used by the interface CL?  If so,
> please rename "method"->"interface_method" and
> "super_class/GetSuperClass"->"struct_super_class/GetStructSuperClass".

Done.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:26:
{{field|name}} = {{build_default(module, field.kind, field.default)|indent(8)}};
On 2014/06/05 11:01:17, rmcilroy wrote:
> nit - I think it would be clearer to separate out InvalidHandle() from
> build_defaults - e.g., revert the change to build_default, and just do:
>   {%   if field.default %}
>          {{field|name}} = {{build_default(module, field.kind,
> field.default)|indent(8)}};
>   {%   elif field.kind|is_handle %} 
>          {{field|name}} = new org.chromium.mojo.system.InvalidHandle();
>   {%   endif %}
>  

Done.

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

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/mojom_java_generator.py:13: import
mojom.generate.pack as pack
On 2014/06/05 11:01:17, rmcilroy wrote:
> nit - remove unneeded import.

Done.

[qsr, 2014-06-05 11:46:11]

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Point.java:21:
}
On 2014/06/05 11:01:17, rmcilroy wrote:
> An extra indentation block seems to have crept in here (from "public int x;"
to
> the end, and in all other struct types.

Done.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Response.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/sample/Response.java:19:
pipe = new org.chromium.mojo.system.InvalidHandle();
On 2014/06/05 11:01:17, rmcilroy wrote:
> Assuming an InvalidHandle isn't really stateful, you might want to consider a
> static singleton INVALID_HANDLE to avoid having to allocate a new one each
time
> (and causing GC pressure) - fine to do this in another CL if you agree.

Done.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
File mojo/public/tools/bindings/generators/java_templates/enum.java.tmpl
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/enum.java.tmpl:4:
{{enum_def(enum, true)}}
On 2014/06/05 11:01:17, rmcilroy wrote:
> Could you remove the enum CL changes from this CL please.

 Forgot to use the right git cl upload parameters. Sorry.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
File mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl
(right):

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:6:
{% set has_response = method.response_parameters %}
On 2014/06/05 11:01:17, rmcilroy wrote:
> has_response doesn't seem to be used?

Done.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:7:
{{ 'static ' if method else 'public '}}final class {{struct|name}} extends
{{method|super_class}} {
On 2014/06/05 11:01:17, rmcilroy wrote:
> I'm assuming the super_class filter will be used by the interface CL?  If so,
> please rename "method"->"interface_method" and
> "super_class/GetSuperClass"->"struct_super_class/GetStructSuperClass".

Done.

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:26:
{{field|name}} = {{build_default(module, field.kind, field.default)|indent(8)}};
On 2014/06/05 11:01:17, rmcilroy wrote:
> nit - I think it would be clearer to separate out InvalidHandle() from
> build_defaults - e.g., revert the change to build_default, and just do:
>   {%   if field.default %}
>          {{field|name}} = {{build_default(module, field.kind,
> field.default)|indent(8)}};
>   {%   elif field.kind|is_handle %} 
>          {{field|name}} = new org.chromium.mojo.system.InvalidHandle();
>   {%   endif %}
>  

Done.

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

https://chromiumcodereview.appspot.com/306893010/diff/20001/mojo/public/tools...
mojo/public/tools/bindings/generators/mojom_java_generator.py:13: import
mojom.generate.pack as pack
On 2014/06/05 11:01:17, rmcilroy wrote:
> nit - remove unneeded import.

Done.

[qsr, 2014-06-23 13:42:48]

Gentle ping?

[rmcilroy, 2014-06-23 13:54:35]

Apologies I forgot about this one.  When the last response came in I was still
waiting for a comment on one set of comments (see below).

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Thing.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Thing.java:18:
public int shape = (int)
org.chromium.mojo.bindings.test.imported.Shape.SHAPE_RECTANGLE;

> > >  - Fields first then constructor (and other methods).  
> > >  - You don't need to output the full package for the field types or
default
> > > constant values since they are in the same package (by virtue of being in
> the
> > > same mojo module).
> > 
> >  I do. mojom can import other mojom that can be in another package. I do not
> > think it is worth having special case in generated code for this. Always
> having
> > FQC simplified the templates.
> 
> I realize that you can't avoid the package name on types imported from another
> package (due to potential for namespace collisions), but you can safely
exclude
> the package name for types in the current package / mojom.  Since this will be
> the common case, I think it is worth the special case to improve readability
of
> the generated code, and it should only require an "if (kind.module == module)"
> check in GetNameForKind() to do this special casing.  WDYT?

I was waiting for a response to this.


> > >  - Do you need the casts here?  Surely we want to generated bindings to
> break
> > at
> > > compile time if someone uses the wrong typed constant as the default to a
> > > particular field.
> > 
> >  I need it because mojom have signed and unsigned data, while java doesn't
> have
> > any unsigned type. I convert unsigned value into signed one (which mean that
a
> > uint8 of 128 will be a byte of -128). This is a little unfortunate, but I do
> not
> > have a good solution.
> 
> As discussed offline, this is only necessary for literal values.  I think it's
> worth special casing the literal values to have this cast, and avoid it on
> non-literal values, both for safety and for code clarity in the generated
code,
> but if you strongly feel this is not worth the effort then I won't block the
> review over this.

And possibly this (even if just a NACK).

[qsr, 2014-06-23 16:14:06]

Sorry. Missed the comment that were on the previous patch instead of the last
one.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
File mojo/android/javatests/src/org/chromium/mojo/TestHelper.java (right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/TestHelper.java:20: public static
void assertArrayEquals(Object a, Object b) {
 Good point. Deleted.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/android/java...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:134:
private static void checkField(Field field, Class<?> expectedClass) {
On 2014/06/05 11:01:17, rmcilroy wrote:
> On 2014/06/04 08:42:13, qsr wrote:
> > On 2014/06/03 22:53:22, rmcilroy wrote:
> > > Would it be possible to parameterize this by the type and do the check on
> the
> > > field value here too? E.g., something like:
> > >  private static <T> void checkField(Object obj, Field field, T
> expectedValue)
> > {
> > >   assertEquals(expectedValue, (T) field.get(obj));
> > >  ...
> > > 
> > > Then both checks could be done as a:
> > >   checkField<byte>(test, getField("foo"), 12);
> > > WDYT?
> > 
> >  This will transform primary type into boxed type. Also, will be a problem
for
> > float and double as I won't be able to use an almostEquals. Should we have 2
> > methods to handle this case?
> 
> I think it should be fine just to have one - you aren't using almostEquals at
> the moment and I don't think you would need to check a double with almostEqual
> in the checkField method, since this will only really be being checked for
> default values where an exact value can be used.  For other checks (e.g., when
> serializing / deserializing) there is no need to check the field types /
> modifiers only the values in the field, so we can use almostEquals directly in
> those test cases.  
> 

Done.

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Thing.java
(right):

https://chromiumcodereview.appspot.com/306893010/diff/10001/mojo/not-to-commi...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/imported/Thing.java:18:
public int shape = (int)
org.chromium.mojo.bindings.test.imported.Shape.SHAPE_RECTANGLE;
On 2014/06/05 11:01:17, rmcilroy wrote:
> On 2014/06/04 08:42:13, qsr wrote:
> > On 2014/06/03 22:53:22, rmcilroy wrote:
> > > A couple of things on formatting:
> > >  - I think it would be better to set the defaults in the constructor (like
> the
> > > C++ bindings). 
> > 
> >  Ok. Any reason for this?
> 
> I think it's a bit more idiomatic, but also could be an optimization since
field
> initializations are always done before every constructor (even if the fields
are
> overwritten in the constructor), so doing it this way avoids setting the
> defaults where they aren't needed (e.g., in a deserialization constructor?),
> which could avoid GC pressure if we ever allow object defaults.

 Deserialization might be missing values, which would need to use the default.
So it would use the same constructor.

> 
> > >  - Fields first then constructor (and other methods).  
> > >  - You don't need to output the full package for the field types or
default
> > > constant values since they are in the same package (by virtue of being in
> the
> > > same mojo module).
> > 
> >  I do. mojom can import other mojom that can be in another package. I do not
> > think it is worth having special case in generated code for this. Always
> having
> > FQC simplified the templates.
> 
> I realize that you can't avoid the package name on types imported from another
> package (due to potential for namespace collisions), but you can safely
exclude
> the package name for types in the current package / mojom.  Since this will be
> the common case, I think it is worth the special case to improve readability
of
> the generated code, and it should only require an "if (kind.module == module)"
> check in GetNameForKind() to do this special casing.  WDYT?

 Just did it. It is not that easy though. Passing the module everywhere is
cumbersome, and contextfilter doesn't get you the upper template parameter when
used in a externally defined macro. I solved this by adding methods in the
generator to parametrize the global variable of the templates.

> 
> > >  - Do you need the casts here?  Surely we want to generated bindings to
> break
> > at
> > > compile time if someone uses the wrong typed constant as the default to a
> > > particular field.
> > 
> >  I need it because mojom have signed and unsigned data, while java doesn't
> have
> > any unsigned type. I convert unsigned value into signed one (which mean that
a
> > uint8 of 128 will be a byte of -128). This is a little unfortunate, but I do
> not
> > have a good solution.
> 
> As discussed offline, this is only necessary for literal values.  I think it's
> worth special casing the literal values to have this cast, and avoid it on
> non-literal values, both for safety and for code clarity in the generated
code,
> but if you strongly feel this is not worth the effort then I won't block the
> review over this.

 I really do not think it is worth changing this. Especially because this is now
mostly only literal values.

>  
> > >  - I don't think you need a newline between each field declaration.
> > 
> >  Done
> > 
>

[rmcilroy, 2014-06-23 18:09:36]

Nice, thanks.  LGTM assuming you address the final comments.

https://codereview.chromium.org/306893010/diff/10001/mojo/android/javatests/s...
File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java
(right):

https://codereview.chromium.org/306893010/diff/10001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:243:
assertFooEquals(typicalFoo, typicalFoo);
On 2014/06/05 11:01:17, rmcilroy wrote:
> On 2014/06/04 08:42:13, qsr wrote:
> > On 2014/06/03 22:53:22, rmcilroy wrote:
> > > Not sure why you are checking the object is equal to itself, especially
> since
> > > the equals check is test code and not something generated by the bindings.

> > I'm
> > > also not entirely sure on the reasoning behind the createFoo code - this
> seem
> > a
> > > lot of complex code (which will break whenever the Foo object is modified
in
> > the
> > > mojom file) and doesn't really exercise the generated binding code much. 
I
> > > think simply checking that Foo and Bar are generated with fields of the
> > correct
> > > type and have the correct initial default values is enough here.
> > 
> >  Well, this will be necessary as soon as I add
serialization/deserialization
> > which will come soon. If you prefer I remove this for now, no problem.
> > 
> >  Also this will not break anymore than any of the other test if we change
the
> > name or type of a field. And this will not break if we add more fields.
Mainly
> > it is as you wish if you prefer this now or with the serialization CL.
> 
> I would prefer this in the followup serialization CL please.  I think it
would
> still be worthwhile having a test in this CL of the Foo (and possibly Bar)
> class's field layouts and their default values (e.g., lines 245 to 253),
similar
> to testStructDefaultValue above, since these are quite complex types and so a
> good test on the bindings generator. 

I still think this needs addressing - I think the "assertFooEquals(typicalFoo,
typicalFoo);" isn't testing anything, and would prefer that "createFoo",
"assertBarEquals", etc. are only added when they are actually used by the
serialisation code.  The tests in L245-L253 can go in testStructDefaults()

https://codereview.chromium.org/306893010/diff/120001/mojo/android/javatests/...
File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java
(right):

https://codereview.chromium.org/306893010/diff/120001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:175:
public void testStructDefaultValue() throws NoSuchFieldException,
SecurityException {
nit - testStructDefaults()

https://codereview.chromium.org/306893010/diff/120001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:180:
checkField(DefaultsTest.class.getField("a0"), byte.class);
Do the same as checkConstantField for checkField and replace all:
         assertEquals((byte) -12, test.a0);
         checkField(DefaultsTest.class.getField("a0"), byte.class);
with:
         checkField(DefaultsTest.class.getField("a0"), byte.class, (byte) -12);

etc.,

https://codereview.chromium.org/306893010/diff/120001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:184: 
nit - remove newlines between checks when you've updated checkField()

https://codereview.chromium.org/306893010/diff/120001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:253:
assertFooEquals(typicalFoo, typicalFoo);
Please see comment on Patch-Set 2

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:5:
{{ 'static ' if inner_class else 'public '}}final class {{struct|name}} extends
org.chromium.mojo.bindings.Struct {
nit -  replace "{{ 'static ' if inner_class else 'public '}}final" with "{{
'static' if inner_class else 'public'}} final"  (move the spaces out of the
if/else and before the "final", if this works)

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/generate/template_expander.py
(right):

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/generate/template_expander.py:33: def
ApplyTemplate(mojo_generator, base_dir, path_to_template, params,
You might want to move these changes to a different CL for clarity (up to
viettrungluu, I don't mind either way).

[viettrungluu, 2014-06-23 23:19:54]

Rubberstamp-ish LGTM, with a small request for comments/docstrings.

(I agree that factoring out the generator.py change would be nicer, but it's
small enough that I can live with it.)

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/generate/generator.py (right):

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/generate/generator.py:102: return {}
A docstring explaining this would be nice.

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/generate/generator.py:105: return {}
ditto

[qsr, 2014-06-24 13:23:59]

https://codereview.chromium.org/306893010/diff/120001/mojo/android/javatests/...
File mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java
(right):

https://codereview.chromium.org/306893010/diff/120001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:175:
public void testStructDefaultValue() throws NoSuchFieldException,
SecurityException {
On 2014/06/23 18:09:36, rmcilroy wrote:
> nit - testStructDefaults()

Done.

https://codereview.chromium.org/306893010/diff/120001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:180:
checkField(DefaultsTest.class.getField("a0"), byte.class);
On 2014/06/23 18:09:36, rmcilroy wrote:
> Do the same as checkConstantField for checkField and replace all:
>          assertEquals((byte) -12, test.a0);
>          checkField(DefaultsTest.class.getField("a0"), byte.class);
> with:
>          checkField(DefaultsTest.class.getField("a0"), byte.class, (byte)
-12);
> 
> etc.,

Done.

https://codereview.chromium.org/306893010/diff/120001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:184: 
On 2014/06/23 18:09:36, rmcilroy wrote:
> nit - remove newlines between checks when you've updated checkField()

Done.

https://codereview.chromium.org/306893010/diff/120001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/BindingsTest.java:253:
assertFooEquals(typicalFoo, typicalFoo);
On 2014/06/23 18:09:36, rmcilroy wrote:
> Please see comment on Patch-Set 2

 I don't really agree, but won't fight on this.

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/struct_definition.tmpl:5:
{{ 'static ' if inner_class else 'public '}}final class {{struct|name}} extends
org.chromium.mojo.bindings.Struct {
On 2014/06/23 18:09:36, rmcilroy wrote:
> nit -  replace "{{ 'static ' if inner_class else 'public '}}final" with "{{
> 'static' if inner_class else 'public'}} final"  (move the spaces out of the
> if/else and before the "final", if this works)

Done.

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/pylib/mojom/generate/generator.py (right):

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/generate/generator.py:102: return {}
On 2014/06/23 23:19:54, viettrungluu wrote:
> A docstring explaining this would be nice.

Done.

https://codereview.chromium.org/306893010/diff/120001/mojo/public/tools/bindi...
mojo/public/tools/bindings/pylib/mojom/generate/generator.py:105: return {}
On 2014/06/23 23:19:54, viettrungluu wrote:
> ditto

Done.

[qsr, 2014-06-24 13:24:03]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-06-24 13:25:06]

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

[qsr, 2014-06-24 14:15:09]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-06-24 14:15:43]

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

[qsr, 2014-06-24 15:27:57]

The CQ bit was checked by qsr@chromium.org

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

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

[qsr, 2014-06-24 15:41:52]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-06-24 15:42:08]

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

[commit-bot: I haz the power, 2014-06-24 18:03:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)

[commit-bot: I haz the power, 2014-06-24 18:19:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-24 18:19:53]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)

[qsr, 2014-06-25 08:01:25]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-06-25 08:02:04]

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

[qsr, 2014-06-25 09:06:55]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-06-25 09:08:13]

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

[qsr, 2014-06-25 09:54:31]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-06-25 09:55:24]

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

[qsr, 2014-06-25 11:52:36]

Committed patchset #15 manually as r279677 (presubmit successful).

[qsr, 2014-06-25 13:08:16]

The CQ bit was checked by qsr@chromium.org

[qsr, 2014-06-25 13:08:24]

The CQ bit was unchecked by qsr@chromium.org

[qsr, 2014-06-25 13:29:04]

Committed patchset #16 manually as r279707 (presubmit successful).