MessagePump implementations should call DoWork and DoDelayedWork at equal priority

MessagePump implementations should call DoWork and DoDelayedWork at equal
priority.

BUG=72007
TEST=base_unittests --gtest_filter=MessageLoopTest.RecursiveDenial3

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75294

[Mark Mentovai, 2011-02-16 20:30:16]

I’m aware that this potentially calls DoDelayedWork even after someone inside of
DoWork calls quit. I believe that’s an existing quality of the Mac message pump
anyway, and I’d like to fix it in a follow-up change to keep things separable.
Fixing it here would add distractions to the diffs.

[Mark Mentovai, 2011-02-16 21:43:30]

Updated to include jamesr’s test from http://codereview.chromium.org/6334126/.

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

http://codereview.chromium.org/6507017/diff/2003/base/message_loop_unittest.cc
File base/message_loop_unittest.cc (right):

http://codereview.chromium.org/6507017/diff/2003/base/message_loop_unittest.c...
base/message_loop_unittest.cc:779: PlatformThread::Sleep(10); // milliseconds
nit: two spaces before '//' for inline comments

http://codereview.chromium.org/6507017/diff/2003/base/message_pump_mac.mm
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/6507017/diff/2003/base/message_pump_mac.mm#new...
base/message_pump_mac.mm:285: delegate_->DoDelayedWork(&next_time);
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]

New version up with minor changes in the unit test.

http://codereview.chromium.org/6507017/diff/2003/base/message_pump_mac.mm
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/6507017/diff/2003/base/message_pump_mac.mm#new...
base/message_pump_mac.mm:285: delegate_->DoDelayedWork(&next_time);
darin wrote:
> i would have expected a "did_work |= delegate_->DoDelayedWork(...)" here

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.

[darin (slow to review), 2011-02-17 18:46:05]

On Thu, Feb 17, 2011 at 10:28 AM, <mark@chromium.org> wrote:

> New version up with minor changes in the unit test.
>
>
>
> http://codereview.chromium.org/6507017/diff/2003/base/message_pump_mac.mm
> File base/message_pump_mac.mm (right):
>
>
>
http://codereview.chromium.org/6507017/diff/2003/base/message_pump_mac.mm#new...
> base/message_pump_mac.mm:285: delegate_->DoDelayedWork(&next_time);
> darin wrote:
>
>> i would have expected a "did_work |= delegate_->DoDelayedWork(...)"
>>
> here
>
> 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.


Yeah, now that I think about it more, I believe message_pump_win.cc is just
optimizing to avoid having to call MsgWaitForMultipleObjectsEx when the
message pump may have more work to do.

LGTM

-Darin



>
>
> http://codereview.chromium.org/6507017/
>