Dart: Removes name conflicts from generated bindings.

Dart: Removes name conflicts from generated bindings.

This change causes the generated abstract class having the same name as the interface to contain only the interface's method declarations. The generated Proxy class then implements the base class. In addition to implementing the interface methods, the generated Proxy class has one private field _proxyImpl, which is a MojoEventStreamListener and manages sending messages and receiving responses. Operations on the ProxyImpl (close, bind, etc.) are exposed through generated utility functions.

The generated Stub is largely as before with the difference that a class providing a service will implement the mojo interface and *have* a Stub rather than be a Stub. Where appropriate, this change also calls listen() immediately where a Stub is constructed.

BUG=
R=hansmuller@google.com, sky@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/96af48303403d1a6d443f5c05f78b6e9480be9c3

[zra, 2015-02-26 00:12:38]

zra@google.com changed reviewers:
+ hansmuller@chromium.org, sky@chromium.org

[zra, 2015-02-26 00:12:39]

I want to merge in the bugfixes tonyg is working on before proceeding with this
change, but I thought I'd send it around for initial comments.

[sky, 2015-02-26 00:51:50]

On 2015/02/26 00:12:39, zra wrote:
> I want to merge in the bugfixes tonyg is working on before proceeding with
this
> change, but I thought I'd send it around for initial comments.

> Operations on the ProxyImpl (close, bind, etc.) are exposed through generated
utility functions.

I'm curious as to why you prefer this approach over something like:

proxy.impl().close() ?

Having standalone functions that take a proxy seems rather obscure.
On the C++ side we wrap the interface implementation in some object that you can
do stuff on (InterfacePtr).

[zra, 2015-02-26 15:22:29]

On 2015/02/26 00:51:50, sky wrote:
> I'm curious as to why you prefer this approach over something like:

It looked like the JS generation was doing something similar:

https://github.com/domokit/mojo/blob/master/mojo/public/tools/bindings/genera...

But it's possible I've misunderstood. After thinking about things a little more,
I want to try to eliminate the top-level functions and see how things look.

[hansmuller1, 2015-02-26 16:27:41]

On 2015/02/26 15:22:29, zra wrote:
> On 2015/02/26 00:51:50, sky wrote:
> > I'm curious as to why you prefer this approach over something like:
> 
> It looked like the JS generation was doing something similar:
> 
>
https://github.com/domokit/mojo/blob/master/mojo/public/tools/bindings/genera...
> 
> But it's possible I've misunderstood. After thinking about things a little
more,
> I want to try to eliminate the top-level functions and see how things look.

The JS bindings associate run-time state with proxies and stubs via
a pair of functions: StubBindings and ProxyBindings. The functions
return the value of a stub/proxy property whose name is defined a
by an essentially collision-proof (hidden) symbol. It's not as nice
as having the run-time features available directly, but much less
risky.  For example:

StubBindings(stub).delegate = myInterfaceImpl;
ProxyBindings(proxy).close();

I would think that you could take the same approach, instead of
generating bindings classes for each interface?

[zra, 2015-02-26 18:06:20]

PTAL

I've made a change so that the generated Proxy class has two fields, |impl| and
|interface|. So, e.g. to close a Proxy you'd say:

proxy.impl.close()

To make a call to the interface, you'd say:

proxy.interface.call()

In the tests and examples, I've been tearing off the |interface|, e.g.:

var interfaceProxy = new interfaceProxy.unbound();
var interface = interfaceProxy.interface;
connectToService("mojo:interface", interfaceProxy);
...
interface.call1();
...
interfaceProxy.impl.close();

What do you think?

[sky, 2015-02-26 18:21:15]

https://codereview.chromium.org/959993002/diff/60001/mojo/dart/test/validatio...
File mojo/dart/test/validation_test.dart (right):

