Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552343008:
Add slave touch device to touch_device_lookup_ and touch_device_list_ (Closed)
DescriptionAdd slave touch device to touch_device_lookup_ and touch_device_list_
Unexpected MouseMove event when touch the touch screen issue is because we
didn't add slave touch device to touch_device_lookup_ and touch_device_list_
then we got false when we check if it is from touch screen by sourceid(not
device id).
This patch we add slave touch device to touch_device_lookup_ and
touch_device_list_.
BUG=678074
Review-Url: https://codereview.chromium.org/2552343008
Cr-Commit-Position: refs/heads/master@{#456488}
Committed: https://chromium.googlesource.com/chromium/src/+/1d64628a6b9a59469503b6a82a68606a09950d9a
Patch Set 1 #
Total comments: 1
Patch Set 2 : add slave touch screen to list and lookup #
Total comments: 2
Patch Set 3 : sadrul's comments addressed #
Total comments: 2
Patch Set 4 : set touch_device_list_[device] false if it is a slave device #
Total comments: 1
Patch Set 5 : remove cache list #Patch Set 6 : add cache list back #
Total comments: 2
Patch Set 7 : sadrul's comment addressed. #
Total comments: 1
Patch Set 8 : fix a ) #Messages
Total messages: 45 (26 generated)
chaopeng@chromium.org changed reviewers: + sadrul@chromium.org
PTAL
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sadrul@chromium.org changed reviewers: + lanwei@chromium.org
+lanwei@ mind doing an initial review? Thanks! https://codereview.chromium.org/2552343008/diff/1/ui/events/devices/x11/touch... File ui/events/devices/x11/touch_factory_x11.cc (right): https://codereview.chromium.org/2552343008/diff/1/ui/events/devices/x11/touch... ui/events/devices/x11/touch_factory_x11.cc:244: return deviceid >= 0; Should we check size of |touch_device_list_| instead?
On 2016/12/08 17:53:16, sadrul wrote: > +lanwei@ mind doing an initial review? Thanks! > > https://codereview.chromium.org/2552343008/diff/1/ui/events/devices/x11/touch... > File ui/events/devices/x11/touch_factory_x11.cc (right): > > https://codereview.chromium.org/2552343008/diff/1/ui/events/devices/x11/touch... > ui/events/devices/x11/touch_factory_x11.cc:244: return deviceid >= 0; > Should we check size of |touch_device_list_| instead? No, in this case: ⎡ Virtual core pointer id=2 [master pointer (3)] ⎜ ↳ Virtual core XTEST pointer id=4 [slave pointer (2)] ⎜ ↳ Logitech USB-PS/2 Optical Mouse id=11 [slave pointer (2)] ⎜ ↳ Acer T231H id=10 [slave pointer (2)] ⎣ Virtual core keyboard id=3 [master keyboard (2)] ↳ Virtual core XTEST keyboard id=5 [slave keyboard (3)] ↳ Power Button id=6 [slave keyboard (3)] ↳ Power Button id=7 [slave keyboard (3)] ↳ USB Keyboard id=8 [slave keyboard (3)] ↳ USB Keyboard id=9 [slave keyboard (3)] ↳ HP WMI hotkeys id=12 [slave keyboard (3)] I have 1 touch screen id=10, touch device id can be 2 and 10. touch_device_list_ size is 2. So touch_device_list_ size can not validate the device id.
Description was changed from ========== Unify touch device container in touch_factory_x11 This patch remove touch_device_lookup_ and keep touch_device_list_ to store touch device. It also fix a bug that touch device missing. BUG=NONE ========== to ========== Add slave touch device to touch_device_lookup_ and touch_device_list_ The touch device missing issue is because we didn't add slave touch device to touch_device_lookup_ and touch_device_list_ but we add |touch_device_list_.operator[]| in TouchFactory::ShouldProcessXI2Event that insert the device id to touch_device_list_. This patch we add slave touch device to touch_device_lookup_ and touch_device_list_. Also we add a DCHECK in TouchFactory::ShouldProcessXI2Event before we call touch_device_list_.operator[]. BUG=NONE ==========
On 2016/12/08 18:47:25, chaopeng wrote: > On 2016/12/08 17:53:16, sadrul wrote: > > +lanwei@ mind doing an initial review? Thanks! > > > > > https://codereview.chromium.org/2552343008/diff/1/ui/events/devices/x11/touch... > > File ui/events/devices/x11/touch_factory_x11.cc (right): > > > > > https://codereview.chromium.org/2552343008/diff/1/ui/events/devices/x11/touch... > > ui/events/devices/x11/touch_factory_x11.cc:244: return deviceid >= 0; > > Should we check size of |touch_device_list_| instead? > > No, in this case: > > ⎡ Virtual core pointer id=2 [master pointer (3)] > ⎜ ↳ Virtual core XTEST pointer id=4 [slave pointer (2)] > ⎜ ↳ Logitech USB-PS/2 Optical Mouse id=11 [slave pointer (2)] > ⎜ ↳ Acer T231H id=10 [slave pointer (2)] > ⎣ Virtual core keyboard id=3 [master keyboard (2)] > ↳ Virtual core XTEST keyboard id=5 [slave keyboard (3)] > ↳ Power Button id=6 [slave keyboard (3)] > ↳ Power Button id=7 [slave keyboard (3)] > ↳ USB Keyboard id=8 [slave keyboard (3)] > ↳ USB Keyboard id=9 [slave keyboard (3)] > ↳ HP WMI hotkeys id=12 [slave keyboard (3)] > > I have 1 touch screen id=10, touch device id can be 2 and 10. touch_device_list_ > size is 2. So touch_device_list_ size can not validate the device id. Updated, PTAL. Thank you.
LGTM! The two lists, touch_device_lookup_ is not a duplicate of touch_device_list_. https://codesearch.chromium.org/chromium/src/ui/events/devices/x11/touch_fact... TouchFactory::SetTouchDeviceList onlys sets the master touch device to true in touch_device_list_, salve is false. Later we can use touch_device_list_ to decide if the device is a master device or slave device. The names in this class are really confusing!
https://codereview.chromium.org/2552343008/diff/20001/ui/events/devices/x11/t... File ui/events/devices/x11/touch_factory_x11.cc (right): https://codereview.chromium.org/2552343008/diff/20001/ui/events/devices/x11/t... ui/events/devices/x11/touch_factory_x11.cc:129: touch_device_list_[devinfo.deviceid] = false; I think this is getting fairly confusing. Can you merge the two loops (this one, and the one in line ~102:111)? Because they are beginning to look really similar. https://codereview.chromium.org/2552343008/diff/20001/ui/events/devices/x11/t... ui/events/devices/x11/touch_factory_x11.cc:134: touch_device_lookup_[devinfo.attachment] = true; Oh hrm. We should really verify that |devinfo.attachment| does not go out of bounds.
Patchset #3 (id:40001) has been deleted
On 2016/12/12 16:02:06, sadrul wrote: > https://codereview.chromium.org/2552343008/diff/20001/ui/events/devices/x11/t... > File ui/events/devices/x11/touch_factory_x11.cc (right): > > https://codereview.chromium.org/2552343008/diff/20001/ui/events/devices/x11/t... > ui/events/devices/x11/touch_factory_x11.cc:129: > touch_device_list_[devinfo.deviceid] = false; > I think this is getting fairly confusing. Can you merge the two loops (this one, > and the one in line ~102:111)? Because they are beginning to look really > similar. > > https://codereview.chromium.org/2552343008/diff/20001/ui/events/devices/x11/t... > ui/events/devices/x11/touch_factory_x11.cc:134: > touch_device_lookup_[devinfo.attachment] = true; > Oh hrm. We should really verify that |devinfo.attachment| does not go out of > bounds. Updated PTAL.
https://codereview.chromium.org/2552343008/diff/60001/ui/events/devices/x11/t... File ui/events/devices/x11/touch_factory_x11.cc (right): https://codereview.chromium.org/2552343008/diff/60001/ui/events/devices/x11/t... ui/events/devices/x11/touch_factory_x11.cc:123: pointer_device_lookup_[devinfo.deviceid] = true; It looks like we should also set pointer_device_lookup_ for slave device. But I am not sure.
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2552343008/diff/60001/ui/events/devices/x11/t... File ui/events/devices/x11/touch_factory_x11.cc (right): https://codereview.chromium.org/2552343008/diff/60001/ui/events/devices/x11/t... ui/events/devices/x11/touch_factory_x11.cc:123: pointer_device_lookup_[devinfo.deviceid] = true; On 2016/12/12 16:35:43, chaopeng wrote: > It looks like we should also set pointer_device_lookup_ for slave device. But I > am not sure. Hm. Are we going to process each events twice after this change? Can you try on http://rbyers.github.io/eventTest.html and see if the page receives two click events for each click-event on the device?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
On 2016/12/12 18:56:11, sadrul wrote: > https://codereview.chromium.org/2552343008/diff/60001/ui/events/devices/x11/t... > File ui/events/devices/x11/touch_factory_x11.cc (right): > > https://codereview.chromium.org/2552343008/diff/60001/ui/events/devices/x11/t... > ui/events/devices/x11/touch_factory_x11.cc:123: > pointer_device_lookup_[devinfo.deviceid] = true; > On 2016/12/12 16:35:43, chaopeng wrote: > > It looks like we should also set pointer_device_lookup_ for slave device. But > I > > am not sure. > > Hm. Are we going to process each events twice after this change? Can you try on > http://rbyers.github.io/eventTest.html and see if the page receives two click > events for each click-event on the device? I changed touch_device_list_to set false if it is slave device. And I tried in http://rbyers.github.io/eventTest.html to make sure we dont received click event twice. PTAL. Thank you.
Can you update the CL description and refer to the bug# in BUG= line? https://codereview.chromium.org/2552343008/diff/80001/ui/events/devices/x11/t... File ui/events/devices/x11/touch_factory_x11.cc (left): https://codereview.chromium.org/2552343008/diff/80001/ui/events/devices/x11/t... ui/events/devices/x11/touch_factory_x11.cc:127: CacheTouchscreenIds(devinfo.deviceid); Looks like we are not caching the ids anymore?
Description was changed from ========== Add slave touch device to touch_device_lookup_ and touch_device_list_ The touch device missing issue is because we didn't add slave touch device to touch_device_lookup_ and touch_device_list_ but we add |touch_device_list_.operator[]| in TouchFactory::ShouldProcessXI2Event that insert the device id to touch_device_list_. This patch we add slave touch device to touch_device_lookup_ and touch_device_list_. Also we add a DCHECK in TouchFactory::ShouldProcessXI2Event before we call touch_device_list_.operator[]. BUG=NONE ========== to ========== Add slave touch device to touch_device_lookup_ and touch_device_list_ Unexpected MouseMove event when touch the touch screen issue is because we didn't add slave touch device to touch_device_lookup_ and touch_device_list_ then we got false when we check if it is from touch screen by sourceid(not device id). This patch we add slave touch device to touch_device_lookup_ and touch_device_list_. BUG=678074 ==========
On 2017/01/03 17:01:51, sadrul wrote: > Can you update the CL description and refer to the bug# in BUG= line? > > https://codereview.chromium.org/2552343008/diff/80001/ui/events/devices/x11/t... > File ui/events/devices/x11/touch_factory_x11.cc (left): > > https://codereview.chromium.org/2552343008/diff/80001/ui/events/devices/x11/t... > ui/events/devices/x11/touch_factory_x11.cc:127: > CacheTouchscreenIds(devinfo.deviceid); > Looks like we are not caching the ids anymore? I updated the description and add the full story to issue. Also I can find any call of touchscreens(the cache) so I delete it. PTAL Thank you.
https://codereview.chromium.org/2552343008/diff/120001/ui/events/devices/x11/... File ui/events/devices/x11/touch_factory_x11.cc (right): https://codereview.chromium.org/2552343008/diff/120001/ui/events/devices/x11/... ui/events/devices/x11/touch_factory_x11.cc:111: touch_device_list_[devinfo.deviceid] = Can you please check what touch_device_list_ really stores, from the comments it seems indicate if the device supports multi-touch, but we also use it as if it is a slave device? Sadrul, do you have any idea how we decide if it is a multi-touch device?
On 2017/01/11 19:40:12, lanwei wrote: > https://codereview.chromium.org/2552343008/diff/120001/ui/events/devices/x11/... > File ui/events/devices/x11/touch_factory_x11.cc (right): > > https://codereview.chromium.org/2552343008/diff/120001/ui/events/devices/x11/... > ui/events/devices/x11/touch_factory_x11.cc:111: > touch_device_list_[devinfo.deviceid] = > Can you please check what touch_device_list_ really stores, from the comments it > seems indicate if the device supports multi-touch, but we also use it as if it > is a slave device? Sadrul, do you have any idea how we decide if it is a > multi-touch device? For my understand, that comment is wrong. touch_device_list_ only store is it a slave device.
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry about sitting on the CL. I had to look at the current code to remember what the various things imply. Separately, it would be nice to rename some of the variables to better explain what they represent (e.g. |touch_device_list_| could really be |multi_touch_capable_|, and so on) https://codereview.chromium.org/2552343008/diff/120001/ui/events/devices/x11/... File ui/events/devices/x11/touch_factory_x11.cc (right): https://codereview.chromium.org/2552343008/diff/120001/ui/events/devices/x11/... ui/events/devices/x11/touch_factory_x11.cc:111: touch_device_list_[devinfo.deviceid] = This is ... confusing :( Let's change this code inside the for-loop like the following: XIAnyClassInfo* xiclassinfo = devinfo.classes[k]; if (xiclassinfo->type == XITouchClass) { XITouchClassInfo* tci = reinterpret_cast<XITouchClassInfo*>(xiclassinfo); // Only care direct touch device (such as touch screen) right now if (tci->mode != XIDirectTouch) continue; int master_id = devinfo.use == XISlavePointer ? devinfo.attachment : devinfo.deviceid; touch_device_lookup_[master_id] = true; touch_device_list_[master_id] = true; if (devinfo.use != XIMasterPointer) CacheTouchscreenIds(devinfo.deviceid); if (devinfo.use == XISlavePointer) device_master_id_list_[devinfo.deviceid] = master_id; } This should be equivalent, right? I feel like this is cleaner.
On 2017/03/10 18:52:01, sadrul wrote: > Sorry about sitting on the CL. I had to look at the current code to remember > what the various things imply. > > Separately, it would be nice to rename some of the variables to better explain > what they represent (e.g. |touch_device_list_| could really be > |multi_touch_capable_|, and so on) > > https://codereview.chromium.org/2552343008/diff/120001/ui/events/devices/x11/... > File ui/events/devices/x11/touch_factory_x11.cc (right): > > https://codereview.chromium.org/2552343008/diff/120001/ui/events/devices/x11/... > ui/events/devices/x11/touch_factory_x11.cc:111: > touch_device_list_[devinfo.deviceid] = > This is ... confusing :( > > Let's change this code inside the for-loop like the following: > > XIAnyClassInfo* xiclassinfo = devinfo.classes[k]; > if (xiclassinfo->type == XITouchClass) { > XITouchClassInfo* tci = > reinterpret_cast<XITouchClassInfo*>(xiclassinfo); > // Only care direct touch device (such as touch screen) right now > if (tci->mode != XIDirectTouch) > continue; > > int master_id = devinfo.use == XISlavePointer ? devinfo.attachment : > devinfo.deviceid; > touch_device_lookup_[master_id] = true; > touch_device_list_[master_id] = true; > > if (devinfo.use != XIMasterPointer) > CacheTouchscreenIds(devinfo.deviceid); > > if (devinfo.use == XISlavePointer) > device_master_id_list_[devinfo.deviceid] = master_id; > } > > > This should be equivalent, right? I feel like this is cleaner. Updated, PTAL. 1. The comment for this touch_device_list_ is wrong, it does not represent multi touch capable. I am agree we should rename and change comments for this file. 2. We still need to put slave device id to touch_device_lookup_ and touch_device_list_ in `if (devinfo.use == XISlavePointer)` statement.
> 1. The comment for this touch_device_list_ is wrong, it does not represent multi > touch capable. I am agree we should rename and change comments for this file. I will leave it to you to find a better name for it. Let's do the rename in a follow-up CL though. > 2. We still need to put slave device id to touch_device_lookup_ and > touch_device_list_ in `if (devinfo.use == XISlavePointer)` statement. lgtm https://codereview.chromium.org/2552343008/diff/140001/ui/events/devices/x11/... File ui/events/devices/x11/touch_factory_x11.cc (right): https://codereview.chromium.org/2552343008/diff/140001/ui/events/devices/x11/... ui/events/devices/x11/touch_factory_x11.cc:105: if (xiclassinfo->type == XITouchClass) { Perhaps we could change this to early-continue too? i.e. if (xiclassinfo->type != XITouchClass) continue; That would reduce the indentation level of the following code.
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lanwei@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2552343008/#ps160001 (title: "fix a )")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489432448355870,
"parent_rev": "44e2279d604959497a7965e86575cc01ef7c98b4", "commit_rev":
"1d64628a6b9a59469503b6a82a68606a09950d9a"}
Message was sent while issue was closed.
Description was changed from ========== Add slave touch device to touch_device_lookup_ and touch_device_list_ Unexpected MouseMove event when touch the touch screen issue is because we didn't add slave touch device to touch_device_lookup_ and touch_device_list_ then we got false when we check if it is from touch screen by sourceid(not device id). This patch we add slave touch device to touch_device_lookup_ and touch_device_list_. BUG=678074 ========== to ========== Add slave touch device to touch_device_lookup_ and touch_device_list_ Unexpected MouseMove event when touch the touch screen issue is because we didn't add slave touch device to touch_device_lookup_ and touch_device_list_ then we got false when we check if it is from touch screen by sourceid(not device id). This patch we add slave touch device to touch_device_lookup_ and touch_device_list_. BUG=678074 Review-Url: https://codereview.chromium.org/2552343008 Cr-Commit-Position: refs/heads/master@{#456488} Committed: https://chromium.googlesource.com/chromium/src/+/1d64628a6b9a59469503b6a82a68... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/1d64628a6b9a59469503b6a82a68... |
