Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified
max_time.
3) Introduced WaitableEvent::TimedWaitUntil method that
waits until the specified absolute deadline.

BUG=623223, 465948
Committed: https://crrev.com/acf6801129dff3dba81f6f105713aab173a73e2c
Cr-Commit-Position: refs/heads/master@{#435388}

[stanisc, 2016-10-19 23:24:15]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 23:24:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 23:56:12]

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

[commit-bot: I haz the power, 2016-10-19 23:56:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[stanisc, 2016-10-20 00:03:00]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 00:03:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 00:54:35]

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

[commit-bot: I haz the power, 2016-10-20 00:54:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[stanisc, 2016-10-20 19:00:41]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 19:01:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 20:43:28]

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

[commit-bot: I haz the power, 2016-10-20 20:43:29]

Dry run: This issue passed the CQ dry run.

[stanisc, 2016-10-20 21:47:20]

Description was changed from

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

BUG=623223
==========

to

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This should also make TimedWait behavior consistent with the posix
implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified max_time.

BUG=623223
==========

[stanisc, 2016-10-20 21:48:47]

Description was changed from

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This should also make TimedWait behavior consistent with the posix
implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified max_time.

BUG=623223
==========

to

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified max_time.

BUG=623223
==========

[stanisc, 2016-10-20 21:49:25]

Description was changed from

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified max_time.

BUG=623223
==========

to

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified
max_time.

BUG=623223
==========

[stanisc, 2016-10-20 21:50:13]

stanisc@chromium.org changed reviewers:
+ danakj@chromium.org

[stanisc, 2016-10-20 21:50:14]

PTAL!

[danakj, 2016-10-21 22:01:22]

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
File base/synchronization/waitable_event_win.cc (right):

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:73: bool
WaitableEvent::TimedWait(const TimeDelta& max_time) {
nit: can you name timedeltas with _delta suffix and timeticks as _time suffix?
That would help reading this a lot.

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:80:
base::ThreadRestrictions::AssertWaitAllowed();
can you put assertions first? (just above event_activity)

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:83: TimeTicks target_time =
TimeTicks::Now() + max_time;
It's possible this was a bad suggestion because it requires an extra Now() call
since we don't have delayed_work_time_ here which we are basically recreating.
WDYT is this ok?

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:91: delay = std::max(delay,
min_delay);
I think you can just write TimeDelta::FromMilliseconds(1) here instead of
|min_delay|, since this is where the comment explaining it is, and the method is
constexpr so it's not like we'll execute code every time. Otherwise this comment
is explaining a constant defined up above which is more awkward.

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:108: NOTREACHED() <<
"WaitForSingleObject failed";
This falls into the NOTREACHED+return antipattern. Another way to write this
would be

DWORD result = WaitForSingleObject(...);
DCHECK(result == WAIT_OBJECT_0 || result == WAIT_TIMEOUT) << result;
switch (result) {
  case WAIT_OBJECT_0:
    ...
  case WAIT_TIMEOUT:
    ...
}

and then just don't handle other cases (we'd call WaitForSingleObject again).
Would that be okay?

Similar comment for IsSignaled, but smaller even:

DCHECK(result == WAIT_OBJECT_0 || result == WAIT_TIMEOUT) << result;
return result == WAIT_OBJECT_0;

[stanisc, 2016-10-21 23:05:25]

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
File base/synchronization/waitable_event_win.cc (right):

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:83: TimeTicks target_time =
TimeTicks::Now() + max_time;
On 2016/10/21 22:01:22, danakj wrote:
> It's possible this was a bad suggestion because it requires an extra Now()
call
> since we don't have delayed_work_time_ here which we are basically recreating.
> WDYT is this ok?

Yeah, that is unfortunate and redundant that the caller code might be
calculating the delay by subtracting Now() from a deadline (like the message
pump code does), and this code has to add Now() back to recreate the deadline.

The posix version does the same thing. I might be good idea to add a TimedWait
overload that takes an actual deadline (TimeTicks) rather than a delay. But I
don't know how many call sites would benefit from that. 

