Update Dart bindings to support updated MojoWait and MojoWaitMany APIs.

Update Dart bindings to support updated MojoWait and MojoWaitMany APIs.

R=zra@google.com, zra
BUG=365436
Committed: https://chromium.googlesource.com/external/mojo/+/84a66f3342250692572cd0d1df3031441df80a0d

[jimbe, 2015-01-06 18:50:55]

Hi Zach,

As part of your code review, can you try to answer my TODO question in
mojo_natives.cc, around line 96? I want to finish that TODO before submitting
this CL.

Thanks,
Jim

[zra, 2015-01-06 19:53:10]

Hi Jim,

Thanks for doing this. Please let me know if the below helps, and I'll continue
with the review.

Cheers,
Zach

https://codereview.chromium.org/834283003/diff/1/mojo/dart/embedder/mojo_nati...
File mojo/dart/embedder/mojo_natives.cc (right):

https://codereview.chromium.org/834283003/diff/1/mojo/dart/embedder/mojo_nati...
mojo/dart/embedder/mojo_natives.cc:98: //  Dart_Handle klass =
Dart_GetClass(MojoLib(),
If this is causing problems, you can put a DART_CHECK_VALID(klass) here (and on
any Dart_Handle where you're not sure of the result) to get a possibly useful
error message.

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

https://codereview.chromium.org/834283003/diff/1/mojo/public/dart/src/types.d...
mojo/public/dart/src/types.dart:141: MojoHandleSignalsState.fromList(List<int>
state) {
I think this is causing the failure in the TODO. Since the fields are final, you
can't initialize them in the constructor body, but you can initialize them in
the initializer list:

MojoHandleSignalsState.fromList(List<int> state) :
    satisfisfied_signals = state[0],
    satisfiable_signals = state[1];

[jimbe, 2015-01-06 22:47:01]

ptal.

Note that the Dart unit tests were failing in compile_all_interfaces_test() even
before I started this work. I have not addressed that failure.

https://codereview.chromium.org/834283003/diff/1/mojo/dart/embedder/mojo_nati...
File mojo/dart/embedder/mojo_natives.cc (right):

https://codereview.chromium.org/834283003/diff/1/mojo/dart/embedder/mojo_nati...
mojo/dart/embedder/mojo_natives.cc:98: //  Dart_Handle klass =
Dart_GetClass(MojoLib(),
On 2015/01/06 19:53:10, zra wrote:
> If this is causing problems, you can put a DART_CHECK_VALID(klass) here (and
on
> any Dart_Handle where you're not sure of the result) to get a possibly useful
> error message.

Done.

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

https://codereview.chromium.org/834283003/diff/1/mojo/public/dart/src/types.d...
mojo/public/dart/src/types.dart:141: MojoHandleSignalsState.fromList(List<int>
state) {
On 2015/01/06 19:53:10, zra wrote:
> I think this is causing the failure in the TODO. Since the fields are final,
you
> can't initialize them in the constructor body, but you can initialize them in
> the initializer list:
> 
> MojoHandleSignalsState.fromList(List<int> state) :
>     satisfisfied_signals = state[0],
>     satisfiable_signals = state[1];

Took out the code completely. No longer needed.

[zra, 2015-01-07 15:56:29]

The fix for that test failure is pending in
https://codereview.chromium.org/800523004/. I'll make sure everything is okay
after merging this change into that change.

A few nits and questions:

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/embedder/mojo_...
File mojo/dart/embedder/mojo_natives.cc (right):

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/embedder/mojo_...
mojo/dart/embedder/mojo_natives.cc:19: #define MOJO_NATIVE_LIST(V)              
 \
Is shifting these \ consistent with other Mojo C++ code? I'm not sure, and just
had these the way we do them in the Dart VM.

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/embedder/mojo_...
mojo/dart/embedder/mojo_natives.cc:196: MojoResult r =
mojo::Wait(mojo::Handle(static_cast<MojoHandle>(handle)),
So, the C++ calls are the updated ones, while the C calls are still the old
ones?

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/test/core_test...
File mojo/dart/test/core_test.dart (right):

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/test/core_test...
mojo/dart/test/core_test.dart:23: [MojoHandleSignals.READWRITE],
Strange indentation here.

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/test/core_test...
mojo/dart/test/core_test.dart:184: [MojoHandleSignals.READABLE],
ditto

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

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:10: static List<Object> wait(int handle, int
signals, int deadline)
For List<Object>, we usually just write List.

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:63: new MojoResult(result[0]), result[1],
result[2]);
indent 4 spaces.

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

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle_watcher.dart:102: MojoWaitManyResult res =
RawMojoHandle.waitMany(watcher._handles,
Does the new call give any additional information, or is there still only
information about one handle at a time?

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle_watcher.dart:261: _close(_handles[i], pruning:
true);
_close() calls _removeHandle() which will mess up looping over _handles, so even
though the extra wait() call isn't needed, I think the auxiliary closed list
still is. Sorry there was no comment here about that.

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/tim...
File mojo/public/dart/src/timer_queue.dart (right):

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/tim...
mojo/public/dart/src/timer_queue.dart:13: int compareTo(Timeout other)  =>
other.deadline - deadline;
Thanks!

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/typ...
File mojo/public/dart/src/types.dart (right):

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/typ...
mojo/public/dart/src/types.dart:140: this.satisfiable_signals);
indentation.

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/typ...
mojo/public/dart/src/types.dart:150: //final MojoHandleSignalsState state;
remove.

[jimbe, 2015-01-08 17:09:21]

Patchset #3 (id:40001) has been deleted

[jimbe, 2015-01-08 17:11:28]

PTAL.

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/embedder/mojo_...
File mojo/dart/embedder/mojo_natives.cc (right):

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/embedder/mojo_...
mojo/dart/embedder/mojo_natives.cc:19: #define MOJO_NATIVE_LIST(V)              
 \
On 2015/01/07 15:56:28, zra wrote:
> Is shifting these \ consistent with other Mojo C++ code? I'm not sure, and
just
> had these the way we do them in the Dart VM.

This is the result of git cl format, not me tinkering :-) My understanding is
that the result of git cl format is, by definition, correct.

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/embedder/mojo_...
mojo/dart/embedder/mojo_natives.cc:196: MojoResult r =
mojo::Wait(mojo::Handle(static_cast<MojoHandle>(handle)),
On 2015/01/07 15:56:28, zra wrote:
> So, the C++ calls are the updated ones, while the C calls are still the old
> ones?

Both the C and the C++ calls were updated. However, (per Darin and Trung),
modules should generally prefer the C++ calls. In this case, the C++ wrap some
ugliness of the C calls and so are easier/safer to use.

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/test/core_test...
File mojo/dart/test/core_test.dart (right):

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/test/core_test...
mojo/dart/test/core_test.dart:23: [MojoHandleSignals.READWRITE],
On 2015/01/07 15:56:28, zra wrote:
> Strange indentation here.

Done.

https://codereview.chromium.org/834283003/diff/20001/mojo/dart/test/core_test...
mojo/dart/test/core_test.dart:184: [MojoHandleSignals.READABLE],
On 2015/01/07 15:56:28, zra wrote:
> ditto

Done.

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

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:10: static List<Object> wait(int handle, int
signals, int deadline)
On 2015/01/07 15:56:28, zra wrote:
> For List<Object>, we usually just write List.

Done.

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle.dart:63: new MojoResult(result[0]), result[1],
result[2]);
On 2015/01/07 15:56:28, zra wrote:
> indent 4 spaces.

Done.

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

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle_watcher.dart:102: MojoWaitManyResult res =
RawMojoHandle.waitMany(watcher._handles,
On 2015/01/07 15:56:28, zra wrote:
> Does the new call give any additional information, or is there still only
> information about one handle at a time?

The new call returns information in WaitManyResult about all of the handles. You
get information about what is currently signaled for each handle (even if you
didn't specifically ask for the information in your signals array. This allows
you to detect peer closed.) You also get information about what can (or can't)
ever be signaled.

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/han...
mojo/public/dart/src/handle_watcher.dart:261: _close(_handles[i], pruning:
true);
On 2015/01/07 15:56:28, zra wrote:
> _close() calls _removeHandle() which will mess up looping over _handles, so
even
> though the extra wait() call isn't needed, I think the auxiliary closed list
> still is. Sorry there was no comment here about that.

Done.

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/tim...
File mojo/public/dart/src/timer_queue.dart (right):

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/tim...
mojo/public/dart/src/timer_queue.dart:13: int compareTo(Timeout other)  =>
other.deadline - deadline;
On 2015/01/07 15:56:28, zra wrote:
> Thanks!

Acknowledged.

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/typ...
File mojo/public/dart/src/types.dart (right):

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/typ...
mojo/public/dart/src/types.dart:140: this.satisfiable_signals);
On 2015/01/07 15:56:29, zra wrote:
> indentation.

Done.

https://codereview.chromium.org/834283003/diff/20001/mojo/public/dart/src/typ...
mojo/public/dart/src/types.dart:150: //final MojoHandleSignalsState state;
On 2015/01/07 15:56:29, zra wrote:
> remove.

Done.

[zra, 2015-01-08 17:23:11]

lgtm with fix for comment.

https://codereview.chromium.org/834283003/diff/60001/mojo/public/dart/src/han...
File mojo/public/dart/src/handle_watcher.dart (right):

https://codereview.chromium.org/834283003/diff/60001/mojo/public/dart/src/han...
mojo/public/dart/src/handle_watcher.dart:269: // and the signals array are
mismatched.
This is not correct. _close() calls _removeHandle(), which keeps everything
consistent.

[jimbe, 2015-01-08 17:38:21]

ptal

https://codereview.chromium.org/834283003/diff/60001/mojo/public/dart/src/han...
File mojo/public/dart/src/handle_watcher.dart (right):

https://codereview.chromium.org/834283003/diff/60001/mojo/public/dart/src/han...
mojo/public/dart/src/handle_watcher.dart:269: // and the signals array are
mismatched.
On 2015/01/08 17:23:11, zra wrote:
> This is not correct. _close() calls _removeHandle(), which keeps everything
> consistent.

Poor choice of words. There are two separate signals arrays, the second of which
is passed into this function. Fixed wording.

[zra, 2015-01-08 17:39:27]

lgtm

[jimbe, 2015-01-08 18:18:29]

Committed patchset #4 (id:80001) manually as
84a66f3342250692572cd0d1df3031441df80a0d (presubmit successful).