WaitSet implementation for old EDK.

WaitSet implementation for old EDK.

BUG=556865
Committed: https://crrev.com/428364d72b6ccf283fff9ce0c9ea58dac3805615
Cr-Commit-Position: refs/heads/master@{#364309}

[Anand Mistry (off Chromium), 2015-11-20 05:18:25]

amistry@chromium.org changed reviewers:
+ jam@chromium.org, rockot@chromium.org, sammc@chromium.org, yzshen@chromium.org

[Anand Mistry (off Chromium), 2015-11-20 05:18:26]

The one thing I haven't done yet is add comments to wait_set_dispatcher.h, so
ignore that. But please review the rest.

[Ken Rockot(use gerrit already), 2015-11-24 16:32:08]

Looks good overall. The only really rough spot is the implementation of
GetWokenDispatcherImplNoLock, imho.

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc (right):

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:124:
awoken_queue_empty = awoken_queue_.empty();
This method in general is pretty hard to follow. It seems like things would be
quite a bit simpler (e.g. you wouldn't need processed_dispatchers_ at all) if
the API were plural (GetWokenDispatchers). What's the downside there?

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:224: LOG(ERROR) <<
"Attempting to serialize WaitSet";
Maybe just CHECK/NOTREACHED?

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:69:
wait_set->AddAwakable(&w, MOJO_HANDLE_SIGNAL_READABLE, 0, &hss));
I have to admit that I found it pretty confusing to read this. It's weird to
wait for a set to be READABLE to know that a waiting handle is WRITABLE. I mean
it makes sense as a person who understands the API, and I don't think it's a
good idea to have the waiting flags here reflect the underlying wait events
(i.e. waiting on WRITABLE here).

Seems like it might be a good idea to just introduce a new signal flag, but I
can't come up with a good name for it. MOJO_HANDLE_SIGNAL_WAIT_SET_READY?
WAIT_SET_AWAKE? I don't know. Or maybe READABLE is just fine since it's
documented and very few people will be using it directly in practice. Mostly
just voicing my confusion as a data point. :)

[Anand Mistry (off Chromium), 2015-12-04 02:31:44]

PTAL.

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc (right):

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:124:
awoken_queue_empty = awoken_queue_.empty();
On 2015/11/24 16:32:07, Ken Rockot (OOO slow til 12-4) wrote:
> This method in general is pretty hard to follow. It seems like things would be
> quite a bit simpler (e.g. you wouldn't need processed_dispatchers_ at all) if
> the API were plural (GetWokenDispatchers). What's the downside there?

Actually, you'd still need it. Firstly, there's no guarantee that the caller
will retrieve all the handles. Secondly, if you don't take care of the signal,
this should continue to return the same handles, so that the logically
level-triggered aspect of waiting in Mojo is preserved.

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:224: LOG(ERROR) <<
"Attempting to serialize WaitSet";
On 2015/11/24 16:32:07, Ken Rockot (OOO slow til 12-4) wrote:
> Maybe just CHECK/NOTREACHED?

I don't think we should CHECK-fail if someone mis-uses the API. Trying to send a
wait set over a message pipe will fail, but it's not a fatal error with respect
to the EDK.

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:69:
wait_set->AddAwakable(&w, MOJO_HANDLE_SIGNAL_READABLE, 0, &hss));
On 2015/11/24 16:32:07, Ken Rockot (OOO slow til 12-4) wrote:
> I have to admit that I found it pretty confusing to read this. It's weird to
> wait for a set to be READABLE to know that a waiting handle is WRITABLE. I
mean
> it makes sense as a person who understands the API, and I don't think it's a
> good idea to have the waiting flags here reflect the underlying wait events
> (i.e. waiting on WRITABLE here).
> 
> Seems like it might be a good idea to just introduce a new signal flag, but I
> can't come up with a good name for it. MOJO_HANDLE_SIGNAL_WAIT_SET_READY?
> WAIT_SET_AWAKE? I don't know. Or maybe READABLE is just fine since it's
> documented and very few people will be using it directly in practice. Mostly
> just voicing my confusion as a data point. :)

