Consolidate/cleanup event cracking code; single out GdkEvents.

Consolidate/cleanup event cracking code; single out GdkEvents; saves ~850 lines.
Move ui::NativeEvent typdefs and common functions to ui/base/events.h.
Remove NativeEvent2 typedef, single out GdkEvent* uses that should be removed.
Implement platform specific ui/base/[platform]/events_[platform].cc.
Revise views::NativeEvent definitions (to support Aura abstraction).

Consolidate Event[Type/Flags/Location]FromNative(), GetMouseWheelOffset(), etc.
Remove GetRepeatCount(), GetWindowsFlags(), IsExtendedKey(), etc.
Add IsMouseEvent(), KeyboardCodeFromNative(), EF_EXTENDED flag, etc.
Localize GetFlagsFromGdkEvent(), move some file locals to new helpers files.
Move views/touchui/touch_factory.h|cc to ui/base/touch.
Stop mixing Windows mouse events' MK_*BUTTON into their wParams.

BUG=93945
TEST=No build breaks (many configs...), no mouse/key behavior changes.

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

[msw, 2011-09-19 23:27:25]

PTAL, thanks!

[Ben Goodger (Google), 2011-09-19 23:49:20]

http://codereview.chromium.org/7942004/diff/5001/ui/base/win/event_util.h
File ui/base/win/event_util.h (right):

http://codereview.chromium.org/7942004/diff/5001/ui/base/win/event_util.h#new...
ui/base/win/event_util.h:5: #ifndef UI_BASE_WIN_EVENT_UTIL_H_
This looks very similar to the Gtk/X stuff.

How about this...

move these decls to ui/base/events.h

Move the contents of the .cc to events_win.cc
We can then create events_gtk/events_x.cc as well.

[msw, 2011-09-19 23:56:27]

On 2011/09/19 23:49:20, Ben Goodger (Google) wrote:
> http://codereview.chromium.org/7942004/diff/5001/ui/base/win/event_util.h
> File ui/base/win/event_util.h (right):
> 
>
http://codereview.chromium.org/7942004/diff/5001/ui/base/win/event_util.h#new...
> ui/base/win/event_util.h:5: #ifndef UI_BASE_WIN_EVENT_UTIL_H_
> This looks very similar to the Gtk/X stuff.
> 
> How about this...
> 
> move these decls to ui/base/events.h
> 
> Move the contents of the .cc to events_win.cc
> We can then create events_gtk/events_x.cc as well.

I'm on it.

[msw, 2011-09-21 00:16:55]

I've started making your requested change; it isn't polished, but please make
sure I'm going about this the right way. It might be prudent to split this up
somehow.

I've moved event typedefs and functions to ui/base/events.h, added the
implementations to each ui/base/[platform]/events_[platform].cc, modified the
views NativeEvent typdefs, consolidated some code, and made supporting changes.
TODO: Polish and fix any build/test issues on all the platforms and configs.

[msw, 2011-09-21 18:04:50]

Ping, I'm fixing errors, but some early high-level feedback is appreciated. (new
file locations/names, general strategy).

Sadrul: PTAL at touch_factory.h|cc move.

[Ben Goodger (Google), 2011-09-21 18:08:04]

Looks good. Let me know when done.

-Ben

On Wed, Sep 21, 2011 at 11:04 AM, <msw@chromium.org> wrote:

> Ping, I'm fixing errors, but some early high-level feedback is appreciated.
> (new
> file locations/names, general strategy).
>
> Sadrul: PTAL at touch_factory.h|cc move.
>
>
http://codereview.chromium.**org/7942004/<http://codereview.chromium.org/7942...
>

[msw, 2011-09-22 12:22:40]

This is ready for review; PTAL, thanks!
I'll be building and testing some configs in parallel.

[Ben Goodger (Google), 2011-09-22 15:40:26]

This is an awesome cleanup! Thanks for doing it.

sadrul can comment on the x/touch/wayland related changes.

Structurally it looks correct to me, except for this one note about USE_AURA:

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

http://codereview.chromium.org/7942004/diff/22001/ui/base/events.h#newcode42
ui/base/events.h:42: #if defined(OS_WIN)
This is going to break in views when USE_AURA is defined.

In views, NativeEvent should be aura::Event when USE_AURA is defined. However in
aura, NativeEvent needs to map to the platform specific types.

So, it looks like we need an AURA_IMPLEMENTATION define that gets defined in
src/aura. When set, these platform defines happen, but in views when USE_AURA is
set aura::Event is used instead.

