Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7927001:
touchui: Convert XI2 MT tracking id to slot id for gesture recognizer (Closed)
|
Created:
9 years, 3 months ago by ningxin.hu Modified:
9 years, 2 months ago CC:
chromium-reviews, dhollowa Base URL:
nhu@powerbuilder.sh.intel.com:/home/www-data/git-repos/chromium/chromium.git@trunk Visibility:
Public. |
DescriptionIt is a followup CL for http://codereview.chromium.org/7792094/.
BUG=95150
TEST=Manually touch-drag to scroll web page and tap to click link
Patch Set 1 #
Total comments: 14
Patch Set 2 : Update the patch set according to comment #2. #
Total comments: 10
Patch Set 3 : Update patch according to cooment #6 & #8 #Patch Set 4 : Fix some typos. #
Total comments: 2
Patch Set 5 : update according to comment #13 #Patch Set 6 : rebase to trunk #
Messages
Total messages: 22 (0 generated)
the follow-up CL for review 7792094 comment #4 (http://codereview.chromium.org/7792094/#msg4). Please review. Thanks.
Thanks! This is looking good. Left a few comments. http://codereview.chromium.org/7927001/diff/1/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7927001/diff/1/views/events/event_x.cc#newcode130 views/events/event_x.cc:130: *xev, TouchFactory::TP_TRACKING_ID, &tracking_id)) 4-space indent here and in line 137 http://codereview.chromium.org/7927001/diff/1/views/events/event_x.cc#newcode433 views/events/event_x.cc:433: TouchFactory* factory = TouchFactory::GetInstance(); Please add a NOTE here explaining that for PRESSED events, the touch-id gets allocated by TouchFactory. http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.cc#... views/touchui/touch_factory.cc:134: slots_used_(), slots_used_ is used even when USE_XI2_MT is not set, right? http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.cc#... views/touchui/touch_factory.cc:330: int slot = ffs(~(slots_used_.to_ulong())) - 1; I think we can spare a few cycles here and do a loop to find an available slot instead! http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.cc#... views/touchui/touch_factory.cc:345: } Add a NOTREACHED or DCHECK on else http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.h File views/touchui/touch_factory.h (right): http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.h#n... views/touchui/touch_factory.h:83: int AllocateSlotIdForTrackingId(uint32 tracking_id); Let's call this GetSlotIdForTracking, and update the doc to explain that this tries to find an existing slot-id first, and allocates a new one when there isn't one already. http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.h#n... views/touchui/touch_factory.h:85: // Release the slot id according to tracking id. Releases
Thanks for review. http://codereview.chromium.org/7927001/diff/1/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7927001/diff/1/views/events/event_x.cc#newcode130 views/events/event_x.cc:130: *xev, TouchFactory::TP_TRACKING_ID, &tracking_id)) On 2011/09/19 16:13:36, sadrul wrote: > 4-space indent here and in line 137 Will fix. Thanks. http://codereview.chromium.org/7927001/diff/1/views/events/event_x.cc#newcode433 views/events/event_x.cc:433: TouchFactory* factory = TouchFactory::GetInstance(); On 2011/09/19 16:13:36, sadrul wrote: > Please add a NOTE here explaining that for PRESSED events, the touch-id gets > allocated by TouchFactory. Will do that. Thanks. http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.cc#... views/touchui/touch_factory.cc:134: slots_used_(), On 2011/09/19 16:13:36, sadrul wrote: > slots_used_ is used even when USE_XI2_MT is not set, right? Yes. http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.cc#... views/touchui/touch_factory.cc:330: int slot = ffs(~(slots_used_.to_ulong())) - 1; On 2011/09/19 16:13:36, sadrul wrote: > I think we can spare a few cycles here and do a loop to find an available slot > instead! OK. Will implement it. http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.cc#... views/touchui/touch_factory.cc:345: } On 2011/09/19 16:13:36, sadrul wrote: > Add a NOTREACHED or DCHECK on else Will add. Thanks for comment. http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.h File views/touchui/touch_factory.h (right): http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.h#n... views/touchui/touch_factory.h:83: int AllocateSlotIdForTrackingId(uint32 tracking_id); On 2011/09/19 16:13:36, sadrul wrote: > Let's call this GetSlotIdForTracking, and update the doc to explain that this > tries to find an existing slot-id first, and allocates a new one when there > isn't one already. Thanks for the comment. Will do that. http://codereview.chromium.org/7927001/diff/1/views/touchui/touch_factory.h#n... views/touchui/touch_factory.h:85: // Release the slot id according to tracking id. On 2011/09/19 16:13:36, sadrul wrote: > Releases Will correct. Thanks.
Update the patch set according to comment #2. Please review. Thanks.
On 2011/09/20 16:09:35, ningxin.hu wrote: > Update the patch set according to comment #2. Please review. Thanks. Please review the updated patch set. Thanks.
I think this patch works, but the variable/function naming is a little confused between slotID and touchID. I believe (but please correct me if I'm misguided) that for XI2_MT case, there are no slot ids, just touch ids. Contacts have monotonically assigned tracking IDs as the leave X. Touch IDs are assigned here in Chromium such that the first touch of a multi-touch event stream is assigned ID=0, subsequent touches are given the lowest available touch ID (up to a max allowed simultaneous touches). This is similar to how, in the pre-XI2_MT case, SLOT_ID is assigned by X. To reduce confusion, would it be possible to use "touchID" consistently in the XI_MT case instead of reusing slotID? Also, why is this a separate CL? I'm just curious why this isn't merged with 7792094? http://codereview.chromium.org/7927001/diff/6001/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7927001/diff/6001/views/events/event_x.cc#newc... views/events/event_x.cc:125: float slot = 0; id is a better name, since it is used for both touchID and slotID http://codereview.chromium.org/7927001/diff/6001/views/events/event_x.cc#newc... views/events/event_x.cc:128: TouchFactory* factory = TouchFactory::GetInstance(); Pull "TouchFactory*" and "return slot" out of the #ifdef block, and for consistency, make the if() logic consistent. http://codereview.chromium.org/7927001/diff/6001/views/events/event_x.cc#newc... views/events/event_x.cc:138: LOG(ERROR) << "Could not get the touch ID for the event. Using 0."; Could not get the slot ID ... http://codereview.chromium.org/7927001/diff/6001/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7927001/diff/6001/views/touchui/touch_factory.... views/touchui/touch_factory.cc:330: int slot = 0; If you store minSlotAvailable as a member variable, you could save yourself some iterating instead of always starting from 0: minSlotAvailable starts as 0. As fingers are added, minSlotAvailable takes the value of the last assigned slot + 1 (actually, iterate from assigned slot up until IsSlotUsed() == false). If a finger is removed, and its slot < minSlotAvailable, then it becomes the new minSlotAvailable.
For the commit message: Make this the first line: Convert XI2 MT tracking id to slot id for gesture recognizer And in the body add a note that it is a follow up to http://codereview.chromium.org/7792094/
[snip] > To reduce confusion, would it be possible to use "touchID" consistently in the > XI_MT case instead of reusing slotID? I have explained this in the comment. I think 'slot id' is fine. > > Also, why is this a separate CL? I'm just curious why this isn't merged with > 7792094? I suggested that he starts working on this while we review the other CL. http://codereview.chromium.org/7927001/diff/6001/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7927001/diff/6001/views/events/event_x.cc#newc... views/events/event_x.cc:125: float slot = 0; On 2011/09/24 01:53:36, Daniel Kurtz wrote: > id is a better name, since it is used for both touchID and slotID I think 'slot' is fine. Right now, 'slot id' is essentially the same as 'touch id'. For _MT, the we compute the 'slot id' from the 'tracking id'. For non-MT, we already have the 'slot id' from X. Ideally, we would always compute the 'touch id' from the 'tracking id'. Once the code around this settles (there are refactoring happening in this space), I will try to consolidate this code to determine the touch-id (this should allow us to get rid of one of the cros patches we carry). http://codereview.chromium.org/7927001/diff/6001/views/events/event_x.cc#newc... views/events/event_x.cc:138: LOG(ERROR) << "Could not get the touch ID for the event. Using 0."; On 2011/09/24 01:53:36, Daniel Kurtz wrote: > Could not get the slot ID ... Ditto. http://codereview.chromium.org/7927001/diff/6001/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7927001/diff/6001/views/touchui/touch_factory.... views/touchui/touch_factory.cc:330: int slot = 0; On 2011/09/24 01:53:36, Daniel Kurtz wrote: > If you store minSlotAvailable as a member variable, you could save yourself some > iterating instead of always starting from 0: > > minSlotAvailable starts as 0. > As fingers are added, minSlotAvailable takes the value of the last assigned slot > + 1 (actually, iterate from assigned slot up until IsSlotUsed() == false). > If a finger is removed, and its slot < minSlotAvailable, then it becomes the new > minSlotAvailable. This sounds like a good proposal; although I don't expect this to make a lot of perf improvement.
On 2011/09/24 02:00:22, Daniel Kurtz wrote: > For the commit message: > > Make this the first line: > Convert XI2 MT tracking id to slot id for gesture recognizer > > And in the body add a note that it is a follow up to > http://codereview.chromium.org/7792094/ Thanks. I will update the commit comment.
Sadurl & Daniel, Thanks for your review. I will update the patch according to your comments. http://codereview.chromium.org/7927001/diff/6001/views/events/event_x.cc File views/events/event_x.cc (right): http://codereview.chromium.org/7927001/diff/6001/views/events/event_x.cc#newc... views/events/event_x.cc:128: TouchFactory* factory = TouchFactory::GetInstance(); On 2011/09/24 01:53:36, Daniel Kurtz wrote: > Pull "TouchFactory*" and "return slot" out of the #ifdef block, and for > consistency, make the if() logic consistent. Thanks. I will follow up. http://codereview.chromium.org/7927001/diff/6001/views/events/event_x.cc#newc... views/events/event_x.cc:138: LOG(ERROR) << "Could not get the touch ID for the event. Using 0."; On 2011/09/24 03:06:31, sadrul wrote: > On 2011/09/24 01:53:36, Daniel Kurtz wrote: > > Could not get the slot ID ... > > Ditto. Thanks. Will correct. http://codereview.chromium.org/7927001/diff/6001/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7927001/diff/6001/views/touchui/touch_factory.... views/touchui/touch_factory.cc:330: int slot = 0; On 2011/09/24 01:53:36, Daniel Kurtz wrote: > If you store minSlotAvailable as a member variable, you could save yourself some > iterating instead of always starting from 0: > > minSlotAvailable starts as 0. > As fingers are added, minSlotAvailable takes the value of the last assigned slot > + 1 (actually, iterate from assigned slot up until IsSlotUsed() == false). > If a finger is removed, and its slot < minSlotAvailable, then it becomes the new > minSlotAvailable. Good idea! I will follow up the solution. Thanks.
Update the patch set: 1. rebase 2. update according to comment #6 & 8. Please review. Thanks.
Update the commit title and comment. Thanks.
nit http://codereview.chromium.org/7927001/diff/18005/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7927001/diff/18005/views/touchui/touch_factory... views/touchui/touch_factory.cc:346: // Updates the minium available slot ID while (++min_available_slot_ < kMaxTouchPoints && IsSlotUsed(min_available_slot_)) continue;
Hi Daniel, Thanks for your comments. I will update the patch accordingly. http://codereview.chromium.org/7927001/diff/18005/views/touchui/touch_factory.cc File views/touchui/touch_factory.cc (right): http://codereview.chromium.org/7927001/diff/18005/views/touchui/touch_factory... views/touchui/touch_factory.cc:346: // Updates the minium available slot ID On 2011/09/26 08:29:17, Daniel Kurtz wrote: > while (++min_available_slot_ < kMaxTouchPoints && > IsSlotUsed(min_available_slot_)) > continue; Good! I will merge the code. Thanks.
Patch is updated according to comment #13. Please review.
On 2011/09/26 15:48:20, ningxin.hu wrote: > Patch is updated according to comment #13. Please review. I will review shortly. It is likely that you will have to merge/rebase once http://codereview.chromium.org/7942004/ lands shortly (that CL moves touch_factory.h|cc into ui/, and refactors event_x.cc); but I think the merge will not be very painful.
LGTM
On 2011/09/26 15:53:06, sadrul wrote: > On 2011/09/26 15:48:20, ningxin.hu wrote: > > Patch is updated according to comment #13. Please review. > > I will review shortly. It is likely that you will have to merge/rebase once > http://codereview.chromium.org/7942004/ lands shortly (that CL moves > touch_factory.h|cc into ui/, and refactors event_x.cc); but I think the merge > will not be very painful. Thanks. I will rebase the patch. And I found CL http://codereview.chromium.org/7942004/ has build error for XI2_MT, I will submit a CL for that issue.
Hi sadurl, The patch is rebased against trunk. Please review. Thanks.
LGTM. I will land this.
On 2011/09/28 14:36:52, sadrul wrote: > LGTM. I will land this. Moved over into http://codereview.chromium.org/8070003/ and sent to tryjobs after merging with ToT. Did a couple of style nits. So closing this now.
On 2011/09/28 15:07:22, sadrul wrote: > On 2011/09/28 14:36:52, sadrul wrote: > > LGTM. I will land this. > > Moved over into http://codereview.chromium.org/8070003/ and sent to tryjobs > after merging with ToT. Did a couple of style nits. So closing this now. Thanks for this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
