Dart: Adds optional named arguments for creating bindings.

Dart: Adds optional named arguments for creating bindings.

Also:
- Ensures that Application and _ApplicationImpl clean up state when the other end goes down.
- Renames generated Stubs' "delegate" field to "impl".
- Plumbs doClosed callback through ApplicationConnection
- Plumbs resolvedUrl through Application.

BUG=
R=hansmuller@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/78d979e29b7ea6c91223c6569483543c5e0b4944

[zra, 2015-03-02 19:13:55]

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

[zra, 2015-03-02 19:13:55]

Based on sky's suggestions after submitting the last CL:
https://codereview.chromium.org/959993002/

[sky, 2015-03-02 20:28:22]

https://codereview.chromium.org/968243003/diff/1/mojo/public/dart/src/applica...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/968243003/diff/1/mojo/public/dart/src/applica...
mojo/public/dart/src/application.dart:17: // necessary cleanup is performed on a
PEER_CLOSED signal.
The C++ side uses connection error and not just closed. Should this be
consistent with the C++ naming and use on onError?

https://codereview.chromium.org/968243003/diff/1/mojo/public/dart/src/applica...
File mojo/public/dart/src/application_connection.dart (right):

https://codereview.chromium.org/968243003/diff/1/mojo/public/dart/src/applica...
mojo/public/dart/src/application_connection.dart:14: ServiceProviderBinding
_binding;
Binding is such a better name than Stub. Thanks!

https://codereview.chromium.org/968243003/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/968243003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:167:
{ {{interface|name}} delegate, bool doListen: true, Function onClosed}) :
Did you consider interfaceImpl or impl over delegate?

Also, should the delegate be non-optional?

[hansmuller1, 2015-03-02 20:42:18]

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

[hansmuller1, 2015-03-02 20:42:18]

https://codereview.chromium.org/968243003/diff/1/sky/framework/embedder.dart
File sky/framework/embedder.dart (right):

https://codereview.chromium.org/968243003/diff/1/sky/framework/embedder.dart#...
sky/framework/embedder.dart:37: var binding = new
ServiceProviderBinding.unbound();
Calling the stub class a "binding" has its merits. On the other hand, it's odd
that a FooProxy can be created "unbound", since "unbound" seems to go with
FooBinding.

It's also odd that you can don't bind a Binding (as you can in C++), you set its
implementation delegate.

I'm not really objecting to any of this, although personally I found it easier
to understand the way it was.

[zra, 2015-03-02 20:52:45]

There is some confusion in terminology that I'm not sure yet how to resolve.

A Proxy can be bound to a handle or pipe.

A Binding (Stub) can be bound both to a handle or pipe, and with this change to
an implementation.

The question is should we have a different verb other than just "bind" for these
two concepts, and what the other verb should be.

I was thinking that for the low-level pipe/handle calls, we'd use unix style
open(), close(), bind(), listen() sort of calls. But then I don't know what to
call binding an implementation to a Binding/Stub. Thoughts?

https://codereview.chromium.org/968243003/diff/1/mojo/public/dart/src/applica...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/968243003/diff/1/mojo/public/dart/src/applica...
mojo/public/dart/src/application.dart:17: // necessary cleanup is performed on a
PEER_CLOSED signal.
On 2015/03/02 20:28:22, sky wrote:
> The C++ side uses connection error and not just closed. Should this be
> consistent with the C++ naming and use on onError?

Do you mean in addition to onDone? What does a connection error mean? Some error
return on a handle from MojoWaitMany? I can add that, but since the change would
have to go down into Dart's handle watcher, it may be easier to do it in a
separate CL.

https://codereview.chromium.org/968243003/diff/1/mojo/public/dart/src/applica...
File mojo/public/dart/src/application_connection.dart (right):

https://codereview.chromium.org/968243003/diff/1/mojo/public/dart/src/applica...
mojo/public/dart/src/application_connection.dart:14: ServiceProviderBinding
_binding;
On 2015/03/02 20:28:22, sky wrote:
> Binding is such a better name than Stub. Thanks!

No problem =)

https://codereview.chromium.org/968243003/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/968243003/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:167:
{ {{interface|name}} delegate, bool doListen: true, Function onClosed}) :
On 2015/03/02 20:28:22, sky wrote:
> Also, should the delegate be non-optional?

