Dart: Simplifies the handle watcher. Various cleanups and bugfixes.

Dart: Simplifies the handle watcher. Various cleanups and bugfixes.

This CL removes the TOGGLE_WRITE command from the Dart handle watcher.
The listen function in the MojoHandle class also no longer wraps
the callback in a closure that automatically adds the handle back to
the watcher after an event has been handled. This was trying to be too
clever. Instead, after handling an event, the callback must explicitly
indicate the events the client is now interested in. To enable this
approach, the handle watcher now also sends the set of signals that
were listened for to the client on an event.

Also:
- Interface implementation functions now return a Future so that e.g. a
  request can be serviced by an Isolate or other async operation, and the
  Interface can immediately return to listening for events on the pipe.
- Clients now set a meaningful request ID so that the right Completer for
  the response can be looked up in a Map.
- Rewrites MojoHandleSignals.
- Renames MojoHandle to MojoEventStream and RawMojoHandle to MojoHandle.

BUG=
R=abarth@chromium.org, asiva@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/6e3be6003da13efc3d7b68cb9dd10aadfc56c088

[zra, 2014-12-23 18:01:37]

zra@google.com changed reviewers:
+ asiva@google.com

[zra, 2014-12-23 18:01:38]

Will add Mojo Team reviewer(s) when everyone is back after next week.

[zra, 2014-12-29 21:19:34]

zra@google.com changed reviewers:
+ jamesr@chromium.org

[zra, 2014-12-29 21:19:35]

+jamesr

[siva, 2014-12-29 23:20:44]

lgtm

https://codereview.chromium.org/800523004/diff/1/mojo/dart/test/handle_watche...
File mojo/dart/test/handle_watcher_test.dart (right):

https://codereview.chromium.org/800523004/diff/1/mojo/dart/test/handle_watche...
mojo/dart/test/handle_watcher_test.dart:29: handle.enableWriteEvents();
Why does this have to be moved down?

https://codereview.chromium.org/800523004/diff/1/mojo/public/dart/src/client....
File mojo/public/dart/src/client.dart (right):

https://codereview.chromium.org/800523004/diff/1/mojo/public/dart/src/client....
mojo/public/dart/src/client.dart:126: if (_nextId.bitLength > 64) {
maybe document what this magic number 64 is.

https://codereview.chromium.org/800523004/diff/1/mojo/public/dart/src/handle....
File mojo/public/dart/src/handle.dart (right):

https://codereview.chromium.org/800523004/diff/1/mojo/public/dart/src/handle....
mojo/public/dart/src/handle.dart:156: bool readEnabled() =>
MojoHandleSignals.isReadable(_signals);
These should also just be getters?

[zra, 2014-12-30 16:29:33]

https://codereview.chromium.org/800523004/diff/1/mojo/dart/test/handle_watche...
File mojo/dart/test/handle_watcher_test.dart (right):

https://codereview.chromium.org/800523004/diff/1/mojo/dart/test/handle_watche...
mojo/dart/test/handle_watcher_test.dart:29: handle.enableWriteEvents();
On 2014/12/29 23:20:44, siva wrote:
> Why does this have to be moved down?

It doesn't have to be. enableWriteEvents() can go either before or after the
call to listen, and the effect will be the same. I moved it because
simpleAsyncAwaitTest already tests setting the signals before the listen call in
the constructor. It can't set the signals after the listen call as it is using
await for.

https://codereview.chromium.org/800523004/diff/1/mojo/public/dart/src/client....
File mojo/public/dart/src/client.dart (right):

https://codereview.chromium.org/800523004/diff/1/mojo/public/dart/src/client....
mojo/public/dart/src/client.dart:126: if (_nextId.bitLength > 64) {
On 2014/12/29 23:20:44, siva wrote:
> maybe document what this magic number 64 is.

Added a comment.

https://codereview.chromium.org/800523004/diff/1/mojo/public/dart/src/handle....
File mojo/public/dart/src/handle.dart (right):

https://codereview.chromium.org/800523004/diff/1/mojo/public/dart/src/handle....
mojo/public/dart/src/handle.dart:156: bool readEnabled() =>
MojoHandleSignals.isReadable(_signals);
On 2014/12/29 23:20:44, siva wrote:
> These should also just be getters?

Yes. These aren't actually be used anywhere though, so I've removed them.

[zra, 2014-12-30 19:01:56]

zra@google.com changed reviewers:
+ abarth@chromium.org

[zra, 2014-12-30 19:01:57]

+abarth in case he is back.

[jamesr, 2014-12-30 22:17:36]

jamesr@chromium.org changed reviewers:
- jamesr@chromium.org

[jamesr, 2014-12-30 22:17:40]

I don't think I'm able to provide a meaningful review of this patch.

[abarth-chromium, 2015-01-04 08:46:07]

I'm not sure how you've been handling reviews while I've been out, but if you'd
like me to take over reviewing these CLs again, I should probably start by
reading through all the code we've got now to understand where we're at.

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/cli...
File mojo/public/dart/src/client.dart (right):

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/cli...
mojo/public/dart/src/client.dart:126: // If the next ID would overflow 64-bits,
wrap back around to 1.
Why?

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/han...
File mojo/public/dart/src/handle.dart (right):

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:103: [MojoHandleSignals signals =
MojoHandleSignals.READABLE]) :
Why should a handle be associated with a set of signals?  Operations on handles
take these flags but the handles themselves can be used in different ways at
different times.

I guess maybe I don't fully understand why a MojoHandle "is a" Stream<int>.  It
seems like performing some operations on a MojoHandle might produce a Stream of
some sort, but the handle itself isn't a stream.

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/int...
File mojo/public/dart/src/interface.dart (right):

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/int...
mojo/public/dart/src/interface.dart:39: }
If these conditions represent bugs, do we need to handle them?

