mojo: generate Proxies and Stubs for java bindings.

mojo: generate Proxies and Stubs for java bindings.

This CL allows to generate Stub and Proxy of mojo service in java. This
allows to call interfaces method through message pipes.

Committed: https://chromium.googlesource.com/chromium/src/+/b2ef91ee4abe584e712db7a51d47a1b6d9a96e57

Committed: https://crrev.com/063e1e3acae7473399f5064c66296de55b0ac04b
Cr-Commit-Position: refs/heads/master@{#292154}

[qsr, 2014-07-23 14:56:53]

This is not ready to land, but given that this is very big, and that I will be
OoO for 3 weeks, it might be useful to start looking at it at your own pace.

 The main things missing is using constants in the generation for the mehod
identifiers, and adding more tests.

https://codereview.chromium.org/411913002/diff/1/mojo/not-to-commit/mojo_publ...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/sample/PortInternal.java
(right):

https://codereview.chromium.org/411913002/diff/1/mojo/not-to-commit/mojo_publ...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/sample/PortInternal.java:63:
case 0: {
I'll add constants for those identifiers.

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

https://codereview.chromium.org/411913002/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:360: S
service = (S) builder.connect(handle);
See my comment on Interface.Builder for why this is needed.

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

https://codereview.chromium.org/411913002/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:257: if (v
instanceof Interface.BaseProxy) {
I do not like this, but I do not have a good idea on how to do this. Mainly, if
someone pass an implementation, we need to create the stub on a handle and pass
the other handle, but if someone pass a Proxy, we want to extract the handle
from it and send it.

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

https://codereview.chromium.org/411913002/diff/1/mojo/public/java/bindings/sr...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:164: *
@param <P> the type of the proxy the builder can handle. To be noted, P always
extends I.
I don't think I have a way to express that P must extends Proxy as well as I.
Unfortunately you can not use I & Proxy because I is a generic parameter. I
tried a few things, but nothing worked.

 I do need to use P, because when constructing a Proxy, having the real class
allows to have access to the setErrorHandler and close method that are necessary
to work with those.

[qsr, 2014-08-19 14:31:53]

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

https://chromiumcodereview.appspot.com/411913002/diff/1/mojo/public/java/bind...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:360: S
service = (S) builder.connect(handle);
 Found a way to not need this unchecked cast (or any cast in fact.)

[qsr, 2014-08-20 09:43:37]

Ordinal now use a constant.

[ppi, 2014-08-20 16:32:51]

Please find some remarks from the first pass (I haven't look at *.tmpl yet)
below.

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

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:44: * The
number of handle in the message.
nit: s/handle/handles/

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:356: *
Deserializes a |ServiceHandle| at the given offset.
nit: ServiceHandle seems not to be a defined thing, maybe "Deserializes a
service handle at the given offset." + "@return |Proxy| to the service" ?

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:361: if
(index == -1) {
Getting a value less than zero here seems to mean that the message was invalid.
Why fail silently (as opposed to allowing Validator to throw
DeserializationException)?

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:365:
MessagePipeHandle handle = mMessage.handles.get(index).toUntypedHandle()
Can we use readHandle(offset).toUntypedHandle() ?

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:509:
result[i] = d.readServiceInterface(8 + 4 * i, builder);
Should we use BindingsHelper.* constants?

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/DelegatingConnectionErrorHandler.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/DelegatingConnectionErrorHandler.java:20:
* handler to be garbage collected.
nit: how about adding a comment that the Boolean is unused, or maybe producing a
set:

Set<Object> weakHashSet = Collections.newSetFromMap(
        new WeakHashMap<Object, Boolean>());

?

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:247:
public <T extends Interface> void encode(T v, int offset, Interface.Builder<T,
?> builder) {
meta question - what are the codepaths that end up using this to encode a proxy?

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:256: // If
the instance is a proxy, pass the handle instead of creating a new stub.
Could we clarify - handle to what?

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:368: *
Encodes an array of doubles.result
Seems unintentional.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:409: *
Encodes an array of {@link Interface}.
nit: s/Interface/InterfaceRequest/

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:20: * A
proxy to the a mojo interface. This is base class for all generated proxy. It
implements
nit: s/generated proxy/generated proxies/

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:21: *
the Interface and each time a method is called, the parameter are serialized and
sent to the
nit: s/parameter/parameters/

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:41:
abstract class BaseProxy implements Proxy {
nit: it feels weird to have Proxy being the base class / interface for
BaseProxy. Maybe it could be ProxyImpl or BasicProxy?

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:49: *
The {@link MessageReceiverWithResponder} that will received serialized message
for each
nit: s/serialized/a serialized/, s/received/receive/

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:161: *
|MessagePipeHandle|.
nit: maybe add "through an implicitly created stub" here?

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:218: *
the first element is the request, and the second element is a proxy. The proxy
can be
The order is actually the other way around? I like the one from code more than
the one from comments :).

[qsr, 2014-08-21 08:49:35]

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

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:44: * The
number of handle in the message.
On 2014/08/20 16:32:49, ppi wrote:
> nit: s/handle/handles/

Done.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:356: *
Deserializes a |ServiceHandle| at the given offset.
On 2014/08/20 16:32:49, ppi wrote:
> nit: ServiceHandle seems not to be a defined thing, maybe "Deserializes a
> service handle at the given offset." + "@return |Proxy| to the service" ?

Done.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:361: if
(index == -1) {
On 2014/08/20 16:32:49, ppi wrote:
> Getting a value less than zero here seems to mean that the message was
invalid.
> Why fail silently (as opposed to allowing Validator to throw
> DeserializationException)?

 Getting a value equals to -1 means than the handle was invalid. This is not a
failure, and this corresponds to not having sent an implementation on the other
side.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:365:
MessagePipeHandle handle = mMessage.handles.get(index).toUntypedHandle()
On 2014/08/20 16:32:49, ppi wrote:
> Can we use readHandle(offset).toUntypedHandle() ?

 Seems we now can. When I started this, I needed to keep track of every
deserialized handle, and this didn't work.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:509:
result[i] = d.readServiceInterface(8 + 4 * i, builder);
On 2014/08/20 16:32:49, ppi wrote:
> Should we use BindingsHelper.* constants?

Done.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/DelegatingConnectionErrorHandler.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/DelegatingConnectionErrorHandler.java:20:
* handler to be garbage collected.
On 2014/08/20 16:32:50, ppi wrote:
> nit: how about adding a comment that the Boolean is unused, or maybe producing
a
> set:
> 
> Set<Object> weakHashSet = Collections.newSetFromMap(
>         new WeakHashMap<Object, Boolean>());
> 
> ?
> 

 I didn't know about newSetFromMap, thanks.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:247:
public <T extends Interface> void encode(T v, int offset, Interface.Builder<T,
?> builder) {
On 2014/08/20 16:32:50, ppi wrote:
> meta question - what are the codepaths that end up using this to encode a
proxy?

I have 3 Interfaces, I1, I2, I3

I1 sends an interface to I2, then I2 sends this interface to I3.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:256: // If
the instance is a proxy, pass the handle instead of creating a new stub.
On 2014/08/20 16:32:50, ppi wrote:
> Could we clarify - handle to what?

Done.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:368: *
Encodes an array of doubles.result
On 2014/08/20 16:32:50, ppi wrote:
> Seems unintentional.

Done.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:409: *
Encodes an array of {@link Interface}.
On 2014/08/20 16:32:50, ppi wrote:
> nit: s/Interface/InterfaceRequest/

Done.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:20: * A
proxy to the a mojo interface. This is base class for all generated proxy. It
implements
On 2014/08/20 16:32:50, ppi wrote:
> nit: s/generated proxy/generated proxies/

Done.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:21: *
the Interface and each time a method is called, the parameter are serialized and
sent to the
On 2014/08/20 16:32:50, ppi wrote:
> nit: s/parameter/parameters/

Done.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:41:
abstract class BaseProxy implements Proxy {
On 2014/08/20 16:32:50, ppi wrote:
> nit: it feels weird to have Proxy being the base class / interface for
> BaseProxy. Maybe it could be ProxyImpl or BasicProxy?

 I went with AbstractProxy. Is that fine with you?

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:49: *
The {@link MessageReceiverWithResponder} that will received serialized message
for each
On 2014/08/20 16:32:50, ppi wrote:
> nit: s/serialized/a serialized/, s/received/receive/

Done.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:161: *
|MessagePipeHandle|.
On 2014/08/20 16:32:50, ppi wrote:
> nit: maybe add "through an implicitly created stub" here?

Done.

https://chromiumcodereview.appspot.com/411913002/diff/80001/mojo/public/java/...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:218: *
the first element is the request, and the second element is a proxy. The proxy
can be
On 2014/08/20 16:32:50, ppi wrote:
> The order is actually the other way around? I like the one from code more than
> the one from comments :).

Done.

[rmcilroy, 2014-08-21 14:43:53]

First pass, I've also not looked at the tmpl file yet.

https://codereview.chromium.org/411913002/diff/100001/mojo/android/javatests/...
File mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:39:
public static class NamedObjectImpl extends CapturingErrorHandler implements
NamedObject {
MockNamedObjectImpl? (and similarly for all below)?

https://codereview.chromium.org/411913002/diff/100001/mojo/not-to-commit/mojo...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/ImportedInterfaceInternal.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/not-to-commit/mojo...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/ImportedInterfaceInternal.java:64:
if (!header.validateHeader(0)) {
nit - only a single indent here.

https://codereview.chromium.org/411913002/diff/100001/mojo/not-to-commit/mojo...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/ImportedInterfaceInternal.java:64:
if (!header.validateHeader(0)) {
Please add a NO_FLAGS constant to MessageHeader and use it here instead of '0'

https://codereview.chromium.org/411913002/diff/100001/mojo/not-to-commit/mojo...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/ImportedInterfaceInternal.java:74:
return false;
move the "return false" into the catch block and add an explicit "default" case
with "return false: so that it's obvious where it is reached by.

https://codereview.chromium.org/411913002/diff/100001/mojo/not-to-commit/mojo...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/math/CalculatorInternal.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/not-to-commit/mojo...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/math/CalculatorInternal.java:59:
message.value = value;
Could you do the parameter building as constructor rather than setting the
fields manually?  In which case you might be able to make the fields final in
the params objects which might be an advantage.

https://codereview.chromium.org/411913002/diff/100001/mojo/not-to-commit/mojo...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/mojo/ConformanceTestInterfaceInternal.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/not-to-commit/mojo...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/mojo/ConformanceTestInterfaceInternal.java:170:
}
Could we do the "if (!header.validateHeader(0))" check once before the switch
statement rather than repeating it with every case statement?  Maybe you would
need some kind of "expectedFlagsForOrdinal" helper function to do this, so I'm
not sure how complex that might end up being.

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

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:356: *
Deserializes an interface  at the given offset.
remove extra space

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/DelegatingConnectionErrorHandler.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/DelegatingConnectionErrorHandler.java:21:
* handler to be garbage collected.
"to be"->"from being"

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

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:257: if (v
instanceof Interface.AbstractProxy) {
Would it work to have a getHandleOwnerMessageReciever method on Interface and
using that if it returns a non-null value?  I realize you want to keep the
"Interface" interface clean to avoid name clash with mojom interface members,
but don't you already have this issue where  mojo functions might overlap with
setErrorHandler() and any functions in Proxy (e.g. getMessageReceiver()), etc?

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/HandleOwner.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/HandleOwner.java:27:
public void close();
Is there a reason to override the close method here?

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:159: *
The |Builder| object allows to build proxies and stubs for a given interface. It
also allows
"allows to build"->"enables building of" (and in next sentence)

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/MessageReceiver.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/MessageReceiver.java:23:
@Override
Same question regarding override of close()

https://codereview.chromium.org/411913002/diff/100001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl:149:
class {{interface|name}}Internal {
Are you sure this is safe to do?  What if there is a mojom type with the same
name?  Personally I think this should be a private inner class, would that be
possible?

[ppi, 2014-08-21 14:47:14]

Thanks! Please find the second set of remarks below.

I have one high-level concern related with hiding the generated classes behind
Interface.BUILDER.

Ggenerating mojo bindings for a service, I think one expects, foremost:

- an interface for the service
- a class for proxy that implements it
- a class for the stub

These three are present alongside each other in the generated header file in
case of C++ bindings.

I see the clear benefit of hiding unneeded details by keeping proxy and stub
hidden behind BUILDER, but I'm concerned about the added surprise - using Java
bindings will feel quite different from the C++ ones.

I'm wondering if it would make sense to go for more verbosity (separate
Interface.java, InterfaceStub.java, InterfaceProxy.java files) getting
simplicity and alignment with C++ in return.

Wdyt?

https://codereview.chromium.org/411913002/diff/80001/mojo/public/java/binding...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://codereview.chromium.org/411913002/diff/80001/mojo/public/java/binding...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:41:
abstract class BaseProxy implements Proxy {
On 2014/08/21 08:49:35, qsr wrote:
> On 2014/08/20 16:32:50, ppi wrote:
> > nit: it feels weird to have Proxy being the base class / interface for
> > BaseProxy. Maybe it could be ProxyImpl or BasicProxy?
> 
>  I went with AbstractProxy. Is that fine with you?

Totally.

https://codereview.chromium.org/411913002/diff/100001/mojo/android/javatests/...
File mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:28: *
Testing interfaces generation.
If we don't want to impose a convention where the test files matches the mojom
sample it refers to, let's maybe add a note, e.g.

"Tests for interfaces / proxies / stubs generated for sample_factory.mojom."

?

https://codereview.chromium.org/411913002/diff/100001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:34:
private final List<Closeable> mCloseableToClose = new ArrayList<Closeable>();
nit: s/mCloseableToClose/mCloseablesToClose/

https://codereview.chromium.org/411913002/diff/100001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:79:
public boolean isCalled() {
nit: s/is/was/

https://codereview.chromium.org/411913002/diff/100001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:109:
public void createNamedObject(InterfaceRequest<NamedObject.Proxy> obj) {
I wonder if we should care that in C++ bindings we have InterfaceRequest
type-parametrized with Interface instead of it's proxy. Adding a comment on
InterfaceRequest.java .

https://codereview.chromium.org/411913002/diff/100001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:139:
private void addToCloseable(Pair<? extends Closeable, ? extends Closeable> pair)
{
nit: s/addToCloseable/addCloseables/ maybe?

https://codereview.chromium.org/411913002/diff/100001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:151:
private <I extends Interface, P extends Interface.Proxy> P newProxyOverPipes(
nit: s/OverPipes/OverPipe/

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:166:
abstract class Builder<I extends Interface, P extends Proxy> {
Initially I was confused about this because I expected this to be an application
of the  builder design pattern. As this is not a "builder" in the oop sense, how
about calling it provider?

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java:17:
public class InterfaceRequest<P extends Interface.Proxy> implements
HandleOwner<MessagePipeHandle> {
I wonder if this would be more intuitive to use if InterfaceRequest had the
interface as the type parameter, not proxy (even though the semantics is "give
me interface impl for this proxy").

The usage would look like this: InterfaceRequest<Interface>. It seems that this
would match C++ bindings, where we have:

virtual void CreateNamedObject(mojo::InterfaceRequest<NamedObject> obj) = 0;

Wdyt? Would that be feasible?

[qsr, 2014-08-22 08:11:19]

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/android/jav...
File mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/android/jav...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:28: *
Testing interfaces generation.
On 2014/08/21 14:47:13, ppi wrote:
> If we don't want to impose a convention where the test files matches the mojom
> sample it refers to, let's maybe add a note, e.g.
> 
> "Tests for interfaces / proxies / stubs generated for sample_factory.mojom."
> 
> ?

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/android/jav...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:34:
private final List<Closeable> mCloseableToClose = new ArrayList<Closeable>();
On 2014/08/21 14:47:14, ppi wrote:
> nit: s/mCloseableToClose/mCloseablesToClose/

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/android/jav...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:39:
public static class NamedObjectImpl extends CapturingErrorHandler implements
NamedObject {
On 2014/08/21 14:43:52, rmcilroy wrote:
> MockNamedObjectImpl? (and similarly for all below)?

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/android/jav...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:79:
public boolean isCalled() {
On 2014/08/21 14:47:14, ppi wrote:
> nit: s/is/was/

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/android/jav...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:109:
public void createNamedObject(InterfaceRequest<NamedObject.Proxy> obj) {
On 2014/08/21 14:47:13, ppi wrote:
> I wonder if we should care that in C++ bindings we have InterfaceRequest
> type-parametrized with Interface instead of it's proxy. Adding a comment on
> InterfaceRequest.java .

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/android/jav...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:139:
private void addToCloseable(Pair<? extends Closeable, ? extends Closeable> pair)
{
On 2014/08/21 14:47:14, ppi wrote:
> nit: s/addToCloseable/addCloseables/ maybe?

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/android/jav...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:151:
private <I extends Interface, P extends Interface.Proxy> P newProxyOverPipes(
On 2014/08/21 14:47:14, ppi wrote:
> nit: s/OverPipes/OverPipe/

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/not-to-comm...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/ImportedInterfaceInternal.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/not-to-comm...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/ImportedInterfaceInternal.java:64:
if (!header.validateHeader(0)) {
On 2014/08/21 14:43:52, rmcilroy wrote:
> nit - only a single indent here.

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/not-to-comm...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/ImportedInterfaceInternal.java:64:
if (!header.validateHeader(0)) {
On 2014/08/21 14:43:52, rmcilroy wrote:
> Please add a NO_FLAGS constant to MessageHeader and use it here instead of '0'

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/not-to-comm...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/imported/ImportedInterfaceInternal.java:74:
return false;
On 2014/08/21 14:43:52, rmcilroy wrote:
> move the "return false" into the catch block and add an explicit "default"
case
> with "return false: so that it's obvious where it is reached by.

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/not-to-comm...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/math/CalculatorInternal.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/not-to-comm...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/math/CalculatorInternal.java:59:
message.value = value;
On 2014/08/21 14:43:52, rmcilroy wrote:
> Could you do the parameter building as constructor rather than setting the
> fields manually?  In which case you might be able to make the fields final in
> the params objects which might be an advantage.

 The generation of the message is the standard struct generation, so I cannot
easily change anything. Moreober, I need to deserialize the message in the stub,
so I cannot have the field as final given the way deserialization works.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/not-to-comm...
File
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/mojo/ConformanceTestInterfaceInternal.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/not-to-comm...
mojo/not-to-commit/mojo_public_test_interfaces/src/org/chromium/mojo/bindings/test/mojom/mojo/ConformanceTestInterfaceInternal.java:170:
}
On 2014/08/21 14:43:52, rmcilroy wrote:
> Could we do the "if (!header.validateHeader(0))" check once before the switch
> statement rather than repeating it with every case statement?  Maybe you would
> need some kind of "expectedFlagsForOrdinal" helper function to do this, so I'm
> not sure how complex that might end up being.

 It was relatively easy. Done.

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

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java:356: *
Deserializes an interface  at the given offset.
On 2014/08/21 14:43:52, rmcilroy wrote:
> remove extra space

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/DelegatingConnectionErrorHandler.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/DelegatingConnectionErrorHandler.java:21:
* handler to be garbage collected.
On 2014/08/21 14:43:52, rmcilroy wrote:
> "to be"->"from being"

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:257: if (v
instanceof Interface.AbstractProxy) {
On 2014/08/21 14:43:52, rmcilroy wrote:
> Would it work to have a getHandleOwnerMessageReciever method on Interface and
> using that if it returns a non-null value?  I realize you want to keep the
> "Interface" interface clean to avoid name clash with mojom interface members,
> but don't you already have this issue where  mojo functions might overlap with
> setErrorHandler() and any functions in Proxy (e.g. getMessageReceiver()), etc?

 The other issue is that Interface will be implemented when implementing a
service, and I don't want to force to implement methods that returns null and do
not give any value. And we would still need a instanceof n Proxy to implement
this because not all MessageReceiver are HandleOwner.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/HandleOwner.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/HandleOwner.java:27:
public void close();
On 2014/08/21 14:43:52, rmcilroy wrote:
> Is there a reason to override the close method here?

 Because close on Handles do not throw IOException, so redefining it here
ensures that it is the case for all HandleOwner.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:159: *
The |Builder| object allows to build proxies and stubs for a given interface. It
also allows
On 2014/08/21 14:43:52, rmcilroy wrote:
> "allows to build"->"enables building of" (and in next sentence)

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:166:
abstract class Builder<I extends Interface, P extends Proxy> {
On 2014/08/21 14:47:14, ppi wrote:
> Initially I was confused about this because I expected this to be an
application
> of the  builder design pattern. As this is not a "builder" in the oop sense,
how
> about calling it provider?

 Changed it to Binder. It is unfortunately the same name as the android binder
class, but given it is the same concept I do not have a better idea.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java:17:
public class InterfaceRequest<P extends Interface.Proxy> implements
HandleOwner<MessagePipeHandle> {
On 2014/08/21 14:47:14, ppi wrote:
> I wonder if this would be more intuitive to use if InterfaceRequest had the
> interface as the type parameter, not proxy (even though the semantics is "give
> me interface impl for this proxy").
> 
> The usage would look like this: InterfaceRequest<Interface>. It seems that
this
> would match C++ bindings, where we have:
> 
> virtual void CreateNamedObject(mojo::InterfaceRequest<NamedObject> obj) = 0;
> 
> Wdyt? Would that be feasible?

Done.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/MessageReceiver.java
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/java...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/MessageReceiver.java:23:
@Override
On 2014/08/21 14:43:52, rmcilroy wrote:
> Same question regarding override of close()

 Same reason, I do not want to let the subclass throw IOException, and having to
handle those everywhere.

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/tool...
File
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl
(right):

https://chromiumcodereview.appspot.com/411913002/diff/100001/mojo/public/tool...
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl:149:
class {{interface|name}}Internal {
On 2014/08/21 14:43:52, rmcilroy wrote:
> Are you sure this is safe to do?  What if there is a mojom type with the same
> name?  Personally I think this should be a private inner class, would that be
> possible?

 It would be possible, but the reason I put it there is because the interface
class would then be cluttered with a lot of code, which makes it harder to see
immediately the important part (the method to implement)

[ppi, 2014-08-25 13:19:09]

Thanks! Please find one more pass below.

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

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:262: }
What happens if the inner condition is false? Would that ever happen? Do we need
to encode -1 then?

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:20: * A
proxy to the a mojo interface. This is base class for all generated proxies. It
implements
nit: s/the a/a/

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:22: *
{@link MessageReceiverWithResponder}, passing it the response callback if
needed.
nit: s/passing it/along with/

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:112: *
Base implementation of Stubs. Stubs are message receiver that will deserialize
the payload
Base implementation of Stub. Stubs are message receivers that deserialize the
payload and call the appropriate method in the implementation. If the method
returns result, the stub serializes the response and sends it back.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:160: *
enables building of proxy from a |MessagePipeHandle| or bind an implementation
to a given
nit: s/bind/binding/ . Or maybe drop everything after the first sentence, it
duplicates documentation of public methods.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:171:
public void bind(MessagePipeHandle handle, I impl) {
Should the parameter order be reversed (as we bind impl to handle)?

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:172: //
The handle is intentionally leaked. It will be closed when the connected handle
Is it actually leaked, or the ownership is passed to the Router?

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:183:
public final void bind(InterfaceRequest<I> request, I impl) {
Likewise, wdyt about inverting the params?

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:215:
final void bind(Core core, Router router, I impl) {
Likewise, wdyt about param order (Core,I,Router)?

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:229:
protected abstract I[] newArray(int size);
Why this is needed?

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:234:
protected abstract Stub<I> buildStub(Core core, I impl);
Maybe getStub to match getInterfaceRequest?

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java
(right):

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java:10:
* Used in methods that return instances of remote objects. Allows to start using
the remote object
I see this is based on the C++ comment. Still, first sentence should state what
the object is, not how is it used.

Maybe: "One end of the data pipe representing a request to create an
implementation to be bound to it. The other end of the pipe is bound to a proxy,
which can be used immediately, while the InterfaceRequest is being sent."

[qsr, 2014-08-25 14:15:52]

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

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:262: }
On 2014/08/25 13:19:09, ppi wrote:
> What happens if the inner condition is false? Would that ever happen? Do we
need
> to encode -1 then?

 If the inner condition is false, then the proxy is not over a stub, but over
another type of MessageReceiver, we just need to go to the default case and
create a pair of handle, bind the proxy to one end, and send the other end away.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:20: * A
proxy to the a mojo interface. This is base class for all generated proxies. It
implements
On 2014/08/25 13:19:09, ppi wrote:
> nit: s/the a/a/

Done.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:22: *
{@link MessageReceiverWithResponder}, passing it the response callback if
needed.
On 2014/08/25 13:19:09, ppi wrote:
> nit: s/passing it/along with/

Done.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:112: *
Base implementation of Stubs. Stubs are message receiver that will deserialize
the payload
On 2014/08/25 13:19:09, ppi wrote:
> Base implementation of Stub. Stubs are message receivers that deserialize the
> payload and call the appropriate method in the implementation. If the method
> returns result, the stub serializes the response and sends it back.

Done.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:160: *
enables building of proxy from a |MessagePipeHandle| or bind an implementation
to a given
On 2014/08/25 13:19:09, ppi wrote:
> nit: s/bind/binding/ . Or maybe drop everything after the first sentence, it
> duplicates documentation of public methods.

Done.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:171:
public void bind(MessagePipeHandle handle, I impl) {
On 2014/08/25 13:19:09, ppi wrote:
> Should the parameter order be reversed (as we bind impl to handle)?

Done.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:172: //
The handle is intentionally leaked. It will be closed when the connected handle
On 2014/08/25 13:19:09, ppi wrote:
> Is it actually leaked, or the ownership is passed to the Router?

 The ownership is passed to router that is leaked, so the handle is leaked.
Tried to make it clearer. WDYT?

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:183:
public final void bind(InterfaceRequest<I> request, I impl) {
On 2014/08/25 13:19:09, ppi wrote:
> Likewise, wdyt about inverting the params?

Done.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:215:
final void bind(Core core, Router router, I impl) {
On 2014/08/25 13:19:09, ppi wrote:
> Likewise, wdyt about param order (Core,I,Router)?

Done.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:229:
protected abstract I[] newArray(int size);
On 2014/08/25 13:19:09, ppi wrote:
> Why this is needed?

 See Decoder.java:506

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:234:
protected abstract Stub<I> buildStub(Core core, I impl);
On 2014/08/25 13:19:09, ppi wrote:
> Maybe getStub to match getInterfaceRequest?

 As discussed, this is not really the same thing. buildStub is a abstract
factory method to allow writing the real method of the manager.
GetInterfaceRequest is part of the public interface.

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java
(right):

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java:10:
* Used in methods that return instances of remote objects. Allows to start using
the remote object
On 2014/08/25 13:19:09, ppi wrote:
> I see this is based on the C++ comment. Still, first sentence should state
what
> the object is, not how is it used.
> 
> Maybe: "One end of the data pipe representing a request to create an
> implementation to be bound to it. The other end of the pipe is bound to a
proxy,
> which can be used immediately, while the InterfaceRequest is being sent."

Done.

[ppi, 2014-08-25 14:32:02]

This lgtm, thanks! Two follow-up comments on comments below.

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

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:262: }
On 2014/08/25 14:15:51, qsr wrote:
> On 2014/08/25 13:19:09, ppi wrote:
> > What happens if the inner condition is false? Would that ever happen? Do we
> need
> > to encode -1 then?
> 
>  If the inner condition is false, then the proxy is not over a stub, but over
> another type of MessageReceiver, we just need to go to the default case and
> create a pair of handle, bind the proxy to one end, and send the other end
away.

Makes sense. Can you put this in a comment around there?

https://codereview.chromium.org/411913002/diff/180001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://codereview.chromium.org/411913002/diff/180001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:170: //
The router (and by consequence the handle) is intentionally leaked. It will be
closed
Clearer, thanks! Can we indicate who will close the handle / how it will be
closed?

[qsr, 2014-08-25 14:58:58]

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

https://codereview.chromium.org/411913002/diff/160001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:262: }
On 2014/08/25 14:32:02, ppi wrote:
> On 2014/08/25 14:15:51, qsr wrote:
> > On 2014/08/25 13:19:09, ppi wrote:
> > > What happens if the inner condition is false? Would that ever happen? Do
we
> > need
> > > to encode -1 then?
> > 
> >  If the inner condition is false, then the proxy is not over a stub, but
over
> > another type of MessageReceiver, we just need to go to the default case and
> > create a pair of handle, bind the proxy to one end, and send the other end
> away.
> 
> Makes sense. Can you put this in a comment around there?

Done.

https://codereview.chromium.org/411913002/diff/180001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://codereview.chromium.org/411913002/diff/180001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:170: //
The router (and by consequence the handle) is intentionally leaked. It will be
closed
On 2014/08/25 14:32:02, ppi wrote:
> Clearer, thanks! Can we indicate who will close the handle / how it will be
> closed?

Done.

[qsr, 2014-08-25 14:59:15]

Ross, do you want to do another pass?

[rmcilroy, 2014-08-26 09:42:06]

On 2014/08/25 14:59:15, qsr wrote:
> Ross, do you want to do another pass?

Yes I'll have a quick look at this this morning (sorry yesterday was a vacation
in the UK).  Thanks.

[rmcilroy, 2014-08-26 10:18:23]

lgtm with a couple of last suggestions. Thanks.

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

https://codereview.chromium.org/411913002/diff/100001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:257: if (v
instanceof Interface.AbstractProxy) {
On 2014/08/22 08:11:18, qsr wrote:
> On 2014/08/21 14:43:52, rmcilroy wrote:
> > Would it work to have a getHandleOwnerMessageReciever method on Interface
and
> > using that if it returns a non-null value?  I realize you want to keep the
> > "Interface" interface clean to avoid name clash with mojom interface
members,
> > but don't you already have this issue where  mojo functions might overlap
with
> > setErrorHandler() and any functions in Proxy (e.g. getMessageReceiver()),
etc?
> 
>  The other issue is that Interface will be implemented when implementing a
> service, and I don't want to force to implement methods that returns null and
do
> not give any value. And we would still need a instanceof n Proxy to implement
> this because not all MessageReceiver are HandleOwner.

Yeah, makes sense (re: not wanting the service to have to implement this
method).

https://codereview.chromium.org/411913002/diff/100001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl:149:
class {{interface|name}}Internal {
On 2014/08/22 08:11:19, qsr wrote:
> On 2014/08/21 14:43:52, rmcilroy wrote:
> > Are you sure this is safe to do?  What if there is a mojom type with the
same
> > name?  Personally I think this should be a private inner class, would that
be
> > possible?
> 
>  It would be possible, but the reason I put it there is because the interface
> class would then be cluttered with a lot of code, which makes it harder to see
> immediately the important part (the method to implement)

That makes sense.  I could still see clients wanting to create an API with an
Baz and BazInternal mojo API though. How about putting it in a separate package
as well as a suffix to the class name (e.g., foo.bar.internal.BazImpl for the
foo.bar.Baz interface)?  That way it should be a bit safer?

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

https://codereview.chromium.org/411913002/diff/200001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:369: *
Encodes an array of doubles
nit - add back the "."

https://codereview.chromium.org/411913002/diff/200001/mojo/public/java/bindin...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java
(right):

https://codereview.chromium.org/411913002/diff/200001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java:28:
* @param handle the handle to send away that will be connected to the
implementation.
I think this would be clearer as "@param handle the handle which will be sent
and will be connected to the implementation." (similarly on the field above)

https://codereview.chromium.org/411913002/diff/200001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/411913002/diff/200001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl:61:
{%- macro manager_class(interface, client, fqbc=False) -%}
not sure what fqbc means, could you make the variable name more descriptive
please.

[qsr, 2014-08-26 12:43:56]

https://codereview.chromium.org/411913002/diff/100001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl:149:
class {{interface|name}}Internal {
On 2014/08/26 10:18:23, rmcilroy wrote:
> On 2014/08/22 08:11:19, qsr wrote:
> > On 2014/08/21 14:43:52, rmcilroy wrote:
> > > Are you sure this is safe to do?  What if there is a mojom type with the
> same
> > > name?  Personally I think this should be a private inner class, would that
> be
> > > possible?
> > 
> >  It would be possible, but the reason I put it there is because the
interface
> > class would then be cluttered with a lot of code, which makes it harder to
see
> > immediately the important part (the method to implement)
> 
> That makes sense.  I could still see clients wanting to create an API with an
> Baz and BazInternal mojo API though. How about putting it in a separate
package
> as well as a suffix to the class name (e.g., foo.bar.internal.BazImpl for the
> foo.bar.Baz interface)?  That way it should be a bit safer?
> 

 The issue with this is that all internal class are right now package protected
and they would have to be public if we move those in their own package. WDYT?

[qsr, 2014-08-26 12:53:35]

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

https://codereview.chromium.org/411913002/diff/200001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Encoder.java:369: *
Encodes an array of doubles
On 2014/08/26 10:18:23, rmcilroy wrote:
> nit - add back the "." 

Done.

https://codereview.chromium.org/411913002/diff/200001/mojo/public/java/bindin...
File
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java
(right):

https://codereview.chromium.org/411913002/diff/200001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceRequest.java:28:
* @param handle the handle to send away that will be connected to the
implementation.
On 2014/08/26 10:18:23, rmcilroy wrote:
> I think this would be clearer as "@param handle the handle which will be sent
> and will be connected to the implementation." (similarly on the field above)

Done.

https://codereview.chromium.org/411913002/diff/200001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/411913002/diff/200001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl:61:
{%- macro manager_class(interface, client, fqbc=False) -%}
On 2014/08/26 10:18:23, rmcilroy wrote:
> not sure what fqbc means, could you make the variable name more descriptive
> please.

Done.

[ppi, 2014-08-26 13:54:40]

https://codereview.chromium.org/411913002/diff/220001/mojo/android/javatests/...
File mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java
(right):

https://codereview.chromium.org/411913002/diff/220001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:106:
public MockFactoryImpl() {
nit: initialize mClosed inline and skip the constructor?

https://codereview.chromium.org/411913002/diff/220001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:163:
public MockFactoryClientImpl() {
nit: initialize the fields inline and skip the constructor?

https://codereview.chromium.org/411913002/diff/220001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://codereview.chromium.org/411913002/diff/220001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:20: *
The close method is called when the connection to the interface is closed.
Please clarify - called by who? How the implementations are advised to implement
this? 

Does this work like a notification ("wasClosed()"), or is it called to take
action (calling .close() closes something)?

[qsr, 2014-08-26 14:57:09]

https://codereview.chromium.org/411913002/diff/220001/mojo/android/javatests/...
File mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java
(right):

https://codereview.chromium.org/411913002/diff/220001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:106:
public MockFactoryImpl() {
On 2014/08/26 13:54:40, ppi wrote:
> nit: initialize mClosed inline and skip the constructor?

Done.

https://codereview.chromium.org/411913002/diff/220001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/bindings/InterfacesTest.java:163:
public MockFactoryClientImpl() {
On 2014/08/26 13:54:40, ppi wrote:
> nit: initialize the fields inline and skip the constructor?

Done.

https://codereview.chromium.org/411913002/diff/220001/mojo/public/java/bindin...
File mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java
(right):

https://codereview.chromium.org/411913002/diff/220001/mojo/public/java/bindin...
mojo/public/java/bindings/src/org/chromium/mojo/bindings/Interface.java:20: *
The close method is called when the connection to the interface is closed.
 As discussed online, this is the contract of Closeable#close

[rmcilroy, 2014-08-26 17:04:03]

https://codereview.chromium.org/411913002/diff/100001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl:149:
class {{interface|name}}Internal {
On 2014/08/26 12:43:56, qsr wrote:
> On 2014/08/26 10:18:23, rmcilroy wrote:
> > On 2014/08/22 08:11:19, qsr wrote:
> > > On 2014/08/21 14:43:52, rmcilroy wrote:
> > > > Are you sure this is safe to do?  What if there is a mojom type with the
> > same
> > > > name?  Personally I think this should be a private inner class, would
that
> > be
> > > > possible?
> > > 
> > >  It would be possible, but the reason I put it there is because the
> interface
> > > class would then be cluttered with a lot of code, which makes it harder to
> see
> > > immediately the important part (the method to implement)
> > 
> > That makes sense.  I could still see clients wanting to create an API with
an
> > Baz and BazInternal mojo API though. How about putting it in a separate
> package
> > as well as a suffix to the class name (e.g., foo.bar.internal.BazImpl for
the
> > foo.bar.Baz interface)?  That way it should be a bit safer?
> > 
> 
>  The issue with this is that all internal class are right now package
protected
> and they would have to be public if we move those in their own package. WDYT?

In that case it's probably better to be in the same package, but could we have a
name which would never be chosen (or at lease less likely to be chosen) as a
mojo interface name - e.g., Baz_InternalImpl (where the '_' should never be in a
mojo name Interface name, right)?

[qsr, 2014-08-27 07:32:26]

https://codereview.chromium.org/411913002/diff/100001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/411913002/diff/100001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/java_templates/interface_definition.tmpl:149:
class {{interface|name}}Internal {
On 2014/08/26 17:04:03, rmcilroy wrote:
> On 2014/08/26 12:43:56, qsr wrote:
> > On 2014/08/26 10:18:23, rmcilroy wrote:
> > > On 2014/08/22 08:11:19, qsr wrote:
> > > > On 2014/08/21 14:43:52, rmcilroy wrote:
> > > > > Are you sure this is safe to do?  What if there is a mojom type with
the
> > > same
> > > > > name?  Personally I think this should be a private inner class, would
> that
> > > be
> > > > > possible?
> > > > 
> > > >  It would be possible, but the reason I put it there is because the
> > interface
> > > > class would then be cluttered with a lot of code, which makes it harder
to
> > see
> > > > immediately the important part (the method to implement)
> > > 
> > > That makes sense.  I could still see clients wanting to create an API with
> an
> > > Baz and BazInternal mojo API though. How about putting it in a separate
> > package
> > > as well as a suffix to the class name (e.g., foo.bar.internal.BazImpl for
> the
> > > foo.bar.Baz interface)?  That way it should be a bit safer?
> > > 
> > 
> >  The issue with this is that all internal class are right now package
> protected
> > and they would have to be public if we move those in their own package.
WDYT?
> 
> In that case it's probably better to be in the same package, but could we have
a
> name which would never be chosen (or at lease less likely to be chosen) as a
> mojo interface name - e.g., Baz_InternalImpl (where the '_' should never be in
a
> mojo name Interface name, right)? 

Changed to Baz_Internal. It is not possible to generate this name. If you name a
class with _, they will be just considered as word separator, but the generated
class will be uppercamelcase.

[qsr, 2014-08-27 07:32:35]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-08-27 07:33:45]

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

[commit-bot: I haz the power, 2014-08-27 08:57:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-27 09:02:45]

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

[commit-bot: I haz the power, 2014-08-27 09:02:47]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[qsr, 2014-08-27 09:18:26]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-08-27 09:18:53]

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

[commit-bot: I haz the power, 2014-08-27 10:45:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-27 10:50:11]

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

[commit-bot: I haz the power, 2014-08-27 10:50:12]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[qsr, 2014-08-27 11:03:38]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-08-27 11:04:07]

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

[commit-bot: I haz the power, 2014-08-27 11:59:42]

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

[commit-bot: I haz the power, 2014-08-27 12:22:56]

Committed patchset #16 (300001) as b2ef91ee4abe584e712db7a51d47a1b6d9a96e57

[Thiemo Nagel, 2014-08-27 12:35:16]

A revert of this CL (patchset #16) has been created in
https://codereview.chromium.org/511543005/ by tnagel@chromium.org.

The reason for reverting is: Fails license check on android bot:

http://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28db....

[qsr, 2014-08-27 13:09:55]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-08-27 13:10:50]

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

[qsr, 2014-08-27 13:51:48]

The CQ bit was unchecked by qsr@chromium.org

[qsr, 2014-08-27 14:10:27]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-08-27 14:11:14]

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

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

Committed patchset #18 (340001) as 11d4e3e519ce08c43ee9398332c38d1e9de888e5

[commit-bot: I haz the power, 2014-09-10 02:51:10]

Patchset 18 (id:??) landed as
https://crrev.com/6be1a71252ffb3988b84314262c73351c4d298cb
Cr-Commit-Position: refs/heads/master@{#292136}

[commit-bot: I haz the power, 2014-09-10 02:51:46]

Patchset 18 (id:??) landed as
https://crrev.com/063e1e3acae7473399f5064c66296de55b0ac04b
Cr-Commit-Position: refs/heads/master@{#292154}