And I've just realized that this code will overflow with TimeDelay::Max() (or
other nearly max delay). So this needs more work to handle a potential overflow.
The posix version has the same issue by the way.

[danakj, 2016-10-21 23:15:26]

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
File base/synchronization/waitable_event_win.cc (right):

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:83: TimeTicks target_time =
TimeTicks::Now() + max_time;
On 2016/10/21 23:05:25, stanisc wrote:
> On 2016/10/21 22:01:22, danakj wrote:
> > It's possible this was a bad suggestion because it requires an extra Now()
> call
> > since we don't have delayed_work_time_ here which we are basically
recreating.
> > WDYT is this ok?
> 
> Yeah, that is unfortunate and redundant that the caller code might be
> calculating the delay by subtracting Now() from a deadline (like the message
> pump code does), and this code has to add Now() back to recreate the deadline.
> 
> The posix version does the same thing. I might be good idea to add a TimedWait
> overload that takes an actual deadline (TimeTicks) rather than a delay. But I
> don't know how many call sites would benefit from that. 
> 
> And I've just realized that this code will overflow with TimeDelay::Max() (or
> other nearly max delay). So this needs more work to handle a potential
overflow.
> The posix version has the same issue by the way.
> 

I was wondering about an overload also, this seems worth doing then for our
tight event loops, esp to benefit all the platforms at once. TimedWaitUntil()?

Thanks for noticing the overflow also. Maybe we make the event loops use another
override here, and then fix the overflow for both windows and posix in another
CL?

[stanisc, 2016-11-10 23:24:41]

Patchset #4 (id:60001) has been deleted

[stanisc, 2016-11-10 23:24:55]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 23:25:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 23:40:25]

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

[commit-bot: I haz the power, 2016-11-10 23:40:26]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[stanisc, 2016-11-11 00:35:54]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 00:36:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 02:04:12]

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

[commit-bot: I haz the power, 2016-11-11 02:04:12]

Dry run: This issue passed the CQ dry run.

[stanisc, 2016-11-11 03:50:48]

OK, I have added TimedWaitUntil which solves the problem with extra Now() calls.
It looks like the overflow should not be a problem because it is handled by
TimeTicks / TimeDelta. And it looks like crbug.com/465948 is now fixed as well.

Is there anything I could have overlooked?

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
File base/synchronization/waitable_event_win.cc (right):

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:73: bool
WaitableEvent::TimedWait(const TimeDelta& max_time) {
On 2016/10/21 22:01:22, danakj wrote:
> nit: can you name timedeltas with _delta suffix and timeticks as _time suffix?
> That would help reading this a lot.

Done.

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:80:
base::ThreadRestrictions::AssertWaitAllowed();
On 2016/10/21 22:01:22, danakj wrote:
> can you put assertions first? (just above event_activity)

Done.

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:91: delay = std::max(delay,
min_delay);
On 2016/10/21 22:01:22, danakj wrote:
> I think you can just write TimeDelta::FromMilliseconds(1) here instead of
> |min_delay|, since this is where the comment explaining it is, and the method
is
> constexpr so it's not like we'll execute code every time. Otherwise this
comment
> is explaining a constant defined up above which is more awkward.

Done.

https://codereview.chromium.org/2433773005/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_win.cc:108: NOTREACHED() <<
"WaitForSingleObject failed";
On 2016/10/21 22:01:22, danakj wrote:
> This falls into the NOTREACHED+return antipattern. Another way to write this
> would be
> 
> DWORD result = WaitForSingleObject(...);
> DCHECK(result == WAIT_OBJECT_0 || result == WAIT_TIMEOUT) << result;
> switch (result) {
>   case WAIT_OBJECT_0:
>     ...
>   case WAIT_TIMEOUT:
>     ...
> }
> 
> and then just don't handle other cases (we'd call WaitForSingleObject again).
> Would that be okay?
> 
> Similar comment for IsSignaled, but smaller even:
> 
> DCHECK(result == WAIT_OBJECT_0 || result == WAIT_TIMEOUT) << result;
> return result == WAIT_OBJECT_0;

Good suggestion. Fixed.

[stanisc, 2016-11-16 22:05:06]

