Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4186004:
touchui: First pass at XInput2 message pump. (Closed)
|
Created:
10 years, 1 month ago by sadrul Modified:
9 years, 6 months ago CC:
chromium-reviews, ben+cc_chromium.org, brettw-cc_chromium.org Base URL:
http://git.chromium.org/git/chromium.git Visibility:
Public. |
Descriptiontouchui: First pass at XInput2 message pump.
Capture X events using XInput2.
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=65959
Patch Set 1 #Patch Set 2 : Fix views::Event flags for XI2 events. #
Total comments: 14
Patch Set 3 : '' #Patch Set 4 : Wrap code within ifdef's. #
Total comments: 9
Patch Set 5 : Select on all master pointer devices. #Patch Set 6 : Change how we decide whether XInput2 is available. #
Total comments: 7
Patch Set 7 : Add comments, and maintain a list of master devices. #
Total comments: 4
Patch Set 8 : '' #Patch Set 9 : gyp fiddling #
Messages
Total messages: 17 (0 generated)
One of the TODOs is done: the flags for the view::Event's are now correctly set.
Most of my comments will probably be swallowed up in your TODOs. You might want to explicitly say that this CL will not contain hierarchy event handling? http://codereview.chromium.org/4186004/diff/3001/4001 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/4186004/diff/3001/4001#newcode27 base/message_pump_glib_x.cc:27: if (!window) Is this anticipated under standard operation? http://codereview.chromium.org/4186004/diff/3001/4001#newcode30 base/message_pump_glib_x.cc:30: // TODO(sad): Do we need to set a flag on |window| to make sure we don't Log something about this if it's an issue. http://codereview.chromium.org/4186004/diff/3001/4001#newcode45 base/message_pump_glib_x.cc:45: evmasks[0].deviceid = 2; // The default cursor. I assume that augmenting this to grab everything is part of the TODOs? http://codereview.chromium.org/4186004/diff/3001/4003 File views/event_x.cc (right): http://codereview.chromium.org/4186004/diff/3001/4003#newcode36 views/event_x.cc:36: // Get the event flag for the button in XButtonEvent. During a ButtonPress Was your thinking that you would add a GetDevicId... function in this file? http://codereview.chromium.org/4186004/diff/3001/4003#newcode99 views/event_x.cc:99: gfx::Point GetMouseEventLocation(XEvent* xev) { Is there a function like this in the x11 utility code? It would be nice to make the xinput2 processing code as re-usable as possible if it would be desirable for the Linux build? http://codereview.chromium.org/4186004/diff/3001/4004 File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/4186004/diff/3001/4004#newcode58 views/focus/accelerator_handler_touch.cc:58: switch (cookie->evtype) { I assume that in here you will insert code to generate views touch events per email? There would (hopefully) be a map of X device ids to WebTouchEvent device/touch id events. http://codereview.chromium.org/4186004/diff/3001/4004#newcode100 views/focus/accelerator_handler_touch.cc:100: DLOG(WARNING) << "Error fetching cookie for event."; Given how strongly biased "cookie" is to a different meaning in Chrome, it would be good to call this something different in the logs: XGenericEventCookie say.
http://codereview.chromium.org/4186004/diff/3001/4001 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/4186004/diff/3001/4001#newcode27 base/message_pump_glib_x.cc:27: if (!window) On 2010/10/27 21:20:21, rjkroege wrote: > Is this anticipated under standard operation? There can be GtkWidget's which may not have a GdkWindow of their own. I am not entirely sure if 'realize' gets emitted for these widgets at all or not. I have added a DCHECK to test this, which will be removed/rectified after testing. http://codereview.chromium.org/4186004/diff/3001/4001#newcode30 base/message_pump_glib_x.cc:30: // TODO(sad): Do we need to set a flag on |window| to make sure we don't On 2010/10/27 21:20:21, rjkroege wrote: > Log something about this if it's an issue. Doing multiple selects on the same Window shouldn't break anything. So I think for now, it's a good idea to always do the select here. I am mostly curious to know which is more efficient between doing this versus checking a flag (which I plan on doing by using g_object_get_data and _set_data on the GdkWindow, which as far as I know does a hashtable lookup). My plan is to revisit these TODO's once the code is usable and stable enough to do some tests and make a comparison between the two. http://codereview.chromium.org/4186004/diff/3001/4001#newcode45 base/message_pump_glib_x.cc:45: evmasks[0].deviceid = 2; // The default cursor. On 2010/10/27 21:20:21, rjkroege wrote: > I assume that augmenting this to grab everything is part of the TODOs? The docs I have read claim that '2' is the device id for the system cursors (pointers?), which I *think* means that the touch events should be covered by this. It captures events from the USB mouse, touchpad and the trackpoint, for example. ('0' covers all devices (XIAllDevices) and '1' covers all master devices (XIAllMasterDevices)). http://codereview.chromium.org/4186004/diff/3001/4003 File views/event_x.cc (right): http://codereview.chromium.org/4186004/diff/3001/4003#newcode36 views/event_x.cc:36: // Get the event flag for the button in XButtonEvent. During a ButtonPress On 2010/10/27 21:20:21, rjkroege wrote: > Was your thinking that you would add a GetDevicId... function in this file? The device-id is immediately available to us (xev->xcookie.data->sourceid). http://codereview.chromium.org/4186004/diff/3001/4003#newcode99 views/event_x.cc:99: gfx::Point GetMouseEventLocation(XEvent* xev) { On 2010/10/27 21:20:21, rjkroege wrote: > Is there a function like this in the x11 utility code? It would be nice to make > the xinput2 processing code as re-usable as possible if it would be desirable > for the Linux build? It doesn't look like there's anything about XEvents in x11_utils [yet]. Would you prefer if I move this code there now, or would it make sense to move the code there when the time comes? http://codereview.chromium.org/4186004/diff/3001/4004 File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/4186004/diff/3001/4004#newcode58 views/focus/accelerator_handler_touch.cc:58: switch (cookie->evtype) { On 2010/10/27 21:20:21, rjkroege wrote: > I assume that in here you will insert code to generate views touch events per > email? There would (hopefully) be a map of X device ids to WebTouchEvent > device/touch id events. The code will go under the TODO above. It should be possible to map the ids, yes. http://codereview.chromium.org/4186004/diff/3001/4004#newcode100 views/focus/accelerator_handler_touch.cc:100: DLOG(WARNING) << "Error fetching cookie for event."; On 2010/10/27 21:20:21, rjkroege wrote: > Given how strongly biased "cookie" is to a different meaning in Chrome, it would > be good to call this something different in the logs: XGenericEventCookie say. Done.
Added ifdef wraps around code. PTAL! :)
the #ifdefs make me sad. But I know that we need them. I agree on the approach. Please remind me (repeatedly as necessary) about when device management will get added. http://codereview.chromium.org/4186004/diff/1005/14001 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/4186004/diff/1005/14001#newcode8 base/message_pump_glib_x.cc:8: #if defined(TOUCH_UI) && defined(OS_CHROMEOS) I assume you checked that there is no configuration contents in Xlib.h? http://codereview.chromium.org/4186004/diff/1005/14001#newcode49 base/message_pump_glib_x.cc:49: evmasks[0].deviceid = 2; // The default cursor. Add TODO about handling all devices so I stop thinking "shouldn't multiple devices be enabled" and then remember that you will write this in the future. http://codereview.chromium.org/4186004/diff/1005/14002 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/4186004/diff/1005/14002#newcode43 base/message_pump_glib_x.h:43: // The opcode used for checking events. I'm not sure what the convention is for sorting these. I would think that they should be in alphabetical order with the #ifdef's at the end. Except that functions come before members. You might want to check the style guide. http://codereview.chromium.org/4186004/diff/1005/14003 File views/event_x.cc (right): http://codereview.chromium.org/4186004/diff/1005/14003#newcode60 views/event_x.cc:60: #if defined(TOUCH_UI) && defined(OS_CHROMEOS) I dislike all of this #ifdefery. Any thoughts on a better way that preserves the same property? Can we define a variable in ,gyp based on the availability of an external include?
I am not sure I understand what you meant by 'device management'. http://codereview.chromium.org/4186004/diff/1005/14001 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/4186004/diff/1005/14001#newcode8 base/message_pump_glib_x.cc:8: #if defined(TOUCH_UI) && defined(OS_CHROMEOS) On 2010/10/28 16:58:35, rjkroege wrote: > I assume you checked that there is no configuration contents in Xlib.h? XInput2 #include's Xlib.h (maybe we don't need the #else?) http://codereview.chromium.org/4186004/diff/1005/14001#newcode49 base/message_pump_glib_x.cc:49: evmasks[0].deviceid = 2; // The default cursor. On 2010/10/28 16:58:35, rjkroege wrote: > Add TODO about handling all devices so I stop thinking "shouldn't multiple > devices be enabled" and then remember that you will write this in the future. I have written it now :) So we will select for all MasterPointer devices. That should be sufficient for the mouse/touch events. I have added a TODO for the MasterKeyboard devices. http://codereview.chromium.org/4186004/diff/1005/14002 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/4186004/diff/1005/14002#newcode43 base/message_pump_glib_x.h:43: // The opcode used for checking events. On 2010/10/28 16:58:35, rjkroege wrote: > I'm not sure what the convention is for sorting these. I would think that they > should be in alphabetical order with the #ifdef's at the end. Except that > functions come before members. You might want to check the style guide. I didn't find anything about this in the style-guide, apart from methods before members. For now, kept as it is since it allows wrapping all of it inside a single #if. http://codereview.chromium.org/4186004/diff/1005/14003 File views/event_x.cc (right): http://codereview.chromium.org/4186004/diff/1005/14003#newcode60 views/event_x.cc:60: #if defined(TOUCH_UI) && defined(OS_CHROMEOS) On 2010/10/28 16:58:35, rjkroege wrote: > I dislike all of this #ifdefery. Any thoughts on a better way that preserves the > same property? > > Can we define a variable in ,gyp based on the availability of an external > include? > In my case, 'dislike' is an understatement. I am looking into ways to make this slightly better. If nothing else, perhaps define HAVE_XINPUT2 when 'xinput2=1' in GYP_DEFINES would be acceptable?
With the current patchset, we have a message-pump working that uses XInput2 (for mouse events) when available, and behaves the same whether XInput2 is used or not. http://codereview.chromium.org/4186004/diff/1005/14003 File views/event_x.cc (right): http://codereview.chromium.org/4186004/diff/1005/14003#newcode60 views/event_x.cc:60: #if defined(TOUCH_UI) && defined(OS_CHROMEOS) > If nothing else, perhaps define HAVE_XINPUT2 when 'xinput2=1' > in GYP_DEFINES would be acceptable? I have changed to check for the version of 'inputproto' in the gyp file. If it's 2.0, then HAVE_XINPUT2 will be defined, otherwise not. It's working correctly for me.
On 2010/10/29 01:28:16, sadrul wrote: > I am not sure I understand what you meant by 'device management'. per the discussions today: I would imagine in some future world that we can automatically determine from the window server how the device ids should translate to WebTouchEvent::sourceid, WebTouchPoint::id tuples. > > http://codereview.chromium.org/4186004/diff/1005/14001 > File base/message_pump_glib_x.cc (right): > > http://codereview.chromium.org/4186004/diff/1005/14001#newcode8 > base/message_pump_glib_x.cc:8: #if defined(TOUCH_UI) && defined(OS_CHROMEOS) > On 2010/10/28 16:58:35, rjkroege wrote: > > I assume you checked that there is no configuration contents in Xlib.h? > > XInput2 #include's Xlib.h (maybe we don't need the #else?) > > http://codereview.chromium.org/4186004/diff/1005/14001#newcode49 > base/message_pump_glib_x.cc:49: evmasks[0].deviceid = 2; // The default cursor. > On 2010/10/28 16:58:35, rjkroege wrote: > > Add TODO about handling all devices so I stop thinking "shouldn't multiple > > devices be enabled" and then remember that you will write this in the future. > > I have written it now :) So we will select for all MasterPointer devices. That > should be sufficient for the mouse/touch events. I have added a TODO for the > MasterKeyboard devices. Excellent. Do we have any idea about the changes to the xconf needed to rationally expose device and finger identities? > > http://codereview.chromium.org/4186004/diff/1005/14002 > File base/message_pump_glib_x.h (right): > > http://codereview.chromium.org/4186004/diff/1005/14002#newcode43 > base/message_pump_glib_x.h:43: // The opcode used for checking events. > On 2010/10/28 16:58:35, rjkroege wrote: > > I'm not sure what the convention is for sorting these. I would think that they > > should be in alphabetical order with the #ifdef's at the end. Except that > > functions come before members. You might want to check the style guide. > > I didn't find anything about this in the style-guide, apart from methods before > members. For now, kept as it is since it allows wrapping all of it inside a > single #if. > > http://codereview.chromium.org/4186004/diff/1005/14003 > File views/event_x.cc (right): > > http://codereview.chromium.org/4186004/diff/1005/14003#newcode60 > views/event_x.cc:60: #if defined(TOUCH_UI) && defined(OS_CHROMEOS) > On 2010/10/28 16:58:35, rjkroege wrote: > > I dislike all of this #ifdefery. Any thoughts on a better way that preserves > the > > same property? > > > > Can we define a variable in ,gyp based on the availability of an external > > include? > > > > In my case, 'dislike' is an understatement. I am looking into ways to make this > slightly better. If nothing else, perhaps define HAVE_XINPUT2 when 'xinput2=1' > in GYP_DEFINES would be acceptable?
lingering comments that I forgot to send. http://codereview.chromium.org/4186004/diff/21001/22001 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/4186004/diff/21001/22001#newcode50 base/message_pump_glib_x.cc:50: XIDeviceInfo* devices = XIQueryDevice(xdisplay, XIAllMasterDevices, &ndevs); PEr our discussion, this isn't really done yet right? Do we know (yet) how (ir)rational the underlying X server will be? http://codereview.chromium.org/4186004/diff/21001/22002 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/4186004/diff/21001/22002#newcode39 base/message_pump_glib_x.h:39: #if defined(HAVE_XINPUT2) I like that this is possible. Excellent.
http://codereview.chromium.org/4186004/diff/21001/22001 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/4186004/diff/21001/22001#newcode50 base/message_pump_glib_x.cc:50: XIDeviceInfo* devices = XIQueryDevice(xdisplay, XIAllMasterDevices, &ndevs); On 2010/11/01 21:18:44, rjkroege wrote: > PEr our discussion, this isn't really done yet right? Do we know (yet) how > (ir)rational the underlying X server will be? There isn't an issue with master-pointer devices, actually. Right now, as it happens, the touch/mouse events all generate an event on the same master-pointer-device, as expected. However, the problem is with the slave-pointer-device id that is used for multi-touches that is incorrect/ill-defined. Single touch/mouse events work OK.
> Do we have any idea about the changes to the xconf needed to > rationally expose device and finger identities? There doesn't seem to be anything that fixes things [yet]. Maybe I should take a look at the device driver?
LGTM. I think another reviewer (evmar?) would be useful. http://codereview.chromium.org/4186004/diff/21001/22001 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/4186004/diff/21001/22001#newcode50 base/message_pump_glib_x.cc:50: XIDeviceInfo* devices = XIQueryDevice(xdisplay, XIAllMasterDevices, &ndevs); On 2010/11/02 16:45:10, sadrul wrote: > On 2010/11/01 21:18:44, rjkroege wrote: > > PEr our discussion, this isn't really done yet right? Do we know (yet) how > > (ir)rational the underlying X server will be? > > There isn't an issue with master-pointer devices, actually. Right now, as it > happens, the touch/mouse events all generate an event on the same > master-pointer-device, as expected. However, the problem is with the > slave-pointer-device id that is used for multi-touches that is > incorrect/ill-defined. Single touch/mouse events work OK. > can you add a comment (a TODO maybe?) that explains where we are with this. http://codereview.chromium.org/4186004/diff/21001/22001#newcode213 base/message_pump_glib_x.cc:213: // TODO(sad): select on root for XI_HierarchyChanged (and XI_DeviceChanged?) this would handle adding and removing mice?
http://codereview.chromium.org/4186004/diff/21001/22001 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/4186004/diff/21001/22001#newcode50 base/message_pump_glib_x.cc:50: XIDeviceInfo* devices = XIQueryDevice(xdisplay, XIAllMasterDevices, &ndevs); I don't believe there's a TODO to do here? I have added an explanation regarding master and slave devices here that I think should clarify things a bit. http://codereview.chromium.org/4186004/diff/21001/22001#newcode213 base/message_pump_glib_x.cc:213: // TODO(sad): select on root for XI_HierarchyChanged (and XI_DeviceChanged?) On 2010/11/08 18:36:54, rjkroege wrote: > this would handle adding and removing mice? Correct.
LGTM http://codereview.chromium.org/4186004/diff/31001/base/message_pump_glib_x.cc File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/4186004/diff/31001/base/message_pump_glib_x.cc... base/message_pump_glib_x.cc:164: int event, err; these can be moved closer to where they're used http://codereview.chromium.org/4186004/diff/31001/base/message_pump_glib_x.cc... base/message_pump_glib_x.cc:169: DLOG(INFO) << "X Input extension not available."; Is this a problem? Should it be WARNING instead of INFO? Do we want to inform the user about this whenever it happens?
http://codereview.chromium.org/4186004/diff/31001/base/message_pump_glib_x.cc File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/4186004/diff/31001/base/message_pump_glib_x.cc... base/message_pump_glib_x.cc:164: int event, err; On 2010/11/11 20:45:44, evanm wrote: > these can be moved closer to where they're used Done. http://codereview.chromium.org/4186004/diff/31001/base/message_pump_glib_x.cc... base/message_pump_glib_x.cc:169: DLOG(INFO) << "X Input extension not available."; On 2010/11/11 20:45:44, evanm wrote: > Is this a problem? Should it be WARNING instead of INFO? Do we want to inform > the user about this whenever it happens? Done (Changed to WARNING). Note: When XInputExtension/XInput2 is unavailable, XInput1 should still be working, and the touch-events will be treated as mouse-events.
Finally landed cleanly. piman checked in a fix for pkg-config-wrapper which was not returning an error from pkg-config (http://src.chromium.org/viewvc/chrome?view=rev&revision=65956 ), and this was necessary to go green on the arm buildbot. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
