MessagePump implementations should call DoWork and DoDelayedWork at equal priority

base/message_pump_mac.mm

Index: base/message_pump_mac.mm
===================================================================
--- base/message_pump_mac.mm	(revision 75131)
+++ base/message_pump_mac.mm	(working copy)
@@ -51,7 +51,6 @@
       run_nesting_level_(0),
       deepest_nesting_level_(0),
       delegateless_work_(false),
-      delegateless_delayed_work_(false),
       delegateless_idle_work_(false) {
   run_loop_ = CFRunLoopGetCurrent();
   CFRetain(run_loop_);
@@ -78,15 +77,9 @@
                                        &source_context);
   CFRunLoopAddSource(run_loop_, work_source_, kCFRunLoopCommonModes);
 
-  source_context.perform = RunDelayedWorkSource;
-  delayed_work_source_ = CFRunLoopSourceCreate(NULL,  // allocator
-                                               2,     // priority
-                                               &source_context);
-  CFRunLoopAddSource(run_loop_, delayed_work_source_, kCFRunLoopCommonModes);
-
   source_context.perform = RunIdleWorkSource;
   idle_work_source_ = CFRunLoopSourceCreate(NULL,  // allocator
-                                            3,     // priority
+                                            2,     // priority
                                             &source_context);
   CFRunLoopAddSource(run_loop_, idle_work_source_, kCFRunLoopCommonModes);
 
@@ -169,9 +162,6 @@
   CFRunLoopRemoveSource(run_loop_, idle_work_source_, kCFRunLoopCommonModes);
   CFRelease(idle_work_source_);
 
-  CFRunLoopRemoveSource(run_loop_, delayed_work_source_, kCFRunLoopCommonModes);
-  CFRelease(delayed_work_source_);
-
   CFRunLoopRemoveSource(run_loop_, work_source_, kCFRunLoopCommonModes);
   CFRelease(work_source_);
 
@@ -199,10 +189,6 @@
       CFRunLoopSourceSignal(work_source_);
       delegateless_work_ = false;
     }
-    if (delegateless_delayed_work_) {
-      CFRunLoopSourceSignal(delayed_work_source_);
-      delegateless_delayed_work_ = false;
-    }
     if (delegateless_idle_work_) {
       CFRunLoopSourceSignal(idle_work_source_);
       delegateless_idle_work_ = false;
@@ -260,11 +246,10 @@
   self->delayed_work_fire_time_ = kCFTimeIntervalMax;
 
   // CFRunLoopTimers fire outside of the priority scheme for CFRunLoopSources.
-  // In order to establish the proper priority where delegate_->DoDelayedWork
-  // can only be called if delegate_->DoWork returns false, the timer used
-  // to schedule delayed work must signal a CFRunLoopSource set at a lower
-  // priority than the one used for delegate_->DoWork.
-  CFRunLoopSourceSignal(self->delayed_work_source_);
+  // In order to establish the proper priority in which work and delayed work
+  // are processed one for one, the timer used to schedule delayed work must
+  // signal a CFRunLoopSource used to dispatch both work and delayed work.
+  CFRunLoopSourceSignal(self->work_source_);
 }
 
 // Called from the run loop.
@@ -291,58 +276,39 @@
   // released promptly even in the absence of UI events.
   MessagePumpScopedAutoreleasePool autorelease_pool(this);
 
-  // Call DoWork once, and if something was done, arrange to come back here
-  // again as long as the loop is still running.
+  // Call DoWork and DoDelayedWork once, and if something was done, arrange to
+  // come back here again as long as the loop is still running.
   bool did_work = delegate_->DoWork();
-  if (did_work) {
-    CFRunLoopSourceSignal(work_source_);
-  }
+  bool resignal_work_source = did_work;
 
