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

base/message_loop/message_pump_default.cc

Index: base/message_loop/message_pump_default.cc
diff --git a/base/message_loop/message_pump_default.cc b/base/message_loop/message_pump_default.cc
index 3449aec8605b32e7eb868573144ecb45929809d7..220625baff498ea0be8496dfe15e16de924df892 100644
--- a/base/message_loop/message_pump_default.cc
+++ b/base/message_loop/message_pump_default.cc
@@ -4,8 +4,6 @@
 
 #include "base/message_loop/message_pump_default.h"
 
-#include <algorithm>
-
 #include "base/logging.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
@@ -54,38 +52,7 @@ void MessagePumpDefault::Run(Delegate* delegate) {
     if (delayed_work_time_.is_null()) {
       event_.Wait();
     } else {
-      TimeDelta delay = delayed_work_time_ - TimeTicks::Now();
-      if (delay > TimeDelta()) {
-#if defined(OS_WIN)
-        // TODO(stanisc): crbug.com/623223: Consider moving the OS_WIN specific
-        // logic into TimedWait implementation in waitable_event_win.cc.
-
-        // crbug.com/487724: on Windows, waiting for less than 1 ms results in
-        // returning from TimedWait promptly and spinning
-        // MessagePumpDefault::Run loop for up to 1 ms - until it is time to
-        // run a delayed task. |min_delay| is the minimum possible wait to
-        // to avoid the spinning.
-        constexpr TimeDelta min_delay = TimeDelta::FromMilliseconds(1);
-        do {
-          delay = std::max(delay, min_delay);
-          if (event_.TimedWait(delay))
-            break;
-
-          // TimedWait can time out earlier than the specified |delay| on
-          // Windows. It doesn't make sense to run the outer loop in that case
-          // because there isn't going to be any new work. It is less overhead
-          // to just go back to wait.
-          // In practice this inner wait loop might have up to 3 iterations.
-          delay = delayed_work_time_ - TimeTicks::Now();
-        } while (delay > TimeDelta());
-#else
-        event_.TimedWait(delay);
-#endif
-      } else {

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

Can u explain why the if and this else block are gone?

[stanisc, 2016/11/17 19:42:22]

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.

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

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?

-        // It looks like delayed_work_time_ indicates a time in the past, so we
-        // need to call DoDelayedWork now.
-        delayed_work_time_ = TimeTicks();
-      }
+      event_.TimedWaitUntil(delayed_work_time_);
     }
     // Since event_ is auto-reset, we don't need to do anything special here
     // other than service each delegate method.