[zra, 2015-01-05 15:45:42]

James and Siva have been reviewing Dart CLs while you've been out. I've emailed
you links to those CLs.

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/cli...
File mojo/public/dart/src/client.dart (right):

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/cli...
mojo/public/dart/src/client.dart:126: // If the next ID would overflow 64-bits,
wrap back around to 1.
On 2015/01/04 08:46:07, abarth wrote:
> Why?

After double checking. This will get masked off anyway before the write into the
typed-data buffer, so I removed this check.

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/han...
File mojo/public/dart/src/handle.dart (right):

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:103: [MojoHandleSignals signals =
MojoHandleSignals.READABLE]) :
On 2015/01/04 08:46:07, abarth wrote:
> Why should a handle be associated with a set of signals?  Operations on
handles
> take these flags but the handles themselves can be used in different ways at
> different times.
> 
> I guess maybe I don't fully understand why a MojoHandle "is a" Stream<int>. 
It
> seems like performing some operations on a MojoHandle might produce a Stream
of
> some sort, but the handle itself isn't a stream.

Maybe the following renaming could clear things up:
MojoHandle -> MojoEventStream (or MojoHandleEventStream)
RawMojoHanlde -> MojoHandle

How does that sound?

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/int...
File mojo/public/dart/src/interface.dart (right):

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/int...
mojo/public/dart/src/interface.dart:39: }
On 2015/01/04 08:46:07, abarth wrote:
> If these conditions represent bugs, do we need to handle them?

No. Changed to asserts here and in client.dart.

[abarth-chromium, 2015-01-05 16:47:39]

On 2015/01/05 at 15:45:42, zra wrote:
> Maybe the following renaming could clear things up:
> MojoHandle -> MojoEventStream (or MojoHandleEventStream)
> RawMojoHanlde -> MojoHandle
> 
> How does that sound?

That would make a lot of sense!

[zra, 2015-01-05 21:22:31]

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/han...
File mojo/public/dart/src/handle.dart (right):

https://codereview.chromium.org/800523004/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:103: [MojoHandleSignals signals =
MojoHandleSignals.READABLE]) :
On 2015/01/05 15:45:41, zra wrote:
> On 2015/01/04 08:46:07, abarth wrote:
> > Why should a handle be associated with a set of signals?  Operations on
> handles
> > take these flags but the handles themselves can be used in different ways at
> > different times.
> > 
> > I guess maybe I don't fully understand why a MojoHandle "is a" Stream<int>. 
> It
> > seems like performing some operations on a MojoHandle might produce a Stream
> of
> > some sort, but the handle itself isn't a stream.
> 
> Maybe the following renaming could clear things up:
> MojoHandle -> MojoEventStream (or MojoHandleEventStream)
> RawMojoHanlde -> MojoHandle
> 
> How does that sound?

Done.

[zra, 2015-01-08 22:07:06]

Merged in Jim's change. PTAL.

[abarth-chromium, 2015-01-09 02:50:21]

LGTM

I can't shake the feeling that this code is doing too much.  I still haven't
figured out exactly what's causing me to think that.  It might be helpful to
meet in person at some point and talk through it.

Thanks!

[zra, 2015-01-09 16:54:08]

Committed patchset #8 (id:140001) manually as
6e3be6003da13efc3d7b68cb9dd10aadfc56c088 (presubmit successful).