Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1705323002:
Follow on for gesture events generated by wheel events. (Closed)
|
Created:
4 years, 10 months ago by dtapuska Modified:
4 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master_wheel_passive_listeners_3 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFollow on for gesture events generated by wheel events.
Provide mechanism to run with wheel events disabled.
Implement overscroll handling for wheel event generation.
Fix input router unit test when wheel events are generated.
BUG=568183
Committed: https://crrev.com/19ebfa5187a21692b8d103c332e9692459d14148
Cr-Commit-Position: refs/heads/master@{#376552}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Fix tdresser's comments #Patch Set 3 : Rebase; resolve merge conflict in unittest #
Messages
Total messages: 30 (13 generated)
Description was changed from ========== Follow on for gesture events generated by wheel events. Provide mechanism to run with wheel events disabled. Implement overscroll handling for wheel event generation. Fix input router unit test when wheel events are generated. BUG=568183 ========== to ========== Follow on for gesture events generated by wheel events. Provide mechanism to run with wheel events disabled. Implement overscroll handling for wheel event generation. Fix input router unit test when wheel events are generated. BUG=568183 ==========
dtapuska@chromium.org changed reviewers: + creis@chromium.org, sadrul@chromium.org, tdresser@chromium.org
https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/overscroll_controller.cc (right): https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:23: bool IsGestureEventFromTouchscreen(const blink::WebInputEvent& event) { Maybe DCHECK that this is a gesture event? https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:39: use_gesutre_wheel_scrolling_(UseGestureBasedWheelScrolling()) {} use_gesutre -> use_gesture https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:50: // Gesture scroll end is sent based on a timeout avoid timeout avoid -> timeout to avoid https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:60: if (!use_gesutre_wheel_scrolling_) { This might be clearer if you invert the condition. if (use_gesture_wheel_scrolling) break; ... break; https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:164: // resetting on events from the touchpad. resetting what? https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:221: } else { Don't need the "else", as there's a return above. https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/overscroll_controller.h (right): https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.h:95: bool ProcessOverscroll(float delta_x, float delta_y, bool is_touchpad); I slightly prefer sending the type here, as it makes the meaning of the parameter more clear from the call site. What's the advantage of this approach?
https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/input_router_impl_unittest.cc (right): https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/input_router_impl_unittest.cc:1003: EXPECT_EQ(2U, ack_handler_->GetAndResetAckCount()); Why do we have two acks here, but only one in the test above?
https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/overscroll_controller.cc (right): https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:164: // resetting on events from the touchpad. On 2016/02/18 14:35:21, tdresser wrote: > resetting what? Resetting the overscroll to OVERSCROLL_NONE state. https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/overscroll_controller.h (right): https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.h:95: bool ProcessOverscroll(float delta_x, float delta_y, bool is_touchpad); On 2016/02/18 14:35:21, tdresser wrote: > I slightly prefer sending the type here, as it makes the meaning of the > parameter more clear from the call site. > > What's the advantage of this approach? ProcessOverscroll is called both from MouseWheel and GestureScroll.. I could keep it; but I'd need to move the check if event_type == MouseWheel || event_type == GestureScroll && !IsGestureEventFromTouchscreen() inside. This felt cleaner; but I can change.
https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/input_router_impl_unittest.cc (right): https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/input_router_impl_unittest.cc:1003: EXPECT_EQ(2U, ack_handler_->GetAndResetAckCount()); On 2016/02/18 14:39:10, tdresser wrote: > Why do we have two acks here, but only one in the test above? Added comments https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/overscroll_controller.cc (right): https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:23: bool IsGestureEventFromTouchscreen(const blink::WebInputEvent& event) { On 2016/02/18 14:35:21, tdresser wrote: > Maybe DCHECK that this is a gesture event? Done. https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:39: use_gesutre_wheel_scrolling_(UseGestureBasedWheelScrolling()) {} On 2016/02/18 14:35:21, tdresser wrote: > use_gesutre -> use_gesture Done. https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:50: // Gesture scroll end is sent based on a timeout avoid On 2016/02/18 14:35:21, tdresser wrote: > timeout avoid -> timeout to avoid Done. https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:60: if (!use_gesutre_wheel_scrolling_) { On 2016/02/18 14:35:21, tdresser wrote: > This might be clearer if you invert the condition. > > if (use_gesture_wheel_scrolling) > break; > ... > break; Done. https://codereview.chromium.org/1705323002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/overscroll_controller.cc:221: } else { On 2016/02/18 14:35:20, tdresser wrote: > Don't need the "else", as there's a return above. Done.
LGTM. I initially didn't believe that for touchpad, we only reset overscroll on mouse move, not when the user's finger leaves the touchpad. I eventually did manage to repro letting go of the touchpad without the overscroll resetting though. It's _really_ hard on a pixel 2.
dtapuska@chromium.org changed reviewers: + sievers@chromium.org - creis@chromium.org
On 2016/02/19 15:10:13, tdresser wrote: > LGTM. > > I initially didn't believe that for touchpad, we only reset overscroll on mouse > move, not when the user's finger leaves the touchpad. > > I eventually did manage to repro letting go of the touchpad without the > overscroll resetting though. It's _really_ hard on a pixel 2. +sievers for content/public/common/content_switches.*
On 2016/02/19 16:32:43, dtapuska wrote: > On 2016/02/19 15:10:13, tdresser wrote: > > LGTM. > > > > I initially didn't believe that for touchpad, we only reset overscroll on > mouse > > move, not when the user's finger leaves the touchpad. > > > > I eventually did manage to repro letting go of the touchpad without the > > overscroll resetting though. It's _really_ hard on a pixel 2. > > +sievers for content/public/common/content_switches.* lgtm
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705323002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705323002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1705323002/#ps40001 (title: "Rebase; resolve merge conflict in unittest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705323002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705323002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705323002/40001
Message was sent while issue was closed.
Description was changed from ========== Follow on for gesture events generated by wheel events. Provide mechanism to run with wheel events disabled. Implement overscroll handling for wheel event generation. Fix input router unit test when wheel events are generated. BUG=568183 ========== to ========== Follow on for gesture events generated by wheel events. Provide mechanism to run with wheel events disabled. Implement overscroll handling for wheel event generation. Fix input router unit test when wheel events are generated. BUG=568183 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Follow on for gesture events generated by wheel events. Provide mechanism to run with wheel events disabled. Implement overscroll handling for wheel event generation. Fix input router unit test when wheel events are generated. BUG=568183 ========== to ========== Follow on for gesture events generated by wheel events. Provide mechanism to run with wheel events disabled. Implement overscroll handling for wheel event generation. Fix input router unit test when wheel events are generated. BUG=568183 Committed: https://crrev.com/19ebfa5187a21692b8d103c332e9692459d14148 Cr-Commit-Position: refs/heads/master@{#376552} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/19ebfa5187a21692b8d103c332e9692459d14148 Cr-Commit-Position: refs/heads/master@{#376552} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