I understand. I guess where I'm coming from is that a wait set can be "read"
when there are ready handles. Specifically, you're "reading" the set of ready
handles. I'm not sure a new signal is warranted here, but I'm open to the
possibility.

[Ken Rockot(use gerrit already), 2015-12-08 20:27:08]

lgtm

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc (right):

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:124:
awoken_queue_empty = awoken_queue_.empty();
On 2015/12/04 at 02:31:44, Anand Mistry wrote:
> On 2015/11/24 16:32:07, Ken Rockot (OOO slow til 12-4) wrote:
> > This method in general is pretty hard to follow. It seems like things would
be
> > quite a bit simpler (e.g. you wouldn't need processed_dispatchers_ at all)
if
> > the API were plural (GetWokenDispatchers). What's the downside there?
> 
> Actually, you'd still need it. Firstly, there's no guarantee that the caller
will retrieve all the handles. Secondly, if you don't take care of the signal,
this should continue to return the same handles, so that the logically
level-triggered aspect of waiting in Mojo is preserved.

Ah, right. OK.

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1461213002/diff/1/third_party/mojo/src/mojo/e...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:69:
wait_set->AddAwakable(&w, MOJO_HANDLE_SIGNAL_READABLE, 0, &hss));
On 2015/12/04 at 02:31:44, Anand Mistry wrote:
> On 2015/11/24 16:32:07, Ken Rockot (OOO slow til 12-4) wrote:
> > I have to admit that I found it pretty confusing to read this. It's weird to
> > wait for a set to be READABLE to know that a waiting handle is WRITABLE. I
mean
> > it makes sense as a person who understands the API, and I don't think it's a
> > good idea to have the waiting flags here reflect the underlying wait events
> > (i.e. waiting on WRITABLE here).
> > 
> > Seems like it might be a good idea to just introduce a new signal flag, but
I
> > can't come up with a good name for it. MOJO_HANDLE_SIGNAL_WAIT_SET_READY?
> > WAIT_SET_AWAKE? I don't know. Or maybe READABLE is just fine since it's
> > documented and very few people will be using it directly in practice. Mostly
> > just voicing my confusion as a data point. :)
> 
> I understand. I guess where I'm coming from is that a wait set can be "read"
when there are ready handles. Specifically, you're "reading" the set of ready
handles. I'm not sure a new signal is warranted here, but I'm open to the
possibility.

Acknowledged. It's acceptable as-is IMHO.

[Anand Mistry (off Chromium), 2015-12-09 06:00:01]

