Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2058723003:
Slop region check for multi-finger scroll (Closed)
DescriptionSlop region check for multi-finger scroll
Two finger scrolling doesn't happen if neither of the pointers has exceeded their slop regions.
The checking is needed to avoid a two finger tap happening after a scroll within slop region.
For scrolling with more than two fingers, slop region check is skipped, because the tap option
is not valid in those cases.
BUG=492185
TEST=GestureProviderTest.SlopRegionCheckOnScroll
Committed: https://crrev.com/033d839437b34f4de578479aea8c4a2669a91945
Cr-Commit-Position: refs/heads/master@{#403827}
Patch Set 1 #
Total comments: 16
Patch Set 2 #
Total comments: 23
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #Messages
Total messages: 39 (17 generated)
sahel@chromium.org changed reviewers: + tdresser@chromium.org
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058723003/1
https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... File ui/events/gesture_detection/gesture_detector.cc (right): https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_detector.cc:190: //Update maximum pointer count // Update maximum pointer count. Note the space before "Update" and the period. I'd say we can probably just get rid of this comment though, it should be clear from the code. https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_detector.cc:193: maximum_pointer_count_ = current_pointer_count; maximum_pointer_count_ = std::max(maximum_pointer_count_, ev.GetPointerCount()); https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_detector.cc:208: if (ev.GetPointerCount() != 2 || maximum_pointer_count_ > 2 || Is the first part of this clause necessary? Couldn't we ignore the current pointer count, and instead just look at the maximum? https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... File ui/events/gesture_detection/gesture_detector.h (right): https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_detector.h:134: bool all_pointers_always_in_tap_region_; Let's rename this to something like: "all_pointers_within_slop_regions" https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_detector.h:152: // Stores the maximum number of pointers that have been down together. The comment should make it clear that this is for the current touch sequence. https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_provider.cc:300: float ratio = std::max(0.f, distance - subtraction_value ) / distance; Is this logic correct? If the two pointers move in opposite directions and exceed their slop regions, and then both scroll 1px down, what will happen? https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... File ui/events/gesture_detection/gesture_provider_unittest.cc (right): https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_provider_unittest.cc:945: // three finger tap doesn't exsit, no slop region for three finger scroll. exsit -> exist https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_provider_unittest.cc:954: // Scroll with one pointer down Missing .'s.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058723003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058723003/60001
https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... File ui/events/gesture_detection/gesture_detector.cc (right): https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_detector.cc:190: //Update maximum pointer count On 2016/06/10 19:34:57, tdresser wrote: > // Update maximum pointer count. > > Note the space before "Update" and the period. > > I'd say we can probably just get rid of this comment though, it should be clear > from the code. Done. https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_detector.cc:193: maximum_pointer_count_ = current_pointer_count; On 2016/06/10 19:34:57, tdresser wrote: > maximum_pointer_count_ = std::max(maximum_pointer_count_, ev.GetPointerCount()); Done. https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_detector.cc:208: if (ev.GetPointerCount() != 2 || maximum_pointer_count_ > 2 || On 2016/06/10 19:34:57, tdresser wrote: > Is the first part of this clause necessary? Couldn't we ignore the current > pointer count, and instead just look at the maximum? You are right, if the current count is not two, the maximum has been updated and is greater than 2 as well. I left it like this, to make it easier to understand, I'll remove the first phrase of the condition. https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... File ui/events/gesture_detection/gesture_detector.h (right): https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_detector.h:134: bool all_pointers_always_in_tap_region_; On 2016/06/10 19:34:57, tdresser wrote: > Let's rename this to something like: > "all_pointers_within_slop_regions" Done. https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_detector.h:152: // Stores the maximum number of pointers that have been down together. On 2016/06/10 19:34:57, tdresser wrote: > The comment should make it clear that this is for the current touch sequence. Done. https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_provider.cc:300: float ratio = std::max(0.f, distance - subtraction_value ) / distance; On 2016/06/10 19:34:57, tdresser wrote: > Is this logic correct? If the two pointers move in opposite directions and > exceed their slop regions, and then both scroll 1px down, what will happen? Done. https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... File ui/events/gesture_detection/gesture_provider_unittest.cc (right): https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_provider_unittest.cc:945: // three finger tap doesn't exsit, no slop region for three finger scroll. On 2016/06/10 19:34:57, tdresser wrote: > exsit -> exist Done. https://codereview.chromium.org/2058723003/diff/1/ui/events/gesture_detection... ui/events/gesture_detection/gesture_provider_unittest.cc:954: // Scroll with one pointer down On 2016/06/10 19:34:58, tdresser wrote: > Missing .'s. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2058723003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/stylus_text_selector.cc (right): https://codereview.chromium.org/2058723003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/stylus_text_selector.cc:114: const MotionEvent& secondary_pointer_down_event, float distance_x, Remove _event. https://codereview.chromium.org/2058723003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/stylus_text_selector.h (right): https://codereview.chromium.org/2058723003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/stylus_text_selector.h:60: const ui::MotionEvent& secondary_pointer_down_event, I'd remove _event from the parameter name. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_detector.cc (right): https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_detector.cc:190: static_cast<int>(ev.GetPointerCount())); Indentation is off here. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_detector.cc:326: *secondary_pointer_down_event_ : ev), Is this indentation right? I'd recommend "git cl format" or binding the tab key in your editor to run clang-format on the current line. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_detector.h (right): https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_detector.h:152: // Stores the maximum number of pointers that have been down together together -> simultaneously Just a bit more precise. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:650: // Calculate the subtraction value for touch scrolls Missing period. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:653: float& scroll_x, float& scroll_y) { Maybe return a Point instead of using out parameters? Let's name this a bit more precisely, something like - ComputeFirstScrollDelta? I think the method comment could be a bit clearer too. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:656: // Slop region is not deducted. "tapping is not possible, so the slop region is not deducted" Let's just make it a bit clearer that these are related. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:691: scroll_y = (dy0 + dy1) / ev2.GetPointerCount(); This makes sense to me, but we should definitely try it on a device or two. One way to make it easier to see issues here would be to try a build with a massive slop region size. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_provider_unittest.cc (right): https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider_unittest.cc:951: TEST_F(GestureProviderTest, SlopRegionCheckOnScroll) { Can we split this into three tests, one per number of fingers? https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider_unittest.cc:958: This is a bit more whitespace than we'd usually use - I'd say just go with a single line of whitespace. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider_unittest.cc:1081: // One finger scroll Missing periods.
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2058723003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/stylus_text_selector.cc (right): https://codereview.chromium.org/2058723003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/stylus_text_selector.cc:114: const MotionEvent& secondary_pointer_down_event, float distance_x, On 2016/06/28 15:24:18, tdresser wrote: > Remove _event. Done. https://codereview.chromium.org/2058723003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/stylus_text_selector.h (right): https://codereview.chromium.org/2058723003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/stylus_text_selector.h:60: const ui::MotionEvent& secondary_pointer_down_event, On 2016/06/28 15:24:18, tdresser wrote: > I'd remove _event from the parameter name. Done. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_detector.cc (right): https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_detector.cc:190: static_cast<int>(ev.GetPointerCount())); On 2016/06/28 15:24:19, tdresser wrote: > Indentation is off here. Done. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_detector.cc:326: *secondary_pointer_down_event_ : ev), On 2016/06/28 15:24:19, tdresser wrote: > Is this indentation right? I'd recommend "git cl format" or binding the tab key > in your editor to run clang-format on the current line. Didn't know about clang-format and git cl format, got it, thanks. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_detector.h (right): https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_detector.h:152: // Stores the maximum number of pointers that have been down together On 2016/06/28 15:24:19, tdresser wrote: > together -> simultaneously > Just a bit more precise. Done. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:650: // Calculate the subtraction value for touch scrolls On 2016/06/28 15:24:19, tdresser wrote: > Missing period. Done. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:653: float& scroll_x, float& scroll_y) { On 2016/06/28 15:24:19, tdresser wrote: > Maybe return a Point instead of using out parameters? > > Let's name this a bit more precisely, something like - ComputeFirstScrollDelta? > I think the method comment could be a bit clearer too. Done. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:656: // Slop region is not deducted. On 2016/06/28 15:24:19, tdresser wrote: > "tapping is not possible, so the slop region is not deducted" > > Let's just make it a bit clearer that these are related. Done. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:691: scroll_y = (dy0 + dy1) / ev2.GetPointerCount(); On 2016/06/28 15:24:19, tdresser wrote: > This makes sense to me, but we should definitely try it on a device or two. > > One way to make it easier to see issues here would be to try a build with a > massive slop region size. Sure, I'll play with the code, with a massive slop region. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_provider_unittest.cc (right): https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider_unittest.cc:951: TEST_F(GestureProviderTest, SlopRegionCheckOnScroll) { On 2016/06/28 15:24:19, tdresser wrote: > Can we split this into three tests, one per number of fingers? Done. https://codereview.chromium.org/2058723003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider_unittest.cc:958: On 2016/06/28 15:24:19, tdresser wrote: > This is a bit more whitespace than we'd usually use - I'd say just go with a > single line of whitespace. Done.
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_...)
SGTM % nits, and I'll want to give it a try on device at some point. https://codereview.chromium.org/2058723003/diff/100001/ui/aura/gestures/gestu... File ui/aura/gestures/gesture_recognizer_unittest.cc (left): https://codereview.chromium.org/2058723003/diff/100001/ui/aura/gestures/gestu... ui/aura/gestures/gesture_recognizer_unittest.cc:2587: ui::ET_GESTURE_SCROLL_UPDATE, ui::ET_GESTURE_SCROLL_UPDATE); Can we expect some number of events here? https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... File ui/events/gesture_detection/gesture_detector.h (right): https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... ui/events/gesture_detection/gesture_detector.h:153: // during the current touch sequenece. sequence https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... ui/events/gesture_detection/gesture_provider.cc:684: if (ev2.GetPointerCount() == 2) { Could we factor out the slop subtraction here? Lines 665-680 feel like they could be in a helper method, used in 681-698. https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... File ui/events/gesture_detection/gesture_provider_unittest.cc (right): https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... ui/events/gesture_detection/gesture_provider_unittest.cc:950: // no slop region check is needed for three-finger scrolls. following Maybe add a blank line after this comment to make it clear that the comment doesn't just refer to this test.
LGTM % nits.
The CQ bit was checked by sahel@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/2058723003/diff/100001/ui/aura/gestures/gestu... File ui/aura/gestures/gesture_recognizer_unittest.cc (left): https://codereview.chromium.org/2058723003/diff/100001/ui/aura/gestures/gestu... ui/aura/gestures/gesture_recognizer_unittest.cc:2587: ui::ET_GESTURE_SCROLL_UPDATE, ui::ET_GESTURE_SCROLL_UPDATE); On 2016/07/01 14:29:15, tdresser wrote: > Can we expect some number of events here? Done. https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... File ui/events/gesture_detection/gesture_detector.h (right): https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... ui/events/gesture_detection/gesture_detector.h:153: // during the current touch sequenece. On 2016/07/01 14:29:15, tdresser wrote: > sequence Done. https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... ui/events/gesture_detection/gesture_provider.cc:684: if (ev2.GetPointerCount() == 2) { On 2016/07/01 14:29:15, tdresser wrote: > Could we factor out the slop subtraction here? > > Lines 665-680 feel like they could be in a helper method, used in 681-698. Done. https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... File ui/events/gesture_detection/gesture_provider_unittest.cc (right): https://codereview.chromium.org/2058723003/diff/100001/ui/events/gesture_dete... ui/events/gesture_detection/gesture_provider_unittest.cc:950: // no slop region check is needed for three-finger scrolls. On 2016/07/01 14:29:15, tdresser wrote: > following > > Maybe add a blank line after this comment to make it clear that the comment > doesn't just refer to this test. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2058723003/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Slop region check for multi-finger scroll Two finger scrolling doesn't happen if neither of the pointers has exceeded their slop regions. The checking is needed to avoid a two finger tap happening after a scroll within slop region. For scrolling with more than two fingers, slop region check is skipped, because the tap option is not valid in those cases. BUG=492185 TEST=GestureProviderTest.SlopRegionCheckOnScroll ========== to ========== Slop region check for multi-finger scroll Two finger scrolling doesn't happen if neither of the pointers has exceeded their slop regions. The checking is needed to avoid a two finger tap happening after a scroll within slop region. For scrolling with more than two fingers, slop region check is skipped, because the tap option is not valid in those cases. BUG=492185 TEST=GestureProviderTest.SlopRegionCheckOnScroll Committed: https://crrev.com/033d839437b34f4de578479aea8c4a2669a91945 Cr-Commit-Position: refs/heads/master@{#403827} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/033d839437b34f4de578479aea8c4a2669a91945 Cr-Commit-Position: refs/heads/master@{#403827} |
