touchui: Directly process key and mouse events.

touchui: Directly process key and mouse events.

Capture the keyboard and mouse events directly from X, create a corresponding
views::Event out of it, and send it to the associated RootView.

Includes Chad's (wyck) function FindRootViewForGdkEvent (from #3704005) slightly
modified (called FindRootViewForGdkWindow).

BUG=None
TEST=Click/Keypress events in a webpage should work correctly.

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

[sadrul, 2010-10-18 15:17:30]

This goes on top of http://codereview.chromium.org/3748002/show

[sky, 2010-10-18 17:14:45]

http://codereview.chromium.org/3801011/diff/3001/4001
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/3801011/diff/3001/4001#newcode8
base/message_pump_glib_x.cc:8: #include "views/event.h"
base can't have a dependency on views.

[rjkroege, 2010-10-18 17:36:49]

http://codereview.chromium.org/3801011/diff/3001/4001
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/3801011/diff/3001/4001#newcode21
base/message_pump_glib_x.cc:21: // TODO(sad): Have |keysym| go through the X map
list?
this style of lookup is replicated in a whole bunch of places. In particular,
Bryan had to write something for building synthetic events.  It is worth looking
for this code and not replicating it.

And: would the map list make this shorter/simpler/nicer to read?

http://codereview.chromium.org/3801011/diff/3001/4001#newcode157
base/message_pump_glib_x.cc:157: // TODO(sad): holy-crap-a-lot are still missing
Strictly speaking the word inside the TODO() is your username. But I appreciate
the subtle humour of TODO(sad). And In general, comments should be sentences
with periods, etc.

http://codereview.chromium.org/3801011/diff/3001/4001#newcode174
base/message_pump_glib_x.cc:174: // gets the RootView associated with the
GdkWindow.
Capitalize gets -> Gets.