[msw, 2011-09-22 19:17:48]

event_aura.cc signatures updated (for const refs) and file permissions set.
Please take a look, thanks!

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

http://codereview.chromium.org/7942004/diff/22001/ui/base/events.h#newcode42
ui/base/events.h:42: #if defined(OS_WIN)
On 2011/09/22 15:40:26, Ben Goodger (Google) wrote:
> This is going to break in views when USE_AURA is defined.
> 
> In views, NativeEvent should be aura::Event when USE_AURA is defined. However
in
> aura, NativeEvent needs to map to the platform specific types.
> 
> So, it looks like we need an AURA_IMPLEMENTATION define that gets defined in
> src/aura. When set, these platform defines happen, but in views when USE_AURA
is
> set aura::Event is used instead.

I solved that conundrum by using two levels of typedefs:
1) ui::NativeEvent is truly native (ui/base/events.h)
2) views::NativeEvent is aura::Event* for USE_AURA otherwise it's
ui::NativeEvent (views/native_types.h).
I hope that's a good approach; otherwise, I'll need clarification on your
suggested solution.

[Ben Goodger (Google), 2011-09-22 19:31:33]

SG

LGTM otherwise.

On Thu, Sep 22, 2011 at 12:17 PM, <msw@chromium.org> wrote:

> event_aura.cc signatures updated (for const refs) and file permissions set.
> Please take a look, thanks!
>
>
>
>
http://codereview.chromium.**org/7942004/diff/22001/ui/**base/events.h<http:/...
> File ui/base/events.h (right):
>
> http://codereview.chromium.**org/7942004/diff/22001/ui/**
>
base/events.h#newcode42<http://codereview.chromium.org/7942004/diff/22001/ui/base/events.h#newcode42>
> ui/base/events.h:42: #if defined(OS_WIN)
> On 2011/09/22 15:40:26, Ben Goodger (Google) wrote:
>
>> This is going to break in views when USE_AURA is defined.
>>
>
>  In views, NativeEvent should be aura::Event when USE_AURA is defined.
>>
> However in
>
>> aura, NativeEvent needs to map to the platform specific types.
>>
>
>  So, it looks like we need an AURA_IMPLEMENTATION define that gets
>>
> defined in
>
>> src/aura. When set, these platform defines happen, but in views when
>>
> USE_AURA is
>
>> set aura::Event is used instead.
>>
>
> I solved that conundrum by using two levels of typedefs:
> 1) ui::NativeEvent is truly native (ui/base/events.h)
> 2) views::NativeEvent is aura::Event* for USE_AURA otherwise it's
> ui::NativeEvent (views/native_types.h).
> I hope that's a good approach; otherwise, I'll need clarification on
> your suggested solution.
>
>
http://codereview.chromium.**org/7942004/<http://codereview.chromium.org/7942...
>

[sadrul, 2011-09-22 20:15:33]

LGTM with the following nits

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

http://codereview.chromium.org/7942004/diff/27001/ui/base/events.h#newcode16
ui/base/events.h:16: #if defined(OS_LINUX)
#if defined(TOOLKIT_USES_GTK)

http://codereview.chromium.org/7942004/diff/27001/ui/base/events.h#newcode60
ui/base/events.h:60: #endif
This has to be a bit more complicated at the moment:

#if defined(OS_WIN)
...
#elif defined(USE_WAYLAND)
...
#elif defined(TOOLKIT_USES_GTK)
... the GdkEvent bits
#elif defined(USE_X11)
typedef XEvent* NativeEvent;
#else
typedef void* NativeEvent;
#endif


#if defined(TOOLKIT_USES_GTK)
typedef XEvent* NativeEvent2;
#else
typedef void* NativeEvent2;
#endif

[msw, 2011-09-22 21:41:42]

