x11: Move X event handling out of the message-pump.

x11: Move X event handling out of the message-pump.

This change moves the X11 event handling out of the X11 message-pump, and uses
the X11 event dispatch code from X11EventSource instead. Overview of the changes:

 * Remove all X event handling code from the message-pump. The X11 message-pump
   only opens the connection to the X11 server. This too will be moved out of
   here in subsequent patches.

 * The X11EventSource sends an XEvent it receives from the X11 server back to
   the X11 message-pump, which triggers the MessagePumpObservers, before sending
   the event to the dispatchers. This is a short-term workaround until the
   message-pump observers are converted into PlatformEventObservers.

 * The MessagePumpDispatcher implementations that deal with X11 events are
   converted into PlatformEventDispatchers.

 * Remove support for starting a nested message-loop with a custom dispatcher
   on non-Windows.

Changes in components:

//ash:
 * Split AcceleratorDispatcher into platform-specific AcceleratorDispatcherWin,
   which remains a MessagePumpDispatcher, and AcceleratorDispatcherLinux, which
   is a PlatformEventDispatcher. It may be possible to do some cleanup in this,
   depending on the outcome of http://crbug.com/357777 and http://crbug.com/357733.

//base:
 * Remove support for providing a custom MessagePumpDispatcher when starting a
   nested message-loop on non-Windows platforms.
 * Remove most of the event-dispatch code from MessagePumpX11. The only remaining
   bits are for triggering MessagePumpObservers, which will be replaced by the newer
   PlatformEventObservers in subsequent patches.

//chrome, //content, //mojo, //ui/aura, //ui/base, //ui/wm:
 * Convert MessagePumpDispatchers that deal with X11 events into
   PlatformEventDispatchers.

//ui/events:
 * Allow creating a 'default' PlatformEventSource. On X11, it creates X11EventSource,
   and on other platforms, it doesn't create an event-source.
 * A temporary measure in X11EventSource to send the event to the message-pump so
   that the message-pump observers can be triggered. This will be removed once the
   MessagePumpObservers that deal with X11 events are turned into
   PlatformEventObservers.

//ui/views:
 * Remove the linux implementation of MenuMessagePumpDispatcher, and replace it with
   MenuEventDispatcherLinux.
 * Platform specific implementation for MenuController::RunMessageLoop(): the Windows
   implementation uses the MessagePumpDispatcher, and the non-windows implementation
   uses the PlatformEventDispatcher. This is somewhat unfortunate, and I am going to
   look for something better for this. But the code duplication here is relatively
   small, I don't want to make this patch any larger.

BUG=354062
R=darin@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262008

[sadrul, 2014-03-31 18:21:09]

Hi. Sorry about the large size of the patch. I tried to split it into smaller
CLs, but I think this is the smallest this can get. If you have suggestions to
split it into smaller CLs, please let me know. I am tried to explain the details
of the changes in the CL description.

[sky, 2014-03-31 20:57:28]

Only thing keeping me from approving is is if some setup should be promoted to
ashtestbase. Automatically creating the PlatformEventSource would be nice, but I
get that it may not be possible.

https://codereview.chromium.org/219743002/diff/20001/ash/wm/ash_native_cursor...
File ash/wm/ash_native_cursor_manager_interactive_uitest.cc (right):

https://codereview.chromium.org/219743002/diff/20001/ash/wm/ash_native_cursor...
ash/wm/ash_native_cursor_manager_interactive_uitest.cc:39: event_source_ =
ui::PlatformEventSource::CreateDefault();
Should this be moved to ashtestbase?

https://codereview.chromium.org/219743002/diff/20001/ui/base/dragdrop/os_exch...
File ui/base/dragdrop/os_exchange_data_unittest.cc (right):

https://codereview.chromium.org/219743002/diff/20001/ui/base/dragdrop/os_exch...
ui/base/dragdrop/os_exchange_data_unittest.cc:23:
scoped_ptr<PlatformEventSource> event_source_;
It sure would be nice if MessageLoopForUI could automatically create the
PlatformEventSource.

[sadrul, 2014-03-31 21:09:54]

On 2014/03/31 20:57:28, sky wrote:
> Only thing keeping me from approving is is if some setup should be promoted to
> ashtestbase. Automatically creating the PlatformEventSource would be nice, but
I
> get that it may not be possible.

I am actually deliberately creating the event-source on-demand only for tests
that need it, instead of for all tests. The main motivation for this is to have
less code running in tests, but I don't think it makes much of a difference. I
am happy to just create the event-source for all tests if you think that'd be
better (it would mean less work when creating a new test-suite, which I guess is
a good thing).