Description was changed from

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified
max_time.

BUG=623223
==========

to

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified
max_time.
3) Introduced WaitableEvent::TimedWaitUntil method that
waits until the specified absolute deadline.

BUG=623223
==========

[stanisc, 2016-11-16 22:05:42]

Description was changed from

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified
max_time.
3) Introduced WaitableEvent::TimedWaitUntil method that
waits until the specified absolute deadline.

BUG=623223
==========

to

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified
max_time.
3) Introduced WaitableEvent::TimedWaitUntil method that
waits until the specified absolute deadline.

BUG=623223,465948
==========

[stanisc, 2016-11-16 22:07:00]

Please take a look

[danakj, 2016-11-16 22:57:29]

https://codereview.chromium.org/2433773005/diff/100001/base/message_loop/mess...
File base/message_loop/message_pump_default.cc (left):

https://codereview.chromium.org/2433773005/diff/100001/base/message_loop/mess...
base/message_loop/message_pump_default.cc:84: } else {
Can u explain why the if and this else block are gone?

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
File base/synchronization/waitable_event_unittest.cc (right):

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
base/synchronization/waitable_event_unittest.cc:168: // Tests TimedWaitUntil.
this comment isn't adding value

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
base/synchronization/waitable_event_unittest.cc:186: // A test similr to the one
in TimedWait test above.
similar

But maybe copy the explanation there over here?

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
File base/synchronization/waitable_event_win.cc (right):

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
base/synchronization/waitable_event_win.cc:101:
base::ThreadRestrictions::AssertWaitAllowed();
This should be after the check for is_zero right?

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
base/synchronization/waitable_event_win.cc:116:
base::ThreadRestrictions::AssertWaitAllowed();
same, but we should check for is_zero explicitly here too not just if the
timeout is done?

[stanisc, 2016-11-17 00:22:00]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 00:22:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 02:15:24]

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

[commit-bot: I haz the power, 2016-11-17 02:15:24]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[stanisc, 2016-11-17 19:42:23]

https://codereview.chromium.org/2433773005/diff/100001/base/message_loop/mess...
File base/message_loop/message_pump_default.cc (left):

https://codereview.chromium.org/2433773005/diff/100001/base/message_loop/mess...
base/message_loop/message_pump_default.cc:84: } else {
On 2016/11/16 22:57:29, danakj wrote:
> Can u explain why the if and this else block are gone?

Good call!
I think it shouldn't matter with the new implementation whether
delayed_work_time_ is set here to TimeTicks() or not.
First of all it is always reset in DoDelayedWork() call above although that
isn't straightforward when looking at the code. Even if it wasn't reset and
stayed in the past that should be OK because (a) it would normally end up being
in the past in the former if block after a wait and (b) TimeWaitUntil is OK with
taking an already expired deadline - it just returns promptly in that case.

The only alternative would be to call Now() to compare delayed_work_time_ to the
current time but that is something this change was trying to avoid.

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
File base/synchronization/waitable_event_unittest.cc (right):

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
base/synchronization/waitable_event_unittest.cc:168: // Tests TimedWaitUntil.
On 2016/11/16 22:57:29, danakj wrote:
> this comment isn't adding value

Done.

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
base/synchronization/waitable_event_unittest.cc:186: // A test similr to the one
in TimedWait test above.
On 2016/11/16 22:57:29, danakj wrote:
> similar
> 
> But maybe copy the explanation there over here?

Done.

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
File base/synchronization/waitable_event_win.cc (right):

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
base/synchronization/waitable_event_win.cc:101:
base::ThreadRestrictions::AssertWaitAllowed();
On 2016/11/16 22:57:29, danakj wrote:
> This should be after the check for is_zero right?

That's what I had before, but I thought you suggested putting the assert at the
top. Actually I've just realized you wanted to put the assert just above
event_activity.

Done.

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
base/synchronization/waitable_event_win.cc:116:
base::ThreadRestrictions::AssertWaitAllowed();
On 2016/11/16 22:57:29, danakj wrote:
> same, but we should check for is_zero explicitly here too not just if the
> timeout is done?

Moved the assert below the check. Shouldn't zero TimeTicks be already handled by
the condition. I could add an explicit check it it seems it would be redundant.

[danakj, 2016-11-17 21:11:08]

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
File base/synchronization/waitable_event_win.cc (right):

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
base/synchronization/waitable_event_win.cc:116:
base::ThreadRestrictions::AssertWaitAllowed();
On 2016/11/17 19:42:22, stanisc wrote:
> On 2016/11/16 22:57:29, danakj wrote:
> > same, but we should check for is_zero explicitly here too not just if the
> > timeout is done?
> 
> Moved the assert below the check. Shouldn't zero TimeTicks be already handled
by
> the condition. I could add an explicit check it it seems it would be
redundant.

0 would be handled, but other things too. I don't want the assert to be skipped
for any non-0 cases.

[stanisc, 2016-11-17 22:29:43]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 22:30:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stanisc, 2016-11-18 02:13:47]

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
File base/synchronization/waitable_event_win.cc (right):

