Hijack mouse-related events for TOUCH_UI builds

Hijack mouse-related events for TOUCH_UI builds

With this change, which is specifically for TOUCH_UI builds, mouse-related
messages are hijacked from the normal Gdk event dispatcher and processed
manually in the file touchui/touch_event_dispatcher_gtk.cc.  GdkEvents are converted directly
to views events, and dispatched to the RootView.  This is preliminary work
that will be followed by more elaborate message pump changes, and ultimately
is in the spirit of removing Gtk entirely for TOUCH_UI (Chromium-Views-Gtk).

Patch from Chad Faragher <wyck@chromium.org>;

BUG=none
TEST=none

[wyck, 2010-10-12 20:45:54]

http://codereview.chromium.org/3704005/diff/2001/3005
File views/widget/widget_gtk.h (right):

http://codereview.chromium.org/3704005/diff/2001/3005#newcode212
views/widget/widget_gtk.h:212: static int GetFlagsForEventButton(const
GdkEventButton& event);
GetFlagsForEventButton is called externally now, so I made it public.

[sky, 2010-10-12 21:52:20]

http://codereview.chromium.org/3704005/diff/5001/6001
File views/focus/accelerator_handler_gtk.cc (right):

http://codereview.chromium.org/3704005/diff/5001/6001#newcode13
views/focus/accelerator_handler_gtk.cc:13: namespace touchui {
It's time. Leave this class as is, but move what you have here into a file named
_touch. The _touch file should be built and used if touchui is enabled. Also,
put what you have in touchui here and put it in an anonymous namespace (no
touchui namespace).

http://codereview.chromium.org/3704005/diff/5001/6001#newcode14
views/focus/accelerator_handler_gtk.cc:14: void
DispatchEventForTouchUI(GdkEvent* event);
no indenting here.

http://codereview.chromium.org/3704005/diff/5001/6002
File views/touchui/touchui.cc (right):

http://codereview.chromium.org/3704005/diff/5001/6002#newcode29
views/touchui/touchui.cc:29: gdk_window_get_user_data(gdk_window,
reinterpret_cast<void**>(&gtk_widget));
You want this:

    gpointer data = NULL;
    gdk_window_get_user_data(active_window, &data);
    GtkWidget* widget = reinterpret_cast<GtkWidget*>(data);

http://codereview.chromium.org/3704005/diff/5001/6002#newcode40
views/touchui/touchui.cc:40: RootView* root_view = widget_gtk->GetRootView();
No need to assign to a local variable, just return it.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode46
views/touchui/touchui.cc:46: RootView* root_view) {
align with ( on previous line.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode54
views/touchui/touchui.cc:54: WidgetGtk::GetFlagsForEventButton(event));
align with ( on previous line. If it goes past 80 chars, then indent by 4.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode68
views/touchui/touchui.cc:68: WidgetGtk::GetFlagsForEventButton(event));
align with ( on previous line or indent by 4.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode75
views/touchui/touchui.cc:75: RootView* root_view) {
align with ( on previous line

http://codereview.chromium.org/3704005/diff/5001/6002#newcode77
views/touchui/touchui.cc:77: gdk_event_request_motions(&event);
How come you need to invoke this?

http://codereview.chromium.org/3704005/diff/5001/6002#newcode81
views/touchui/touchui.cc:81: MouseEvent mouse_move(Event::ET_MOUSE_MOVED,
event.x, event.y, flags);
If the mouse is down don't you want a drag?

http://codereview.chromium.org/3704005/diff/5001/6002#newcode156
views/touchui/touchui.cc:156: case GDK_BUTTON_PRESS:
What about 2button_press and 3button_press?

http://codereview.chromium.org/3704005/diff/5001/6004
File views/widget/widget_gtk.h (right):

http://codereview.chromium.org/3704005/diff/5001/6004#newcode211
views/widget/widget_gtk.h:211: // Returns the view::Event::flags for a
GdkEventButton.
Update comments in .cc above GetFlagsForEventButton.

[wyck, 2010-10-14 14:24:28]

http://codereview.chromium.org/3704005/diff/5001/6001
File views/focus/accelerator_handler_gtk.cc (right):

http://codereview.chromium.org/3704005/diff/5001/6001#newcode13
views/focus/accelerator_handler_gtk.cc:13: namespace touchui {
On 2010/10/12 21:52:20, sky wrote:
> It's time. Leave this class as is, but move what you have here into a file
named
> _touch. The _touch file should be built and used if touchui is enabled. Also,
> put what you have in touchui here and put it in an anonymous namespace (no
> touchui namespace).

I don't understand what you mean.  I think I get what you mean about moving it
into another file.  The file will be named
views/focus/accelerator_handler_touch.cc  But I don't understand what you mean
by "put what you have in touchui here".  Are you referring to the entire
contents of the views/touchui/touchui.cc file file that I added?  If so then I
disagree.  I don't even want any of that code in the accelerator handler.  The
dispatch to gtk shouldn't even be done in the accelerator handler, in my
opinion.  Why would I put all that code here?  What I'm ideally trying to write
is a message pump.  Just because the code can be localized to this spot doesn't
mean it belongs here.  I really expected it to be in its own file.  It doesn't
have anything to do with accelerator handlers.  In fact, there are other places
where events are dispatched via gtk_main_do_event. (See my comment in
AcceleratorHandler::Dispatch about the message pump, menu controller and native
menu gtk).  Those calls may also need to be redirected to
DispatchEventForTouchUI, and if it were in an anonymous namespace in this file,
that wouldn't be possible.

http://codereview.chromium.org/3704005/diff/5001/6001#newcode14
views/focus/accelerator_handler_gtk.cc:14: void
DispatchEventForTouchUI(GdkEvent* event);
On 2010/10/12 21:52:20, sky wrote:
> no indenting here.

Done.

http://codereview.chromium.org/3704005/diff/5001/6002
File views/touchui/touchui.cc (right):

http://codereview.chromium.org/3704005/diff/5001/6002#newcode29
views/touchui/touchui.cc:29: gdk_window_get_user_data(gdk_window,
reinterpret_cast<void**>(&gtk_widget));
On 2010/10/12 21:52:20, sky wrote:
> You want this:
> 
>     gpointer data = NULL;
>     gdk_window_get_user_data(active_window, &data);
>     GtkWidget* widget = reinterpret_cast<GtkWidget*>(data);
> 

Done.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode40
views/touchui/touchui.cc:40: RootView* root_view = widget_gtk->GetRootView();
On 2010/10/12 21:52:20, sky wrote:
> No need to assign to a local variable, just return it.

Done.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode46
views/touchui/touchui.cc:46: RootView* root_view) {
On 2010/10/12 21:52:20, sky wrote:
> align with ( on previous line.

Done.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode54
views/touchui/touchui.cc:54: WidgetGtk::GetFlagsForEventButton(event));
On 2010/10/12 21:52:20, sky wrote:
> align with ( on previous line. If it goes past 80 chars, then indent by 4.

Done.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode68
views/touchui/touchui.cc:68: WidgetGtk::GetFlagsForEventButton(event));
On 2010/10/12 21:52:20, sky wrote:
> align with ( on previous line or indent by 4.

Done.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode75
views/touchui/touchui.cc:75: RootView* root_view) {
On 2010/10/12 21:52:20, sky wrote:
> align with ( on previous line

Done.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode77
views/touchui/touchui.cc:77: gdk_event_request_motions(&event);
On 2010/10/12 21:52:20, sky wrote:
> How come you need to invoke this?

GDK_MOTION_NOTIFY events are sent, but they can be configured to arrive in
different ways, depending on the GDK_POINTER_MOTION_HINT_MASK.  If it's used
then you will get an event with the is_hint flag set.  This means that the
system is attempting to thin out the number of messages that you will receive. 
A message with is_hint is the last one you will receive.  If you want more
events after a hint, then you need to call gdk_event_request_motions.

I have updated the comments to indicate this.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode81
views/touchui/touchui.cc:81: MouseEvent mouse_move(Event::ET_MOUSE_MOVED,
event.x, event.y, flags);
On 2010/10/12 21:52:20, sky wrote:
> If the mouse is down don't you want a drag?

Probably yes.  But I'm just not there yet.  I also am opposed to tracking state
in the message dispatcher.  I believe it should be the responsibility of the
message handlers to track state.  I could use an explanation as to why the views
want a separate "Dragged" event in addition to the "Moved" event.  To me, it's
all "moved" and the view should track the state of whether any buttons are up or
down.  The abstraction of "Dragged" as a message dispatching API entry point
doesn't make sense to me.  (I realize lots of code has already been written.)  

But until I figure it all out, I just haven't implemented it yet.

http://codereview.chromium.org/3704005/diff/5001/6002#newcode156
views/touchui/touchui.cc:156: case GDK_BUTTON_PRESS:
On 2010/10/12 21:52:20, sky wrote:
> What about 2button_press and 3button_press?

This is a complicated issue for me.  The existing code in widget_gtk has an
anonymous TODO in WidgetGtk::ProcessMousePressed about ignoring
GDK_2BUTTON_PRESS and GDK_3BUTTON_PRESS messages.  I figured we weren't missing
much.  It's also unclear to me what messages should be dispatched for a double
click.  Do you get a click AND a double click?  and what about triple click?  Do
you get a click AND a double click AND a triple click?  Or 3 clicks, two double
clicks and a triple click?  It's a little mysterious to process double-click in
this way.  I prefer that for my first cut, the single click is passed as normal
and we will deal with multiple clicks later.

Additionally, capture/grab is another issue that I haven't dealt with in this
patch.

http://codereview.chromium.org/3704005/diff/5001/6004
File views/widget/widget_gtk.h (right):

http://codereview.chromium.org/3704005/diff/5001/6004#newcode211
views/widget/widget_gtk.h:211: // Returns the view::Event::flags for a
GdkEventButton.
On 2010/10/12 21:52:20, sky wrote:
> Update comments in .cc above GetFlagsForEventButton.

Done.

[sky, 2010-10-14 16:22:56]

http://codereview.chromium.org/3704005/diff/5001/6001
File views/focus/accelerator_handler_gtk.cc (right):

http://codereview.chromium.org/3704005/diff/5001/6001#newcode13
views/focus/accelerator_handler_gtk.cc:13: namespace touchui {
On 2010/10/14 14:24:29, wyck wrote:
> On 2010/10/12 21:52:20, sky wrote:
> > It's time. Leave this class as is, but move what you have here into a file
> named
> > _touch. The _touch file should be built and used if touchui is enabled.
Also,
> > put what you have in touchui here and put it in an anonymous namespace (no
> > touchui namespace).
> 
> I don't understand what you mean.  I think I get what you mean about moving it
> into another file.  The file will be named
> views/focus/accelerator_handler_touch.cc  But I don't understand what you mean
> by "put what you have in touchui here".  Are you referring to the entire
> contents of the views/touchui/touchui.cc file file that I added?  If so then I
> disagree.  I don't even want any of that code in the accelerator handler.  The
> dispatch to gtk shouldn't even be done in the accelerator handler, in my
> opinion.  Why would I put all that code here?  What I'm ideally trying to
write
> is a message pump.  Just because the code can be localized to this spot
doesn't
> mean it belongs here.  I really expected it to be in its own file.  It doesn't
> have anything to do with accelerator handlers.  In fact, there are other
places
> where events are dispatched via gtk_main_do_event. (See my comment in
> AcceleratorHandler::Dispatch about the message pump, menu controller and
native
> menu gtk).  Those calls may also need to be redirected to
> DispatchEventForTouchUI, and if it were in an anonymous namespace in this
file,
> that wouldn't be possible.

I figured you only needed the code in touchui from the accelerator handler,
which is why I suggested moving it into the same file. But if you're going to
need touchui from other files than keep it in its own file. But create the
accelerator_handler_touch file.

[wyck, 2010-10-18 15:14:07]

How do things look now?  Is the namespace still a problem?

On 2010/10/14 16:22:56, sky wrote:
> http://codereview.chromium.org/3704005/diff/5001/6001
> File views/focus/accelerator_handler_gtk.cc (right):
> 
> http://codereview.chromium.org/3704005/diff/5001/6001#newcode13
> views/focus/accelerator_handler_gtk.cc:13: namespace touchui {
> On 2010/10/14 14:24:29, wyck wrote:
> > On 2010/10/12 21:52:20, sky wrote:
> > > It's time. Leave this class as is, but move what you have here into a file
> > named
> > > _touch. The _touch file should be built and used if touchui is enabled.
> Also,
> > > put what you have in touchui here and put it in an anonymous namespace (no
> > > touchui namespace).
> > 
> > I don't understand what you mean.  I think I get what you mean about moving
it
> > into another file.  The file will be named
> > views/focus/accelerator_handler_touch.cc  But I don't understand what you
mean
> > by "put what you have in touchui here".  Are you referring to the entire
> > contents of the views/touchui/touchui.cc file file that I added?  If so then
I
> > disagree.  I don't even want any of that code in the accelerator handler. 
The
> > dispatch to gtk shouldn't even be done in the accelerator handler, in my
> > opinion.  Why would I put all that code here?  What I'm ideally trying to
> write
> > is a message pump.  Just because the code can be localized to this spot
> doesn't
> > mean it belongs here.  I really expected it to be in its own file.  It
doesn't
> > have anything to do with accelerator handlers.  In fact, there are other
> places
> > where events are dispatched via gtk_main_do_event. (See my comment in
> > AcceleratorHandler::Dispatch about the message pump, menu controller and
> native
> > menu gtk).  Those calls may also need to be redirected to
> > DispatchEventForTouchUI, and if it were in an anonymous namespace in this
> file,
> > that wouldn't be possible.
> 
> I figured you only needed the code in touchui from the accelerator handler,
> which is why I suggested moving it into the same file. But if you're going to
> need touchui from other files than keep it in its own file. But create the
> accelerator_handler_touch file.

[sky, 2010-10-18 15:56:24]

Don't forget to send mail when you update a patch.

  -Scott

http://codereview.chromium.org/3704005/diff/15001/16001
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3704005/diff/15001/16001#newcode12
views/focus/accelerator_handler_touch.cc:12: namespace touchui {
We don't have a namespace for any other ports in views, so I'm hesitant to think
we need one for touch.

http://codereview.chromium.org/3704005/diff/15001/16002
File views/touchui/touchui.cc (right):

http://codereview.chromium.org/3704005/diff/15001/16002#newcode16
views/touchui/touchui.cc:16: using views::Event;
sort

http://codereview.chromium.org/3704005/diff/15001/16002#newcode62
views/touchui/touchui.cc:62: WidgetGtk::GetFlagsForEventButton(event));
indent 4

http://codereview.chromium.org/3704005/diff/15001/16002#newcode76
views/touchui/touchui.cc:76: WidgetGtk::GetFlagsForEventButton(event));
indent 4

http://codereview.chromium.org/3704005/diff/15001/16002#newcode137
views/touchui/touchui.cc:137: root_view);
align with ( on previous line for 137 and similar lines in this function.

http://codereview.chromium.org/3704005/diff/15001/16002#newcode164
views/touchui/touchui.cc:164: // Oh well!
NOTREACHED?

http://codereview.chromium.org/3704005/diff/15001/16004
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/3704005/diff/15001/16004#newcode915
views/widget/widget_gtk.cc:915: int WidgetGtk::GetFlagsForEventButton(const
GdkEventButton& event) {
Move this above 912 and leave 912 as is.

[wyck, 2010-10-18 17:42:41]

Things are in the 'views' namespace rather than the 'touchui' namespace now.

http://codereview.chromium.org/3704005/diff/15001/16001
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3704005/diff/15001/16001#newcode12
views/focus/accelerator_handler_touch.cc:12: namespace touchui {
On 2010/10/18 15:56:24, sky wrote:
> We don't have a namespace for any other ports in views, so I'm hesitant to
think
> we need one for touch.

Removed touchui namespce.  Things are now in the views namespace.

http://codereview.chromium.org/3704005/diff/15001/16002
File views/touchui/touchui.cc (right):

http://codereview.chromium.org/3704005/diff/15001/16002#newcode16
views/touchui/touchui.cc:16: using views::Event;
On 2010/10/18 15:56:24, sky wrote:
> sort

since things are in the views namespace now, these usings are removed entirely.

http://codereview.chromium.org/3704005/diff/15001/16002#newcode62
views/touchui/touchui.cc:62: WidgetGtk::GetFlagsForEventButton(event));
On 2010/10/18 15:56:24, sky wrote:
> indent 4

Done.

http://codereview.chromium.org/3704005/diff/15001/16002#newcode76
views/touchui/touchui.cc:76: WidgetGtk::GetFlagsForEventButton(event));
On 2010/10/18 15:56:24, sky wrote:
> indent 4

Done.

http://codereview.chromium.org/3704005/diff/15001/16002#newcode137
views/touchui/touchui.cc:137: root_view);
On 2010/10/18 15:56:24, sky wrote:
> align with ( on previous line for 137 and similar lines in this function.

Done.

http://codereview.chromium.org/3704005/diff/15001/16002#newcode164
views/touchui/touchui.cc:164: // Oh well!
On 2010/10/18 15:56:24, sky wrote:
> NOTREACHED?

Done.

http://codereview.chromium.org/3704005/diff/15001/16004
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/3704005/diff/15001/16004#newcode915
views/widget/widget_gtk.cc:915: int WidgetGtk::GetFlagsForEventButton(const
GdkEventButton& event) {
On 2010/10/18 15:56:24, sky wrote:
> Move this above 912 and leave 912 as is.

Done.

[rjkroege, 2010-10-18 18:03:37]

http://codereview.chromium.org/3704005/diff/24001/25002
File views/touchui/touchui.cc (right):

http://codereview.chromium.org/3704005/diff/24001/25002#newcode120
views/touchui/touchui.cc:120: static void DispatchEventToRootView(GdkEvent*
event, RootView* root_view) {
This method might need a sibling for dispatching XEvents. See my muttering to
Sadrul about this -- the logical place for XEvent dispatching is probably here
-- that way the messagepump code in base does not need to include any views
header files.

http://codereview.chromium.org/3704005/diff/24001/25002#newcode177
views/touchui/touchui.cc:177: static bool IsTouchEvent(GdkEvent* event) {
This code seems possibly unnecessary given the bit vector stuff that Sadrul has
written in the messagepump.

[wyck, 2010-10-18 18:58:17]

Responding to Rob's comments about Sadrul's work.

http://codereview.chromium.org/3704005/diff/24001/25002
File views/touchui/touchui.cc (right):

http://codereview.chromium.org/3704005/diff/24001/25002#newcode120
views/touchui/touchui.cc:120: static void DispatchEventToRootView(GdkEvent*
event, RootView* root_view) {
On 2010/10/18 18:03:37, rjkroege wrote:
> This method might need a sibling for dispatching XEvents. See my muttering to
> Sadrul about this -- the logical place for XEvent dispatching is probably here
> -- that way the messagepump code in base does not need to include any views
> header files.

Yes, it will need a sibling.  To be parallel with this one, there would be a
method that dispatches a generic X input message, and then individual methods to
convert specific X input events into views events.  I assume that's coming in
Sadrul's patch, or patch+1.

Any action required from me in this patch set?

http://codereview.chromium.org/3704005/diff/24001/25002#newcode177
views/touchui/touchui.cc:177: static bool IsTouchEvent(GdkEvent* event) {
On 2010/10/18 18:03:37, rjkroege wrote:
> This code seems possibly unnecessary given the bit vector stuff that Sadrul
has
> written in the messagepump.

OK.  Sadrul's patch can trim it all out.  I'm fine with that.  This is the only
function I have at the moment that discriminates between touch-related events
and non-touch-related events.  Any action required from me?

[wyck, 2010-10-19 13:25:05]

PTAL

[rjkroege, 2010-10-19 13:38:26]

On 2010/10/18 18:58:17, wyck wrote:
> Responding to Rob's comments about Sadrul's work.
> 
> http://codereview.chromium.org/3704005/diff/24001/25002
> File views/touchui/touchui.cc (right):
> 
> http://codereview.chromium.org/3704005/diff/24001/25002#newcode120
> views/touchui/touchui.cc:120: static void DispatchEventToRootView(GdkEvent*
> event, RootView* root_view) {
> On 2010/10/18 18:03:37, rjkroege wrote:
> > This method might need a sibling for dispatching XEvents. See my muttering
to
> > Sadrul about this -- the logical place for XEvent dispatching is probably
here
> > -- that way the messagepump code in base does not need to include any views
> > header files.
> 
> Yes, it will need a sibling.  To be parallel with this one, there would be a
> method that dispatches a generic X input message, and then individual methods
to
> convert specific X input events into views events.  I assume that's coming in
> Sadrul's patch, or patch+1.
> 
> Any action required from me in this patch set?

Add a TODO.

> 
> http://codereview.chromium.org/3704005/diff/24001/25002#newcode177
> views/touchui/touchui.cc:177: static bool IsTouchEvent(GdkEvent* event) {
> On 2010/10/18 18:03:37, rjkroege wrote:
> > This code seems possibly unnecessary given the bit vector stuff that Sadrul
> has
> > written in the messagepump.
> 
> OK.  Sadrul's patch can trim it all out.  I'm fine with that.  This is the
only
> function I have at the moment that discriminates between touch-related events
> and non-touch-related events.  Any action required from me?

Add a TODO.

[wyck, 2010-10-19 14:14:26]

Since all these functions are specific to GdkEvents, and we are planning on also
writing a version that deals with X events, is it appropriate to name the
functions according to the types they deal with?  Something like:

  DispatchGdkEventForTouchUI, and DispatchXEventForTouchUI.

or maybe just suffixes, like: 

  DispatchEventForTouchUIGdk, and DispatchEventForTouchUIX.

Or perhaps just overload the function DispatchEventforTouchUI and have an
overload that accepts a GdkEvent* and an overload that accepts [whatever the X
event type is]?

Or there's even yet another pattern I've seen, where we could define a
NativeMessage type and replace the entire file with implementations for each
native type selection.  In that case, we would have a touchui_gdk.cc and a
touchui_x11.cc  or something like that.  And the calling code would call a
unified entry point based on a "NativeMessage" parameter that is defined as
something differently between _gdk and _x11.

So what's best?

[sky, 2010-10-19 15:36:48]

http://codereview.chromium.org/3704005/diff/24001/25001
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3704005/diff/24001/25001#newcode14
views/focus/accelerator_handler_touch.cc:14: void
DispatchEventForTouchUI(GdkEvent* event);
You said you were going to need this function else where. If that's the case
shouldn't you put the declaration it in it's own header?

http://codereview.chromium.org/3704005/diff/24001/25001#newcode24
views/focus/accelerator_handler_touch.cc:24: // as well as native_menu_gtk.  :(
Nuke the :(
Add a TODO for fixing other places

http://codereview.chromium.org/3704005/diff/24001/25002
File views/touchui/touchui.cc (right):

http://codereview.chromium.org/3704005/diff/24001/25002#newcode1
views/touchui/touchui.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
Name this file more descriptively. Something like touch_event_dispatcher

[wyck, 2010-10-19 16:44:59]

Clarification needed, please.

http://codereview.chromium.org/3704005/diff/24001/25001
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3704005/diff/24001/25001#newcode24
views/focus/accelerator_handler_touch.cc:24: // as well as native_menu_gtk.  :(
On 2010/10/19 15:36:49, sky wrote:
> Nuke the :(
> Add a TODO for fixing other places
Wait, could you clarify.  Do you want me to add the TODO HERE, or in the other
places where gtk_main_do_event is called?

[sky, 2010-10-19 16:59:33]

On Tue, Oct 19, 2010 at 9:45 AM,  <wyck@chromium.org> wrote:
> Clarification needed, please.
>
>
> http://codereview.chromium.org/3704005/diff/24001/25001
> File views/focus/accelerator_handler_touch.cc (right):
>
> http://codereview.chromium.org/3704005/diff/24001/25001#newcode24
> views/focus/accelerator_handler_touch.cc:24: // as well as
> native_menu_gtk.  :(
> On 2010/10/19 15:36:49, sky wrote:
>>
>> Nuke the :(
>> Add a TODO for fixing other places
>
> Wait, could you clarify.  Do you want me to add the TODO HERE, or in the
> other places where gtk_main_do_event is called?

TODO right here is fine.

  -Scott

[wyck, 2010-10-19 19:50:29]

I also made a few extra changes.  I fixed some out-of-order files in views.gyp
(from Rob's CL). They are neighbors of my change so I just cleaned up a bit.

I called everything touch_event_dispatcher_gtk, in anticipation of a
touch_event_dispatcher_x11.  I also made liberal comments about this.

I changed the calling API slightly so that the caller dispatches their own
message if the touch event dispatcher returns false.  This will make it easier
to track all the places where gtk_main_do_event are originally intended to be
called.  It will also allow the authoritative dispatching code to be written in
only one place (the original caller) -- which is relevant in the case of a new
message pump (such as Sadrul's X11 message pump in an upcoming CL.)

There is a proper header file now, although the interface is still just the
single function, and the implementation is just comprised of static functions. 
(As opposed to singleton/static classes).

http://codereview.chromium.org/3704005/diff/24001/25001
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3704005/diff/24001/25001#newcode14
views/focus/accelerator_handler_touch.cc:14: void
DispatchEventForTouchUI(GdkEvent* event);
On 2010/10/19 15:36:49, sky wrote:
> You said you were going to need this function else where. If that's the case
> shouldn't you put the declaration it in it's own header?

Done.

http://codereview.chromium.org/3704005/diff/24001/25001#newcode24
views/focus/accelerator_handler_touch.cc:24: // as well as native_menu_gtk.  :(
On 2010/10/19 15:36:49, sky wrote:
> Nuke the :(
> Add a TODO for fixing other places

Done.

http://codereview.chromium.org/3704005/diff/24001/25002
File views/touchui/touchui.cc (right):

http://codereview.chromium.org/3704005/diff/24001/25002#newcode1
views/touchui/touchui.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
On 2010/10/19 15:36:49, sky wrote:
> Name this file more descriptively. Something like touch_event_dispatcher 

touch_event_dispatcher_gtk.cc

Done

http://codereview.chromium.org/3704005/diff/24001/25002#newcode120
views/touchui/touchui.cc:120: static void DispatchEventToRootView(GdkEvent*
event, RootView* root_view) {
On 2010/10/18 18:03:37, rjkroege wrote:
> This method might need a sibling for dispatching XEvents. See my muttering to
> Sadrul about this -- the logical place for XEvent dispatching is probably here
> -- that way the messagepump code in base does not need to include any views
> header files.

I have improved much of the documentation, and refactored both the files and
functions to indicate that this is the Gtk implementation of the touch event
dispatcher.  I have added TODO's for the X Windows / X11 versions where
appropriate.

http://codereview.chromium.org/3704005/diff/24001/25002#newcode177
views/touchui/touchui.cc:177: static bool IsTouchEvent(GdkEvent* event) {
On 2010/10/18 18:03:37, rjkroege wrote:
> This code seems possibly unnecessary given the bit vector stuff that Sadrul
has
> written in the messagepump.

I now have a comment to that effect.  It makes specific reference to the X
Windows message pump and how any extra filtering it may offer will make this
filter unnecessary.

http://codereview.chromium.org/3704005/diff/24001/25003
File views/views.gyp (right):

http://codereview.chromium.org/3704005/diff/24001/25003#newcode270
views/views.gyp:270: 'touchui/gesture_manager.h',
I noticed these two gesture_manager includes were out of order so I'm going to
swap them too in this CL.

[sky, 2010-10-19 20:05:50]

LGTM

[rjkroege, 2010-10-20 15:08:02]

LGTM