Dart: Removes need to call listen().

Dart: Removes need to call listen().

Also, changes onClosed to onError, and changes it from an optional named
parameter to listen() to a setter on which handlers can be enqueued.

BUG=
R=hansmuller@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/7d85a1d59b61d96b1b7104517b0b986578455644

[zra, 2015-03-05 00:09:58]

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

[hansmuller1, 2015-03-05 00:35:32]

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

[hansmuller1, 2015-03-05 00:35:32]

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

https://codereview.chromium.org/982673002/diff/40001/examples/dart/hello_worl...
examples/dart/hello_world/hello/main.dart:26: c.onError = close;
I like this change.  Another incremental improvement!

https://codereview.chromium.org/982673002/diff/40001/mojo/public/dart/src/eve...
File mojo/public/dart/src/event_stream.dart (right):

https://codereview.chromium.org/982673002/diff/40001/mojo/public/dart/src/eve...
mojo/public/dart/src/event_stream.dart:220: Function cachedOnError = _onError;
Why are we accumulating onError functions here? Developers who really want to
wrap the value of onError can easily do so, doing it automatically seems like it
would lead to confusion.

[zra, 2015-03-05 16:39:29]

https://codereview.chromium.org/982673002/diff/40001/mojo/public/dart/src/eve...
File mojo/public/dart/src/event_stream.dart (right):

https://codereview.chromium.org/982673002/diff/40001/mojo/public/dart/src/eve...
mojo/public/dart/src/event_stream.dart:220: Function cachedOnError = _onError;
On 2015/03/05 00:35:32, hansmuller1 wrote:
> Why are we accumulating onError functions here? Developers who really want to
> wrap the value of onError can easily do so, doing it automatically seems like
it
> would lead to confusion.

I was worried about people accidentally overwriting the onError callbacks added
for closing ApplicationConnection and LocalServiceProvider, but I can try to
work around that a different way. I'll write it up and see what it looks like.

[zra, 2015-03-05 21:05:16]

Removed the piling up callbacks. PTAL.

[hansmuller1, 2015-03-06 17:12:38]

https://codereview.chromium.org/982673002/diff/60001/mojo/public/dart/src/eve...
File mojo/public/dart/src/event_stream.dart (right):

https://codereview.chromium.org/982673002/diff/60001/mojo/public/dart/src/eve...
mojo/public/dart/src/event_stream.dart:136:
MojoEventStreamListener.fromEndpoint(MojoMessagePipeEndpoint endpoint)
We've run into the need for |doListen: false| when the implementation of a
stub isn't available when the stub is created from an endpoint.

class Bar {
  final FooStub _stub;
  Bar(MojoMessagePipeEndpoint endpoint)
      : _stub = new FooStub.fromEndpoint(endpoint, doListen: false);
  void set foo(Foo impl) {
    _stub.impl = impl;
    _stub.listen();
  }
}

The problem can be avoided by creating _stub lazily, however then 
it must be a read/write member.

It also comes up in a more idiomatic use case, since one can't refer
to |this| in an initializer:

class FooImpl implements Foo {
  final FooStub _stub;
  FooImpl(MojoMessagePipeEndpoint endpoint)
      : _stub = new FooStub.fromEndpoint(endpoint, doListen: false) {
    _stub.impl = this;
    _stub.listen();
  }
}

The idiomatic case can be rewritten easily enough, by removing the 
final qualifier and initializing _stub in the constructor:

class FooImpl implements Foo {
  FooStub _stub;
  FooImpl(MojoMessagePipeEndpoint endpoint) {
    _stub = new FooStub.fromEndpoint(endpoint, impl) {
  }
}

Unfortunately (as before) this leaves _stub as a read/write member,
which isn't what's intended.

[zra, 2015-03-06 21:12:00]

https://codereview.chromium.org/982673002/diff/60001/mojo/public/dart/src/eve...
File mojo/public/dart/src/event_stream.dart (right):

https://codereview.chromium.org/982673002/diff/60001/mojo/public/dart/src/eve...
mojo/public/dart/src/event_stream.dart:136:
MojoEventStreamListener.fromEndpoint(MojoMessagePipeEndpoint endpoint)
On 2015/03/06 17:12:38, hansmuller1 wrote:
> We've run into the need for |doListen: false| when the implementation of a
> stub isn't available when the stub is created from an endpoint.
> 
> class Bar {
>   final FooStub _stub;
>   Bar(MojoMessagePipeEndpoint endpoint)
>       : _stub = new FooStub.fromEndpoint(endpoint, doListen: false);
>   void set foo(Foo impl) {
>     _stub.impl = impl;
>     _stub.listen();
>   }
> }
> 
> The problem can be avoided by creating _stub lazily, however then 
> it must be a read/write member.
> 
> It also comes up in a more idiomatic use case, since one can't refer
> to |this| in an initializer:
> 
> class FooImpl implements Foo {
>   final FooStub _stub;
>   FooImpl(MojoMessagePipeEndpoint endpoint)
>       : _stub = new FooStub.fromEndpoint(endpoint, doListen: false) {
>     _stub.impl = this;
>     _stub.listen();
>   }
> }
> 
> The idiomatic case can be rewritten easily enough, by removing the 
> final qualifier and initializing _stub in the constructor:
> 
> class FooImpl implements Foo {
>   FooStub _stub;
>   FooImpl(MojoMessagePipeEndpoint endpoint) {
>     _stub = new FooStub.fromEndpoint(endpoint, impl) {
>   }
> }
> 
> Unfortunately (as before) this leaves _stub as a read/write member,
> which isn't what's intended.
> 
> 

Scott pointed out that it is okay to wait to set impl until after the listen()
so long as impl is set before execution returns to the event loop. If that
sounds too error-prone or not flexible enough, I can add back the doListen.
Also, in this CL, the impl argument to the Stub constructor is optional, so the
_stub fields could still be final, you just set the impl in the constructor as
before.

Let me know how you and Scott would like to go with the doListen parameter.
Either way, it's not hard to change back after getting more experience.

[hansmuller1, 2015-03-06 22:48:50]

We've talked this over and are OK with dropping doListen. Thanks for your
patience!

[hansmuller1, 2015-03-06 22:50:01]

LGTM

[zra, 2015-03-07 00:27:42]

Committed patchset #6 (id:100001) manually as
7d85a1d59b61d96b1b7104517b0b986578455644 (presubmit successful).