https://codereview.chromium.org/2433773005/diff/100001/base/synchronization/w...
base/synchronization/waitable_event_win.cc:116:
base::ThreadRestrictions::AssertWaitAllowed();
On 2016/11/17 21:11:08, danakj wrote:
> On 2016/11/17 19:42:22, stanisc wrote:
> > On 2016/11/16 22:57:29, danakj wrote:
> > > same, but we should check for is_zero explicitly here too not just if the
> > > timeout is done?
> > 
> > Moved the assert below the check. Shouldn't zero TimeTicks be already
handled
> by
> > the condition. I could add an explicit check it it seems it would be
> redundant.
> 
> 0 would be handled, but other things too. I don't want the assert to be
skipped
> for any non-0 cases.

OK, I've added a special case for zero TimeTicks.

[commit-bot: I haz the power, 2016-11-18 02:25:13]

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

[commit-bot: I haz the power, 2016-11-18 02:25:14]

Dry run: This issue passed the CQ dry run.

[stanisc, 2016-11-22 19:56:42]

Please take another look when you have time.

[danakj, 2016-11-29 02:46:45]

LGTM

https://codereview.chromium.org/2433773005/diff/100001/base/message_loop/mess...
File base/message_loop/message_pump_default.cc (left):

https://codereview.chromium.org/2433773005/diff/100001/base/message_loop/mess...
base/message_loop/message_pump_default.cc:84: } else {
On 2016/11/17 19:42:22, stanisc wrote:
> On 2016/11/16 22:57:29, danakj wrote:
> > Can u explain why the if and this else block are gone?
> 
> Good call!
> I think it shouldn't matter with the new implementation whether
> delayed_work_time_ is set here to TimeTicks() or not.
> First of all it is always reset in DoDelayedWork() call above although that
> isn't straightforward when looking at the code. Even if it wasn't reset and
> stayed in the past that should be OK because (a) it would normally end up
being
> in the past in the former if block after a wait and (b) TimeWaitUntil is OK
with
> taking an already expired deadline - it just returns promptly in that case.
> 
> The only alternative would be to call Now() to compare delayed_work_time_ to
the
> current time but that is something this change was trying to avoid.

Ok thanks. It's true that in order to reach this point in the function, did_work
has to be false throughout, which means DoDelayedWork must have been called,
which means it was set. Can you drop a comment here saying this?

https://codereview.chromium.org/2433773005/diff/140001/base/synchronization/w...
File base/synchronization/waitable_event_posix.cc (right):

https://codereview.chromium.org/2433773005/diff/140001/base/synchronization/w...
base/synchronization/waitable_event_posix.cc:160: bool
WaitableEvent::TimedWait(const TimeDelta& max_delta) {
one more nit: i think wait_delta or just delta even would be better. there's a
bunch of use of Max() for end_time and stuff and this isn't that and it's
confusing

https://codereview.chromium.org/2433773005/diff/140001/base/synchronization/w...
File base/synchronization/waitable_event_win.cc (right):

https://codereview.chromium.org/2433773005/diff/140001/base/synchronization/w...
base/synchronization/waitable_event_win.cc:96: }  // namespace
nit: whitespace lines around the namespace { and }

https://codereview.chromium.org/2433773005/diff/140001/base/synchronization/w...
base/synchronization/waitable_event_win.cc:98: bool
WaitableEvent::TimedWait(const TimeDelta& max_delta) {
same name nit

[stanisc, 2016-11-29 22:32:35]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[stanisc, 2016-11-29 22:33:21]

https://codereview.chromium.org/2433773005/diff/140001/base/synchronization/w...
File base/synchronization/waitable_event_posix.cc (right):

https://codereview.chromium.org/2433773005/diff/140001/base/synchronization/w...
base/synchronization/waitable_event_posix.cc:160: bool
WaitableEvent::TimedWait(const TimeDelta& max_delta) {
On 2016/11/29 02:46:45, danakj wrote:
> one more nit: i think wait_delta or just delta even would be better. there's a
> bunch of use of Max() for end_time and stuff and this isn't that and it's
> confusing

Done.

https://codereview.chromium.org/2433773005/diff/140001/base/synchronization/w...
File base/synchronization/waitable_event_win.cc (right):

https://codereview.chromium.org/2433773005/diff/140001/base/synchronization/w...
base/synchronization/waitable_event_win.cc:96: }  // namespace
On 2016/11/29 02:46:45, danakj wrote:
> nit: whitespace lines around the namespace { and }

Done.

https://codereview.chromium.org/2433773005/diff/140001/base/synchronization/w...
base/synchronization/waitable_event_win.cc:98: bool
WaitableEvent::TimedWait(const TimeDelta& max_delta) {
On 2016/11/29 02:46:45, danakj wrote:
> same name nit

Done.

[commit-bot: I haz the power, 2016-11-29 22:33:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 00:35:45]

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

[commit-bot: I haz the power, 2016-11-30 00:35:46]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[stanisc, 2016-11-30 00:52:49]

The CQ bit was checked by stanisc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 00:53:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 02:56:03]

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

[commit-bot: I haz the power, 2016-11-30 02:56:04]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[stanisc, 2016-11-30 18:25:09]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2016-11-30 18:25:10]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2433773005/#ps160001
(title: "Addressed CR feedback")

[commit-bot: I haz the power, 2016-11-30 18:25:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 19:56:13]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480530309682470,
"parent_rev": "ec8df9f53b6dbe665cc33966ecebbfe190bfd914", "commit_rev":
"d000a504d656ffb1099e77127075614c65941cfb"}

