DefaultDispatcher for MessagePumpX

DefaultDispatcher for MessagePumpX
 MPX itself doesn't know how to handle events and we've been converting MessageLoop::Run() -> Desktop::Run() or RunAllPending to use ui_test_utils::RunAllPendingInMessageLoop(). It's no longer necessary with this CL.

Windows's message pump can dispatch events without dispatcher.

BUG=none
TEST=none, but I believe some of browser_tests on aura is flaky because of this, and this CL should make them less/non flaky.


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

[sadrul, 2011-11-22 20:25:55]

http://codereview.chromium.org/8635014/diff/5001/base/message_pump_x.cc
File base/message_pump_x.cc (right):

http://codereview.chromium.org/8635014/diff/5001/base/message_pump_x.cc#newco...
base/message_pump_x.cc:117: DCHECK(!g_default_dispatcher || !dispatcher);
Is this check necessary? Do we want to prevent a default dispatcher to be
replaced by another one?

http://codereview.chromium.org/8635014/diff/5001/base/message_pump_x.cc#newco...
base/message_pump_x.cc:168: GetDispatcher() ? GetDispatcher() :
g_default_dispatcher;
Does it make sense to override GetDispatcher to return the default dispatcher if
none is set for the current state?

[oshima, 2011-11-22 21:20:13]

http://codereview.chromium.org/8635014/diff/5001/base/message_pump_x.cc
File base/message_pump_x.cc (right):

http://codereview.chromium.org/8635014/diff/5001/base/message_pump_x.cc#newco...
base/message_pump_x.cc:117: DCHECK(!g_default_dispatcher || !dispatcher);
On 2011/11/22 20:25:55, sadrul wrote:
> Is this check necessary? Do we want to prevent a default dispatcher to be
> replaced by another one?

I wanted to do this to make sure that each tests runs using the correct default
dispatcher.

http://codereview.chromium.org/8635014/diff/5001/base/message_pump_x.cc#newco...
base/message_pump_x.cc:168: GetDispatcher() ? GetDispatcher() :
g_default_dispatcher;
On 2011/11/22 20:25:55, sadrul wrote:
> Does it make sense to override GetDispatcher to return the default dispatcher
if
> none is set for the current state?

GetDispatcher is defined in MessagePumpGlib to give a subclass access to the
dispatcher in current state, so I think this is better, but i don't feel
strongly about it, so I can make that change if you want. Please let me know.

[sadrul, 2011-11-22 21:36:30]

I don't feel strongly either way either (I just wanted to bring it up to make
sure the change is intentionally the way it is). LGTM.

[darin (slow to review), 2011-11-22 22:38:57]

It occurs to me that this change makes it so that Desktop must be a singleton
(since there can only be one global MessagePumpDispatcher).  Is that your
intent?

Looking at desktop.h, I don't see anything there that would enforce singleton
semantics.

[darin (slow to review), 2011-11-22 22:39:46]

To be clear, my point is that since Desktop's constructor and destructor are
public, one doesn't need to use Desktop::GetInstance() to use an instance of a
Desktop class.

[oshima, 2011-11-22 22:44:43]

On 2011/11/22 22:39:46, darin wrote:
> To be clear, my point is that since Desktop's constructor and destructor are
> public, one doesn't need to use Desktop::GetInstance() to use an instance of a
> Desktop class.

It is singleton, but you're right about it's not enforced. I think we should
make constructor private so that only GetInstance() can instantiate it. I'll
update the CL.

[oshima, 2011-11-22 22:50:59]

uploaded new patch. PTAL.

[oshima, 2011-11-22 23:13:52]

+ben for ui/

[oshima, 2011-11-22 23:44:16]

Please hold. Fixing compilation error.

[oshima, 2011-11-23 00:35:02]

uploaded new patch. PTAL

[Ben Goodger (Google), 2011-11-23 01:04:11]

LGTM for aura/shell bits.

[darin (slow to review), 2011-11-23 04:53:26]

LGTM

[commit-bot: I haz the power, 2011-11-23 05:06:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/8635014/19007

[commit-bot: I haz the power, 2011-11-23 06:59:00]

Change committed as 111324