Mac-specific CFRunLoop-based MessagePump implementation

Mac-specific CFRunLoop-based MessagePump implementation

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

[Amanda Walker, 2008-09-05 01:04:16]

Drive-by comments: is adding Foundation to the googleurl project actually
related to the MessagePump stuff?  same question re: adding AppKit to net.  They
seem like fine changes, just unrelated to the rest of this change...

[darin (slow to review), 2008-09-10 01:30:36]

LGTM w/ some comments.

http://codereview.chromium.org/444/diff/401/604
File base/message_pump_mac.h (right):

http://codereview.chromium.org/444/diff/401/604#newcode169
Line 169: class MessagePumpMac {
Perhaps this should be renamed MessagePumpForUI.  The plan is to use that name,
as well as MessagePumpForIO, on the other platforms as well.

It looks like this is the only class that needs to be defined in this header
file.  I would move the rest of the classes into the source file.

http://codereview.chromium.org/444/diff/401/605
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/444/diff/401/605#newcode102
Line 102: Time::Exploded exploded;
it seems like this code should live in a utility somewhere.  i'm guessing that
it won't be the last time that we need to map a Time to a CFAbsoluteTime.

http://codereview.chromium.org/444/diff/401/605#newcode142
Line 142: if (self->delegate_->DoWork()) {
What if this is called from a run loop outside of our control, for example
before MessagePump::Run is called or after MessagePump::Run exits.  In other
words, delegate_ may be null here if something else pumps the current CFRunLoop.

This problem occurs in tests that call out to system libraries.

http://codereview.chromium.org/444/diff/401/605#newcode153
Line 153: self->delegate_->DoDelayedWork(&next_time);
ditto... how do we know that we are inside MessagePump::Run?

[Avi (use Gerrit), 2008-09-11 20:53:04]

http://codereview.chromium.org/444/diff/401/602
File base/message_loop.cc (right):

http://codereview.chromium.org/444/diff/401/602#newcode82
Line 82: pump_ = new base::MessagePumpDefault();
Why is this a different order than the above? If "UI" means platform, we should
use that convention in both sides of the if.

[Mark Mentovai, 2008-09-11 20:56:13]

http://codereview.chromium.org/444/diff/401/602
File base/message_loop.cc (right):

http://codereview.chromium.org/444/diff/401/602#newcode82
Line 82: pump_ = new base::MessagePumpDefault();
Because there's another type, IO.  The logic here is correct, and the
conditionals are organized to pluck the odd man out first and let everyone else
fall through.

[darin (slow to review), 2008-09-11 20:57:59]

http://codereview.chromium.org/444/diff/401/602
File base/message_loop.cc (right):

http://codereview.chromium.org/444/diff/401/602#newcode82
Line 82: pump_ = new base::MessagePumpDefault();
Mark is doing it properly.  On Windows we use the currently use the same
MessagePump implementation for TYPE_UI and TYPE_IO, but on Mac we don't have a
TYPE_IO implementation yet.

Anyways, this code is probably going to be changing again once we flesh out more
implementations.  It should probably be a case-switch with a shared default:
option.

[Amanda Walker, 2008-09-11 21:17:23]

LGTM, though I expect it will need revision.  I also wonder what we can mock for
unit testing it, but that can be a separate CL.

[Amanda Walker, 2008-09-11 21:17:30]

http://codereview.chromium.org/444/diff/401/604
File base/message_pump_mac.h (right):

http://codereview.chromium.org/444/diff/401/604#newcode169
Line 169: class MessagePumpMac {
MessagePumpForUI isn't a good name, since CFRunLoops can handle more than just
UI messages.  MessagePumpMac is OK, and better than MessagePumpCF or something
more opaque.

http://codereview.chromium.org/444/diff/401/605
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/444/diff/401/605#newcode309
Line 309: [NSApplication sharedApplication];
Do we need to do this in every DoRun invocation?

[Mark Mentovai, 2008-09-23 21:10:28]

Landed because it blocked McTestShell, but filed
http://code.google.com/p/chromium/issues/list?thanks=2708 for further cleanup.