Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7792094:
touchui: support XInput2 MT (Closed)
|
Created:
9 years, 3 months ago by ningxin.hu Modified:
9 years, 3 months ago CC:
chromium-reviews, dhollowa, brettw-cc_chromium.org Visibility:
Public. |
Descriptiontouchui: support XInput2 multitouch
Use XI2 multitouch events instead of mouse events as touch event input for touchui build.
Note: XI MT will be supported in X server 1.12 and XI2.2. Please use build switch "use_xi2_mt=<minor version number>" to specify the minimum XI2 minor version. It is useful to test on experimental XI2.1 with MT support (e.g. build with use_xi2_mt=1).
BUG=95150
TEST=(1) build with touchui=1 use_xi2_mt=1 (2) test on ubuntu 11.04 (X server 1.10 and XI2.1 with experimental MT support). (3) manually test if touch works on browser UI and JS touch events.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102668
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add gyp switch to support both XI2.0 and XI2.1. Correct the style issues also. #
Total comments: 10
Patch Set 3 : select mouse and touch events when XI2.1 #
Total comments: 8
Patch Set 4 : Use USE_XI2_MT instead of specific version number #
Total comments: 6
Patch Set 5 : update patch set according to comment #12 #
Total comments: 8
Patch Set 6 : Update patch set according to comment #16 #
Total comments: 9
Patch Set 7 : update according to comment #20 and #21 #
Total comments: 2
Patch Set 8 : Update patch according to comment #27 #
Total comments: 1
Patch Set 9 : Fix the build issue by rebasing to trunk #Patch Set 10 : Update patch set according to comment #33 #
Messages
Total messages: 45 (0 generated)
The patch looks pretty good. There are some style nits. I think it will be necessary to continue to support XInput2.0 when XInput2.1 is not available. This can be controlled by a gyp-switch. (my development machine, for example, does not have XInput2.1, and I would like to be able continue to work on this :-) ) Note: After submitting a CL, please 'Publish + Mail comments' (from the link on the left panel), because otherwise the reviewers do not receive a mail requesting review. http://codereview.chromium.org/7792094/diff/1/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7792094/diff/1/views/events/event_x.cc#newcode82 views/events/event_x.cc:82: *xev, TouchFactory::TP_TRACKING_ID, &id)) Are TRACKING_ID's recycled by XInput2.1? As far as I know, the drivers send out TRACKING_ID's in increasing order, i.e. if a finger is pressed, then released, and then another finger is pressed, then for the last event, it will send TRACKING_ID 1 instead of 0. Is that not the case with XI2.1? http://codereview.chromium.org/7792094/diff/1/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7792094/diff/1/views/touchui/touch_factory.cc#... views/touchui/touch_factory.cc:209: XITouchClassInfo *tci = (XITouchClassInfo *)xiclassinfo; '* ' instead of ' *' (here and other places). http://codereview.chromium.org/7792094/diff/1/views/touchui/touch_factory.cc#... views/touchui/touch_factory.cc:212: { Brace goes at the end of the previous line. http://codereview.chromium.org/7792094/diff/1/views/touchui/touch_factory.cc#... views/touchui/touch_factory.cc:220: } No braces necessary here http://codereview.chromium.org/7792094/diff/1/views/touchui/touch_factory.cc#... views/touchui/touch_factory.cc:406: { brace in the previous line
http://codereview.chromium.org/7792094/diff/1/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7792094/diff/1/views/events/event_x.cc#newcode82 views/events/event_x.cc:82: *xev, TouchFactory::TP_TRACKING_ID, &id)) On 2011/09/06 14:35:21, sadrul wrote: > Are TRACKING_ID's recycled by XInput2.1? As far as I know, the drivers send out > TRACKING_ID's in increasing order, i.e. if a finger is pressed, then released, > and then another finger is pressed, then for the last event, it will send > TRACKING_ID 1 instead of 0. Is that not the case with XI2.1? With XI2.1, TRACKING_ID is increased for each new touch. It is defined as uint32. It warps to 0 if reaches the num limit. for your example: ... (a bunch of touch events) a finger is pressed: XI_TouchBegin with TRACKING_ID 1890 then released: XI_TouchEnd with TRACKING_ID 1890 then another finger is pressed: XI_TouchBegin with TRACKING_ID 1891 ....
Thanks for your review. As your comments: 1. add a gyp switch to support XI2.0 and 2.1. 2. correct the style issues.
On 2011/09/07 09:24:05, ningxin.hu wrote: > Thanks for your review. > > As your comments: > 1. add a gyp switch to support XI2.0 and 2.1. > 2. correct the style issues. This is looking very nice! I have left a few more comments. The problem with using TRACKING_ID as the touch-id is that the gesture-recognizer in webkit (and possibly on chromium) expects touch-id's starting from 0 (i.e. when a touch-sequence begins, it expects the earliest touch-point has id 0, the next has id 1 and so on). So some changes will be necessary (possibly you will want to use |TouchFactory::slots_used_| in some fashion) for the touch-events to be used properly with webkit (e.g. touch-drag to scroll, tap to click etc.) You can address this particular issue in a follow-up CL if you want.
http://codereview.chromium.org/7792094/diff/7001/views/focus/accelerator_hand... File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/7792094/diff/7001/views/focus/accelerator_hand... views/focus/accelerator_handler_touch.cc:58: if (TouchFactory::GetInstance()->IsTouchDevice(xievent->sourceid)) { This check is probably not necessary here. If something is generating XI_Touch events, we should just assume it is a touch device. http://codereview.chromium.org/7792094/diff/7001/views/focus/accelerator_hand... views/focus/accelerator_handler_touch.cc:63: // it fall through so it can be used as a MouseEvent, if desired. |root| should become |widget| (and possibly corrected in a comment below too). Do you know if unprocessed XI_Touch events do generate a mouse event? If not, this comment can just be removed. http://codereview.chromium.org/7792094/diff/7001/views/focus/accelerator_hand... views/focus/accelerator_handler_touch.cc:127: #endif #endif // defined(USE_XI2_1) http://codereview.chromium.org/7792094/diff/7001/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7792094/diff/7001/views/touchui/touch_factory.... views/touchui/touch_factory.cc:294: #else I think it'd be a good idea to select for XI_Button/Motion events here even when XInput2.1 is available. (the event dipatching code may need to be updated accordingly) http://codereview.chromium.org/7792094/diff/7001/views/touchui/touch_factory.... views/touchui/touch_factory.cc:354: #else ditto
http://codereview.chromium.org/7792094/diff/7001/views/focus/accelerator_hand... File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/7792094/diff/7001/views/focus/accelerator_hand... views/focus/accelerator_handler_touch.cc:58: if (TouchFactory::GetInstance()->IsTouchDevice(xievent->sourceid)) { On 2011/09/07 14:56:37, sadrul wrote: > This check is probably not necessary here. If something is generating XI_Touch > events, we should just assume it is a touch device. Agree. I am removing the check. http://codereview.chromium.org/7792094/diff/7001/views/focus/accelerator_hand... views/focus/accelerator_handler_touch.cc:63: // it fall through so it can be used as a MouseEvent, if desired. On 2011/09/07 14:56:37, sadrul wrote: > |root| should become |widget| (and possibly corrected in a comment below too). > > Do you know if unprocessed XI_Touch events do generate a mouse event? If not, Will correct the comments. Thanks. > this comment can just be removed. http://codereview.chromium.org/7792094/diff/7001/views/focus/accelerator_hand... views/focus/accelerator_handler_touch.cc:127: #endif On 2011/09/07 14:56:37, sadrul wrote: > #endif // defined(USE_XI2_1) Added. Thanks. http://codereview.chromium.org/7792094/diff/7001/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7792094/diff/7001/views/touchui/touch_factory.... views/touchui/touch_factory.cc:294: #else On 2011/09/07 14:56:37, sadrul wrote: > I think it'd be a good idea to select for XI_Button/Motion events here even when > XInput2.1 is available. (the event dipatching code may need to be updated > accordingly) Yes. It is really a good idea. I am working on this. http://codereview.chromium.org/7792094/diff/7001/views/touchui/touch_factory.... views/touchui/touch_factory.cc:354: #else On 2011/09/07 14:56:37, sadrul wrote: > ditto Will select touch and mouse events for XI2.1
Updated the patch: 1. Select both touch and mouse events when XI2.1. modify the event dispatching logic to handle touch and mouse respectively. 2. Update comments and coding style issues. Thanks in advance.
Please note: XI2.1 is being released without support for multitouch, which has been pushed back to XI2.2, and should be available in xorg-server-1.12: http://who-t.blogspot.com/2011/09/whats-new-in-xi-21-versioning.html Thus, we probably shouldn't use "xi2_1" as the name of the flag that is determining which version to use. In fact, this suggests we probably shouldn't use a version number at all, but instead a flag that more succinctly summarizes what exactly is the feature that we require. Perhaps something like "use_xi2_mt".
It may be possible to build one client that works with whichever server (Xorg standard or ubuntu experimental) it finds. http://codereview.chromium.org/7792094/diff/12001/base/message_pump_x.cc File base/message_pump_x.cc (left): http://codereview.chromium.org/7792094/diff/12001/base/message_pump_x.cc#oldc... base/message_pump_x.cc:78: } Save the server's reported version (returned in major, minor), and, if it is 2.0, use the (#ifndef USE_XI2_1) way to pass multitouch events. http://codereview.chromium.org/7792094/diff/12001/base/message_pump_x.cc File base/message_pump_x.cc (right): http://codereview.chromium.org/7792094/diff/12001/base/message_pump_x.cc#newc... base/message_pump_x.cc:79: VLOG(1) << "XInput" << major << "." << minor This will actually print the server's supported version, not 2.1: http://www.x.org/releases/X11R7.5/doc/man/man3/XIQueryVersion.3.html http://codereview.chromium.org/7792094/diff/12001/views/touchui/touch_factory.h File views/touchui/touch_factory.h (right): http://codereview.chromium.org/7792094/diff/12001/views/touchui/touch_factory... views/touchui/touch_factory.h:51: // track individual touch. 'Tracking ID' is a unsigned 32-bit value and is an unsigned http://codereview.chromium.org/7792094/diff/12001/views/touchui/touch_factory... views/touchui/touch_factory.h:162: #if !defined(USE_XI2_1) Was there a reason to move this up?
On 2011/09/08 17:58:49, Daniel Kurtz wrote: > Please note: > > XI2.1 is being released without support for multitouch, which has been pushed > back to XI2.2, and should be available in xorg-server-1.12: > http://who-t.blogspot.com/2011/09/whats-new-in-xi-21-versioning.html > > Thus, we probably shouldn't use "xi2_1" as the name of the flag that is > determining which version to use. In fact, this suggests we probably shouldn't > use a version number at all, but instead a flag that more succinctly summarizes > what exactly is the feature that we require. Perhaps something like > "use_xi2_mt". Thanks for the note! I will change the patch to use 'use_xi2_mt' instead of using specific minor version.
Updated the patch based on comment #8 and #9. Thanks. http://codereview.chromium.org/7792094/diff/12001/base/message_pump_x.cc File base/message_pump_x.cc (left): http://codereview.chromium.org/7792094/diff/12001/base/message_pump_x.cc#oldc... base/message_pump_x.cc:78: } On 2011/09/08 23:09:58, Daniel Kurtz wrote: > Save the server's reported version (returned in major, minor), and, if it is > 2.0, use the (#ifndef USE_XI2_1) way to pass multitouch events. Thanks for the suggestion. Since ubuntu supports multitouch in its XI2.1 and Xorg will support multitouch in XI2.2, we properly cannot use minor version to detect MT support simplely. So I propose to still use 'use_xi2_mt' gyp switch to turn on the XI2 MT support when on ubuntu 11.04 or later xorg XI2.2. http://codereview.chromium.org/7792094/diff/12001/base/message_pump_x.cc File base/message_pump_x.cc (right): http://codereview.chromium.org/7792094/diff/12001/base/message_pump_x.cc#newc... base/message_pump_x.cc:79: VLOG(1) << "XInput" << major << "." << minor On 2011/09/08 23:09:58, Daniel Kurtz wrote: > This will actually print the server's supported version, not 2.1: > > http://www.x.org/releases/X11R7.5/doc/man/man3/XIQueryVersion.3.html Thanks. I will revert this change. http://codereview.chromium.org/7792094/diff/12001/views/touchui/touch_factory.h File views/touchui/touch_factory.h (right): http://codereview.chromium.org/7792094/diff/12001/views/touchui/touch_factory... views/touchui/touch_factory.h:51: // track individual touch. 'Tracking ID' is a unsigned 32-bit value and On 2011/09/08 23:09:58, Daniel Kurtz wrote: > is an unsigned Thanks. Will correct. http://codereview.chromium.org/7792094/diff/12001/views/touchui/touch_factory... views/touchui/touch_factory.h:162: #if !defined(USE_XI2_1) On 2011/09/08 23:09:58, Daniel Kurtz wrote: > Was there a reason to move this up? The reason is member initialization sequence when #if defined(USE_XI2_MT). I will correct this and change in initializaer.
Also, for this to work on Ubuntu, do you still need to register for major=2, minor=1 using XIQueryVersion() in base/message_pump_x.cc? http://codereview.chromium.org/7792094/diff/17001/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7792094/diff/17001/views/touchui/touch_factory... views/touchui/touch_factory.cc:228: if (devices) { This isn't needed. On error, devices = NULL, but count = -1, so the for loop will have no iterations, and devices is never accessed. This is the same logic as above. http://codereview.chromium.org/7792094/diff/17001/views/touchui/touch_factory... views/touchui/touch_factory.cc:232: XIDeviceInfo* devinfo = devices + i; For consistency, assign devinfo first, then do: XIDeviceInfo* devinfo = devices + i; if (!devinfo->enabled) continue; Also, you might consider implementing this 'enabled' check in its own patch; it seems like a generally good thing that is independent or the XI2_MT logic. http://codereview.chromium.org/7792094/diff/17001/views/touchui/touch_factory... views/touchui/touch_factory.cc:237: XITouchClassInfo* tci = (XITouchClassInfo *)xiclassinfo; reinterpret_cast<XITouchClassInfo*>xiclassinfo;
On 2011/09/09 14:26:18, Daniel Kurtz wrote: > Also, for this to work on Ubuntu, do you still need to register for major=2, > minor=1 using XIQueryVersion() in base/message_pump_x.cc? > Thanks. I will still use major=2, minor=1 in USE_XI2_MT code path. It works on ubuntu 11.04.
http://codereview.chromium.org/7792094/diff/17001/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7792094/diff/17001/views/touchui/touch_factory... views/touchui/touch_factory.cc:228: if (devices) { On 2011/09/09 14:26:18, Daniel Kurtz wrote: > This isn't needed. On error, devices = NULL, but count = -1, so the for loop > will have no iterations, and devices is never accessed. This is the same logic > as above. Thanks for the comments. I will correct this. http://codereview.chromium.org/7792094/diff/17001/views/touchui/touch_factory... views/touchui/touch_factory.cc:232: XIDeviceInfo* devinfo = devices + i; On 2011/09/09 14:26:18, Daniel Kurtz wrote: > For consistency, assign devinfo first, then do: > > XIDeviceInfo* devinfo = devices + i; > if (!devinfo->enabled) > continue; > > Also, you might consider implementing this 'enabled' check in its own patch; it > seems like a generally good thing that is independent or the XI2_MT logic. OK. I will follow up with a separated patch for this. http://codereview.chromium.org/7792094/diff/17001/views/touchui/touch_factory... views/touchui/touch_factory.cc:237: XITouchClassInfo* tci = (XITouchClassInfo *)xiclassinfo; On 2011/09/09 14:26:18, Daniel Kurtz wrote: > reinterpret_cast<XITouchClassInfo*>xiclassinfo; I will correct this.
Updated patch set according to comment #12.
Looking very good. A few minor style/comment issues. http://codereview.chromium.org/7792094/diff/22001/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7792094/diff/22001/views/events/event_x.cc#new... views/events/event_x.cc:127: TouchFactory::TouchParam tp = TouchFactory::TP_TRACKING_ID; Please add a TODO for fixing up the touch-id (i.e. make it always start from 0 for a new touch-sequence) when TRACKING_ID is used to extract the touch-id. http://codereview.chromium.org/7792094/diff/22001/views/focus/accelerator_han... File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/7792094/diff/22001/views/focus/accelerator_han... views/focus/accelerator_handler_touch.cc:68: } I think you should just #endif here, and let the existing code for Mouse events happen as usual. http://codereview.chromium.org/7792094/diff/22001/views/focus/accelerator_han... views/focus/accelerator_handler_touch.cc:122: // let it fall through so it can be used as a MouseEvent, if desired. Thanks! http://codereview.chromium.org/7792094/diff/22001/views/focus/accelerator_han... views/focus/accelerator_handler_touch.cc:146: #endif // defined(USE_XI2_MT) Two-blank spaces before '//' comments.
Thanks for the comments. I will update the patch set accordingly. http://codereview.chromium.org/7792094/diff/22001/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7792094/diff/22001/views/events/event_x.cc#new... views/events/event_x.cc:127: TouchFactory::TouchParam tp = TouchFactory::TP_TRACKING_ID; On 2011/09/12 16:20:38, sadrul wrote: > Please add a TODO for fixing up the touch-id (i.e. make it always start from 0 > for a new touch-sequence) when TRACKING_ID is used to extract the touch-id. Will add the TODO and follow up with a patch. Thanks. http://codereview.chromium.org/7792094/diff/22001/views/focus/accelerator_han... File views/focus/accelerator_handler_touch.cc (right): http://codereview.chromium.org/7792094/diff/22001/views/focus/accelerator_han... views/focus/accelerator_handler_touch.cc:68: } On 2011/09/12 16:20:38, sadrul wrote: > I think you should just #endif here, and let the existing code for Mouse events > happen as usual. It's a good idea. Thanks for the comment. I used to avoid to generate touch event from a mouse event if it is from a touch device. But the mouse event from a touch device is already filtered out by TouchFactory::ShouldProcessXI2Event(XEvent* xev). So it is OK to let mouse event go through usual logic. http://codereview.chromium.org/7792094/diff/22001/views/focus/accelerator_han... views/focus/accelerator_handler_touch.cc:122: // let it fall through so it can be used as a MouseEvent, if desired. On 2011/09/12 16:20:38, sadrul wrote: > Thanks! My pleasure. http://codereview.chromium.org/7792094/diff/22001/views/focus/accelerator_han... views/focus/accelerator_handler_touch.cc:146: #endif // defined(USE_XI2_MT) On 2011/09/12 16:20:38, sadrul wrote: > Two-blank spaces before '//' comments. Thanks. Will correct it.
patch set is updated according to comment #16, could you please review?
Please review. Thanks.
LGTM! Please wait for an LGTM from Daniel (and let me know if you need help with landing the CL) http://codereview.chromium.org/7792094/diff/7010/base/message_pump_x.cc File base/message_pump_x.cc (right): http://codereview.chromium.org/7792094/diff/7010/base/message_pump_x.cc#newco... base/message_pump_x.cc:74: int major = 2, minor = 1; Given that the official XInput2.1 will not have the MT support, I wonder if we should use 2.2 here, and Ubuntu (and/or other distros) carries a patch that enables it in 2.1, or if we should just use 2.1 as it is in this CL, and give it a second thought if/when we need to. Daniel? http://codereview.chromium.org/7792094/diff/7010/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7792094/diff/7010/views/events/event_x.cc#newc... views/events/event_x.cc:121: #endif #endif // defined(USE_XI2_MT)
Almost! http://codereview.chromium.org/7792094/diff/7010/base/message_pump_x.cc File base/message_pump_x.cc (right): http://codereview.chromium.org/7792094/diff/7010/base/message_pump_x.cc#newco... base/message_pump_x.cc:74: int major = 2, minor = 1; I agree with sadrul. Please make 2.2 by default. Perhaps we could pass in the minimum minor version supported via a build-time flag... like the value of USE_XI2_MT_MINOR? http://codereview.chromium.org/7792094/diff/7010/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/7792094/diff/7010/build/common.gypi#newcode808 build/common.gypi:808: 'defines': ['USE_XI2_MT=1'], Here, could you set USE_XI2_MT equal to the required XI2 minor minimum version? Is there any precedence for non-boolean define values? http://codereview.chromium.org/7792094/diff/7010/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7792094/diff/7010/views/touchui/touch_factory.... views/touchui/touch_factory.cc:458: // With XInput 2.1, Tracking ID could be provided in the detail field XInput 2.2 "Could be provided", or "is provided"? Protect this with USE_XI2_MT define block, since it is incorrect for current XI2 behavior.
sadrul, Daniel, Thanks for your comments. Will update the patch set according to them. http://codereview.chromium.org/7792094/diff/7010/base/message_pump_x.cc File base/message_pump_x.cc (right): http://codereview.chromium.org/7792094/diff/7010/base/message_pump_x.cc#newco... base/message_pump_x.cc:74: int major = 2, minor = 1; On 2011/09/20 05:12:27, Daniel Kurtz wrote: > I agree with sadrul. Please make 2.2 by default. > Perhaps we could pass in the minimum minor version supported via a build-time > flag... like the value of USE_XI2_MT_MINOR? Thanks for comments. Will use USE_XI2_MT as minimum required XI2 minor version. http://codereview.chromium.org/7792094/diff/7010/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/7792094/diff/7010/build/common.gypi#newcode808 build/common.gypi:808: 'defines': ['USE_XI2_MT=1'], On 2011/09/20 05:12:27, Daniel Kurtz wrote: > Here, could you set USE_XI2_MT equal to the required XI2 minor minimum version? > Is there any precedence for non-boolean define values? Good idea! Will implement it. http://codereview.chromium.org/7792094/diff/7010/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7792094/diff/7010/views/events/event_x.cc#newc... views/events/event_x.cc:121: #endif On 2011/09/19 15:58:08, sadrul wrote: > #endif // defined(USE_XI2_MT) Will add. Thanks. http://codereview.chromium.org/7792094/diff/7010/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7792094/diff/7010/views/touchui/touch_factory.... views/touchui/touch_factory.cc:458: // With XInput 2.1, Tracking ID could be provided in the detail field On 2011/09/20 05:12:27, Daniel Kurtz wrote: > XInput 2.2 > > "Could be provided", or "is provided"? > > Protect this with USE_XI2_MT define block, since it is incorrect for current XI2 > behavior. Thanks for correction. Will fix it.
Patch set is updated according to comment #20 and #21. Please review. Thanks.
Daniel, could you please review the updated patch set. Thanks.
On 2011/09/22 14:16:42, ningxin.hu wrote: > Daniel, could you please review the updated patch set. Thanks. It looks like canonical themselves are working on a rearchitecture of uTouch: https://lists.launchpad.net/multi-touch-dev/msg00932.html They are moving the stack out of the X server and making it a client of X, which depends on the forthcoming XInput multitouch support slated for X.org server 1.12, hopefully some time early 2012. How does this rearchitecture affect this patch?
This patch doesn't have anything related to uTouch which is a gesture framework. The patch only depends on XI2 multitouch support, which is currently in experimental XI2.1 in ubuntu 11.04 and upcoming XI2.2 in upstream X.org. Chromium only needs the touch event and recognizes to gesture itself, it doesn't depends on uTouch or toolkits. So the reachitecture of uTouch doesn't affect this patch. Thanks, - Ning-Xin On Fri, Sep 23, 2011 at 11:23 AM, <djkurtz@chromium.org> wrote: > On 2011/09/22 14:16:42, ningxin.hu wrote: > >> Daniel, could you please review the updated patch set. Thanks. >> > > It looks like canonical themselves are working on a rearchitecture of > uTouch: > https://lists.launchpad.net/**multi-touch-dev/msg00932.html<https://lists.lau... > > They are moving the stack out of the X server and making it a client of X, > which > depends on the forthcoming XInput multitouch support slated for X.org > server > 1.12, hopefully some time early 2012. > > How does this rearchitecture affect this patch? > > > http://codereview.chromium.**org/7792094/<http://codereview.chromium.org/7792... >
LGTM w/ version nit http://codereview.chromium.org/7792094/diff/38001/base/message_pump_x.cc File base/message_pump_x.cc (right): http://codereview.chromium.org/7792094/diff/38001/base/message_pump_x.cc#newc... base/message_pump_x.cc:87: << "But " << major << "." << USE_XI2_MT << " is required."; nit, (major,minor) is server's version, but required is 2.USE_XI2_MT: if (major < 2 || minor < USE_XI2_MT) { .. << "But 2." << USE_XI2_MT << " is required.";
Thanks for the review. http://codereview.chromium.org/7792094/diff/38001/base/message_pump_x.cc File base/message_pump_x.cc (right): http://codereview.chromium.org/7792094/diff/38001/base/message_pump_x.cc#newc... base/message_pump_x.cc:87: << "But " << major << "." << USE_XI2_MT << " is required."; On 2011/09/23 07:36:59, Daniel Kurtz wrote: > nit, (major,minor) is server's version, but required is 2.USE_XI2_MT: > > if (major < 2 || minor < USE_XI2_MT) { > .. > << "But 2." << USE_XI2_MT << " is required."; Sorry for this mistake. Will correct it. Thanks.
Patch is updated according to comment #27. If it is OK, please help land the patch. Thanks in advance.
BTW, the followup CL http://codereview.chromium.org/7927001/ is updated also. Could you please review and make the gesture work on XI MT support. Thanks.
On 2011/09/23 14:43:04, ningxin.hu wrote: > Patch is updated according to comment #27. > > If it is OK, please help land the patch. > > Thanks in advance. I see that you are already in AUTHORS (Ningxin Hu <ningxin.hu@intel.com>, right?). Once you confirm, I will land this for you. In the meantime, sending this to the trybots.
On 2011/09/23 19:03:20, sadrul wrote: > On 2011/09/23 14:43:04, ningxin.hu wrote: > > Patch is updated according to comment #27. > > > > If it is OK, please help land the patch. > > > > Thanks in advance. > > I see that you are already in AUTHORS (Ningxin Hu <mailto:ningxin.hu@intel.com>, > right?). Once you confirm, I will land this for you. In the meantime, sending > this to the trybots. I confirm. Thanks.
Small detail. http://codereview.chromium.org/7792094/diff/44002/base/message_pump_x.cc File base/message_pump_x.cc (right): http://codereview.chromium.org/7792094/diff/44002/base/message_pump_x.cc#newc... base/message_pump_x.cc:85: if (major < 2 || minor < USE_XI2_MT) { Sorry, this is my fault. This isn't quite right. It should be: if (major < 2 || (major == 2 && minor < USE_XI2_MT)) {
On 2011/09/24 00:29:10, Daniel Kurtz wrote: > Small detail. > > http://codereview.chromium.org/7792094/diff/44002/base/message_pump_x.cc > File base/message_pump_x.cc (right): > > http://codereview.chromium.org/7792094/diff/44002/base/message_pump_x.cc#newc... > base/message_pump_x.cc:85: if (major < 2 || minor < USE_XI2_MT) { > Sorry, this is my fault. This isn't quite right. It should be: > > if (major < 2 || (major == 2 && minor < USE_XI2_MT)) { Yes. I will merge the correction. Thanks
On 2011/09/24 00:29:10, Daniel Kurtz wrote: > Small detail. > > http://codereview.chromium.org/7792094/diff/44002/base/message_pump_x.cc > File base/message_pump_x.cc (right): > > http://codereview.chromium.org/7792094/diff/44002/base/message_pump_x.cc#newc... > base/message_pump_x.cc:85: if (major < 2 || minor < USE_XI2_MT) { > Sorry, this is my fault. This isn't quite right. It should be: > > if (major < 2 || (major == 2 && minor < USE_XI2_MT)) { Yes. I will merge the correction. Thanks
The build error is caused the patch is not rebased on trunk. I try to rebase and upload, but the latest patch set seems include diff in trunk. How could I do to resolve this issue? Thanks.
On 2011/09/24 01:03:32, ningxin.hu wrote: > The build error is caused the patch is not rebased on trunk. I try to rebase and > upload, but the latest patch set seems include diff in trunk. How could I do to > resolve this issue? Thanks. Sorry for this question. I found after that rebase, the patch set against to trunk is OK.
Hi Daniel, Patch is updated to your comment #33. Please review. If possible, please help land the patch. Thanks.
LGTM!
LGTM, but could you actually add a bit more to the commit message? Like: how you build and test this change, what X version you are testing against, and a bit more about what this change actually does.
Update the commit comments. Please review. Thanks.
Beautiful. LGTM. Thank you Ningxin, for your effort and patience!
On 2011/09/24 03:34:34, Daniel Kurtz wrote: > Beautiful. LGTM. > Thank you Ningxin, for your effort and patience! Daniel and sadrul, thanks a lot for your review too! I will update the followup CL http://codereview.chromium.org/7927001/ based on your comments ASAP.
CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/ningxin.hu@gmail.com/7792094/52009
Presubmit check for 7792094-52009 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** ningxin.hu@gmail.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. ** Presubmit ERRORS ** Missing LGTM from an OWNER for: base/message_pump_x.cc Presubmit checks took 2.3s to calculate. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
