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

base/message_loop/message_pump_default.cc

 // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 
 #if defined(OS_MACOSX)
 #include "base/mac/scoped_nsautorelease_pool.h"
 #endif
 
 namespace base {
 
@@ skipping 28 matching lines @@
     if (!keep_running_)
       break;
 
     if (did_work)
       continue;
 
     ThreadRestrictions::ScopedAllowWait allow_wait;
     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.
   }
 
   keep_running_ = true;
 }
 
 void MessagePumpDefault::Quit() {
   keep_running_ = false;
 }
 
 void MessagePumpDefault::ScheduleWork() {
   // Since this can be called on any thread, we need to ensure that our Run
   // loop wakes up.
   event_.Signal();
 }
 
 void MessagePumpDefault::ScheduleDelayedWork(
     const TimeTicks& delayed_work_time) {
   // We know that we can't be blocked on Wait right now since this method can
   // only be called on the same thread as Run, so we only need to update our
   // record of how long to sleep when we do sleep.
   delayed_work_time_ = delayed_work_time;
 }
 
 }  // namespace base