Consolidate ui::NativeEvent and base::NativeEvent

Consolidate ui::NativeEvent and base::NativeEvent

Note: I didn't add Wayland version of NativeEvent because the type is defined in ui, which base shouldn't depend on. I looked at the original CL and this typedef in message_pump_wayland seems to be added after reviewer gave LGTM.

BUG=none
TEST=none

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

[msw, 2011-10-04 18:10:37]

You should add another reviewer for OWNERS approval on the new base filename,
etc. Also, can you check a Wayland build? This likely breaks it, if it wasn't
broken already.

http://codereview.chromium.org/8113028/diff/5001/base/message_pump_observer.h
File base/message_pump_observer.h (right):

http://codereview.chromium.org/8113028/diff/5001/base/message_pump_observer.h...
base/message_pump_observer.h:35: virtual EventStatus WillProcessEvent(const
base::NativeEvent& event) = 0;
The base namespace qualifier isn't needed here or in DidProcessEvent.

http://codereview.chromium.org/8113028/diff/5001/ui/base/events.h
File ui/base/events.h (right):

http://codereview.chromium.org/8113028/diff/5001/ui/base/events.h#newcode20
ui/base/events.h:20: union WaylandEvent;
If we are removing support for WaylandEvent as a NativeEvent, then you might as
well delete ui/base/wayland and any other wayland code...

http://codereview.chromium.org/8113028/diff/5001/ui/base/wayland/events_wayla...
File ui/base/wayland/events_wayland.cc (right):

