Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2158423002:
Wheel scroll latching enabled behind flag. (Closed)
|
Created:
4 years, 5 months ago by sahel Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, cc-bugs_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, dtapuska+chromiumwatch_chromium.org, jam, kinuko+watch, tdresser+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe scroll latching feature for touchpad and wheel scrolls is enabled behind a flag.
The current patch includes the flag definition and wheel scroll latching.
The touchpad scroll latching support will be added in different cls.
To test it, use the --enable-features=TouchpadScrollLatching runtime flag.
BUG=526463
TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/0, MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/1
Committed: https://crrev.com/b9e7f19ba7f4d11ce46fc3217d542193b7904ed8
Cr-Commit-Position: refs/heads/master@{#411974}
Patch Set 1 #
Total comments: 8
Patch Set 2 : features deleted from blink features. #
Total comments: 9
Patch Set 3 : unit_tests_fixed #
Total comments: 11
Patch Set 4 : The patch is only for flag definition and wheel scroll latiching. #
Total comments: 8
Patch Set 5 : The name of the flag modified to represent both wheel and touchpad scroll latching. #
Total comments: 6
Patch Set 6 : flag name changed. #
Total comments: 3
Patch Set 7 : Scroll latching unit test added. #
Total comments: 7
Patch Set 8 : #
Total comments: 18
Patch Set 9 : Wheel scroll latching enabled behind flag #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Messages
Total messages: 81 (44 generated)
Description was changed from ========== Touchpad scroll latching for Mac. The feature in enabled for Mac, behind a flag. To test it, use --enable-features=touchpad-scroll-latching runtime flag. BUG=526463 ========== to ========== Touchpad scroll latching for Mac. The feature in enabled for Mac, behind a flag. To test it, use --enable-features=touchpad-scroll-latching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Touchpad scroll latching for Mac. The feature in enabled for Mac, behind a flag. To test it, use --enable-features=touchpad-scroll-latching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Touchpad scroll latching for Mac. The feature in enabled for Mac, behind a flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
sahel@chromium.org changed reviewers: + tdresser@chromium.org
sahel@chromium.org changed reviewers: + ericrk@chromium.org, esprehn@chromium.org, nasko@chromium.org
ericrk@chromium.org: Please review changes in layer_tree_host_impl.cc nasko@chromium.org: Please review changes in runtime_features.cc content_features.h content_features.cc esprehn@chromium.org: Please review changes in RuntimeEnabledFeatures.in WebRuntimeFeatures.cpp WebRuntimeFeatures.h
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2158423002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:46: base::FeatureList::IsEnabled(features::kTouchpadScrollLatching); I don't think this will build on mac. It is a little odd that we adjust the kDefaultWheelScrollTransactionMs. I think perhaps we should make two defaults. One for latching enabled; one with it disabled. Then the caller can determine which to use. Also the value is used before this method is initialized. So you are initializing a static field inside a constructor of an object. So its first use will be 0. https://codereview.chromium.org/2158423002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:179: } else if (has_phase_info && !touchpad_scroll_latching_) { Is it always scroll_transaction_ms_ is 0 then touchpad_scroll_latching_ is false. Do we need both fields? Perhaps we can just have a method bool touch_pad_scroll_latching() { return scroll_transaction_ms_ == 0; } https://codereview.chromium.org/2158423002/diff/1/content/public/common/conte... File content/public/common/content_features.h (right): https://codereview.chromium.org/2158423002/diff/1/content/public/common/conte... content/public/common/content_features.h:56: #if defined(OS_MACOSX) Why the ifdef? I don't see why we just have the flag for mac. https://codereview.chromium.org/2158423002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2158423002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:135: if (event.data.scrollBegin.inertialPhase == Is this going to work for devices that don't have momentum phase on mac?
On 2016/07/19 17:41:31, dtapuska wrote: > https://codereview.chromium.org/2158423002/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): > > https://codereview.chromium.org/2158423002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:46: > base::FeatureList::IsEnabled(features::kTouchpadScrollLatching); > I don't think this will build on mac. It is a little odd that we adjust the > kDefaultWheelScrollTransactionMs. > > I think perhaps we should make two defaults. One for latching enabled; one with > it disabled. Then the caller can determine which to use. > > Also the value is used before this method is initialized. So you are > initializing a static field inside a constructor of an object. So its first use > will be 0. > > https://codereview.chromium.org/2158423002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:179: } else if > (has_phase_info && !touchpad_scroll_latching_) { > Is it always scroll_transaction_ms_ is 0 then touchpad_scroll_latching_ is > false. > > Do we need both fields? Perhaps we can just have a method bool > touch_pad_scroll_latching() { return scroll_transaction_ms_ == 0; } > > https://codereview.chromium.org/2158423002/diff/1/content/public/common/conte... > File content/public/common/content_features.h (right): > > https://codereview.chromium.org/2158423002/diff/1/content/public/common/conte... > content/public/common/content_features.h:56: #if defined(OS_MACOSX) > Why the ifdef? I don't see why we just have the flag for mac. > > https://codereview.chromium.org/2158423002/diff/1/ui/events/blink/input_handl... > File ui/events/blink/input_handler_proxy.cc (right): > > https://codereview.chromium.org/2158423002/diff/1/ui/events/blink/input_handl... > ui/events/blink/input_handler_proxy.cc:135: if > (event.data.scrollBegin.inertialPhase == > Is this going to work for devices that don't have momentum phase on mac? You'll likely need to fix a number of unit tests as well. Do all the layout tests pass?
Why are we addinga runtime flag for blink if nothing uses it in this patch?
On 2016/07/19 19:21:37, esprehn wrote: > Why are we addinga runtime flag for blink if nothing uses it in this patch? I discussed that with sahel@ in person; she is going to remove it.
On 2016/07/19 at 19:39:23, dtapuska wrote: > On 2016/07/19 19:21:37, esprehn wrote: > > Why are we addinga runtime flag for blink if nothing uses it in this patch? > > I discussed that with sahel@ in person; she is going to remove it. Great, super excited about seeing this feature ship. :)
https://codereview.chromium.org/2158423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2158423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:222: TouchpadScrollLatching You don't need this one either. :)
https://codereview.chromium.org/2158423002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2158423002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2691: return FlingScrollBegin(); To make sure I'm understanding this: Is the idea that, in the case of is_in_inertial_phase, we want to behave as though this is a fling, not a scroll? Can you add a comment here? https://codereview.chromium.org/2158423002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3246: if (scroll_state->is_in_inertial_phase()) { I though that is_in_inertial_phase indicated that we had a new fling? I'm probably just misunderstanding, but it seems like we are clearing the currently scrolling layer iff we have a new fling? https://codereview.chromium.org/2158423002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:44: touchpad_scroll_latching_ = nit: it seems odd/fragile to set this variable based on comparison with scroll_transaction_ms_... might it be cleaner to pass in "bool touchpad_scroll_latching" and then set the scroll_transaction_ms_ based on that in the constructor?
https://codereview.chromium.org/2158423002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:46: base::FeatureList::IsEnabled(features::kTouchpadScrollLatching); On 2016/07/19 17:41:31, dtapuska wrote: > I don't think this will build on mac. It is a little odd that we adjust the > kDefaultWheelScrollTransactionMs. > > I think perhaps we should make two defaults. One for latching enabled; one with > it disabled. Then the caller can determine which to use. > > Also the value is used before this method is initialized. So you are > initializing a static field inside a constructor of an object. So its first use > will be 0. Done. https://codereview.chromium.org/2158423002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:179: } else if (has_phase_info && !touchpad_scroll_latching_) { On 2016/07/19 17:41:31, dtapuska wrote: > Is it always scroll_transaction_ms_ is 0 then touchpad_scroll_latching_ is > false. > > Do we need both fields? Perhaps we can just have a method bool > touch_pad_scroll_latching() { return scroll_transaction_ms_ == 0; } I kept the boolean value, but calculated it based on the scroll_transaction_ms_ in constructor. The reason for keeping the boolean value is easier code change when we want to remove the flag and have the feature always enabled. https://codereview.chromium.org/2158423002/diff/1/content/public/common/conte... File content/public/common/content_features.h (right): https://codereview.chromium.org/2158423002/diff/1/content/public/common/conte... content/public/common/content_features.h:56: #if defined(OS_MACOSX) On 2016/07/19 17:41:31, dtapuska wrote: > Why the ifdef? I don't see why we just have the flag for mac. Done. https://codereview.chromium.org/2158423002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2158423002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:135: if (event.data.scrollBegin.inertialPhase == On 2016/07/19 17:41:31, dtapuska wrote: > Is this going to work for devices that don't have momentum phase on mac? I think yes, because there is no fling for devices that don't have the momentum phase. The condition will be always false for such devices and the is_inertial_phase won't become true. https://codereview.chromium.org/2158423002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2158423002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2691: return FlingScrollBegin(); On 2016/07/19 20:21:51, ericrk wrote: > To make sure I'm understanding this: > > Is the idea that, in the case of is_in_inertial_phase, we want to behave as > though this is a fling, not a scroll? > > Can you add a comment here? Yes, because in mac flings are handled as scrolls. I added the comment. Done. https://codereview.chromium.org/2158423002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3246: if (scroll_state->is_in_inertial_phase()) { On 2016/07/19 20:21:51, ericrk wrote: > I though that is_in_inertial_phase indicated that we had a new fling? I'm > probably just misunderstanding, but it seems like we are clearing the currently > scrolling layer iff we have a new fling? in ScrollEnd, is_in_inertial_phase is true iff the fling is happening. The idea is not to clear the scrolling layer in cases that we are in the end of a scroll because a fling might happen afterwards and we don't want a new hit test for the fling that happens after a scroll. It's safe not to clear scrolling layer, because we clear it in scrollBegin. I can remove the condition and never clear the scrollinglayer in this function. But after a fling we know that it's the end of current gesture sequence and anything that happens after needs a new hit testing. So, it is safe to clear the scrollinglayer. https://codereview.chromium.org/2158423002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:44: touchpad_scroll_latching_ = On 2016/07/19 20:21:51, ericrk wrote: > nit: it seems odd/fragile to set this variable based on comparison with > scroll_transaction_ms_... might it be cleaner to pass in "bool > touchpad_scroll_latching" and then set the scroll_transaction_ms_ based on that > in the constructor? Done. https://codereview.chromium.org/2158423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2158423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:222: TouchpadScrollLatching On 2016/07/19 19:42:03, esprehn wrote: > You don't need this one either. :) Done.
content/public/common looks good. I'll stamp it once all the other reviews are complete. https://codereview.chromium.org/2158423002/diff/40001/content/public/common/c... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2158423002/diff/40001/content/public/common/c... content/public/common/content_features.cc:123: // For now, it works only for Mac. nit: Combine the two lines into one.
layer_tree_host_impl.cc LGTM https://codereview.chromium.org/2158423002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2158423002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3246: if (scroll_state->is_in_inertial_phase()) { On 2016/07/19 22:47:03, sahel wrote: > On 2016/07/19 20:21:51, ericrk wrote: > > I though that is_in_inertial_phase indicated that we had a new fling? I'm > > probably just misunderstanding, but it seems like we are clearing the > currently > > scrolling layer iff we have a new fling? > > in ScrollEnd, is_in_inertial_phase is true iff the fling is happening. The idea > is not to clear the scrolling layer in cases that we are in the end of a scroll > because a fling might happen afterwards and we don't want a new hit test for the > fling that happens after a scroll. > > It's safe not to clear scrolling layer, because we clear it in scrollBegin. > > I can remove the condition and never clear the scrollinglayer in this function. > But after a fling we know that it's the end of current gesture sequence and > anything that happens after needs a new hit testing. So, it is safe to clear the > scrollinglayer. Ok, this makes sense, thanks for explaining.
esprehn@chromium.org changed reviewers: - esprehn@chromium.org
https://codereview.chromium.org/2158423002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2158423002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2690: // In Mac a scroll begin with inertial_phase = true happens to handle a fling. In Mac -> On Mac https://codereview.chromium.org/2158423002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3250: // fling. This comment is a bit confusing, since it's talking about not clearing the scrolling layer in some cases, but the code clears the scrolling layer in some cases. You can probably word the comment as "Only clear the currently scrolling layer if we know the scroll is done. A non-inertial scroll end could be followed by an inertial scroll." or something like that. https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:46: : kDefaultWheelScrollTransactionMs; I'm a bit confused here. Scroll transactions only matter for latching, so I'm not sure what the difference between these constants is. https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.h:23: const int64_t kDefaultWheelScrollLatchingTransactionMs = 100; What's the difference between these constants? Why do we need a transaction time for Mac? https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:33: const bool kTouchpadScrollLating = true; Spelling.
Description was changed from ========== Touchpad scroll latching for Mac. The feature in enabled for Mac, behind a flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== The feature in enabled for Mac, behind a flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== The feature in enabled for Mac, behind a flag. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== The scroll latching feature for touchpad and wheel scrolls is enabled behind a flag. The current patch includes the flag definition and wheel scroll latching. The touchpad scroll latching support will be added in different cls. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
As discussed with tdresser@ I split the cl into two different cls: The current patch includes only flag definition and wheel scroll latching support behind the runtime flag. https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:46: : kDefaultWheelScrollTransactionMs; On 2016/07/22 15:43:15, tdresser wrote: > I'm a bit confused here. Scroll transactions only matter for latching, so I'm > not sure what the difference between these constants is. If the flag is enabled, wheel scroll latching will also happen. https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.h:23: const int64_t kDefaultWheelScrollLatchingTransactionMs = 100; On 2016/07/22 15:43:15, tdresser wrote: > What's the difference between these constants? Why do we need a transaction time > for Mac? This is for handling the cases that phase info is not available. e.g. Wheel scroll events On Mac. Maybe I should change the title of the patch, so that it represents both latching cases. https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:33: const bool kTouchpadScrollLating = true; On 2016/07/22 15:43:15, tdresser wrote: > Spelling. Done. https://codereview.chromium.org/2158423002/diff/40001/content/public/common/c... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2158423002/diff/40001/content/public/common/c... content/public/common/content_features.cc:123: // For now, it works only for Mac. On 2016/07/20 00:07:34, nasko wrote: > nit: Combine the two lines into one. Done.
https://codereview.chromium.org/2158423002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:41: touchpad_scroll_latching_(touchpad_scroll_latching), It doesn't seem this member variable is used anywhere in this class. Can't we just set the scroll_transaction_ms_ based on the constructor parameter and remove the member?
https://codereview.chromium.org/2158423002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:41: touchpad_scroll_latching_(touchpad_scroll_latching), On 2016/07/25 16:00:18, nasko wrote: > It doesn't seem this member variable is used anywhere in this class. Can't we > just set the scroll_transaction_ms_ based on the constructor parameter and > remove the member? You are right, for now it is not used. But I am working on a dependent patch that uses this flag to latch the touchpad scrolls. (we discussed with tdresser@ and he recommended to split the previous cl into two.) I didn't want to revert it to use the number in constructor and then change it back to this boolean parameter.
https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2158423002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.h:23: const int64_t kDefaultWheelScrollLatchingTransactionMs = 100; On 2016/07/25 15:54:53, sahel wrote: > On 2016/07/22 15:43:15, tdresser wrote: > > What's the difference between these constants? Why do we need a transaction > time > > for Mac? > > This is for handling the cases that phase info is not available. e.g. Wheel > scroll events On Mac. > Maybe I should change the title of the patch, so that it represents both > latching cases. We don't need a timeout when phase information is available. I think we just need one number, the transaction timeout for wheel. If we aren't latching for wheel, then we'll use 0ms, otherwise, we'll use the value of the flag. https://codereview.chromium.org/2158423002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:44: scroll_transaction_ms_ = touchpad_scroll_latching_ The timeout is only for wheel, not touchpad, right? So this bool is named incorrectly? https://codereview.chromium.org/2158423002/diff/60001/content/public/common/c... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2158423002/diff/60001/content/public/common/c... content/public/common/content_features.cc:122: // Enables touchpad scroll latching. For now, it works only for Mac. Are the comment and flag name correct? I'm not sure what the flag name should be, ideally it would indicate that this is for touchpad and wheel. Maybe kTouchpadAndWheelScrollLatching? https://codereview.chromium.org/2158423002/diff/60001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2158423002/diff/60001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:134: // In Mac flings are also scrolls. Why is there still Mac specific code?
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2158423002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:44: scroll_transaction_ms_ = touchpad_scroll_latching_ On 2016/07/25 16:46:55, tdresser wrote: > The timeout is only for wheel, not touchpad, right? > So this bool is named incorrectly? Done. https://codereview.chromium.org/2158423002/diff/60001/content/public/common/c... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2158423002/diff/60001/content/public/common/c... content/public/common/content_features.cc:122: // Enables touchpad scroll latching. For now, it works only for Mac. On 2016/07/25 16:46:55, tdresser wrote: > Are the comment and flag name correct? > > I'm not sure what the flag name should be, ideally it would indicate that this > is for touchpad and wheel. > > Maybe kTouchpadAndWheelScrollLatching? Done. https://codereview.chromium.org/2158423002/diff/60001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2158423002/diff/60001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:134: // In Mac flings are also scrolls. On 2016/07/25 16:46:55, tdresser wrote: > Why is there still Mac specific code? The code is not mac specific. if the inertialPhase of a scrollBegin event is momentumPhase, it means the scroll begin is for a fling and it happens on devices that don't send a different type of event for flings. I deleted this part of change for this cl.
Looking great! Can you add a test? https://codereview.chromium.org/2158423002/diff/120001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/120001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:41: touchpad_wheel_scroll_latching_(touchpad_wheel_scroll_latching), Sorry for the multiple rename requests... Since this logic is in the MouseWheelEventQueue, it would be reasonable to just call this enable_scroll_latching, or something like that. https://codereview.chromium.org/2158423002/diff/120001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2158423002/diff/120001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.h:21: // crbug.com/526463 is fully implemented. Update comment. https://codereview.chromium.org/2158423002/diff/120001/content/public/common/... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2158423002/diff/120001/content/public/common/... content/public/common/content_features.cc:122: // Enables touchpad and wheel scroll latching. For now, it works only for Mac. Doesn't it work on non-mac too?
https://codereview.chromium.org/2158423002/diff/120001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/120001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:41: touchpad_wheel_scroll_latching_(touchpad_wheel_scroll_latching), On 2016/07/25 20:37:29, tdresser wrote: > Sorry for the multiple rename requests... > > Since this logic is in the MouseWheelEventQueue, it would be reasonable to just > call this enable_scroll_latching, or something like that. Done. https://codereview.chromium.org/2158423002/diff/120001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2158423002/diff/120001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.h:21: // crbug.com/526463 is fully implemented. On 2016/07/25 20:37:29, tdresser wrote: > Update comment. Done. https://codereview.chromium.org/2158423002/diff/120001/content/public/common/... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2158423002/diff/120001/content/public/common/... content/public/common/content_features.cc:122: // Enables touchpad and wheel scroll latching. For now, it works only for Mac. On 2016/07/25 20:37:29, tdresser wrote: > Doesn't it work on non-mac too? The wheel part works, touchpad part, not yet. I deleted the comment.
https://codereview.chromium.org/2158423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2158423002/diff/140001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:151: new MouseWheelEventQueue(this, kTouchpadAndWheelScrollLatching)); should this be a parameterized test? that it runs with it enabled and not enabled?
https://codereview.chromium.org/2158423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2158423002/diff/140001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:151: new MouseWheelEventQueue(this, kTouchpadAndWheelScrollLatching)); On 2016/07/25 22:31:07, dtapuska wrote: > should this be a parameterized test? that it runs with it enabled and not > enabled? +1! (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...)
Description was changed from ========== The scroll latching feature for touchpad and wheel scrolls is enabled behind a flag. The current patch includes the flag definition and wheel scroll latching. The touchpad scroll latching support will be added in different cls. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== The scroll latching feature for touchpad and wheel scrolls is enabled behind a flag. The current patch includes the flag definition and wheel scroll latching. The touchpad scroll latching support will be added in different cls. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/0, MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/1 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
https://codereview.chromium.org/2158423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2158423002/diff/140001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:151: new MouseWheelEventQueue(this, kTouchpadAndWheelScrollLatching)); On 2016/07/25 22:31:07, dtapuska wrote: > should this be a parameterized test? that it runs with it enabled and not > enabled? Done.
I added a unittest to test the latching feature with both enabled and disabled values. Please let me know if a layout test is needed as well.
We do want an end-to-end integration test as well, which is probably easiest to write as a layout test. https://codereview.chromium.org/2158423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2158423002/diff/160001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:404: TEST_P(MouseWheelEventQueueTest, Basic) { Do we need to change all of these to TEST_P? (I'm not sure) https://codereview.chromium.org/2158423002/diff/160001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:563: TEST_P(MouseWheelEventQueueTest, WheelScrollLatching) { Add a local variable equal to GetParam(), with a better name. https://codereview.chromium.org/2158423002/diff/160001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:563: TEST_P(MouseWheelEventQueueTest, WheelScrollLatching) { Great, thanks!
I had to add a virtual test suite because I couldn't enable the feature with internals runtime flags. https://codereview.chromium.org/2158423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2158423002/diff/160001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:404: TEST_P(MouseWheelEventQueueTest, Basic) { On 2016/07/27 14:38:11, tdresser wrote: > Do we need to change all of these to TEST_P? (I'm not sure) Yes, because the constructor is called in these tests, and GetParam() is called in the constructor. TEST_F causes an error. https://codereview.chromium.org/2158423002/diff/160001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:563: TEST_P(MouseWheelEventQueueTest, WheelScrollLatching) { On 2016/07/27 14:38:12, tdresser wrote: > Add a local variable equal to GetParam(), with a better name. Done. https://codereview.chromium.org/2158423002/diff/160001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:563: TEST_P(MouseWheelEventQueueTest, WheelScrollLatching) { On 2016/07/27 14:38:12, tdresser wrote: > Great, thanks! Acknowledged.
tdresser@chromium.org changed reviewers: + lanwei@chromium.org
+lanwei@ to comment on use of gpu benchmarking extension. https://codereview.chromium.org/2158423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2158423002/diff/160001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:404: TEST_P(MouseWheelEventQueueTest, Basic) { On 2016/08/11 15:57:39, sahel wrote: > On 2016/07/27 14:38:11, tdresser wrote: > > Do we need to change all of these to TEST_P? (I'm not sure) > > Yes, because the constructor is called in these tests, and GetParam() is called > in the constructor. > TEST_F causes an error. Acknowledged. https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:41: enable_scroll_latching_(enable_scroll_latching), Do we need a local variable here? Or can we just assign scroll_transaction_ms_ based on the value of the constructor parameter? https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.h:19: // ScrollUpdate was sent for wheel based gesture scrolls. Indicate that this is only used if |enable_scroll_latching_| is true. (Or if we get rid of enable_scroll_latching_, if |enable_scroll_latching| is true in the constructor). https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:180: bool ScrollLatchingEnabled() { return ScrollLatchingEnabled_; } scroll_latching_enabled() Thin getters should be in underscore case. https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:402: int64_t ScrollEndTimeoutMs_; scroll_end_timeout_ms_ https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:403: bool ScrollLatchingEnabled_; scroll_latching_enabled_ https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:590: // Scroll end time out happens. If scroll latching isn't enabled, there's no timeout, is there? Isn't it sent immediately? https://codereview.chromium.org/2158423002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/wheel/mouse-wheel-scroll-latching.html (right): https://codereview.chromium.org/2158423002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/wheel/mouse-wheel-scroll-latching.html:52: if (!window.chrome || !chrome.gpuBenchmarking) { Should we output some kind of message that the test didn't run? See if you can find a test doing something similar, if they don't output a warning, this is fine. https://codereview.chromium.org/2158423002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/wheel/mouse-wheel-scroll-latching.html:56: chrome.gpuBenchmarking.smoothScrollBy(150, function() {}, Can you pass null instead of an empty function? I'm not sure, but I think you may want to test scrollTop in the callback for smoothScrollBy. +lanwei to comment on that. https://codereview.chromium.org/2158423002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/wheel/mouse-wheel-scroll-latching.html:64: setTimeout(parentDivScrollCheck, 500); You might not need this setTimeout if you move to testing scrollTop in the callback.
https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:41: enable_scroll_latching_(enable_scroll_latching), On 2016/08/12 13:28:32, tdresser wrote: > Do we need a local variable here? Or can we just assign scroll_transaction_ms_ > based on the value of the constructor parameter? For now the boolean variable is only used to set the scroll_transaction_ms_, but we will need the boolean value to avoid sending the synthesized scroll begins and ends for every update, when we have the phase info. https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.h:19: // ScrollUpdate was sent for wheel based gesture scrolls. On 2016/08/12 13:28:32, tdresser wrote: > Indicate that this is only used if |enable_scroll_latching_| is true. > (Or if we get rid of enable_scroll_latching_, if |enable_scroll_latching| is > true in the constructor). Done. https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:180: bool ScrollLatchingEnabled() { return ScrollLatchingEnabled_; } On 2016/08/12 13:28:32, tdresser wrote: > scroll_latching_enabled() > > Thin getters should be in underscore case. Done. https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:402: int64_t ScrollEndTimeoutMs_; On 2016/08/12 13:28:32, tdresser wrote: > scroll_end_timeout_ms_ Done. https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:403: bool ScrollLatchingEnabled_; On 2016/08/12 13:28:32, tdresser wrote: > scroll_latching_enabled_ Done. https://codereview.chromium.org/2158423002/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:590: // Scroll end time out happens. On 2016/08/12 13:28:32, tdresser wrote: > If scroll latching isn't enabled, there's no timeout, is there? > Isn't it sent immediately? Yes, the timeout is zero. I think the comment is more confusing rather than clarifying. I'll delete it. https://codereview.chromium.org/2158423002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/wheel/mouse-wheel-scroll-latching.html (right): https://codereview.chromium.org/2158423002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/wheel/mouse-wheel-scroll-latching.html:52: if (!window.chrome || !chrome.gpuBenchmarking) { On 2016/08/12 13:28:32, tdresser wrote: > Should we output some kind of message that the test didn't run? > See if you can find a test doing something similar, if they don't output a > warning, this is fine. Done. https://codereview.chromium.org/2158423002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/wheel/mouse-wheel-scroll-latching.html:56: chrome.gpuBenchmarking.smoothScrollBy(150, function() {}, On 2016/08/12 13:28:32, tdresser wrote: > Can you pass null instead of an empty function? > > I'm not sure, but I think you may want to test scrollTop in the callback for > smoothScrollBy. > > +lanwei to comment on that. Done. https://codereview.chromium.org/2158423002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/wheel/mouse-wheel-scroll-latching.html:64: setTimeout(parentDivScrollCheck, 500); On 2016/08/12 13:28:32, tdresser wrote: > You might not need this setTimeout if you move to testing scrollTop in the > callback. Done.
LGTM!
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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.blink
For more details, see http://crbug.com/617627.
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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.blink
For more details, see http://crbug.com/617627.
LGTM on the chrome.gpuBenchmarking function. You cannot pass null to the function, have to be an empty function.
content/ rubberstamp LGTM
Description was changed from ========== The scroll latching feature for touchpad and wheel scrolls is enabled behind a flag. The current patch includes the flag definition and wheel scroll latching. The touchpad scroll latching support will be added in different cls. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/0, MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/1 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== The scroll latching feature for touchpad and wheel scrolls is enabled behind a flag. The current patch includes the flag definition and wheel scroll latching. The touchpad scroll latching support will be added in different cls. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/0, MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/1 ==========
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_asan_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: linux_chromium_chromeos_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: 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
The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org, dtapuska@chromium.org, lanwei@chromium.org, nasko@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2158423002/#ps240001 (title: " ")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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.
The CQ bit was checked by sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org, dtapuska@chromium.org, lanwei@chromium.org, nasko@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2158423002/#ps260001 (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.
Description was changed from ========== The scroll latching feature for touchpad and wheel scrolls is enabled behind a flag. The current patch includes the flag definition and wheel scroll latching. The touchpad scroll latching support will be added in different cls. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/0, MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/1 ========== to ========== The scroll latching feature for touchpad and wheel scrolls is enabled behind a flag. The current patch includes the flag definition and wheel scroll latching. The touchpad scroll latching support will be added in different cls. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/0, MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/1 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== The scroll latching feature for touchpad and wheel scrolls is enabled behind a flag. The current patch includes the flag definition and wheel scroll latching. The touchpad scroll latching support will be added in different cls. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/0, MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/1 ========== to ========== The scroll latching feature for touchpad and wheel scrolls is enabled behind a flag. The current patch includes the flag definition and wheel scroll latching. The touchpad scroll latching support will be added in different cls. To test it, use the --enable-features=TouchpadScrollLatching runtime flag. BUG=526463 TEST=MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/0, MouseWheelEventQueueTests/MouseWheelEventQueueTest.WheelScrollLatching/1 Committed: https://crrev.com/b9e7f19ba7f4d11ce46fc3217d542193b7904ed8 Cr-Commit-Position: refs/heads/master@{#411974} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/b9e7f19ba7f4d11ce46fc3217d542193b7904ed8 Cr-Commit-Position: refs/heads/master@{#411974} |