http://codereview.chromium.org/3801011/diff/3001/4001#newcode175
base/message_pump_glib_x.cc:175: RootView* FindRootViewForGdkWindow(GdkWindow*
gdk_window) {
I had thought that this functionality came from wyck's
http://codereview.chromium.org/3704005

yes?

http://codereview.chromium.org/3801011/diff/3001/4001#newcode245
base/message_pump_glib_x.cc:245: if (RootView *root =
FindRootViewForGdkWindow(gwind)) {
RootView*

http://codereview.chromium.org/3801011/diff/3001/4001#newcode252
base/message_pump_glib_x.cc:252: KeyEvent keyev(xev.type == KeyPress ?
Event::ET_KEY_PRESSED
The KeyEvent constructor should have this logic?

http://codereview.chromium.org/3801011/diff/3001/4001#newcode262
base/message_pump_glib_x.cc:262: MouseEvent mouseev(xev.type == ButtonPress ?
Event::ET_MOUSE_PRESSED
augment the views event constructor yes?

http://codereview.chromium.org/3801011/diff/3001/4001#newcode300
base/message_pump_glib_x.cc:300: // TODO(sad): A couple of extra events can
still sneak in during this
Add a . to the sentence. And what you need to do about it.

[rjkroege, 2010-10-18 17:52:51]

On 2010/10/18 17:14:45, sky wrote:
> http://codereview.chromium.org/3801011/diff/3001/4001
> File base/message_pump_glib_x.cc (right):
> 
> http://codereview.chromium.org/3801011/diff/3001/4001#newcode8
> base/message_pump_glib_x.cc:8: #include "views/event.h"
> base can't have a dependency on views.

sky: In our foolhardy-new-world of no Gtk/Gdk for event distribution, the
message pump needs to call eventually into views code to deliver events. Is it
reasonable to break this possible dependency by forward-declaring a function
here that is actually defined in views/* and invoking this to dispatch events?

[sky, 2010-10-18 19:12:09]

On Mon, Oct 18, 2010 at 10:52 AM,  <rjkroege@chromium.org> wrote:
> On 2010/10/18 17:14:45, sky wrote:
>>
>> http://codereview.chromium.org/3801011/diff/3001/4001
>> File base/message_pump_glib_x.cc (right):
>
>> http://codereview.chromium.org/3801011/diff/3001/4001#newcode8
>> base/message_pump_glib_x.cc:8: #include "views/event.h"
>> base can't have a dependency on views.
>
> sky: In our foolhardy-new-world of no Gtk/Gdk for event distribution, the
> message pump needs to call eventually into views code to deliver events. Is
> it
> reasonable to break this possible dependency by forward-declaring a function
> here that is actually defined in views/* and invoking this to dispatch
> events?

You need to work with whoever owns this code for the definitive
solution. I would be inclined to define a pure virtual class that
views implements some where.

  -Scott

[sadrul, 2010-10-19 15:33:46]

I have addressed most of Rob's suggestions. I have also removed views-dependency
from base to address Sky's suggestion, by moving the XEvent dispatching into
MessageLoopForUI::Dispatcher (which is now MessagePumpGlibXDispatcher, a
subclass of current MessagePumpForUI::Dispatcher + support for XEvent handling).

The change in accelerator_handler_gtk.cc will be moved to
acceletator_handler_touch.cc from http://codereview.chromium.org/3704005/show
when these changes get merged.

http://codereview.chromium.org/3801011/diff/3001/4001
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/3801011/diff/3001/4001#newcode21
base/message_pump_glib_x.cc:21: // TODO(sad): Have |keysym| go through the X map
list?
On 2010/10/18 17:36:49, rjkroege wrote:
> this style of lookup is replicated in a whole bunch of places. In particular,
> Bryan had to write something for building synthetic events.  It is worth
looking
> for this code and not replicating it.
> 
> And: would the map list make this shorter/simpler/nicer to read?

I believe bryeung's code (in app/keyboard_code_conversion*) was for GDK event
translation.

This function perhaps should be moved to app/keyboard_code_conversion_x.cc|h.

http://codereview.chromium.org/3801011/diff/3001/4001#newcode157
base/message_pump_glib_x.cc:157: // TODO(sad): holy-crap-a-lot are still missing
On 2010/10/18 17:36:49, rjkroege wrote:
> Strictly speaking the word inside the TODO() is your username. But I
appreciate
> the subtle humour of TODO(sad). And In general, comments should be sentences
> with periods, etc.
> 

Done.

http://codereview.chromium.org/3801011/diff/3001/4001#newcode174
base/message_pump_glib_x.cc:174: // gets the RootView associated with the
GdkWindow.
On 2010/10/18 17:36:49, rjkroege wrote:
> Capitalize gets -> Gets.

Done.

http://codereview.chromium.org/3801011/diff/3001/4001#newcode175
base/message_pump_glib_x.cc:175: RootView* FindRootViewForGdkWindow(GdkWindow*
gdk_window) {
On 2010/10/18 17:36:49, rjkroege wrote:
> I had thought that this functionality came from wyck's
> http://codereview.chromium.org/3704005
> 
> yes?

That is correct (as mentioned in the issue Description). I have added a TODO in
the comments to make sure this gets merged with his function.

http://codereview.chromium.org/3801011/diff/3001/4001#newcode245
base/message_pump_glib_x.cc:245: if (RootView *root =
FindRootViewForGdkWindow(gwind)) {
On 2010/10/18 17:36:49, rjkroege wrote:
> RootView* 

Done.

http://codereview.chromium.org/3801011/diff/3001/4001#newcode300
base/message_pump_glib_x.cc:300: // TODO(sad): A couple of extra events can
still sneak in during this
On 2010/10/18 17:36:49, rjkroege wrote:
> Add a . to the sentence. And what you need to do about it.

Done.

[sky, 2010-10-19 15:51:10]

You need to get someone intimately familiar with the messageloop to review this
patch. Evan Martin should be able to point you in the right direction.

http://codereview.chromium.org/3801011/diff/14001/15002
File base/message_loop.h (right):

http://codereview.chromium.org/3801011/diff/14001/15002#newcode28
base/message_loop.h:28: #if defined(TOUCH_UI)
move this fine to around line 32ish so that it's easier to read this code.

http://codereview.chromium.org/3801011/diff/14001/15004
File base/message_pump_glib.h (right):

http://codereview.chromium.org/3801011/diff/14001/15004#newcode96
base/message_pump_glib.h:96: RunState* state_;
style guide says no protected fields.

http://codereview.chromium.org/3801011/diff/14001/15004#newcode99
base/message_pump_glib.h:99: Dispatcher* GetDispatcher();
methods before fields.

http://codereview.chromium.org/3801011/diff/14001/15004#newcode102
base/message_pump_glib.h:102: 
nit:no newline here.

http://codereview.chromium.org/3801011/diff/14001/15006
File base/message_pump_glib_x_dispatch.h (right):

http://codereview.chromium.org/3801011/diff/14001/15006#newcode16
base/message_pump_glib_x_dispatch.h:16: public:
spacing is off.

http://codereview.chromium.org/3801011/diff/14001/15008
File views/controls/menu/menu_controller.h (right):

http://codereview.chromium.org/3801011/diff/14001/15008#newcode193
views/controls/menu/menu_controller.h:193: #if defined(TOUCH_UI)
Don't make nested if here.

http://codereview.chromium.org/3801011/diff/14001/15011
File views/event.h (right):

http://codereview.chromium.org/3801011/diff/14001/15011#newcode14
views/event.h:14: #include "views/event_x.h"
Why include this here? Folks that need DispatchXEvent should include this
themselves.

http://codereview.chromium.org/3801011/diff/14001/15012
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/14001/15012#newcode13
views/event_x.cc:13: namespace app {
Why is this in namespace app?

http://codereview.chromium.org/3801011/diff/14001/15013
File views/event_x.h (right):

http://codereview.chromium.org/3801011/diff/14001/15013#newcode13
views/event_x.h:13: bool DispatchXEvent(XEvent* xevent);
Description?

http://codereview.chromium.org/3801011/diff/14001/15015
File views/focus/accelerator_handler_gtk.cc (right):

http://codereview.chromium.org/3801011/diff/14001/15015#newcode28
views/focus/accelerator_handler_gtk.cc:28: bool
AcceleratorHandler::Dispatch(XEvent* xev) {
This should only be in the touch handler, not here.

[sadrul, 2010-10-19 16:49:20]

http://codereview.chromium.org/3801011/diff/14001/15002
File base/message_loop.h (right):

http://codereview.chromium.org/3801011/diff/14001/15002#newcode28
base/message_loop.h:28: #if defined(TOUCH_UI)
On 2010/10/19 15:51:10, sky wrote:
> move this fine to around line 32ish so that it's easier to read this code.

Done.

http://codereview.chromium.org/3801011/diff/14001/15004
File base/message_pump_glib.h (right):

http://codereview.chromium.org/3801011/diff/14001/15004#newcode96
base/message_pump_glib.h:96: RunState* state_;
On 2010/10/19 15:51:10, sky wrote:
> style guide says no protected fields.

Done.

http://codereview.chromium.org/3801011/diff/14001/15004#newcode99
base/message_pump_glib.h:99: Dispatcher* GetDispatcher();
On 2010/10/19 15:51:10, sky wrote:
> methods before fields.

Done.

http://codereview.chromium.org/3801011/diff/14001/15004#newcode102
base/message_pump_glib.h:102: 
On 2010/10/19 15:51:10, sky wrote:
> nit:no newline here.

Done.

http://codereview.chromium.org/3801011/diff/14001/15006
File base/message_pump_glib_x_dispatch.h (right):

http://codereview.chromium.org/3801011/diff/14001/15006#newcode16
base/message_pump_glib_x_dispatch.h:16: public:
On 2010/10/19 15:51:10, sky wrote:
> spacing is off.

Done.

http://codereview.chromium.org/3801011/diff/14001/15008
File views/controls/menu/menu_controller.h (right):

http://codereview.chromium.org/3801011/diff/14001/15008#newcode193
views/controls/menu/menu_controller.h:193: #if defined(TOUCH_UI)
On 2010/10/19 15:51:10, sky wrote:
> Don't make nested if here.

Done.

http://codereview.chromium.org/3801011/diff/14001/15011
File views/event.h (right):

http://codereview.chromium.org/3801011/diff/14001/15011#newcode14
views/event.h:14: #include "views/event_x.h"
On 2010/10/19 15:51:10, sky wrote:
> Why include this here? Folks that need DispatchXEvent should include this
> themselves.

Done.

http://codereview.chromium.org/3801011/diff/14001/15012
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/14001/15012#newcode13
views/event_x.cc:13: namespace app {
On 2010/10/19 15:51:10, sky wrote:
> Why is this in namespace app?

I was actually moving this piece of code into app/, where GTK+'s code-conversion
is in. I have done so in the latest patch.

http://codereview.chromium.org/3801011/diff/14001/15013
File views/event_x.h (right):

http://codereview.chromium.org/3801011/diff/14001/15013#newcode13
views/event_x.h:13: bool DispatchXEvent(XEvent* xevent);
On 2010/10/19 15:51:10, sky wrote:
> Description?

Done.

http://codereview.chromium.org/3801011/diff/14001/15015
File views/focus/accelerator_handler_gtk.cc (right):

http://codereview.chromium.org/3801011/diff/14001/15015#newcode28
views/focus/accelerator_handler_gtk.cc:28: bool
AcceleratorHandler::Dispatch(XEvent* xev) {
On 2010/10/19 15:51:10, sky wrote:
> This should only be in the touch handler, not here.

Correct. This will be moved into accelerator_handler_touch.cc when that patch
from wyck lands. I have added a TODO to that effect.

[rjkroege, 2010-10-19 18:42:44]

http://codereview.chromium.org/3801011/diff/26001/23003
File app/keyboard_code_conversion_x.cc (right):

http://codereview.chromium.org/3801011/diff/26001/23003#newcode154
app/keyboard_code_conversion_x.cc:154: // TODO(sad): holy-crap-a-lot are still
missing.
Change this -- be more formal.

http://codereview.chromium.org/3801011/diff/26001/23004
File app/keyboard_code_conversion_x.h (right):

http://codereview.chromium.org/3801011/diff/26001/23004#newcode14
app/keyboard_code_conversion_x.h:14: app::KeyboardCode
KeyboardCodeFromXKeyEvent(XEvent* xev);
I am not a c++ expert but the app:: is unnecessary here?

http://codereview.chromium.org/3801011/diff/26001/23006
File base/message_loop.h (right):

http://codereview.chromium.org/3801011/diff/26001/23006#newcode29
base/message_loop.h:29: 
remove this \n?

http://codereview.chromium.org/3801011/diff/26001/23009
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/3801011/diff/26001/23009#newcode47
base/message_pump_glib_x.cc:47: processed =
static_cast<MessagePumpGlibXDispatcher*>
this is about jumping into views right?  I wouldn't have done it this way -- it
seems like extra work. What about just declaring: bool EventDispatcherX(XEvent&)
and calling it?

http://codereview.chromium.org/3801011/diff/26001/23009#newcode85
base/message_pump_glib_x.cc:85:
capture_gdk_events_[static_cast<int>(GDK_KEY_PRESS)] = true;
the static_cast is to fix the build in the other CL?

http://codereview.chromium.org/3801011/diff/26001/23011
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/3801011/diff/26001/23011#newcode882
views/controls/menu/menu_controller.cc:882: // We don not want Gtk to handle
keyboard events, otherwise if they get
don -> don't -- while editing this file anyway.

http://codereview.chromium.org/3801011/diff/26001/23015
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/26001/23015#newcode13
views/event_x.cc:13: namespace {
Is there a reason why FindRootView* isn't in the views namespace?

http://codereview.chromium.org/3801011/diff/26001/23015#newcode41
views/event_x.cc:41: int GetEventFlagsFromXState(unsigned int state) {
You will want to remove the code from wyck's CL at some point.

http://codereview.chromium.org/3801011/diff/26001/23015#newcode63
views/event_x.cc:63: bool DispatchXEvent(XEvent* xev) {
I would assume that when there is an X11 widget, it would subsume this function
as is currently handled with widget_gtk?

http://codereview.chromium.org/3801011/diff/26001/23015#newcode72
views/event_x.cc:72: app::KeyboardCode keycode =
app::KeyboardCodeFromXKeyEvent(xev);
Could we move this code into views:: ?

http://codereview.chromium.org/3801011/diff/26001/23015#newcode84
views/event_x.cc:84: // TODO(sad): detect double/triple clicks?
This is not how I would have done this. I would think that there should be
constructors in event_x or event_mpx (pick a reasonable name). The
DispatchXEvent can be a static that takes X events. Pre-declare the
DispatchXEvent in the message_pump_*_x and call that?

[sadrul, 2010-10-19 19:39:14]

http://codereview.chromium.org/3801011/diff/26001/23003
File app/keyboard_code_conversion_x.cc (right):

http://codereview.chromium.org/3801011/diff/26001/23003#newcode154
app/keyboard_code_conversion_x.cc:154: // TODO(sad): holy-crap-a-lot are still
missing.
On 2010/10/19 18:42:45, rjkroege wrote:
> Change this -- be more formal.

Done.

http://codereview.chromium.org/3801011/diff/26001/23004
File app/keyboard_code_conversion_x.h (right):

http://codereview.chromium.org/3801011/diff/26001/23004#newcode14
app/keyboard_code_conversion_x.h:14: app::KeyboardCode
KeyboardCodeFromXKeyEvent(XEvent* xev);
On 2010/10/19 18:42:45, rjkroege wrote:
> I am not a c++ expert but the app:: is unnecessary here?

It isn't. Fixed!

http://codereview.chromium.org/3801011/diff/26001/23006
File base/message_loop.h (right):

http://codereview.chromium.org/3801011/diff/26001/23006#newcode29
base/message_loop.h:29: 
On 2010/10/19 18:42:45, rjkroege wrote:
> remove this \n?

Done.

http://codereview.chromium.org/3801011/diff/26001/23009
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/3801011/diff/26001/23009#newcode47
base/message_pump_glib_x.cc:47: processed =
static_cast<MessagePumpGlibXDispatcher*>
On 2010/10/19 18:42:45, rjkroege wrote:
> this is about jumping into views right?  I wouldn't have done it this way --
it
> seems like extra work. What about just declaring: bool
EventDispatcherX(XEvent&)
> and calling it?

If I understand correctly, the EventDispatcherX will be in views::? In that
case, we would still have views dependency in base (which I understand may
eventually be something that cannot be avoided)?

Even with MessagePumpGlibXDispatcher, this essentially ends up calling the
static DispatchXEvent function (which sounds like the EventDispatcherX you
describe).

Using MessagePumpGlibXDispatcher could have some use, eventually, I think. It
would make it possible to customize the XEvent conversion into views::Event for
different dispatchers (for a contrived ill-thought out example, the
MenuController could generate a mouse-event from an XEvent that the default
handler would otherwise have translated into a touch-event).

http://codereview.chromium.org/3801011/diff/26001/23009#newcode85
base/message_pump_glib_x.cc:85:
capture_gdk_events_[static_cast<int>(GDK_KEY_PRESS)] = true;
On 2010/10/19 18:42:45, rjkroege wrote:
> the static_cast is to fix the build in the other CL?

Correct. (the other CL lacks a number of the events captured here)

http://codereview.chromium.org/3801011/diff/26001/23011
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/3801011/diff/26001/23011#newcode882
views/controls/menu/menu_controller.cc:882: // We don not want Gtk to handle
keyboard events, otherwise if they get
On 2010/10/19 18:42:45, rjkroege wrote:
> don -> don't -- while editing this file anyway.

Done.

http://codereview.chromium.org/3801011/diff/26001/23015
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/26001/23015#newcode13
views/event_x.cc:13: namespace {
On 2010/10/19 18:42:45, rjkroege wrote:
> Is there a reason why FindRootView* isn't in the views namespace?

I was actually thinking of moving this into xll_util namespace? Or are views/
dependency not allowed in app/?

http://codereview.chromium.org/3801011/diff/26001/23015#newcode72
views/event_x.cc:72: app::KeyboardCode keycode =
app::KeyboardCodeFromXKeyEvent(xev);
On 2010/10/19 18:42:45, rjkroege wrote:
> Could we move this code into views:: ?

You mean app::KeyboardCodeFromXKeyEvent into views:KeyboardCodeFromXKeyEvent? I
moved it into app since the GDK version for code-conversion is there
(app::WindowsKeyCodeForGdkKeyCode).

http://codereview.chromium.org/3801011/diff/26001/23015#newcode84
views/event_x.cc:84: // TODO(sad): detect double/triple clicks?
On 2010/10/19 18:42:45, rjkroege wrote:
> This is not how I would have done this. I would think that there should be
> constructors in event_x or event_mpx (pick a reasonable name). The
> DispatchXEvent can be a static that takes X events. Pre-declare the
> DispatchXEvent in the message_pump_*_x and call that?

There could be KeyEvent and MouseEvent constructors that take XEvent's, yes.
Sounds like a good idea to me.

[sadrul, 2010-10-21 19:12:53]

Since #3704005 has landed, I have updated my patch accordingly my moving the
changes from accelerator_handler_gtk.cc into accelerator_handler_touch.cc. A
subsequent patch will be submitted to clean up the code from #3704005 that will
no longer be necessary.

http://codereview.chromium.org/3801011/diff/26001/23009
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/3801011/diff/26001/23009#newcode85
base/message_pump_glib_x.cc:85:
capture_gdk_events_[static_cast<int>(GDK_KEY_PRESS)] = true;
On 2010/10/19 19:39:15, sadrul wrote:
> On 2010/10/19 18:42:45, rjkroege wrote:
> > the static_cast is to fix the build in the other CL?
> 
> Correct. (the other CL lacks a number of the events captured here)

Turns out this isn't really necessary. So reverted the change.

http://codereview.chromium.org/3801011/diff/26001/23015
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/26001/23015#newcode41
views/event_x.cc:41: int GetEventFlagsFromXState(unsigned int state) {
On 2010/10/19 18:42:45, rjkroege wrote:
> You will want to remove the code from wyck's CL at some point.

I plan on submitting a separate CL to clean up wyck's code that are no longer
necessary, and merge the rest of the code.

http://codereview.chromium.org/3801011/diff/26001/23015#newcode63
views/event_x.cc:63: bool DispatchXEvent(XEvent* xev) {
On 2010/10/19 18:42:45, rjkroege wrote:
> I would assume that when there is an X11 widget, it would subsume this
function
> as is currently handled with widget_gtk?

Since we already generate views::Event's here and send it down to the rootview,
I think it wouldn't be necessary to do any additional conversion for widget_x?

http://codereview.chromium.org/3801011/diff/26001/23015#newcode84
views/event_x.cc:84: // TODO(sad): detect double/triple clicks?
On 2010/10/19 19:39:15, sadrul wrote:
> On 2010/10/19 18:42:45, rjkroege wrote:
> > This is not how I would have done this. I would think that there should be
> > constructors in event_x or event_mpx (pick a reasonable name). The
> > DispatchXEvent can be a static that takes X events. Pre-declare the
> > DispatchXEvent in the message_pump_*_x and call that?
> 
> There could be KeyEvent and MouseEvent constructors that take XEvent's, yes.
> Sounds like a good idea to me.

Done.

[sky, 2010-10-21 19:41:42]

http://codereview.chromium.org/3801011/diff/37001/38014
File views/event.h (right):

http://codereview.chromium.org/3801011/diff/37001/38014#newcode231
views/event.h:231: explicit MouseEvent(XEvent* xevent, bool drag);
no explicit

http://codereview.chromium.org/3801011/diff/37001/38016
File views/event_x.h (right):

http://codereview.chromium.org/3801011/diff/37001/38016#newcode15
views/event_x.h:15: bool DispatchXEvent(XEvent* xevent);
This should go in accelerator_handler.

[sadrul, 2010-10-21 20:17:44]

http://codereview.chromium.org/3801011/diff/37001/38014
File views/event.h (right):

http://codereview.chromium.org/3801011/diff/37001/38014#newcode231
views/event.h:231: explicit MouseEvent(XEvent* xevent, bool drag);
On 2010/10/21 19:41:42, sky wrote:
> no explicit

Done.

http://codereview.chromium.org/3801011/diff/37001/38016
File views/event_x.h (right):

http://codereview.chromium.org/3801011/diff/37001/38016#newcode15
views/event_x.h:15: bool DispatchXEvent(XEvent* xevent);
On 2010/10/21 19:41:42, sky wrote:
> This should go in accelerator_handler.

Done.

[sky, 2010-10-21 20:26:17]

http://codereview.chromium.org/3801011/diff/44001/45015
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/44001/45015#newcode23
views/event_x.cc:23: RootView* FindRootViewForGdkWindow(GdkWindow* gdk_window) {
Can you merge now?

http://codereview.chromium.org/3801011/diff/44001/45015#newcode86
views/event_x.cc:86: bool DispatchXEvent(XEvent* xev) {
This should be in accelerator_handler.

[sadrul, 2010-10-22 01:30:38]

http://codereview.chromium.org/3801011/diff/44001/45015
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/44001/45015#newcode23
views/event_x.cc:23: RootView* FindRootViewForGdkWindow(GdkWindow* gdk_window) {
On 2010/10/21 20:26:17, sky wrote:
> Can you merge now?

I was hoping to do the merge, removal of now-unnecessary code, and other clean
up in a separate CL. Should I do all of that for this CL or would it be too big?

[sadrul, 2010-10-22 03:57:22]

I have removed some now-unnecessary code and merged with wyck's CL.

http://codereview.chromium.org/3801011/diff/44001/45015
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/44001/45015#newcode23
views/event_x.cc:23: RootView* FindRootViewForGdkWindow(GdkWindow* gdk_window) {
On 2010/10/22 01:30:38, sadrul wrote:
> On 2010/10/21 20:26:17, sky wrote:
> > Can you merge now?
> 
> I was hoping to do the merge, removal of now-unnecessary code, and other clean
> up in a separate CL. Should I do all of that for this CL or would it be too
big?

I have gone ahead and remove/merge/clean'ed the code. Done.

http://codereview.chromium.org/3801011/diff/44001/45015#newcode86
views/event_x.cc:86: bool DispatchXEvent(XEvent* xev) {
On 2010/10/21 20:26:17, sky wrote:
> This should be in accelerator_handler.

Done.

[sky, 2010-10-22 15:28:59]

http://codereview.chromium.org/3801011/diff/46002/59014
File views/event.h (right):

http://codereview.chromium.org/3801011/diff/46002/59014#newcode230
views/event.h:230: explicit MouseEvent(XEvent* xevent);
Do we really need both of these? Can't we just have the two arg one? As it
stands MouseEvent(xevent) gives different results than MouseEvent(xevent, false)
which is very confusing.

http://codereview.chromium.org/3801011/diff/46002/59015
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/46002/59015#newcode13
views/event_x.cc:13: namespace {
put this anonymous namespace in views so that you don't need all the usings.

http://codereview.chromium.org/3801011/diff/46002/59015#newcode17
views/event_x.cc:17: using views::Event;
nit: in the future sort these, but since you can get rid of them it won't
matter.

http://codereview.chromium.org/3801011/diff/46002/59017
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3801011/diff/46002/59017#newcode32
views/focus/accelerator_handler_touch.cc:32: processed = true;
You should just return here and other places in this method. Early returns are
the norm and not the exception in chrome code.

http://codereview.chromium.org/3801011/diff/46002/59021
File views/touchui/touch_event_dispatcher_x.h (right):

http://codereview.chromium.org/3801011/diff/46002/59021#newcode14
views/touchui/touch_event_dispatcher_x.h:14: RootView*
FindRootViewForGdkWindow(GdkWindow* window);
This should live in a better place. In fact can't you just put it in
accelerator_handler_touch as that is the only place that uses it?

[sadrul, 2010-10-22 16:10:40]

http://codereview.chromium.org/3801011/diff/46002/59014
File views/event.h (right):

http://codereview.chromium.org/3801011/diff/46002/59014#newcode230
views/event.h:230: explicit MouseEvent(XEvent* xevent);
On 2010/10/22 15:28:59, sky wrote:
> Do we really need both of these? Can't we just have the two arg one? As it
> stands MouseEvent(xevent) gives different results than MouseEvent(xevent,
false)
> which is very confusing.

I attempted to use a single constructor. But it gets really confusing because
there are two checks involved for every value (e.g. xev->type == ButtonPress ?
ET_MOUSE_PRESSED : xev->type == ButtonRelease ? ET_MOUSE_RELEASED : drag ?
ET_MOUSE_DRAGGED : ET_MOUSE_MOVED, and then to determine if xev->xbutton. or
xev->xmotion for x, y, state).

http://codereview.chromium.org/3801011/diff/46002/59015
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/46002/59015#newcode13
views/event_x.cc:13: namespace {
On 2010/10/22 15:28:59, sky wrote:
> put this anonymous namespace in views so that you don't need all the usings.

Done.

http://codereview.chromium.org/3801011/diff/46002/59015#newcode17
views/event_x.cc:17: using views::Event;
On 2010/10/22 15:28:59, sky wrote:
> nit: in the future sort these, but since you can get rid of them it won't
> matter.

Noted. Thanks.

http://codereview.chromium.org/3801011/diff/46002/59017
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3801011/diff/46002/59017#newcode32
views/focus/accelerator_handler_touch.cc:32: processed = true;
On 2010/10/22 15:28:59, sky wrote:
> You should just return here and other places in this method. Early returns are
> the norm and not the exception in chrome code.

Noted. Done.

http://codereview.chromium.org/3801011/diff/46002/59021
File views/touchui/touch_event_dispatcher_x.h (right):

http://codereview.chromium.org/3801011/diff/46002/59021#newcode14
views/touchui/touch_event_dispatcher_x.h:14: RootView*
FindRootViewForGdkWindow(GdkWindow* window);
On 2010/10/22 15:28:59, sky wrote:
> This should live in a better place. In fact can't you just put it in
> accelerator_handler_touch as that is the only place that uses it?

I kept it here thinking we may eventually need it elsewhere. But I guess we can
move it to a better place when the time comes. Moved to a_h_t.

Done.

[sky, 2010-10-22 18:08:20]

http://codereview.chromium.org/3801011/diff/46002/59014
File views/event.h (right):

http://codereview.chromium.org/3801011/diff/46002/59014#newcode230
views/event.h:230: explicit MouseEvent(XEvent* xevent);
On 2010/10/22 16:10:40, sadrul wrote:
> On 2010/10/22 15:28:59, sky wrote:
> > Do we really need both of these? Can't we just have the two arg one? As it
> > stands MouseEvent(xevent) gives different results than MouseEvent(xevent,
> false)
> > which is very confusing.
> 
> I attempted to use a single constructor. But it gets really confusing because
> there are two checks involved for every value (e.g. xev->type == ButtonPress ?
> ET_MOUSE_PRESSED : xev->type == ButtonRelease ? ET_MOUSE_RELEASED : drag ?
> ET_MOUSE_DRAGGED : ET_MOUSE_MOVED, and then to determine if xev->xbutton. or
> xev->xmotion for x, y, state).

That's an implementation detail. It's better to keep the api simple. Can't you
hide all this in the implementation based on the type? If you need to call a
static function from the constructor, do it.

[rjkroege, 2010-10-22 18:09:35]

http://codereview.chromium.org/3801011/diff/65001/66005
File base/message_loop.h (right):

http://codereview.chromium.org/3801011/diff/65001/66005#newcode28
base/message_loop.h:28: #endif
I am suspecting that some of these changes will disappear with the merge of your
last change

http://codereview.chromium.org/3801011/diff/65001/66013
File views/controls/menu/native_menu_gtk.h (right):

http://codereview.chromium.org/3801011/diff/65001/66013#newcode51
views/controls/menu/native_menu_gtk.h:51: virtual bool Dispatch(GdkEvent*
event);
it might be useful to add some sort of DLOG(warning) (or even an assertion) for
each invocation of Dispatch(GdkEvent*) when TOUCH_UI=1 -- we would not expect
this when the modified message_pump is working properly and it will prove very
useful to know as this work proceeds.

http://codereview.chromium.org/3801011/diff/65001/66015
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/65001/66015#newcode69
views/event_x.cc:69: MouseEvent::MouseEvent(XEvent* xev)
Question for you: under xinput2, X is sending us mouse events and some of them
are touches and some aren't (as per their touchid). So, see comment in
DispatchXEvent.

http://codereview.chromium.org/3801011/diff/65001/66017
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3801011/diff/65001/66017#newcode15
views/focus/accelerator_handler_touch.cc:15: namespace views {
is the goal to put DispatchXEvent in the views name space and FindRootView...
not? if so, don't you need to push this line below } // namespace below?

http://codereview.chromium.org/3801011/diff/65001/66017#newcode43
views/focus/accelerator_handler_touch.cc:43: switch (xev->type) {
Additional commentary from the event_x: incoming XEvents can be mouse events
that are actually touches right?

In which case, you need to sort them here correct? you could put a TODO about
this if you like. And we'll need some way (ideally) to inquire what the X server
gives us in terms of touch ids?

Also, when you go write the code that handles XI2 events here, can you arrange
to make this code compile with and without the use of XI2?

[sadrul, 2010-10-22 18:47:03]

http://codereview.chromium.org/3801011/diff/65001/66005
File base/message_loop.h (right):

http://codereview.chromium.org/3801011/diff/65001/66005#newcode28
base/message_loop.h:28: #endif
On 2010/10/22 18:09:35, rjkroege wrote:
> I am suspecting that some of these changes will disappear with the merge of
your
> last change 

This is actually on top of 'the other one'. I am hoping there won't be merge
conflicts from that.

http://codereview.chromium.org/3801011/diff/65001/66013
File views/controls/menu/native_menu_gtk.h (right):

http://codereview.chromium.org/3801011/diff/65001/66013#newcode51
views/controls/menu/native_menu_gtk.h:51: virtual bool Dispatch(GdkEvent*
event);
On 2010/10/22 18:09:35, rjkroege wrote:
> it might be useful to add some sort of DLOG(warning) (or even an assertion)
for
> each invocation of Dispatch(GdkEvent*) when TOUCH_UI=1 -- we would not expect
> this when the modified message_pump is working properly and it will prove very
> useful to know as this work proceeds.

I have added DLOG's in the message pump code which invokes the Dispatch method
with GdkEvents (of NativeMenuGtk, AcceleratorHandler and MenuController).

http://codereview.chromium.org/3801011/diff/65001/66015
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/65001/66015#newcode69
views/event_x.cc:69: MouseEvent::MouseEvent(XEvent* xev)
On 2010/10/22 18:09:35, rjkroege wrote:
> Question for you: under xinput2, X is sending us mouse events and some of them
> are touches and some aren't (as per their touchid). So, see comment in
> DispatchXEvent.

Explained in the comment in DispatchXEvent.

http://codereview.chromium.org/3801011/diff/65001/66017
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3801011/diff/65001/66017#newcode15
views/focus/accelerator_handler_touch.cc:15: namespace views {
On 2010/10/22 18:09:35, rjkroege wrote:
> is the goal to put DispatchXEvent in the views name space and FindRootView...
> not? if so, don't you need to push this line below } // namespace below?

That's the purpose, yes. This anonymous namespace is brought into the views
namespace so we don't need to use a bunch of 'using's here (as per sky's
suggestion).

http://codereview.chromium.org/3801011/diff/65001/66017#newcode43
views/focus/accelerator_handler_touch.cc:43: switch (xev->type) {
On 2010/10/22 18:09:35, rjkroege wrote:
> Additional commentary from the event_x: incoming XEvents can be mouse events
> that are actually touches right?

Correct.

> In which case, you need to sort them here correct? you could put a TODO about
> this if you like. And we'll need some way (ideally) to inquire what the X
server
> gives us in terms of touch ids?

It will have to be in this function, a little farther below. With XI2, the
events come in as a GenericEvent from X. We can then issue a XGetEventData
request to fetch the cookie and then process the event from there. So it would
look something like:

    case GenericEvent:
      return DispatchX2Event(xev);

> Also, when you go write the code that handles XI2 events here, can you arrange
> to make this code compile with and without the use of XI2?

Will do.

[sadrul, 2010-10-22 18:51:08]

On 2010/10/22 18:08:20, sky wrote:
> http://codereview.chromium.org/3801011/diff/46002/59014
> File views/event.h (right):
> 
> http://codereview.chromium.org/3801011/diff/46002/59014#newcode230
> views/event.h:230: explicit MouseEvent(XEvent* xevent);
> On 2010/10/22 16:10:40, sadrul wrote:
> > On 2010/10/22 15:28:59, sky wrote:
> > > Do we really need both of these? Can't we just have the two arg one? As it
> > > stands MouseEvent(xevent) gives different results than MouseEvent(xevent,
> > false)
> > > which is very confusing.
> > 
> > I attempted to use a single constructor. But it gets really confusing
because
> > there are two checks involved for every value (e.g. xev->type == ButtonPress
?
> > ET_MOUSE_PRESSED : xev->type == ButtonRelease ? ET_MOUSE_RELEASED : drag ?
> > ET_MOUSE_DRAGGED : ET_MOUSE_MOVED, and then to determine if xev->xbutton. or
> > xev->xmotion for x, y, state).
> 
> That's an implementation detail. It's better to keep the api simple. Can't you
> hide all this in the implementation based on the type? If you need to call a
> static function from the constructor, do it.

Done.

[rjkroege, 2010-10-22 19:32:43]

LGTM.

[sky, 2010-10-23 00:04:55]

LGTM, but you need sign off from Evan or someone on the message loop changes.

  -Scott

[sadrul, 2010-10-25 14:02:44]

I have now updated the patch to cleanly merge with trunk (there were some merge
conflicts for the newly added NestedDispatcherGtk), and also fix a compile
failure for chromeos=1. PTAL! :)

[sky, 2010-10-25 17:13:52]

LGTM with the following nits fixed.

http://codereview.chromium.org/3801011/diff/77001/39016
File chrome/browser/chromeos/views/native_menu_domui.cc (right):

http://codereview.chromium.org/3801011/diff/77001/39016#newcode24
chrome/browser/chromeos/views/native_menu_domui.cc:24: #if defined(TOUCH_UI)
newline between 23 and 24.

http://codereview.chromium.org/3801011/diff/77001/39022
File views/controls/menu/nested_dispatcher_gtk.cc (right):

http://codereview.chromium.org/3801011/diff/77001/39022#newcode52
views/controls/menu/nested_dispatcher_gtk.cc:52: } else {
no point in an else here.

http://codereview.chromium.org/3801011/diff/77001/39025
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/77001/39025#newcode11
views/event_x.cc:11: #include <gdk/gdkx.h>
This include should be between the include for event.h and the other non-system
includes, eg:

#include "views/event.h"

#include <gdk/gdkx.h>

#include "app/keyboard_code_conversion_x.h"
#include "views/widget/root_view.h"

http://codereview.chromium.org/3801011/diff/77001/39027
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3801011/diff/77001/39027#newcode10
views/focus/accelerator_handler_touch.cc:10: #include
"views/focus/accelerator_handler.h"
This include should be first, eg:

#include "views/focus/accelerator_handler.h"

#include <gtk/gtk.h>
#include <X11/Xlib.h>

#include "views/accelerator.h" ...

[sadrul, 2010-10-25 17:30:34]

Thanks! Made the suggested changes.

Evan, could you PTAL! :)

http://codereview.chromium.org/3801011/diff/77001/39016
File chrome/browser/chromeos/views/native_menu_domui.cc (right):

http://codereview.chromium.org/3801011/diff/77001/39016#newcode24
chrome/browser/chromeos/views/native_menu_domui.cc:24: #if defined(TOUCH_UI)
On 2010/10/25 17:13:52, sky wrote:
> newline between 23 and 24.

Done.

http://codereview.chromium.org/3801011/diff/77001/39022
File views/controls/menu/nested_dispatcher_gtk.cc (right):

http://codereview.chromium.org/3801011/diff/77001/39022#newcode52
views/controls/menu/nested_dispatcher_gtk.cc:52: } else {
On 2010/10/25 17:13:52, sky wrote:
> no point in an else here.

Done.

http://codereview.chromium.org/3801011/diff/77001/39025
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/77001/39025#newcode11
views/event_x.cc:11: #include <gdk/gdkx.h>
On 2010/10/25 17:13:52, sky wrote:
> This include should be between the include for event.h and the other
non-system
> includes, eg:
> 
> #include "views/event.h"
> 
> #include <gdk/gdkx.h>
> 
> #include "app/keyboard_code_conversion_x.h"
> #include "views/widget/root_view.h"

Done.

http://codereview.chromium.org/3801011/diff/77001/39027
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3801011/diff/77001/39027#newcode10
views/focus/accelerator_handler_touch.cc:10: #include
"views/focus/accelerator_handler.h"
On 2010/10/25 17:13:52, sky wrote:
> This include should be first, eg:
> 
> #include "views/focus/accelerator_handler.h"
> 
> #include <gtk/gtk.h>
> #include <X11/Xlib.h>
> 
> #include "views/accelerator.h" ...

Done.

I also took the liberty of making the same change in accelerator_handler_gtk.cc

[Evan Martin, 2010-10-25 17:44:29]

LGTM, minor nits follow (I only skimmed the views/ code)

http://codereview.chromium.org/3801011/diff/87001/88002
File app/keyboard_code_conversion_x.cc (right):

http://codereview.chromium.org/3801011/diff/87001/88002#newcode154
app/keyboard_code_conversion_x.cc:154: // TODO(sad): A poop-load are still
missing.
I think this comment is kind of embarrassing to have etched into the project's
history forevermore, but that's your call.

http://codereview.chromium.org/3801011/diff/87001/88008
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/3801011/diff/87001/88008#newcode48
base/message_pump_glib_x.cc:48: (GetDispatcher())->Dispatch(&xev);
Why not just:
bool processed =
    static_cast<MessagePumpGlibXDispatcher*>(GetDispatcher())->Dispatch(&xev);

http://codereview.chromium.org/3801011/diff/87001/88009
File base/message_pump_glib_x_dispatch.h (right):

http://codereview.chromium.org/3801011/diff/87001/88009#newcode15
base/message_pump_glib_x_dispatch.h:15: class MessagePumpGlibXDispatcher :
public MessagePumpForUI::Dispatcher {
Can you provide documentation above this class about what it is?  (Imagine e.g.
someone is making message pump changes on Windows and wants to whether they need
to worry about this class.)

http://codereview.chromium.org/3801011/diff/87001/88009#newcode18
base/message_pump_glib_x_dispatch.h:18: // normal. If false is returned, the
nested loop exits immediately.
You might want to explicitly state that this is a new method, not an override of
the parent class's one (I was confused at first).

http://codereview.chromium.org/3801011/diff/87001/88019
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/87001/88019#newcode17
views/event_x.cc:17: // Does what it says on the label.
This comment is not useful.

A useful comment might describe what "event flags" are (e.g. "a bitwise-ored set
of Event::SomeEnum"), or what an XState is.  Or just delete the comment.

http://codereview.chromium.org/3801011/diff/87001/88019#newcode74
views/event_x.cc:74: gfx::Point GetMouseEventLocation(XEvent* xev) {
(I think functions like these have names that are self-describing enough to not
warrant a comment.)

[sadrul, 2010-10-25 18:23:29]

Added some new comments, and sanitized a few existing comments. Also corrected
the order of #include's in some additional files I missed in the earlier
revision.

Thanks!

http://codereview.chromium.org/3801011/diff/87001/88002
File app/keyboard_code_conversion_x.cc (right):

http://codereview.chromium.org/3801011/diff/87001/88002#newcode154
app/keyboard_code_conversion_x.cc:154: // TODO(sad): A poop-load are still
missing.
On 2010/10/25 17:44:29, Evan Martin wrote:
> I think this comment is kind of embarrassing to have etched into the project's
> history forevermore, but that's your call.

Sanitized the comment :)

http://codereview.chromium.org/3801011/diff/87001/88008
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/3801011/diff/87001/88008#newcode48
base/message_pump_glib_x.cc:48: (GetDispatcher())->Dispatch(&xev);
On 2010/10/25 17:44:29, Evan Martin wrote:
> Why not just:
> bool processed =
>     static_cast<MessagePumpGlibXDispatcher*>(GetDispatcher())->Dispatch(&xev);

Done (the RHS still had to be split into two lines to fit into 80 columns)

http://codereview.chromium.org/3801011/diff/87001/88009
File base/message_pump_glib_x_dispatch.h (right):

http://codereview.chromium.org/3801011/diff/87001/88009#newcode15
base/message_pump_glib_x_dispatch.h:15: class MessagePumpGlibXDispatcher :
public MessagePumpForUI::Dispatcher {
On 2010/10/25 17:44:29, Evan Martin wrote:
> Can you provide documentation above this class about what it is?  (Imagine
e.g.
> someone is making message pump changes on Windows and wants to whether they
need
> to worry about this class.)

Done.

http://codereview.chromium.org/3801011/diff/87001/88009#newcode18
base/message_pump_glib_x_dispatch.h:18: // normal. If false is returned, the
nested loop exits immediately.
On 2010/10/25 17:44:29, Evan Martin wrote:
> You might want to explicitly state that this is a new method, not an override
of
> the parent class's one (I was confused at first).

I think the comment added for the above address this issue?

http://codereview.chromium.org/3801011/diff/87001/88019
File views/event_x.cc (right):

http://codereview.chromium.org/3801011/diff/87001/88019#newcode17
views/event_x.cc:17: // Does what it says on the label.
On 2010/10/25 17:44:29, Evan Martin wrote:
> This comment is not useful.
> 
> A useful comment might describe what "event flags" are (e.g. "a bitwise-ored
set
> of Event::SomeEnum"), or what an XState is.  Or just delete the comment.

The name of the function sounds self-explanatory enough. So I removed the
unhelpful comment.

http://codereview.chromium.org/3801011/diff/87001/88019#newcode74
views/event_x.cc:74: gfx::Point GetMouseEventLocation(XEvent* xev) {
On 2010/10/25 17:44:29, Evan Martin wrote:
> (I think functions like these have names that are self-describing enough to
not
> warrant a comment.)

I agree!

[rjkroege, 2010-10-25 19:01:07]

still LGTM.

please remember to add the test that I mention as part of the next phase.

http://codereview.chromium.org/3801011/diff/94001/95002
File app/keyboard_code_conversion_x.cc (right):

http://codereview.chromium.org/3801011/diff/94001/95002#newcode156
app/keyboard_code_conversion_x.cc:156: // TODO(sad): A lot of keycodes still
missing.
add "are"

http://codereview.chromium.org/3801011/diff/94001/95022
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3801011/diff/94001/95022#newcode20
views/focus/accelerator_handler_touch.cc:20: RootView*
FindRootViewForGdkWindow(GdkWindow* gdk_window) {
In a subsequent CL, as you finish wiring this up to the RWHVV etc., this code
ought to have a unit test.

[sadrul, 2010-10-25 19:07:50]

I will add unit-test(s) in subsequent CLs.

http://codereview.chromium.org/3801011/diff/94001/95002
File app/keyboard_code_conversion_x.cc (right):

http://codereview.chromium.org/3801011/diff/94001/95002#newcode156
app/keyboard_code_conversion_x.cc:156: // TODO(sad): A lot of keycodes still
missing.
On 2010/10/25 19:01:08, rjkroege wrote:
> add "are"

Done.

http://codereview.chromium.org/3801011/diff/94001/95022
File views/focus/accelerator_handler_touch.cc (right):

http://codereview.chromium.org/3801011/diff/94001/95022#newcode20
views/focus/accelerator_handler_touch.cc:20: RootView*
FindRootViewForGdkWindow(GdkWindow* gdk_window) {
On 2010/10/25 19:01:08, rjkroege wrote:
> In a subsequent CL, as you finish wiring this up to the RWHVV etc., this code
> ought to have a unit test.

Noted.