http://codereview.chromium.org/8113028/diff/5001/ui/base/wayland/events_wayla...
ui/base/wayland/events_wayland.cc:39: int
GetButtonEventFlagsFromNativeEvent(const base::NativeEvent& native_event) {
I don't know what this will do in a Wayland build if there is no WaylandEvent
NativeEvent...

http://codereview.chromium.org/8113028/diff/5001/ui/base/wayland/events_wayla...
ui/base/wayland/events_wayland.cc:57: EventType EventTypeFromNative(const
NativeEvent& native_event) {
How is this (and the functions below) building if there is no NativeEvent type
defined in namespace ui? (also these functions expect WaylandEvent...)

http://codereview.chromium.org/8113028/diff/5001/views/events/event.cc
File views/events/event.cc (right):

http://codereview.chromium.org/8113028/diff/5001/views/events/event.cc#newcod...
views/events/event.cc:246: MouseWheelEvent::MouseWheelEvent(const
base::NativeEvent& native_event)
You can omit the base namespace here and use the views:: typdef, I could have
done that originally, sorry I missed it.

http://codereview.chromium.org/8113028/diff/5001/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/8113028/diff/5001/views/events/event_x.cc#newc...
views/events/event_x.cc:172: TouchEvent::TouchEvent(const base::NativeEvent&
native_event)
I *think* you can omit the base namespace here and use the views:: typdef (the
declaration uses the views typedef); but this isn't in a !USE_ARUA block... I
also wonder where the aura build gets the definition for this ctor; there's
likely an error here on linux USE_AURA.

[oshima, 2011-10-04 19:42:48]

For wayland part, I didn't add Wayland not because we want to remove it, but
because base shouldn't use ui component.
 Wayland build is already broken, and we decided to keep it for now because msb
is interested in supporting it. So, we don't deliberately break it, but it's
maintainer's responsibility to fix breakage. I hope this helps.

I'll add OWNERS for ui/base once this looks good to you.

http://codereview.chromium.org/8113028/diff/5001/base/message_pump_observer.h
File base/message_pump_observer.h (right):

http://codereview.chromium.org/8113028/diff/5001/base/message_pump_observer.h...
base/message_pump_observer.h:35: virtual EventStatus WillProcessEvent(const
base::NativeEvent& event) = 0;
On 2011/10/04 18:10:37, msw wrote:
> The base namespace qualifier isn't needed here or in DidProcessEvent.

Done.

http://codereview.chromium.org/8113028/diff/5001/views/events/event.cc
File views/events/event.cc (right):

http://codereview.chromium.org/8113028/diff/5001/views/events/event.cc#newcod...
views/events/event.cc:246: MouseWheelEvent::MouseWheelEvent(const
base::NativeEvent& native_event)
On 2011/10/04 18:10:37, msw wrote:
> You can omit the base namespace here and use the views:: typdef, I could have
> done that originally, sorry I missed it.

Done.

http://codereview.chromium.org/8113028/diff/5001/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/8113028/diff/5001/views/events/event_x.cc#newc...
views/events/event_x.cc:172: TouchEvent::TouchEvent(const base::NativeEvent&
native_event)
On 2011/10/04 18:10:37, msw wrote:
> I *think* you can omit the base namespace here and use the views:: typdef (the
> declaration uses the views typedef); but this isn't in a !USE_ARUA block... I
> also wonder where the aura build gets the definition for this ctor; there's
> likely an error here on linux USE_AURA.

This is created in accelerator_handler_touch.cc using xevent, so base seems to
be correct choice. We probably need to revisit when we add touch support for
aura though.

[msw, 2011-10-04 20:45:40]

Afaict you're defining base::NativeEvent as void* for Wayland, but that type is
used (expecting ui::WaylandEvent*) in ui/base/wayland/events_wayland.cc (event
cracking), which is required for views/events/event.h|cc (event processing).
This seems like you are deliberately breaking Wayland (albeit in a trivial
manner). Is there any way to reconcile this with a more graceful [temporary]
measure that would theoretically leave Wayland building (I can't easily verify,
need kbproto/xkbcommon or something...)?

http://codereview.chromium.org/8113028/diff/5001/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/8113028/diff/5001/views/events/event_x.cc#newc...
views/events/event_x.cc:172: TouchEvent::TouchEvent(const base::NativeEvent&
native_event)
On 2011/10/04 19:42:48, oshima wrote:
> On 2011/10/04 18:10:37, msw wrote:
> > I *think* you can omit the base namespace here and use the views:: typdef
(the
> > declaration uses the views typedef); but this isn't in a !USE_ARUA block...
I
> > also wonder where the aura build gets the definition for this ctor; there's
> > likely an error here on linux USE_AURA.
> 
> This is created in accelerator_handler_touch.cc using xevent, so base seems to
> be correct choice. We probably need to revisit when we add touch support for
> aura though.

Okay, either way the base:: qualifier should either be present in both the decl
and def or neither. Optionally add a TODO about aura support.

http://codereview.chromium.org/8113028/diff/9002/ui/base/events.h
File ui/base/events.h (right):

http://codereview.chromium.org/8113028/diff/9002/ui/base/events.h#newcode19
ui/base/events.h:19: #if defined(USE_WAYLAND)
This isn't currently used anywhere.

[oshima, 2011-10-04 22:06:48]

On 2011/10/04 20:45:40, msw wrote:
> Afaict you're defining base::NativeEvent as void* for Wayland, but that type
is
> used (expecting ui::WaylandEvent*) in ui/base/wayland/events_wayland.cc (event
> cracking), which is required for views/events/event.h|cc (event processing).
> This seems like you are deliberately breaking Wayland (albeit in a trivial
> manner). Is there any way to reconcile this with a more graceful [temporary]
> measure that would theoretically leave Wayland building (I can't easily
verify,
> need kbproto/xkbcommon or something...)?

Unfortunately WaylandEvent is defined in ui component and we can't simply use it
in base component. I left a comment in event_types.h so that wayland maintainer
can look at it and fix.

> 
> http://codereview.chromium.org/8113028/diff/5001/views/events/event_x.cc
> File views/events/event_x.cc (right):
> 
>
http://codereview.chromium.org/8113028/diff/5001/views/events/event_x.cc#newc...
> views/events/event_x.cc:172: TouchEvent::TouchEvent(const base::NativeEvent&
> native_event)
> On 2011/10/04 19:42:48, oshima wrote:
> > On 2011/10/04 18:10:37, msw wrote:
> > > I *think* you can omit the base namespace here and use the views:: typdef
> (the
> > > declaration uses the views typedef); but this isn't in a !USE_ARUA
block...
> I
> > > also wonder where the aura build gets the definition for this ctor;
there's
> > > likely an error here on linux USE_AURA.
> > 
> > This is created in accelerator_handler_touch.cc using xevent, so base seems
to
> > be correct choice. We probably need to revisit when we add touch support for
> > aura though.
> 
> Okay, either way the base:: qualifier should either be present in both the
decl
> and def or neither. Optionally add a TODO about aura support.

I looked at the code again and turns out that this has to be views::NativeEvent.
Looks like event_x.cc is excluded from aura build (i'll fix this in separate
CL).

> 
> http://codereview.chromium.org/8113028/diff/9002/ui/base/events.h
> File ui/base/events.h (right):
> 
> http://codereview.chromium.org/8113028/diff/9002/ui/base/events.h#newcode19
> ui/base/events.h:19: #if defined(USE_WAYLAND)
> This isn't currently used anywhere.

Done.

[msw, 2011-10-04 22:49:13]

LGTM since you said we can ignore Wayland for now.
Weird win trybot error; I'm curious how you'll fix that.
Please do seek OWNERS approval for the new file in base.
You might want to give Sadrul a heads up too.

[oshima, 2011-10-04 23:13:01]

On 2011/10/04 22:49:13, msw wrote:
> LGTM since you said we can ignore Wayland for now.
> Weird win trybot error; I'm curious how you'll fix that.
> Please do seek OWNERS approval for the new file in base.
> You might want to give Sadrul a heads up too.
Looks like new function was added. Updated new patch. thanks.

[oshima, 2011-10-04 23:15:23]

Mark, ben, I need OWNERS approval.

mark: please review base
ben: please review ui/views

Thanks!

[Ben Goodger (Google), 2011-10-05 01:30:33]

LGTM for ui bits.

[Mark Mentovai, 2011-10-05 12:49:38]

LGTM otherwise. I only reviewed the changes in base.

http://codereview.chromium.org/8113028/diff/4005/base/event_types.h
File base/event_types.h (right):

http://codereview.chromium.org/8113028/diff/4005/base/event_types.h#newcode1
base/event_types.h:1: // Copyright (c) 2011 The Chromium Authors. All rights
reserved.
You should add this file to base.gypi.

[oshima, 2011-10-05 15:09:20]

On 2011/10/05 01:30:33, Ben Goodger (Google) wrote:
> LGTM for ui bits.

Ben, can you take a look at views/ as well?

[oshima, 2011-10-05 17:14:21]

http://codereview.chromium.org/8113028/diff/4005/base/event_types.h
File base/event_types.h (right):

http://codereview.chromium.org/8113028/diff/4005/base/event_types.h#newcode1
base/event_types.h:1: // Copyright (c) 2011 The Chromium Authors. All rights
reserved.
On 2011/10/05 12:49:38, Mark Mentovai wrote:
> You should add this file to base.gypi.

Done.