yzshen: I need your review because OWNERS (//mojo/public).

[yzshen1, 2015-12-09 07:18:10]

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/core.cc (right):

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/core.cc:259:
signals_states.At(i).Put(awoken_dispatchers[i]->GetHandleSignalsState());
nit: Do we also need to set signals_states.At(i) to some reasonable value if
result is not MOJO_RESULT_OK?

The document of this API doesn't say that it will leave signals_states.At(i)
untouched if the corresponding result is not MOJO_RESULT_OK.

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/dispatcher.h (right):

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/dispatcher.h:157: // Add a dispatcher to
wait on. When the dispatcher satisfies |signals|, it
nit: Add -> Adds
Remove -> Removes on line 165.

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc (right):

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:274:
awakable_list_.AwakeForStateChange(signals_state);
I notice that on line 164, we call this method in a loop.
That means we may notify |awakable_list_.AwakeForStateChange| repeatedly. Is
that expected?

[Anand Mistry (off Chromium), 2015-12-09 22:42:24]

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/core.cc (right):

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/core.cc:259:
signals_states.At(i).Put(awoken_dispatchers[i]->GetHandleSignalsState());
On 2015/12/09 07:18:09, yzshen1 wrote:
> nit: Do we also need to set signals_states.At(i) to some reasonable value if
> result is not MOJO_RESULT_OK?
> 
> The document of this API doesn't say that it will leave signals_states.At(i)
> untouched if the corresponding result is not MOJO_RESULT_OK.

I've updated the API and decided to be consistent with how WaitMany functions.
WaitMany fills it out for every handle (unless you pass in an invalid handle).
I'm now filling this out with every handle that is returned.

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/dispatcher.h (right):

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/dispatcher.h:157: // Add a dispatcher to
wait on. When the dispatcher satisfies |signals|, it
On 2015/12/09 07:18:09, yzshen1 wrote:
> nit: Add -> Adds
> Remove -> Removes on line 165.

Done.

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc (right):

https://codereview.chromium.org/1461213002/diff/100001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:274:
awakable_list_.AwakeForStateChange(signals_state);
On 2015/12/09 07:18:09, yzshen1 wrote:
> I notice that on line 164, we call this method in a loop.
> That means we may notify |awakable_list_.AwakeForStateChange| repeatedly. Is
> that expected?

Yes. What happens is that for async awakable (i.e. WaitSets and
Core::AsyncWait()), they get signalled and removed from the awakable list. For
sync awakables (Wait/WaitMany), they only respond to the first wake-up.
Regardless, it should always be safe to call AwakeForStateChange() provided you
have the appropriate locking. The problem here is the unnecessary overhead. This
basically no-ops after the first wake-up, so the locking and function call
represent unnecessary overhead. However, 95% of wakeups (tested with Chrome)
only affect a single dispatcher, so I'm not too concerned about performance
here.

For a first CL, I'd like to keep this simple because calling |WakeDispatcher()|
lets me maximise code re-use and keep this as simple as possible. For a
follow-up, I can optimise the loop to only call this once after processing all
pending dispatchers. Of course, I'll only bother with the new EDK for that.

[yzshen1, 2015-12-09 23:16:19]

lgtm

[Sam McNally, 2015-12-10 02:38:18]

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/core.cc (right):

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/core.cc:253: count, &awoken_dispatchers,
results, MakeUserPointer(&contexts->front()));
contexts->data()

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc (right):

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:49: for (auto entry
: waiting_dispatchers_)
const auto&

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:107: for (auto it =
awoken_queue_.begin(); it != awoken_queue_.end(); ) {
Why not std::remove_if()?

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.h (right):

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.h:88:
scoped_ptr<Waiter> waiter_;
Why is this a scoped_ptr?

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:63:
scoped_refptr<MessagePipeDispatcher> dispatcher0;
dispatcher0_

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:64:
scoped_refptr<MessagePipeDispatcher> dispatcher1;
dispatcher1_

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:71:
EXPECT_EQ(MOJO_RESULT_OK,
ASSERT_EQ

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:74:
EXPECT_EQ(MOJO_RESULT_OK,
ASSERT_EQ

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:102:
EXPECT_EQ(MOJO_RESULT_OK, wait_set->RemoveWaitingDispatcher(dispatcher1));
ASSERT_EQ

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:116:
EXPECT_EQ(MOJO_RESULT_OK,
ASSERT_EQ

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:153:
EXPECT_EQ(MOJO_RESULT_OK,
ASSERT_EQ

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:156:
EXPECT_EQ(MOJO_RESULT_OK,
ASSERT_EQ

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:159:
EXPECT_EQ(MOJO_RESULT_OK,
ASSERT_EQ

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:162:
EXPECT_EQ(MOJO_RESULT_OK,
ASSERT_EQ

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:218:
EXPECT_EQ(MOJO_RESULT_OK,
ASSERT_EQ

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:239:
EXPECT_EQ(MOJO_RESULT_OK,
ASSERT_EQ

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:260:
EXPECT_EQ(MOJO_RESULT_OK, wait_set->Close());
How about a DispatcherVector of dispatchers to close on TearDown()?

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:320:
EXPECT_EQ(MOJO_RESULT_OK,
ASSERT_EQ

[Anand Mistry (off Chromium), 2015-12-10 04:45:24]

sammc: Also added the test you suggested offline.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/core.cc (right):

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/core.cc:253: count, &awoken_dispatchers,
results, MakeUserPointer(&contexts->front()));
On 2015/12/10 02:38:17, Sam McNally wrote:
> contexts->data()

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc (right):

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:49: for (auto entry
: waiting_dispatchers_)
On 2015/12/10 02:38:17, Sam McNally wrote:
> const auto&

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.cc:107: for (auto it =
awoken_queue_.begin(); it != awoken_queue_.end(); ) {
On 2015/12/10 02:38:17, Sam McNally wrote:
> Why not std::remove_if()?

Makes the counting weird. I know the counting is only for dchecks, but 95% of
the time, awoken_queue_.size() == 1.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.h (right):

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher.h:88:
scoped_ptr<Waiter> waiter_;
On 2015/12/10 02:38:18, Sam McNally wrote:
> Why is this a scoped_ptr?

Because I didn't want to fully declare this class in this header. Having it as a
plain member would require the full declaration above.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
File third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:63:
scoped_refptr<MessagePipeDispatcher> dispatcher0;
On 2015/12/10 02:38:18, Sam McNally wrote:
> dispatcher0_

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:64:
scoped_refptr<MessagePipeDispatcher> dispatcher1;
On 2015/12/10 02:38:18, Sam McNally wrote:
> dispatcher1_

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:71:
EXPECT_EQ(MOJO_RESULT_OK,
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:74:
EXPECT_EQ(MOJO_RESULT_OK,
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:102:
EXPECT_EQ(MOJO_RESULT_OK, wait_set->RemoveWaitingDispatcher(dispatcher1));
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:116:
EXPECT_EQ(MOJO_RESULT_OK,
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:153:
EXPECT_EQ(MOJO_RESULT_OK,
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:156:
EXPECT_EQ(MOJO_RESULT_OK,
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:159:
EXPECT_EQ(MOJO_RESULT_OK,
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:162:
EXPECT_EQ(MOJO_RESULT_OK,
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:218:
EXPECT_EQ(MOJO_RESULT_OK,
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:239:
EXPECT_EQ(MOJO_RESULT_OK,
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:260:
EXPECT_EQ(MOJO_RESULT_OK, wait_set->Close());
On 2015/12/10 02:38:18, Sam McNally wrote:
> How about a DispatcherVector of dispatchers to close on TearDown()?

Done.

https://codereview.chromium.org/1461213002/diff/140001/third_party/mojo/src/m...
third_party/mojo/src/mojo/edk/system/wait_set_dispatcher_unittest.cc:320:
EXPECT_EQ(MOJO_RESULT_OK,
On 2015/12/10 02:38:18, Sam McNally wrote:
> ASSERT_EQ

Done.

[Sam McNally, 2015-12-10 04:50:38]

lgtm

[Anand Mistry (off Chromium), 2015-12-10 05:28:56]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-12-10 05:28:57]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1461213002/#ps160001
(title: "Address comments, add test, and format.")

[commit-bot: I haz the power, 2015-12-10 05:31:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461213002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461213002/160001

[commit-bot: I haz the power, 2015-12-10 06:13:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 06:13:02]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Anand Mistry (off Chromium), 2015-12-10 06:21:53]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-12-10 06:21:53]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
yzshen@chromium.org, sammc@chromium.org
Link to the patchset: https://codereview.chromium.org/1461213002/#ps170001
(title: "Fix windows build and style issues.")

[commit-bot: I haz the power, 2015-12-10 06:22:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461213002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461213002/170001

[commit-bot: I haz the power, 2015-12-10 07:29:24]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-12-10 07:30:03]

Description was changed from

==========
WaitSet implementation for old EDK.

BUG=556865
==========

to

==========
WaitSet implementation for old EDK.

BUG=556865

Committed: https://crrev.com/428364d72b6ccf283fff9ce0c9ea58dac3805615
Cr-Commit-Position: refs/heads/master@{#364309}
==========

[commit-bot: I haz the power, 2015-12-10 07:30:04]

Patchset 10 (id:??) landed as
https://crrev.com/428364d72b6ccf283fff9ce0c9ea58dac3805615
Cr-Commit-Position: refs/heads/master@{#364309}