> 
>
https://codereview.chromium.org/219743002/diff/20001/ash/wm/ash_native_cursor...
> File ash/wm/ash_native_cursor_manager_interactive_uitest.cc (right):
> 
>
https://codereview.chromium.org/219743002/diff/20001/ash/wm/ash_native_cursor...
> ash/wm/ash_native_cursor_manager_interactive_uitest.cc:39: event_source_ =
> ui::PlatformEventSource::CreateDefault();
> Should this be moved to ashtestbase?
> 
>
https://codereview.chromium.org/219743002/diff/20001/ui/base/dragdrop/os_exch...
> File ui/base/dragdrop/os_exchange_data_unittest.cc (right):
> 
>
https://codereview.chromium.org/219743002/diff/20001/ui/base/dragdrop/os_exch...
> ui/base/dragdrop/os_exchange_data_unittest.cc:23:
> scoped_ptr<PlatformEventSource> event_source_;
> It sure would be nice if MessageLoopForUI could automatically create the
> PlatformEventSource.

[sky, 2014-03-31 21:41:56]

I like just doing it. That way everyone doesn't have to think about
this. Also, our tests create so much state now that I don't think it's
worth worry about the platformeventsource.

  -Scott

On Mon, Mar 31, 2014 at 2:09 PM,  <sadrul@chromium.org> wrote:
> On 2014/03/31 20:57:28, sky wrote:
>>
>> Only thing keeping me from approving is is if some setup should be
>> promoted to
>> ashtestbase. Automatically creating the PlatformEventSource would be nice,
>> but
>
> I
>>
>> get that it may not be possible.
>
>
> I am actually deliberately creating the event-source on-demand only for
> tests
> that need it, instead of for all tests. The main motivation for this is to
> have
> less code running in tests, but I don't think it makes much of a difference.
> I
> am happy to just create the event-source for all tests if you think that'd
> be
> better (it would mean less work when creating a new test-suite, which I
> guess is
> a good thing).
>
>
>
>
>
https://codereview.chromium.org/219743002/diff/20001/ash/wm/ash_native_cursor...
>>
>> File ash/wm/ash_native_cursor_manager_interactive_uitest.cc (right):
>
>
>
>
https://codereview.chromium.org/219743002/diff/20001/ash/wm/ash_native_cursor...
>>
>> ash/wm/ash_native_cursor_manager_interactive_uitest.cc:39: event_source_ =
>> ui::PlatformEventSource::CreateDefault();
>> Should this be moved to ashtestbase?
>
>
>
>
https://codereview.chromium.org/219743002/diff/20001/ui/base/dragdrop/os_exch...
>>
>> File ui/base/dragdrop/os_exchange_data_unittest.cc (right):
>
>
>
>
https://codereview.chromium.org/219743002/diff/20001/ui/base/dragdrop/os_exch...
>>
>> ui/base/dragdrop/os_exchange_data_unittest.cc:23:
>> scoped_ptr<PlatformEventSource> event_source_;
>> It sure would be nice if MessageLoopForUI could automatically create the
>> PlatformEventSource.
>
>
>
>
> https://codereview.chromium.org/219743002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-03-31 23:30:15]

https://codereview.chromium.org/219743002/diff/20001/ash/wm/ash_native_cursor...
File ash/wm/ash_native_cursor_manager_interactive_uitest.cc (right):

https://codereview.chromium.org/219743002/diff/20001/ash/wm/ash_native_cursor...
ash/wm/ash_native_cursor_manager_interactive_uitest.cc:39: event_source_ =
ui::PlatformEventSource::CreateDefault();
On 2014/03/31 20:57:28, sky wrote:
> Should this be moved to ashtestbase?

Moved the creation of the event-source into WMState. That minimizes the number
of places where we need to create it (and kind of makes sense too, I think).

https://codereview.chromium.org/219743002/diff/20001/ui/base/dragdrop/os_exch...
File ui/base/dragdrop/os_exchange_data_unittest.cc (right):

https://codereview.chromium.org/219743002/diff/20001/ui/base/dragdrop/os_exch...
ui/base/dragdrop/os_exchange_data_unittest.cc:23:
scoped_ptr<PlatformEventSource> event_source_;
On 2014/03/31 20:57:28, sky wrote:
> It sure would be nice if MessageLoopForUI could automatically create the
> PlatformEventSource.

I looked into adding this to PlatformTest (since this is arguably a platform
setup), but it doesn't look like we can add that dependency. So some of these
tests still need to explicitly create it.

[sky, 2014-03-31 23:37:36]

LGTM

[sadrul, 2014-03-31 23:40:09]

Thanks!

+darin@ for changes in base/

[sadrul, 2014-04-01 20:18:30]

> +darin@ for changes in base/

ping

[darin (slow to review), 2014-04-05 05:40:37]

base/ LGTM

[darin (slow to review), 2014-04-05 05:40:40]

base/ LGTM

[sadrul, 2014-04-05 15:24:10]

Committed patchset #7 manually as r262008 (presubmit successful).