Eliminate TimerManager::GetCurrentDelay in favor of always referring to the fire time of the next timer. I changed the MessagePump API to refer to a delayed_work_time instead of a delay.

I moved the ceil-based rounding code into the Window's implementations of WaitableEvent and MessagePump.

R=jar


git-svn-id: svn://svn.chromium.org/chrome/trunk/src@1075 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 81 insertions, 63 deletions

diff --git a/base/message_loop.cc b/base/message_loop.cc
index 4964dfe..eb10ca7 100644
--- a/base/message_loop.cc
+++ b/base/message_loop.cc
@@ -386,16 +386,16 @@ void MessageLoop::DeletePendingTasks() {
 }
 
 void MessageLoop::DidChangeNextTimerExpiry() {
-  int delay = timer_manager_.GetCurrentDelay(); 
-  if (delay == -1)
+  Time next_delayed_work_time = timer_manager_.GetNextFireTime(); 
+  if (next_delayed_work_time.is_null())
     return;
 
   // Simulates malfunctioning, early firing timers. Pending tasks should only
   // be invoked when the delay they specify has elapsed.
   if (timer_manager_.use_broken_delay())
-    delay = 10;
+    next_delayed_work_time = Time::Now() + TimeDelta::FromMilliseconds(10);
 
-  pump_->ScheduleDelayedWork(TimeDelta::FromMilliseconds(delay));
+  pump_->ScheduleDelayedWork(next_delayed_work_time);
 }
 
 bool MessageLoop::DoWork() {
@@ -403,12 +403,12 @@ bool MessageLoop::DoWork() {
   return QueueOrRunTask(NULL);
 }
 
-bool MessageLoop::DoDelayedWork(TimeDelta* next_delay) {
+bool MessageLoop::DoDelayedWork(Time* next_delayed_work_time) {
   bool did_work = timer_manager_.RunSomePendingTimers();
 
   // We may not have run any timers, but we may still have future timers to
   // run, so we need to inform the pump again of pending timers.
-  *next_delay = TimeDelta::FromMilliseconds(timer_manager_.GetCurrentDelay());
+  *next_delayed_work_time = timer_manager_.GetNextFireTime();
 
   return did_work;
 }
diff --git a/base/message_loop.h b/base/message_loop.h
index afcab64..3821519 100644
--- a/base/message_loop.h
+++ b/base/message_loop.h
@@ -415,7 +415,7 @@ class MessageLoop : public base::MessagePump::Delegate {
 
   // base::MessagePump::Delegate methods:
   virtual bool DoWork();
-  virtual bool DoDelayedWork(TimeDelta* next_delay);
+  virtual bool DoDelayedWork(Time* next_delayed_work_time);
   virtual bool DoIdleWork();
 
   // Start recording histogram info about events and action IF it was enabled
diff --git a/base/message_pump.h b/base/message_pump.h
index a06a435..06cef43 100644
--- a/base/message_pump.h
+++ b/base/message_pump.h
@@ -32,7 +32,7 @@
 
 #include "base/ref_counted.h"
 
-class TimeDelta;
+class Time;
 
 namespace base {
 
@@ -52,11 +52,12 @@ class MessagePump : public RefCountedThreadSafe<MessagePump> {
     // Called from within Run in response to ScheduleDelayedWork or when the
     // message pump would otherwise sleep waiting for more work.  Returns true
     // to indicate that delayed work was done.  DoIdleWork will not be called
-    // if DoDelayedWork returns true.  Upon return |next_delay| indicates the
-    // next delayed work interval.  If |next_delay| is negative, then the queue
-    // of future delayed work (timer events) is currently empty, and no
-    // additional calls to this function need to be scheduled.
-    virtual bool DoDelayedWork(TimeDelta* next_delay) = 0;
+    // if DoDelayedWork returns true.  Upon return |next_delayed_work_time|
+    // indicates the time when DoDelayedWork should be called again.  If
+    // |next_delayed_work_time| is null (per Time::is_null), then the queue of
+    // future delayed work (timer events) is currently empty, and no additional
+    // calls to this function need to be scheduled.
+    virtual bool DoDelayedWork(Time* next_delayed_work_time) = 0;
 
     // Called from within Run just before the message pump goes to sleep.
     // Returns true to indicate that idle work was done.
@@ -137,10 +138,10 @@ class MessagePump : public RefCountedThreadSafe<MessagePump> {
   // until it returns a value of false.
   virtual void ScheduleWork() = 0;
 
-  // Schedule a DoDelayedWork callback to happen after the specified delay,
+  // Schedule a DoDelayedWork callback to happen at the specified time,
   // cancelling any pending DoDelayedWork callback.  This method may only be
   // used on the thread that called Run.
-  virtual void ScheduleDelayedWork(const TimeDelta& delay) = 0;
+  virtual void ScheduleDelayedWork(const Time& delayed_work_time) = 0;
 };
 
 }  // namespace base
diff --git a/base/message_pump_default.cc b/base/message_pump_default.cc
index 916d035..a45f480 100644
--- a/base/message_pump_default.cc
+++ b/base/message_pump_default.cc
@@ -51,9 +51,13 @@ void MessagePumpDefault::Run(Delegate* delegate) {
 
     // TODO(darin): Delayed work will be starved if DoWork continues to return
     // true.  We should devise a better strategy.
+    //
+    // It is tempting to call DoWork followed by DoDelayedWork before checking
+    // did_work, but we need to make sure that any tasks that were dispatched
+    // prior to a timer actually run before the timer.  Getting that right may
+    // require some additional changes.
 
-    TimeDelta delay;
-    did_work = delegate->DoDelayedWork(&delay);
+    did_work = delegate->DoDelayedWork(&delayed_work_time_);
     if (!keep_running_)
       break;
     if (did_work)
@@ -64,13 +68,18 @@ void MessagePumpDefault::Run(Delegate* delegate) {
       break;
     if (did_work)
       continue;
-    // When DoIdleWork does not work, we also assume that it ran very quickly
-    // such that |delay| still properly indicates how long we are to sleep.
 
-    if (delay < TimeDelta::FromMilliseconds(0)) {
+    if (delayed_work_time_.is_null()) {
       event_.Wait();
     } else {
-      event_.TimedWait(delay);
+      TimeDelta delay = delayed_work_time_ - Time::Now();
+      if (delay > TimeDelta()) {
+        event_.TimedWait(delay);
+      } else {
+        // It looks like delayed_work_time_ indicates a time in the past, so we
+        // need to call DoDelayedWork now.
+        delayed_work_time_ = Time();
+      }
     }
     // Since event_ is auto-reset, we don't need to do anything special here
     // other than service each delegate method.
@@ -89,12 +98,11 @@ void MessagePumpDefault::ScheduleWork() {
   event_.Signal();
 }
 
-void MessagePumpDefault::ScheduleDelayedWork(const TimeDelta& delay) {
+void MessagePumpDefault::ScheduleDelayedWork(const Time& 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, but to ensure that when we do
-  // sleep, we sleep for the right time, we signal the event to cause the Run
-  // loop to do one more iteration.
-  event_.Signal();
+  // 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
diff --git a/base/message_pump_default.h b/base/message_pump_default.h
index 1c1af71..7c65013 100644
--- a/base/message_pump_default.h
+++ b/base/message_pump_default.h
@@ -44,7 +44,7 @@ class MessagePumpDefault : public MessagePump {
   virtual void Run(Delegate* delegate);
   virtual void Quit();
   virtual void ScheduleWork();
-  virtual void ScheduleDelayedWork(const TimeDelta& delay);
+  virtual void ScheduleDelayedWork(const Time& delayed_work_time);
 
  private:
   // This flag is set to false when Run should return.
@@ -53,6 +53,9 @@ class MessagePumpDefault : public MessagePump {
   // Used to sleep until there is more work to do.
   WaitableEvent event_;
 
+  // The time at which we should call DoDelayedWork.
+  Time delayed_work_time_;
+
   DISALLOW_COPY_AND_ASSIGN(MessagePumpDefault);
 };
 
diff --git a/base/message_pump_win.cc b/base/message_pump_win.cc
index f290e44..28191ea 100644
--- a/base/message_pump_win.cc
+++ b/base/message_pump_win.cc
@@ -29,6 +29,8 @@
 
 #include "base/message_pump_win.h"
 
+#include <math.h>
+
 #include "base/histogram.h"
 #include "base/win_util.h"
 
@@ -160,7 +162,7 @@ void MessagePumpWin::ScheduleWork() {
   PostMessage(message_hwnd_, kMsgHaveWork, reinterpret_cast<WPARAM>(this), 0);
 }
 
-void MessagePumpWin::ScheduleDelayedWork(const TimeDelta& delay) {
+void MessagePumpWin::ScheduleDelayedWork(const Time& delayed_work_time) {
   //
   // We would *like* to provide high resolution timers.  Windows timers using
   // SetTimer() have a 10ms granularity.  We have to use WM_TIMER as a wakeup
@@ -181,7 +183,9 @@ void MessagePumpWin::ScheduleDelayedWork(const TimeDelta& delay) {
   // Getting a spurrious SetTimer event firing is benign, as we'll just be
   // processing an empty timer queue.
   //
-  int delay_msec = static_cast<int>(delay.InMilliseconds());
+  delayed_work_time_ = delayed_work_time;
+
+  int delay_msec = GetCurrentDelay();
   DCHECK(delay_msec >= 0);
   if (delay_msec < USER_TIMER_MINIMUM)
     delay_msec = USER_TIMER_MINIMUM;
@@ -250,10 +254,11 @@ void MessagePumpWin::HandleTimerMessage() {
   if (!state_)
     return;
 
-  TimeDelta next_delay;
-  state_->delegate->DoDelayedWork(&next_delay);
-  if (next_delay >= TimeDelta::FromMilliseconds(0))
-    ScheduleDelayedWork(next_delay);
+  state_->delegate->DoDelayedWork(&delayed_work_time_);
+  if (!delayed_work_time_.is_null()) {
+    // A bit gratuitous to set delayed_work_time_ again, but oh well.
+    ScheduleDelayedWork(delayed_work_time_);
+  }
 }
 
 void MessagePumpWin::DoRunLoop() {
@@ -295,14 +300,13 @@ void MessagePumpWin::DoRunLoop() {
     if (more_work_is_plausible)
       continue;
 
-    TimeDelta next_delay;
-    more_work_is_plausible = state_->delegate->DoDelayedWork(&next_delay);
+    more_work_is_plausible =
+        state_->delegate->DoDelayedWork(&delayed_work_time_);
     // If we did not process any delayed work, then we can assume that our
     // existing WM_TIMER if any will fire when delayed work should run.  We
     // don't want to disturb that timer if it is already in flight.  However,
     // if we did do all remaining delayed work, then lets kill the WM_TIMER.
-    if (more_work_is_plausible &&
-        next_delay < TimeDelta::FromMilliseconds(0))
+    if (more_work_is_plausible && delayed_work_time_.is_null())
       KillTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this));
     if (state_->should_quit)
       break;
@@ -519,8 +523,17 @@ int MessagePumpWin::GetCurrentDelay() const {
   if (delayed_work_time_.is_null())
     return -1;
 
-  // This could be a negative value, but that's OK.
-  return static_cast<int>((Time::Now() - delayed_work_time_).InMilliseconds());
+  // Be careful here.  TimeDelta has a precision of microseconds, but we want a
+  // value in milliseconds.  If there are 5.5ms left, should the delay be 5 or
+  // 6?  It should be 6 to avoid executing delayed work too early.
+  double timeout = ceil((Time::Now() - delayed_work_time_).InMillisecondsF());
+
+  // If this value is negative, then we need to run delayed work soon.
+  int delay = static_cast<int>(timeout);
+  if (delay < 0)
+    delay = 0;
+
+  return delay;
 }
 
 }  // namespace base
diff --git a/base/message_pump_win.h b/base/message_pump_win.h
index 6699fde..ff021e3 100644
--- a/base/message_pump_win.h
+++ b/base/message_pump_win.h
@@ -161,7 +161,7 @@ class MessagePumpWin : public MessagePump {
   virtual void Run(Delegate* delegate) { RunWithDispatcher(delegate, NULL); }
   virtual void Quit();
   virtual void ScheduleWork();
-  virtual void ScheduleDelayedWork(const TimeDelta& delay);
+  virtual void ScheduleDelayedWork(const Time& delayed_work_time);
 
  private:
   struct RunState {
diff --git a/base/timer.cc b/base/timer.cc
index 46fae6e..aced8cb 100644
--- a/base/timer.cc
+++ b/base/timer.cc
@@ -55,14 +55,6 @@ Timer::Timer(int delay, Task* task, bool repeating)
   Reset();
 }
 
-int Timer::GetCurrentDelay() const {
-  // Be careful here.  Timers have a precision of microseconds, but this API is
-  // in milliseconds.  If there are 5.5ms left, should the delay be 5 or 6?  It
-  // should be 6 to avoid timers firing early.
-  double delay = ceil((fire_time_ - Time::Now()).InMillisecondsF());
-  return static_cast<int>(delay);
-}
-
 void Timer::Reset() {
   creation_time_ = Time::Now();
   fire_time_ = creation_time_ + TimeDelta::FromMilliseconds(delay_);
@@ -163,7 +155,7 @@ bool TimerManager::RunSomePendingTimers() {
   // timers have been set.
   const int kMaxTimersPerCall = 2;
   for (int i = 0; i < kMaxTimersPerCall; ++i) {
-    if (timers_.empty() || GetCurrentDelay() > 0)
+    if (timers_.empty() || timers_.top()->fire_time() > Time::Now())
       break;
 
     // Get a pending timer.  Deal with updating the timers_ queue and setting
@@ -211,13 +203,11 @@ void TimerManager::StartTimer(Timer* timer) {
     DidChangeNextTimer();
 }
 
-int TimerManager::GetCurrentDelay() {
+Time TimerManager::GetNextFireTime() const {
   if (timers_.empty())
-    return -1;
-  int delay = timers_.top()->GetCurrentDelay();
-  if (delay < 0)
-    delay = 0;
-  return delay;
+    return Time();
+
+  return timers_.top()->fire_time();
 }
 
 void TimerManager::DidChangeNextTimer() {
diff --git a/base/timer.h b/base/timer.h
index c23f725..74fe27b 100644
--- a/base/timer.h
+++ b/base/timer.h
@@ -59,9 +59,6 @@ class Timer {
   Task* task() const { return task_; }
   void set_task(Task* task) { task_ = task; }
 
-  // Returns the time in msec relative to now that the timer should fire.
-  int GetCurrentDelay() const;
-
   // Returns the absolute time at which the timer should fire.
   const Time &fire_time() const { return fire_time_; }
 
@@ -177,9 +174,10 @@ class TimerManager {
   // Returns true if it runs a task, false otherwise.
   bool RunSomePendingTimers();
 
-  // The number of milliseconds remaining until the pending timer (top of the
-  // pqueue) needs to be fired. Returns -1 if no timers are pending.
-  int GetCurrentDelay();
+  // The absolute time at which the next timer is to fire.  If there is not a
+  // next timer to run, then the is_null property of the returned Time object
+  // will be true.  NOTE: This could be a time in the past!
+  Time GetNextFireTime() const;
 
 #ifdef UNIT_TEST
   // For testing only, used to simulate broken early-firing WM_TIMER
diff --git a/base/waitable_event_win.cc b/base/waitable_event_win.cc
index da2d2d8..e23eea0 100644
--- a/base/waitable_event_win.cc
+++ b/base/waitable_event_win.cc
@@ -29,6 +29,7 @@
 
 #include "base/waitable_event.h"
 
+#include <math.h>
 #include <windows.h>
 
 #include "base/logging.h"
@@ -68,8 +69,12 @@ bool WaitableEvent::Wait() {
 }
 
 bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
-  int32 timeout = static_cast<int32>(max_time.InMilliseconds());
-  DWORD result = WaitForSingleObject(event_, timeout);
+  DCHECK(max_time >= TimeDelta::FromMicroseconds(0));
+  // Be careful here.  TimeDelta has a precision of microseconds, but this API
+  // is in milliseconds.  If there are 5.5ms left, should the delay be 5 or 6?
+  // It should be 6 to avoid returning too early.
+  double timeout = ceil(max_time.InMillisecondsF());
+  DWORD result = WaitForSingleObject(event_, static_cast<DWORD>(timeout));
   switch (result) {
     case WAIT_OBJECT_0:
       return true;