It has to be optional in at least one constructor because the Decoder needs to
be able to construct a Binding for an InterfaceRequest using the
|newFromEndpoint| static below without knowing what the delegate will be. The
implementation servicing the InterfaceRequest sets up the delegate. See
getPingPongService in:

//services/dart/test/pingpong/main.dart

[zra, 2015-03-03 17:57:56]

Sorry, just want to make sure I understand. Is the naming currently in this CL
okay? Shall I move ahead with it?

Thanks!

[hansmuller1, 2015-03-03 18:27:18]

On 2015/03/03 17:57:56, zra wrote:
> Sorry, just want to make sure I understand. Is the naming currently in this CL
> okay? Shall I move ahead with it?
> 
> Thanks!

I don't like the stub Binding suffix but I think calling the stub's delegate
"implementation" or "impl" would lend some clarity.

[zra, 2015-03-03 19:22:04]

Okay =)

Renamed Binding -> Stub and delegate -> impl.

[sky, 2015-03-03 20:14:13]

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
mojo/public/dart/src/application.dart:47: if(onClosed != null) {
nit: space after if?

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
File mojo/public/dart/src/application_connection.dart (right):

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
mojo/public/dart/src/application_connection.dart:20: listen({Function onClosed})
=> _stub.listen(onClosed: onClosed);
Is there a reason not to follow the closer pattern every where like you did in
Application? Seems like everything should be closed nicely without having to
supply onClosed. The only reason to supply an onClosed is if additional
processing is necessary when something is closed unexpectedly.

[hansmuller1, 2015-03-03 20:32:16]

LGTM

https://codereview.chromium.org/968243003/diff/20001/examples/dart/hello_worl...
File examples/dart/hello_world/hello/main.dart (right):

https://codereview.chromium.org/968243003/diff/20001/examples/dart/hello_worl...
examples/dart/hello_world/hello/main.dart:20: c.listen(onClosed: close);
I like this.  It's subtly different from the original, since before we
unconditionally closed the hello app. Since this is an example, you might
include a comment about waiting for the world app to close, and then closing
this app.

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
mojo/public/dart/src/application.dart:142: // then you must invoke
connection.listen().
Is it still necessary to call connection.listen() here?

[zra, 2015-03-03 21:28:54]

https://codereview.chromium.org/968243003/diff/20001/examples/dart/hello_worl...
File examples/dart/hello_world/hello/main.dart (right):

https://codereview.chromium.org/968243003/diff/20001/examples/dart/hello_worl...
examples/dart/hello_world/hello/main.dart:20: c.listen(onClosed: close);
On 2015/03/03 20:32:16, hansmuller1 wrote:
> I like this.  It's subtly different from the original, since before we
> unconditionally closed the hello app. Since this is an example, you might
> include a comment about waiting for the world app to close, and then closing
> this app.

Added comment.

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
mojo/public/dart/src/application.dart:47: if(onClosed != null) {
On 2015/03/03 20:14:13, sky wrote:
> nit: space after if?

Done.

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
mojo/public/dart/src/application.dart:142: // then you must invoke
connection.listen().
On 2015/03/03 20:32:16, hansmuller1 wrote:
> Is it still necessary to call connection.listen() here? 

Yes at the moment. listen() can't be called until after all the factories are
set up.

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
File mojo/public/dart/src/application_connection.dart (right):

https://codereview.chromium.org/968243003/diff/20001/mojo/public/dart/src/app...
mojo/public/dart/src/application_connection.dart:20: listen({Function onClosed})
=> _stub.listen(onClosed: onClosed);
On 2015/03/03 20:14:13, sky wrote:
> Is there a reason not to follow the closer pattern every where like you did in
> Application? Seems like everything should be closed nicely without having to
> supply onClosed. The only reason to supply an onClosed is if additional
> processing is necessary when something is closed unexpectedly.

Done. Added here and in ApplicationConnection.

[zra, 2015-03-04 16:52:54]

Committed patchset #5 (id:80001) manually as
78d979e29b7ea6c91223c6569483543c5e0b4944 (presubmit successful).