[commit-bot: I haz the power, 2016-11-30 19:56:39]

Description was changed from

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified
max_time.
3) Introduced WaitableEvent::TimedWaitUntil method that
waits until the specified absolute deadline.

BUG=623223,465948
==========

to

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified
max_time.
3) Introduced WaitableEvent::TimedWaitUntil method that
waits until the specified absolute deadline.

BUG=623223,465948
==========

[commit-bot: I haz the power, 2016-11-30 19:56:40]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-11-30 20:00:26]

Description was changed from

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified
max_time.
3) Introduced WaitableEvent::TimedWaitUntil method that
waits until the specified absolute deadline.

BUG=623223,465948
==========

to

==========
Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

This is a followup for https://codereview.chromium.org/2086123002/.
This change removes the WIN_OS specific timed wait logic
from MessagePumpDefault::Run implementation and moves it
to OS_WIN specific WaitableEvent implementation.

This will make OS_WIN TimedWait behavior consistent with
the posix implementation:
1) A sub-ms wait doesn't time out promptly
(unless the timeout is exactly zero).
2) Also it doesn't time out earlier than the specified
max_time.
3) Introduced WaitableEvent::TimedWaitUntil method that
waits until the specified absolute deadline.

BUG=623223,465948

Committed: https://crrev.com/acf6801129dff3dba81f6f105713aab173a73e2c
Cr-Commit-Position: refs/heads/master@{#435388}
==========

[commit-bot: I haz the power, 2016-11-30 20:00:27]

Patchset 8 (id:??) landed as
https://crrev.com/acf6801129dff3dba81f6f105713aab173a73e2c
Cr-Commit-Position: refs/heads/master@{#435388}