Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2256733003:
Touchpad scroll latching enabled for Mac behind flag. (Closed)
|
Created:
4 years, 4 months ago by sahel Modified:
4 years, 3 months ago CC:
cc-bugs_chromium.org, chromium-reviews, darin-cc_chromium.org, dtapuska+chromiumwatch_chromium.org, jam, tdresser+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTouchpad scroll latching enabled for Mac behind flag.
To test it, use the --enable-features=TouchpadScrollLatching runtime flag.
BUG=526463
TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.GestureSendingWithPhaseInformation/[0/1]
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/7adc4a771383af81e2411e755bdce689c803b939
Cr-Commit-Position: refs/heads/master@{#414515}
Patch Set 1 #
Total comments: 6
Patch Set 2 : unittests fixed #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : comments edited. #
Total comments: 4
Patch Set 6 : ClearCurrentlyScrollingLayerForTesting added. #
Messages
Total messages: 39 (24 generated)
Description was changed from ========== Touchpad scroll latching enabled for Mac behind flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 ========== to ========== Touchpad scroll latching enabled for Mac behind flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
sahel@chromium.org changed reviewers: + ericrk@chromium.org, tdresser@chromium.org
ericrk@chromium.org: Please review changes in layer_tree_host_impl.cc tdresser@chromium.org: Please review changes in
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2256733003/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2256733003/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:3243: if (scroll_state->is_in_inertial_phase()) { I'm a bit concerned this isn't correct for devices that don't have inertial phase information or when the feature is off. Shouldnt it be clearing the scrolling layer in those cases?
We'll need a unit test here. https://codereview.chromium.org/2256733003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (left): https://codereview.chromium.org/2256733003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:177: // implemented. These comments are still relevant, aren't they? https://codereview.chromium.org/2256733003/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2256733003/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:134: // On Mac flings are aslo scrolls. aslo -> also Or perhaps: // On Mac, a GestureScrollBegin in the inertial phase indicates a fling start.
Description was changed from ========== Touchpad scroll latching enabled for Mac behind flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Touchpad scroll latching enabled for Mac behind flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.GestureSendingWithPhaseInformation/[0/1] CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
I changed the unittests. I think MouseWheelEventQueueTests/MouseWheelEventQueueTest.GestureSendingWithPhaseInformation/1 test already addresses what we want. If not, please let me know. https://codereview.chromium.org/2256733003/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2256733003/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:3243: if (scroll_state->is_in_inertial_phase()) { On 2016/08/18 00:30:45, dtapuska wrote: > I'm a bit concerned this isn't correct for devices that don't have inertial > phase information or when the feature is off. Shouldnt it be clearing the > scrolling layer in those cases? It's ok if we don't do the latching for whatever reason and don't clear it here, because in scrollBegin we clear it. https://codereview.chromium.org/2256733003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (left): https://codereview.chromium.org/2256733003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:177: // implemented. On 2016/08/18 13:59:47, tdresser wrote: > These comments are still relevant, aren't they? Done. https://codereview.chromium.org/2256733003/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2256733003/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:134: // On Mac flings are aslo scrolls. On 2016/08/18 13:59:47, tdresser wrote: > aslo -> also > > Or perhaps: > // On Mac, a GestureScrollBegin in the inertial phase indicates a fling start. Done.
LGTM! https://codereview.chromium.org/2256733003/diff/20001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2256733003/diff/20001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:137: WebGestureEvent::MomentumPhase) Although this isn't technically required by the style guide, I much prefer {} when there are multiline conditionals (as well as multiline bodies).
https://codereview.chromium.org/2256733003/diff/20001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2256733003/diff/20001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:137: WebGestureEvent::MomentumPhase) On 2016/08/18 16:55:54, tdresser wrote: > Although this isn't technically required by the style guide, I much prefer {} > when there are multiline conditionals (as well as multiline bodies). Done.
On 2016/08/18 17:37:59, sahel wrote: > https://codereview.chromium.org/2256733003/diff/20001/ui/events/blink/input_h... > File ui/events/blink/input_handler_proxy.cc (right): > > https://codereview.chromium.org/2256733003/diff/20001/ui/events/blink/input_h... > ui/events/blink/input_handler_proxy.cc:137: WebGestureEvent::MomentumPhase) > On 2016/08/18 16:55:54, tdresser wrote: > > Although this isn't technically required by the style guide, I much prefer {} > > when there are multiline conditionals (as well as multiline bodies). > > Done. lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I had to change cc_unittests because, after calling scrollEnd it was expected to have currentlyscrollinglayer cleared. I made the function public and called it in the tests. scrollAnimated relies on clearing the scurrentlyscrollinglayer after the animation is done. Which shouldn't be the case after the latching is done for non-mac devices. I left a TODO to handle it in the follow up patches.
Still LGTM https://codereview.chromium.org/2256733003/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2256733003/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2934: // impelemented, current scrolling layer should not get cleared after each implemented, the https://codereview.chromium.org/2256733003/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:4112: // impelemented, current scrolling layer should not get cleared after each implemented, the
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/2256733003/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2256733003/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2934: // impelemented, current scrolling layer should not get cleared after each On 2016/08/24 19:20:32, tdresser wrote: > implemented, the Done. https://codereview.chromium.org/2256733003/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:4112: // impelemented, current scrolling layer should not get cleared after each On 2016/08/24 19:20:32, tdresser wrote: > implemented, the Done.
lgtm https://codereview.chromium.org/2256733003/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2256733003/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2895: scroll_state_data.is_in_inertial_phase = false; I assume this change is so we don't go down the fling scroll path in ScrollBegin? Will changing this here have any unintended side effects? https://codereview.chromium.org/2256733003/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2256733003/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:612: void ClearCurrentlyScrollingLayer(); If this is only made public for testing, can you leave this function private and add a "ClearCurrentlyScrollingLayerForTesting"?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I added a DCHECK to make sure that the is_in_inertial_phase value is always false when the scroll_animated is called. https://codereview.chromium.org/2256733003/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2256733003/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2895: scroll_state_data.is_in_inertial_phase = false; On 2016/08/25 16:56:50, ericrk wrote: > I assume this change is so we don't go down the fling scroll path in > ScrollBegin? Will changing this here have any unintended side effects? You are right about the reason of the change. The change is safe because the only place that scrollAnimated is called is within handlegesturescrollupdate. I can add a DCHECK in this to make sure the is_in_inertial_phase is false when scrollAnimated is called. https://codereview.chromium.org/2256733003/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2256733003/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:612: void ClearCurrentlyScrollingLayer(); On 2016/08/25 16:56:50, ericrk wrote: > If this is only made public for testing, can you leave this function private and > add a "ClearCurrentlyScrollingLayerForTesting"? Done.
The CQ bit was checked by sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, tdresser@chromium.org, ericrk@chromium.org Link to the patchset: https://codereview.chromium.org/2256733003/#ps100001 (title: "ClearCurrentlyScrollingLayerForTesting added.")
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.
Description was changed from ========== Touchpad scroll latching enabled for Mac behind flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.GestureSendingWithPhaseInformation/[0/1] CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Touchpad scroll latching enabled for Mac behind flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.GestureSendingWithPhaseInformation/[0/1] CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Touchpad scroll latching enabled for Mac behind flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.GestureSendingWithPhaseInformation/[0/1] CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Touchpad scroll latching enabled for Mac behind flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.GestureSendingWithPhaseInformation/[0/1] CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/7adc4a771383af81e2411e755bdce689c803b939 Cr-Commit-Position: refs/heads/master@{#414515} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7adc4a771383af81e2411e755bdce689c803b939 Cr-Commit-Position: refs/heads/master@{#414515} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