-  return did_work;
-}
-
-// Called from the run loop.
-// static
-void MessagePumpCFRunLoopBase::RunDelayedWorkSource(void* info) {
-  MessagePumpCFRunLoopBase* self = static_cast<MessagePumpCFRunLoopBase*>(info);
-  self->RunDelayedWork();
-}
-
-// Called by MessagePumpCFRunLoopBase::RunDelayedWorkSource.
-bool MessagePumpCFRunLoopBase::RunDelayedWork() {
-  if (!delegate_) {
-    // This point can be reached with a NULL delegate_ if Run is not on the
-    // stack but foreign code is spinning the CFRunLoop.  Arrange to come back
-    // here when a delegate is available.
-    delegateless_delayed_work_ = true;
-    return false;
-  }
-
-  // The NSApplication-based run loop only drains the autorelease pool at each
-  // UI event (NSEvent).  The autorelease pool is not drained for each
-  // CFRunLoopSource target that's run.  Use a local pool for any autoreleased
-  // objects if the app is not currently handling a UI event to ensure they're
-  // released promptly even in the absence of UI events.
-  MessagePumpScopedAutoreleasePool autorelease_pool(this);
-
   TimeTicks next_time;
   delegate_->DoDelayedWork(&next_time);

[darin (slow to review), 2011/02/16 23:58:02]

i would have expected a "did_work |= delegate_->DoDelayedWork(...)" here, and i
would expect resignal_work_source to be true if DoDelayedWork returned true.

i'm basing this assertion on my reading of message_pump_win.cc.

the idea is fairly simple:  if we did work, then more work is plausible, so we
"spin the loop" again.

i don't know for sure if this is important; however, it might be nice to just
make the windows and mac message pumps more similar.

[Mark Mentovai, 2011/02/17 18:28:39]

darin wrote:
did_work |= doesn’t really help here. As you can see, I use the value of
did_work from DoWork to determine whether it’s necessary to try to schedule more
work; I use next_time to determine whether the work should be scheduled
immediately or in the future.

Different implementation, different strategy. I’ve got to hit
ScheduleDelayedWork from here. The Windows pump seems to do that right on the
timer callback, but it also has a DoDelayedWork call in its timer callback. That
won’t work here on the Mac, we’ll wind up with another starvation or ordering
problem (which the “timer signals run loop source” architecture was intended to
resolve.)

As cute as the |= is, this code is much clearer ignoring the result of
DoDelayedWork and then interpreting next_time on its own.

-
-  bool more_work = !next_time.is_null();
-  if (more_work) {
-    TimeDelta delay = next_time - TimeTicks::Now();
-    if (delay > TimeDelta()) {
-      // There's more delayed work to be done in the future.
-      ScheduleDelayedWork(next_time);
-    } else {
-      // There's more delayed work to be done, and its time is in the past.
-      // Arrange to come back here directly as long as the loop is still
-      // running.
-      CFRunLoopSourceSignal(delayed_work_source_);
+  if (!did_work) {
+    // Determine whether there's more delayed work, and if so, if it needs to
+    // be done at some point in the future or if it's already time to do it.
+    // Only do these checks if did_work is false. If did_work is true, this
+    // function, and therefore any additional delayed work, will get another
+    // chance to run before the loop goes to sleep.
+    bool more_delayed_work = !next_time.is_null();
+    if (more_delayed_work) {
+      TimeDelta delay = next_time - TimeTicks::Now();
+      if (delay > TimeDelta()) {
+        // There's more delayed work to be done in the future.
+        ScheduleDelayedWork(next_time);
+      } else {
+        // There's more delayed work to be done, and its time is in the past.
+        // Arrange to come back here directly as long as the loop is still
+        // running.
+        resignal_work_source = true;
+      }
     }
   }
 
-  return more_work;
+  if (resignal_work_source) {
+    CFRunLoopSourceSignal(work_source_);
+  }
+
+  return resignal_work_source;
 }
 
 // Called from the run loop.
@@ -398,19 +364,12 @@
 
   // Immediately try work in priority order.
   if (!RunWork()) {
-    if (!RunDelayedWork()) {
-      if (!RunIdleWork()) {
-        return false;
-      }
-    } else {
-      // There was no work, and delayed work was done.  Arrange for the loop
-      // to try non-nestable idle work on a subsequent pass.
-      CFRunLoopSourceSignal(idle_work_source_);
+    if (!RunIdleWork()) {
+      return false;
     }
   } else {
-    // Work was done.  Arrange for the loop to try non-nestable delayed and
-    // idle work on a subsequent pass.
-    CFRunLoopSourceSignal(delayed_work_source_);
+    // Work was done.  Arrange for the loop to try non-nestable idle work on
+    // a subsequent pass.
     CFRunLoopSourceSignal(idle_work_source_);
   }