https://codereview.chromium.org/959993002/diff/60001/mojo/dart/test/validatio...
mojo/dart/test/validation_test.dart:20:
ConfrmanceTestInterfaceImpl(this._completer,
You've got a mispelling here.

https://codereview.chromium.org/959993002/diff/80001/examples/dart/console_ex...
File examples/dart/console_example/main.dart (right):

https://codereview.chromium.org/959993002/diff/80001/examples/dart/console_ex...
examples/dart/console_example/main.dart:14: Console _console;
I think this pattern (making both the proxy and interface as members) is error
prone. I lean toward using proxy.interface.foo every where. Less error prone
(but more code).

[zra, 2015-02-26 19:33:58]

https://codereview.chromium.org/959993002/diff/80001/examples/dart/console_ex...
File examples/dart/console_example/main.dart (right):

https://codereview.chromium.org/959993002/diff/80001/examples/dart/console_ex...
examples/dart/console_example/main.dart:14: Console _console;
On 2015/02/26 18:21:15, sky wrote:
> I think this pattern (making both the proxy and interface as members) is error
> prone. I lean toward using proxy.interface.foo every where. Less error prone
> (but more code).

I agree, just not sure how annoying it will be to type proxy.interface.call()
everywhere. I've gone ahead and removed the tear-off from all the tests and
examples. I guess if folks find it annoying, then they're welcome to tear-off on
their own.

[zra, 2015-02-26 19:58:30]

https://codereview.chromium.org/959993002/diff/60001/mojo/dart/test/validatio...
File mojo/dart/test/validation_test.dart (right):

https://codereview.chromium.org/959993002/diff/60001/mojo/dart/test/validatio...
mojo/dart/test/validation_test.dart:20:
ConfrmanceTestInterfaceImpl(this._completer,
On 2015/02/26 18:21:15, sky wrote:
> You've got a mispelling here.

Fixed.

[sky, 2015-02-26 20:47:37]

https://codereview.chromium.org/959993002/diff/100001/examples/dart/wget/main...
File examples/dart/wget/main.dart (right):

https://codereview.chromium.org/959993002/diff/100001/examples/dart/wget/main...
examples/dart/wget/main.dart:69: _networkService.impl.close();
Can we have a cover method for close, so that foo.impl.close() becomes
foo.close() ?

[hansmuller1, 2015-02-26 21:06:40]

hansmuller@google.com changed reviewers:
+ hansmuller@google.com

[hansmuller1, 2015-02-26 21:06:41]

This LGTM although time will tell if writing foo.interface.bar() is
tedious, or likewise for forwarding close() and listen() to a service
impl class's stub. The new bindings are definitely a huge improvement
in terms of collision avoidance.

<bikeshedding>
Maybe using "ptr" instead of "interface" would be a reasonable concession to
how frequently it's going to occur.
</bikeshedding>

[hansmuller1, 2015-02-26 21:31:55]

One other point (Scott suggested this as well): why bother with
proxy.impl.close(), etc?  Why not just put the methods directly on the proxy
class?

[esprehn, 2015-02-26 21:43:32]

On 2015/02/26 at 21:31:55, hansmuller wrote:
> One other point (Scott suggested this as well): why bother with
proxy.impl.close(), etc?  Why not just put the methods directly on the proxy
class?

That conflicts with methods named close()? It seems like Mojo API should live on
another object and you access it through a separate function that does a lookup
in an external table.

[zra, 2015-02-26 22:04:16]

On 2015/02/26 21:43:32, esprehn wrote:
> It seems like Mojo API should live on
> another object.

That's what this change does. The generated private class _InterfaceProxyCalls
implements the generated abstract base class Interface, and is accessed through
the |interface| field (or the |ptr| field for brevity) of InterfaceProxy.

[zra, 2015-02-26 22:09:12]

I think having a shorter name to get at the calls is a good idea, so I've
changed interface -> ptr.

I've only added a cover call for close() since the other calls probably won't be
used outside of the bindings.

https://codereview.chromium.org/959993002/diff/100001/examples/dart/wget/main...
File examples/dart/wget/main.dart (right):

https://codereview.chromium.org/959993002/diff/100001/examples/dart/wget/main...
examples/dart/wget/main.dart:69: _networkService.impl.close();
On 2015/02/26 20:47:37, sky wrote:
> Can we have a cover method for close, so that foo.impl.close() becomes
> foo.close() ?

Done.

[sky, 2015-02-26 22:53:09]

LGTM

[zra, 2015-02-26 23:16:19]

Committed patchset #8 (id:140001) manually as
96af48303403d1a6d443f5c05f78b6e9480be9c3 (presubmit successful).

[zra, 2015-02-26 23:16:45]

Thanks for the reviews!

[sky, 2015-02-27 17:00:22]

https://codereview.chromium.org/959993002/diff/140001/services/dart/test/ping...
File services/dart/test/pingpong_target/main.dart (right):

https://codereview.chromium.org/959993002/diff/140001/services/dart/test/ping...
services/dart/test/pingpong_target/main.dart:19: _stub = new
PingPongServiceStub.fromEndpoint(endpoint)
In looking at implementing this in the modular side I'm thinking these names are
rather confusing. WDYT of making this code:

_binding = new PingPongServiceBinding.fromEndpoint(endpoint)
               ..ptr = this
               ..listen();

I prefer 'interface' over 'ptr', but 'ptr' is consistent with the name you went
with.

The reason I like Binding over Stub is it better conveys what the object is
doing (and it more closely mirrors the C++ side).

[sky, 2015-02-27 17:02:06]

https://codereview.chromium.org/959993002/diff/140001/services/dart/test/ping...
File services/dart/test/pingpong_target/main.dart (right):

https://codereview.chromium.org/959993002/diff/140001/services/dart/test/ping...
services/dart/test/pingpong_target/main.dart:19: _stub = new
PingPongServiceStub.fromEndpoint(endpoint)
On 2015/02/27 17:00:22, sky wrote:
> In looking at implementing this in the modular side I'm thinking these names
are
> rather confusing. WDYT of making this code:
> 
> _binding = new PingPongServiceBinding.fromEndpoint(endpoint)
>                ..ptr = this
>                ..listen();
> 
> I prefer 'interface' over 'ptr', but 'ptr' is consistent with the name you
went
> with.
> 
> The reason I like Binding over Stub is it better conveys what the object is
> doing (and it more closely mirrors the C++ side).

One more suggestion:

new PingPongServiceBinding(endpoint, this);

? No need for a listen. That's done for me.

[hansmuller1, 2015-02-27 18:41:32]

On 2015/02/26 23:16:45, zra wrote:
> Thanks for the reviews!

Having the Stub constructor listen() by default seems like it would cover the
common case and reduce mistakes. Maybe a special stub constructor that doesn't
listen() by default, for the special case where you want to call listen() later?

[sky, 2015-02-27 18:50:31]

https://codereview.chromium.org/959993002/diff/140001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl
(left):

https://codereview.chromium.org/959993002/diff/140001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:23:
stub.listen(onClosed: onClosed);
Sorry, one last question. How do you observe a close now?

[zra, 2015-02-27 19:04:47]

https://codereview.chromium.org/959993002/diff/140001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl
(left):

https://codereview.chromium.org/959993002/diff/140001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:23:
stub.listen(onClosed: onClosed);
On 2015/02/27 18:50:31, sky wrote:
> Sorry, one last question. How do you observe a close now?

The generated Stub class inherits listen from bindings.Stub which inherits
listen with the onClosed call back argument from MojoEventStreamListener.

[zra, 2015-02-27 19:23:06]

On 2015/02/27 18:41:32, hansmuller1 wrote:
> On 2015/02/26 23:16:45, zra wrote:
> > Thanks for the reviews!
> 
> Having the Stub constructor listen() by default seems like it would cover the
> common case and reduce mistakes. Maybe a special stub constructor that doesn't
> listen() by default, for the special case where you want to call listen()
later?

I'll add an optional named boolean argument called listen to the constructor
that defaults to true, and plumb it through.