events: Introduce PlatformEventDispatcher and PlatformEventSource.

events: Introduce PlatformEventDispatcher and PlatformEventSource.

This patch introduces PlatformEventSource and PlatformEventDispatcher for
dispatching events from native system. The event dispatch mechanism in
message-pumps will be replaced by this. In brief:

 * PlatformEventSource is a singleton instance that receives events from
   the platform.
 * Any number of PlatformEventDispatcher can be installed on a source for
   receiving events from the source. The dispatchers can then choose to
   dispatch the event or not. PlatformEventDispatcher will replace
   MessagePumpDispatcher.
 * Any number of PlatformEventObserver can be installed on a source.
   PlatformEventObserver will replace MessagePumpObserver.

The linked-to bug has a link to a more detail design doc.

BUG=354062
R=sky@chromium.org

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

[sadrul, 2014-03-24 05:15:11]

Hi. Sorry about the large size of the patch (although almost half of it is
tests, and a lot of the remaining code are boilerplates). Let me know if you
have suggestions for splitting it up into smaller pieces.

The larger patch for switching to this mechanism for dispatching X11 events and
removing is WIP at: https://codereview.chromium.org/205253003/

[sky, 2014-03-25 15:51:15]

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
File ui/events/platform/platform_event_dispatcher.h (right):

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_dispatcher.h:27: virtual
~PlatformEventDispatcher() {}
protected?

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
File ui/events/platform/platform_event_observer.h (right):

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_observer.h:17: virtual
~PlatformEventObserver() {}
protected?

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_observer.h:21: // observers by returning true
from this function. The event dispatch
nit: I think an enum would be cleaner here.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
File ui/events/platform/platform_event_source.cc (right):

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_source.cc:21: : overridden_dispatcher_(NULL),
overridden_dispatcher_restored_(false) {
nit: when you wrap one param per line.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_source.cc:35: CHECK(dispatcher);
DCHECK not already in dispatchers_?

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_source.cc:91: if (should_quit ||
overridden_dispatcher_restored_) {
Why does overridden_dispatcher_restored_ == true mean you need to quit the
current loop? OverrideDispatcher doesn't spawn a nested message loop. Maybe
you're assuming each override corresponds to a nested message loop? If so, can
you document that.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
File ui/events/platform/platform_event_source.h (right):

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_source.h:37: // dispatched can process the
event, or request that the default dispatchers
dispatched->dispatcher

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
File ui/events/platform/platform_event_types.h (right):

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_types.h:16: typedef XEvent* PlatformEvent;
Aren't you duplicating NativeEvent here?

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_types.h:20: typedef void* PlatformEvent;
Maybe this should be an error?

[sky, 2014-03-25 15:51:50]

Also, this is intended to replace dispatching a NativeEvent
(MessagePumpDispatcher::Dispatch(NativeEvent)). Maybe the names should better
mirror that? Eg NativeEventDispatcher, NativeEventSource. I'm not sure what to
name the directory though.

[sadrul, 2014-03-25 18:33:15]

On 2014/03/25 15:51:50, sky wrote:
> Also, this is intended to replace dispatching a NativeEvent
> (MessagePumpDispatcher::Dispatch(NativeEvent)). Maybe the names should better
> mirror that? Eg NativeEventDispatcher, NativeEventSource. I'm not sure what to
> name the directory though.

I went with Platform* instead of Native* since we have a few gfx::Native*s in
use (including gfx::NativeEvent), and I wanted to avoid making this more
confusing.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
File ui/events/platform/platform_event_dispatcher.h (right):

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_dispatcher.h:27: virtual
~PlatformEventDispatcher() {}
On 2014/03/25 15:51:15, sky wrote:
> protected?

Done.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
File ui/events/platform/platform_event_observer.h (right):

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_observer.h:17: virtual
~PlatformEventObserver() {}
On 2014/03/25 15:51:15, sky wrote:
> protected?

Done.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_observer.h:21: // observers by returning true
from this function. The event dispatch
On 2014/03/25 15:51:15, sky wrote:
> nit: I think an enum would be cleaner here.

Done.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
File ui/events/platform/platform_event_source.cc (right):

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_source.cc:21: : overridden_dispatcher_(NULL),
overridden_dispatcher_restored_(false) {
On 2014/03/25 15:51:15, sky wrote:
> nit: when you wrap one param per line.

Done.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_source.cc:35: CHECK(dispatcher);
On 2014/03/25 15:51:15, sky wrote:
> DCHECK not already in dispatchers_?

Done.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_source.cc:91: if (should_quit ||
overridden_dispatcher_restored_) {
On 2014/03/25 15:51:15, sky wrote:
> Why does overridden_dispatcher_restored_ == true mean you need to quit the
> current loop? OverrideDispatcher doesn't spawn a nested message loop. Maybe
> you're assuming each override corresponds to a nested message loop? If so, can
> you document that.

Yeah. This code was a bit misleading. I have clarified here with comments and
rearranged the code a little to make it clearer.

We should not terminate the message-loop if the override dispatcher is
destroyed. But it is necessary to stop dispatching the events from the
event-source in the current iteration of the message-loop, so that if there was
a nested message-loop active that has been terminated, it gets a chance to
unwind before events are dispatched again.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
File ui/events/platform/platform_event_source.h (right):

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_source.h:37: // dispatched can process the
event, or request that the default dispatchers
On 2014/03/25 15:51:15, sky wrote:
> dispatched->dispatcher

Done.

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
File ui/events/platform/platform_event_types.h (right):

https://codereview.chromium.org/203483004/diff/90001/ui/events/platform/platf...
ui/events/platform/platform_event_types.h:16: typedef XEvent* PlatformEvent;
On 2014/03/25 15:51:15, sky wrote:
> Aren't you duplicating NativeEvent here?

I am, yes. My plan is to get rid of base::NativeEvent altogether (it's currently
a bit confusing with base::NativeEvent and gfx::NativeEvent), since
MessagePumpDispatcher/MessagePumpObservers are the only users of this in base/.

I have changed this block to simply 'typedef base::NativeEvent PlatformEvent;'
Does this look more reasonable?

[sky, 2014-03-25 20:15:04]

All sounds good, LGTM

[sadrul, 2014-03-26 00:29:46]

Committed patchset #8 manually as r259417 (presubmit successful).