Ben: After talking with Sadrul, I'm going to try to further ostracize GdkEvents
(move what's needed to views), and possibly eliminate the NativeEvent2
abstraction (perhaps using  GdkEvent* as needed). So NativeEvent will be
Win/Wayland/Xlib/void*(for when none of those are defined), and then
views::Event will have separate signatures for either views::NativeEvent2
(GdkEvent*/void*) or un-abstracted GdkEvent* inside #if
defined(TOOLKIT_USES_GTK) blocks.

Please let me know if you have any advice or qualms; I'll start hacking away and
ping back here with the change asap.

[Ben Goodger (Google), 2011-09-23 20:30:07]

Nope please proceed!

On Thu, Sep 22, 2011 at 2:41 PM, <msw@chromium.org> wrote:

> Ben: After talking with Sadrul, I'm going to try to further ostracize
> GdkEvents
> (move what's needed to views), and possibly eliminate the NativeEvent2
> abstraction (perhaps using  GdkEvent* as needed). So NativeEvent will be
> Win/Wayland/Xlib/void*(for when none of those are defined), and then
> views::Event will have separate signatures for either views::NativeEvent2
> (GdkEvent*/void*) or un-abstracted GdkEvent* inside #if
> defined(TOOLKIT_USES_GTK) blocks.
>
> Please let me know if you have any advice or qualms; I'll start hacking
> away and
> ping back here with the change asap.
>
>
http://codereview.chromium.**org/7942004/<http://codereview.chromium.org/7942...
>

[msw, 2011-09-23 21:56:01]

Ben, Sadrul: PTAL, the updated approach is working.
I'm just checking the unexpected Mac webkit_unit_test failure.

[Ben Goodger (Google), 2011-09-24 16:39:04]

LGTM with one comment and one change requested:

http://codereview.chromium.org/7942004/diff/34049/chrome/browser/ui/views/com...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc
(right):

http://codereview.chromium.org/7942004/diff/34049/chrome/browser/ui/views/com...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:78: #elif
defined(OS_LINUX)
This should be TOOLKIT_USES_GTK, no?

http://codereview.chromium.org/7942004/diff/34049/ui/base/touchui/OWNERS
File ui/base/touchui/OWNERS (right):

http://codereview.chromium.org/7942004/diff/34049/ui/base/touchui/OWNERS#newc...
ui/base/touchui/OWNERS:1: sadrul@chromium.org
One nit: I'd rather the directory this goes into be called ui/base/touch. There
may be some benefit to us down the line disambiguating touch ui from other touch
infrastructure. Having a crisper dir name now makes it easier.

[sadrul, 2011-09-24 16:56:08]

LGTM

http://codereview.chromium.org/7942004/diff/34049/chrome/browser/ui/views/com...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc
(right):

http://codereview.chromium.org/7942004/diff/34049/chrome/browser/ui/views/com...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:79: bool
IsMouseEvent(GdkEvent* gdk_event);
Are IsMouseEvent/IsSameTopLevelWindow not used/necessary when USE_AURA?

[msw, 2011-09-26 05:07:59]

Done; landing.

http://codereview.chromium.org/7942004/diff/34049/chrome/browser/ui/views/com...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc
(right):

http://codereview.chromium.org/7942004/diff/34049/chrome/browser/ui/views/com...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:78: #elif
defined(OS_LINUX)
On 2011/09/24 16:39:04, Ben Goodger (Google) wrote:
> This should be TOOLKIT_USES_GTK, no?

Done.

http://codereview.chromium.org/7942004/diff/34049/chrome/browser/ui/views/com...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:79: bool
IsMouseEvent(GdkEvent* gdk_event);
On 2011/09/24 16:56:08, sadrul wrote:
> Are IsMouseEvent/IsSameTopLevelWindow not used/necessary when USE_AURA?

The only caller, MouseObserver::DidProcessEvent, is not built for USE_AURA. This
builds but compact nav is deprecated, I'll fix it up later if needed.

http://codereview.chromium.org/7942004/diff/34049/ui/base/touchui/OWNERS
File ui/base/touchui/OWNERS (right):

http://codereview.chromium.org/7942004/diff/34049/ui/base/touchui/OWNERS#newc...
ui/base/touchui/OWNERS:1: sadrul@chromium.org
On 2011/09/24 16:39:04, Ben Goodger (Google) wrote:
> One nit: I'd rather the directory this goes into be called ui/base/touch.
There
> may be some benefit to us down the line disambiguating touch ui from other
touch
> infrastructure. Having a crisper dir name now makes it easier.

Done.

[commit-bot: I haz the power, 2011-09-26 05:08:25]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/7942004/39007

[commit-bot: I haz the power, 2011-09-26 05:08:37]

Can't apply patch for file ui/base/touch/touch_factory.cc.
While running patch -p1 --forward --force;
patching file ui/base/touch/touch_factory.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 29.
Hunk #3 FAILED at 82.
Hunk #4 FAILED at 101.
Hunk #5 FAILED at 122.
Hunk #6 FAILED at 499.
6 out of 6 hunks FAILED -- saving rejects to file
ui/base/touch/touch_factory.cc.rej