Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2625453002:
Touchpad and wheel scroll latching for ChromeOS behind flag. (Closed)
|
Created:
3 years, 11 months ago by sahel Modified:
3 years, 11 months ago CC:
chromium-reviews, jam, tdresser+watch_chromium.org, darin-cc_chromium.org, dtapuska+chromiumwatch_chromium.org, mlamouri+watch-content_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTouchpad and wheel scroll latching for ChromeOS behind flag.
To enable latching, MouseWheelEventQueue(MWEQ) uses a timer to send GSE
events. When a fling arrives if the timer is running, MWEQ will stop it
and won't send the pending GSE.
In input_handler_proxy, ScrollBegin is called only once in
HandleGestureFlingStart. This ScrollBegin won't do a new hittesting
because CurrentlyScrollingLayer already exists (No GSE is sent before
fling to clear the CurrentlyScrollingLayer when it's being handled).
ScrollEnd won't get called in FlingScrollByMouseWheel for every wheel tick.
Instead, it will be called only once in CancelCurrentFlingWithoutNotifyingClient,
when the fling is over (last ScrollBy didn't scroll or fling time is terminated).
To test locally use the --enable-features=TouchpadAndWheelScrollLatching
runtime flag.
BUG=526463
TEST=InputHandlerProxyTest.GestureFlingTouchpadScrollLatchingEnabled
Review-Url: https://codereview.chromium.org/2625453002
Cr-Commit-Position: refs/heads/master@{#443332}
Committed: https://chromium.googlesource.com/chromium/src/+/4ced61f2e1215b847fc726343aecd410db03da5c
Patch Set 1 #
Total comments: 32
Patch Set 2 : unittest changed #Patch Set 3 : ScrollEnd gets called when the fling ends before hitting a scroll extent. #
Total comments: 2
Messages
Total messages: 36 (19 generated)
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.
sahel@chromium.org changed reviewers: + tdresser@chromium.org
tdresser@chromium.org changed reviewers: + bokan@chromium.org
LGTM. +bokan to take a look as well. https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:210: // no phase info exists, start the scroll_end_timer_. The scroll end timer should only be running if latching is enabled, right? https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:281: if (scroll_end_timer_.IsRunning()) { This should only happen if latching is enabled, right? Can we DCHECK that latching is enabled?
https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:210: // no phase info exists, start the scroll_end_timer_. On 2017/01/09 18:25:19, tdresser wrote: > The scroll end timer should only be running if latching is enabled, right? Right, it should be running, but we need to reset it which is done in start. https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:281: if (scroll_end_timer_.IsRunning()) { On 2017/01/09 18:25:19, tdresser wrote: > This should only happen if latching is enabled, right? Can we DCHECK that > latching is enabled? If the latching is not enabled then the timeout for the timer will be zero, is it possible (in some rare cases) to have the timer running when Fling arrives? If not, then you are right we can add a DCHECK and get rid of the else part.
https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:281: if (scroll_end_timer_.IsRunning()) { On 2017/01/09 20:02:01, sahel wrote: > On 2017/01/09 18:25:19, tdresser wrote: > > This should only happen if latching is enabled, right? Can we DCHECK that > > latching is enabled? > > If the latching is not enabled then the timeout for the timer will be zero, is > it possible (in some rare cases) to have the timer running when Fling arrives? > If not, then you are right we can add a DCHECK and get rid of the else part. I missed that we do start the timer when latching is disabled. This LGTM.
Cool, mostly questions for my own benefit and some suggested tweaks to the test but looks good otherwise. https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:210: // no phase info exists, start the scroll_end_timer_. On 2017/01/09 20:02:01, sahel wrote: > On 2017/01/09 18:25:19, tdresser wrote: > > The scroll end timer should only be running if latching is enabled, right? > > Right, it should be running, but we need to reset it which is done in start. For my own understanding: there's a comment above "Only OSX populates the phase and momentumPhase", does this mean that momentum_phase_ended and scroll_phase_ended will always be false here for non-OSX (but we'll come in here through |!has_phase_info|? https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:281: if (scroll_end_timer_.IsRunning()) { On 2017/01/09 20:02:01, sahel wrote: > On 2017/01/09 18:25:19, tdresser wrote: > > This should only happen if latching is enabled, right? Can we DCHECK that > > latching is enabled? > > If the latching is not enabled then the timeout for the timer will be zero, is > it possible (in some rare cases) to have the timer running when Fling arrives? > If not, then you are right we can add a DCHECK and get rid of the else part. I don't actually know how we implement task queues, but I'd assume that a 0-delay timer would get queued but might be behind an already queued FlingStart. (unless they're in separate queues and timers get priority) Perhaps if latching is disabled, rather than queuing the 0-delay timer, we should just synchronously run it? https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:286: needs_scroll_begin_ = true; If the event is a GFS, do we still need a GSB? https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:623: DCHECK(!wheel_event.railsMode); Is this because the TODO was resolved? https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:626: DCHECK(!wheel_event.scrollByPage); The bug linked with the TODO(jamesr) is still open. Why is it ok to DCHECK this now? https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:629: if (gesture_scroll_on_impl_thread_) { nit: if (!gesture_scroll_on_impl_thread_) return DID_NOT_HANDLE; https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:639: scroll_state_update_data.position_y = wheel_event.y; This code is duplicated from below. Instead of copying it up here, find a way to reuse it. e.g. Presumably you want to avoid the ScrollBegin, just guard it in |!touchpad_and_wheel_scroll_latching_enabled_| https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:970: GetEventListenerProperties(cc::EventListenerClass::kMouseWheel)) I don't think we need this check. It seems to be over-specifying the behavior and isn't really what we're testing here. That is, if we changed the behavior so that the first Animate call does actually scroll, that wouldn't be breaking the latching behavior you're introducing. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:981: .WillOnce(testing::Return(cc::EventListenerProperties::kNone)); Also don't need this, I think. The ScrollBy below should be enough. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:1021: EXPECT_CALL(mock_input_handler_, I don't think you need this. All we do form here is clean up.
https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:210: // no phase info exists, start the scroll_end_timer_. On 2017/01/10 18:53:58, bokan wrote: > On 2017/01/09 20:02:01, sahel wrote: > > On 2017/01/09 18:25:19, tdresser wrote: > > > The scroll end timer should only be running if latching is enabled, right? > > > > Right, it should be running, but we need to reset it which is done in start. > > For my own understanding: there's a comment above "Only OSX populates the phase > and momentumPhase", does this mean that momentum_phase_ended and > scroll_phase_ended will always be false here for non-OSX (but we'll come in here > through |!has_phase_info|? correct. https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:281: if (scroll_end_timer_.IsRunning()) { On 2017/01/10 18:53:58, bokan wrote: > On 2017/01/09 20:02:01, sahel wrote: > > On 2017/01/09 18:25:19, tdresser wrote: > > > This should only happen if latching is enabled, right? Can we DCHECK that > > > latching is enabled? > > > > If the latching is not enabled then the timeout for the timer will be zero, is > > it possible (in some rare cases) to have the timer running when Fling arrives? > > If not, then you are right we can add a DCHECK and get rid of the else part. > > I don't actually know how we implement task queues, but I'd assume that a > 0-delay timer would get queued but might be behind an already queued FlingStart. > (unless they're in separate queues and timers get priority) > > Perhaps if latching is disabled, rather than queuing the 0-delay timer, we > should just synchronously run it? I don't know if timer tasks have priority or not, maybe @tdresser can help with the answer. The timer starts even when the latching is disabled. I don't know if we always want to have zero timeout in this case or not, if the answer is yes, then we can get rid of the timer when latching is disabled. https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:286: needs_scroll_begin_ = true; On 2017/01/10 18:53:58, bokan wrote: > If the event is a GFS, do we still need a GSB? Yes, this is for making the gesture event sequences for touchscreen and touchpad scrolls similar. For touchpad we will have GSB (GSU)xn GFS we won't send a GSE after/before GFS and that's why needs_scroll_begin_ is set to true directly for the next GSU. The next GSU won't be for this scroll sequence because when we are handling touchpad fling, we don't generate GSU events after the fling. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:623: DCHECK(!wheel_event.railsMode); On 2017/01/10 18:53:58, bokan wrote: > Is this because the TODO was resolved? No it's not, it is because this function is only called in touchpadScrollFling and the synthetic_wheel event there doesn't have vertical/horizontal rail mode. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:626: DCHECK(!wheel_event.scrollByPage); On 2017/01/10 18:53:58, bokan wrote: > The bug linked with the TODO(jamesr) is still open. Why is it ok to DCHECK this > now? Similarly, this part of the code was never executed. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:629: if (gesture_scroll_on_impl_thread_) { On 2017/01/10 18:53:58, bokan wrote: > nit: > > if (!gesture_scroll_on_impl_thread_) > return DID_NOT_HANDLE; Done. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:639: scroll_state_update_data.position_y = wheel_event.y; On 2017/01/10 18:53:58, bokan wrote: > This code is duplicated from below. Instead of copying it up here, find a way to > reuse it. e.g. Presumably you want to avoid the ScrollBegin, just guard it in > |!touchpad_and_wheel_scroll_latching_enabled_| I initially did that in mouse_wheel_event_queue. I got this review comment saying that since we will get rid of the flag, copying some parts of code is prefered because this way it's easier to delete the |!touchpad_and_wheel_scroll_latching_enabled_| code path. I can still change the code to avoid copying but I will have more fine grain if(touchpad...) conditions. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:970: GetEventListenerProperties(cc::EventListenerClass::kMouseWheel)) On 2017/01/10 18:53:59, bokan wrote: > I don't think we need this check. It seems to be over-specifying the behavior > and isn't really what we're testing here. That is, if we changed the behavior so > that the first Animate call does actually scroll, that wouldn't be breaking the > latching behavior you're introducing. Done. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:981: .WillOnce(testing::Return(cc::EventListenerProperties::kNone)); On 2017/01/10 18:53:58, bokan wrote: > Also don't need this, I think. The ScrollBy below should be enough. I wanted to show that while ScrollBy in FlingScrollByMouseWheel returns true, the next Animate will (indirectly) call the FlingScrollByMouseWheel and once the ScrollBy result is false, the next Animate won't cause FlingScrollByMouseWheel to get called anymore. I can get rid of it, if it's not needed. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:1021: EXPECT_CALL(mock_input_handler_, On 2017/01/10 18:53:59, bokan wrote: > I don't think you need this. All we do form here is clean up. When latching is enabled, ScrollEnd is called in FlingScrollByMouseWheel only when did_scroll is false (rather than every time after ScrollBy). This is for showing that once scrollEnd is called, Animate won't call FlingScrollByMouseWheel anymore. I can get read of this part if you still think it's not needed.
Ok, still have some more questions but you don't need to wait to land. lgtm. Another thing, with this patch we send a GSE when did_scroll is false. Does this meant that if each scroll tick has did_scroll == true, so the fling doesn't hit an extent, that means we don't get a GSE? Put another way, we only send a GSE if the fling hits a scroll extent? https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:286: needs_scroll_begin_ = true; On 2017/01/10 21:10:06, sahel wrote: > On 2017/01/10 18:53:58, bokan wrote: > > If the event is a GFS, do we still need a GSB? > > Yes, this is for making the gesture event sequences for touchscreen and touchpad > scrolls similar. > For touchpad we will have GSB (GSU)xn GFS > we won't send a GSE after/before GFS and that's why needs_scroll_begin_ is set > to true directly for the next GSU. The next GSU won't be for this scroll > sequence because when we are handling touchpad fling, we don't generate GSU > events after the fling. Ah, ok. Thanks for the explanation. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:639: scroll_state_update_data.position_y = wheel_event.y; On 2017/01/10 21:10:06, sahel wrote: > On 2017/01/10 18:53:58, bokan wrote: > > This code is duplicated from below. Instead of copying it up here, find a way > to > > reuse it. e.g. Presumably you want to avoid the ScrollBegin, just guard it in > > |!touchpad_and_wheel_scroll_latching_enabled_| > > I initially did that in mouse_wheel_event_queue. I got this review comment > saying that since we will get rid of the flag, copying some parts of code is > prefered because this way it's easier to delete the > |!touchpad_and_wheel_scroll_latching_enabled_| code path. > > I can still change the code to avoid copying but I will have more fine grain > if(touchpad...) conditions. Ok, if it's going away eventually duplication is fine. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:981: .WillOnce(testing::Return(cc::EventListenerProperties::kNone)); On 2017/01/10 21:10:06, sahel wrote: > On 2017/01/10 18:53:58, bokan wrote: > > Also don't need this, I think. The ScrollBy below should be enough. > > I wanted to show that while ScrollBy in FlingScrollByMouseWheel returns true, > the next Animate will (indirectly) call the FlingScrollByMouseWheel and once the > ScrollBy result is false, the next Animate won't cause FlingScrollByMouseWheel > to get called anymore. > > I can get rid of it, if it's not needed. I guess I'm confused, how does GetEventListenerProperties relate to all this? https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:1021: EXPECT_CALL(mock_input_handler_, On 2017/01/10 21:10:06, sahel wrote: > On 2017/01/10 18:53:59, bokan wrote: > > I don't think you need this. All we do form here is clean up. > > When latching is enabled, ScrollEnd is called in FlingScrollByMouseWheel only > when did_scroll is false (rather than every time after ScrollBy). > > This is for showing that once scrollEnd is called, Animate won't call > FlingScrollByMouseWheel anymore. I can get read of this part if you still think > it's not needed. But you don't call Animate after it...
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) 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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Touchpad and wheel scroll latching for ChromeOS behind flag. To enable latching, MouseWheelEventQueue(MWEQ) uses a timer to send GSE events. When a fling arrives if the timer is running, MWEQ will stop it and won't send the pending GSE. In input_handler_proxy, ScrollBegin is called only once in HandleGestureFlingStart. This ScrollBegin won't do a new hittesting because CurrentlyScrollingLayer already exists (No GSE is sent before fling to clear the CurrentlyScrollingLayer when it's being handled). ScrollEnd is called only once in FlingScrollByMouseWheel when the last ScrollBy did not scroll. To test locally use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 TEST=InputHandlerProxyTest.GestureFlingTouchpadScrollLatchingEnabled ========== to ========== Touchpad and wheel scroll latching for ChromeOS behind flag. To enable latching, MouseWheelEventQueue(MWEQ) uses a timer to send GSE events. When a fling arrives if the timer is running, MWEQ will stop it and won't send the pending GSE. In input_handler_proxy, ScrollBegin is called only once in HandleGestureFlingStart. This ScrollBegin won't do a new hittesting because CurrentlyScrollingLayer already exists (No GSE is sent before fling to clear the CurrentlyScrollingLayer when it's being handled). ScrollEnd won't get called in FlingScrollByMouseWheel for every wheel tick. Instead, it will be called only once in CancelCurrentFlingWithoutNotifyingClient when the fling is over (last ScrollBy didn't scroll or fling time is terminated). To test locally use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 TEST=InputHandlerProxyTest.GestureFlingTouchpadScrollLatchingEnabled ==========
Description was changed from ========== Touchpad and wheel scroll latching for ChromeOS behind flag. To enable latching, MouseWheelEventQueue(MWEQ) uses a timer to send GSE events. When a fling arrives if the timer is running, MWEQ will stop it and won't send the pending GSE. In input_handler_proxy, ScrollBegin is called only once in HandleGestureFlingStart. This ScrollBegin won't do a new hittesting because CurrentlyScrollingLayer already exists (No GSE is sent before fling to clear the CurrentlyScrollingLayer when it's being handled). ScrollEnd won't get called in FlingScrollByMouseWheel for every wheel tick. Instead, it will be called only once in CancelCurrentFlingWithoutNotifyingClient when the fling is over (last ScrollBy didn't scroll or fling time is terminated). To test locally use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 TEST=InputHandlerProxyTest.GestureFlingTouchpadScrollLatchingEnabled ========== to ========== Touchpad and wheel scroll latching for ChromeOS behind flag. To enable latching, MouseWheelEventQueue(MWEQ) uses a timer to send GSE events. When a fling arrives if the timer is running, MWEQ will stop it and won't send the pending GSE. In input_handler_proxy, ScrollBegin is called only once in HandleGestureFlingStart. This ScrollBegin won't do a new hittesting because CurrentlyScrollingLayer already exists (No GSE is sent before fling to clear the CurrentlyScrollingLayer when it's being handled). ScrollEnd won't get called in FlingScrollByMouseWheel for every wheel tick. Instead, it will be called only once in CancelCurrentFlingWithoutNotifyingClient, when the fling is over (last ScrollBy didn't scroll or fling time is terminated). To test locally use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 TEST=InputHandlerProxyTest.GestureFlingTouchpadScrollLatchingEnabled ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/10 21:25:06, bokan wrote: > Ok, still have some more questions but you don't need to wait to land. lgtm. > > Another thing, with this patch we send a GSE when did_scroll is false. Does this > meant that if each scroll tick has did_scroll == true, so the fling doesn't hit > an extent, that means we don't get a GSE? Put another way, we only send a GSE if > the fling hits a scroll extent? You are right, and that was a bug, I fixed it by calling the scrollEnd once when the fling is over (either because of the last scrollBy not scrolling or fling time termination.) I didn't need to add a new test because my current test was failing by only adding the code to CancelCurrentFlingWithoutNotifyingClient and not deleting the ScrollEnd call in FlingScrollByMouseWheel. > > https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): > > https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:286: > needs_scroll_begin_ = true; > On 2017/01/10 21:10:06, sahel wrote: > > On 2017/01/10 18:53:58, bokan wrote: > > > If the event is a GFS, do we still need a GSB? > > > > Yes, this is for making the gesture event sequences for touchscreen and > touchpad > > scrolls similar. > > For touchpad we will have GSB (GSU)xn GFS > > we won't send a GSE after/before GFS and that's why needs_scroll_begin_ is set > > to true directly for the next GSU. The next GSU won't be for this scroll > > sequence because when we are handling touchpad fling, we don't generate GSU > > events after the fling. > > Ah, ok. Thanks for the explanation. > > https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... > File ui/events/blink/input_handler_proxy.cc (right): > > https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... > ui/events/blink/input_handler_proxy.cc:639: scroll_state_update_data.position_y > = wheel_event.y; > On 2017/01/10 21:10:06, sahel wrote: > > On 2017/01/10 18:53:58, bokan wrote: > > > This code is duplicated from below. Instead of copying it up here, find a > way > > to > > > reuse it. e.g. Presumably you want to avoid the ScrollBegin, just guard it > in > > > |!touchpad_and_wheel_scroll_latching_enabled_| > > > > I initially did that in mouse_wheel_event_queue. I got this review comment > > saying that since we will get rid of the flag, copying some parts of code is > > prefered because this way it's easier to delete the > > |!touchpad_and_wheel_scroll_latching_enabled_| code path. > > > > I can still change the code to avoid copying but I will have more fine grain > > if(touchpad...) conditions. > > Ok, if it's going away eventually duplication is fine. > > https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... > File ui/events/blink/input_handler_proxy_unittest.cc (right): > > https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... > ui/events/blink/input_handler_proxy_unittest.cc:981: > .WillOnce(testing::Return(cc::EventListenerProperties::kNone)); > On 2017/01/10 21:10:06, sahel wrote: > > On 2017/01/10 18:53:58, bokan wrote: > > > Also don't need this, I think. The ScrollBy below should be enough. > > > > I wanted to show that while ScrollBy in FlingScrollByMouseWheel returns true, > > the next Animate will (indirectly) call the FlingScrollByMouseWheel and once > the > > ScrollBy result is false, the next Animate won't cause FlingScrollByMouseWheel > > to get called anymore. > > > > I can get rid of it, if it's not needed. > > I guess I'm confused, how does GetEventListenerProperties relate to all this? > > https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... > ui/events/blink/input_handler_proxy_unittest.cc:1021: > EXPECT_CALL(mock_input_handler_, > On 2017/01/10 21:10:06, sahel wrote: > > On 2017/01/10 18:53:59, bokan wrote: > > > I don't think you need this. All we do form here is clean up. > > > > When latching is enabled, ScrollEnd is called in FlingScrollByMouseWheel only > > when did_scroll is false (rather than every time after ScrollBy). > > > > This is for showing that once scrollEnd is called, Animate won't call > > FlingScrollByMouseWheel anymore. I can get read of this part if you still > think > > it's not needed. > > But you don't call Animate after it...
https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:639: scroll_state_update_data.position_y = wheel_event.y; On 2017/01/10 21:25:06, bokan wrote: > On 2017/01/10 21:10:06, sahel wrote: > > On 2017/01/10 18:53:58, bokan wrote: > > > This code is duplicated from below. Instead of copying it up here, find a > way > > to > > > reuse it. e.g. Presumably you want to avoid the ScrollBegin, just guard it > in > > > |!touchpad_and_wheel_scroll_latching_enabled_| > > > > I initially did that in mouse_wheel_event_queue. I got this review comment > > saying that since we will get rid of the flag, copying some parts of code is > > prefered because this way it's easier to delete the > > |!touchpad_and_wheel_scroll_latching_enabled_| code path. > > > > I can still change the code to avoid copying but I will have more fine grain > > if(touchpad...) conditions. > > Ok, if it's going away eventually duplication is fine. Acknowledged. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:981: .WillOnce(testing::Return(cc::EventListenerProperties::kNone)); On 2017/01/10 21:25:06, bokan wrote: > On 2017/01/10 21:10:06, sahel wrote: > > On 2017/01/10 18:53:58, bokan wrote: > > > Also don't need this, I think. The ScrollBy below should be enough. > > > > I wanted to show that while ScrollBy in FlingScrollByMouseWheel returns true, > > the next Animate will (indirectly) call the FlingScrollByMouseWheel and once > the > > ScrollBy result is false, the next Animate won't cause FlingScrollByMouseWheel > > to get called anymore. > > > > I can get rid of it, if it's not needed. > > I guess I'm confused, how does GetEventListenerProperties relate to all this? I want to check whether FlingScrollByMouseWheel is called or not. I cannot MOCK the function, because the change for latching is done in the body of this function. Therefore, I check GetEventListenerProperties which is called in this function to indirectly check if the FlingScrollByMouseWheel is called or not. https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy_unittest.cc:1021: EXPECT_CALL(mock_input_handler_, On 2017/01/10 21:25:06, bokan wrote: > On 2017/01/10 21:10:06, sahel wrote: > > On 2017/01/10 18:53:59, bokan wrote: > > > I don't think you need this. All we do form here is clean up. > > > > When latching is enabled, ScrollEnd is called in FlingScrollByMouseWheel only > > when did_scroll is false (rather than every time after ScrollBy). > > > > This is for showing that once scrollEnd is called, Animate won't call > > FlingScrollByMouseWheel anymore. I can get read of this part if you still > think > > it's not needed. > > But you don't call Animate after it... I didn't know the behavior of VERIFY_AND_RESET_MOCKS() As we discussed I moved this checking before the last VERIFY_AND_RESET_MOCKS();
lgtm % comment https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_... ui/events/blink/input_handler_proxy_unittest.cc:1014: EXPECT_CALL(mock_input_handler_, ScrollBy(testing::_)).Times(0); This needs to go above the Animate call to be effective I think. btw, if you haven't seen it, reading through at least the beginning of https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide is super helpful.
https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_... ui/events/blink/input_handler_proxy_unittest.cc:1014: EXPECT_CALL(mock_input_handler_, ScrollBy(testing::_)).Times(0); On 2017/01/12 19:33:17, bokan wrote: > This needs to go above the Animate call to be effective I think. btw, if you > haven't seen it, reading through at least the beginning of > https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide is super helpful. Thanks for the doc, I will do that. I think it's effective as it is now, when I change the Times(0) to Timers(1) the test fails.
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/2625453002/#ps100001 (title: "ScrollEnd gets called when the fling ends before hitting a scroll extent.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484250329310430,
"parent_rev": "1b3c106d1b1ad224ab5099d52fee173e95ff02ae", "commit_rev":
"4ced61f2e1215b847fc726343aecd410db03da5c"}
Message was sent while issue was closed.
Description was changed from ========== Touchpad and wheel scroll latching for ChromeOS behind flag. To enable latching, MouseWheelEventQueue(MWEQ) uses a timer to send GSE events. When a fling arrives if the timer is running, MWEQ will stop it and won't send the pending GSE. In input_handler_proxy, ScrollBegin is called only once in HandleGestureFlingStart. This ScrollBegin won't do a new hittesting because CurrentlyScrollingLayer already exists (No GSE is sent before fling to clear the CurrentlyScrollingLayer when it's being handled). ScrollEnd won't get called in FlingScrollByMouseWheel for every wheel tick. Instead, it will be called only once in CancelCurrentFlingWithoutNotifyingClient, when the fling is over (last ScrollBy didn't scroll or fling time is terminated). To test locally use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 TEST=InputHandlerProxyTest.GestureFlingTouchpadScrollLatchingEnabled ========== to ========== Touchpad and wheel scroll latching for ChromeOS behind flag. To enable latching, MouseWheelEventQueue(MWEQ) uses a timer to send GSE events. When a fling arrives if the timer is running, MWEQ will stop it and won't send the pending GSE. In input_handler_proxy, ScrollBegin is called only once in HandleGestureFlingStart. This ScrollBegin won't do a new hittesting because CurrentlyScrollingLayer already exists (No GSE is sent before fling to clear the CurrentlyScrollingLayer when it's being handled). ScrollEnd won't get called in FlingScrollByMouseWheel for every wheel tick. Instead, it will be called only once in CancelCurrentFlingWithoutNotifyingClient, when the fling is over (last ScrollBy didn't scroll or fling time is terminated). To test locally use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 TEST=InputHandlerProxyTest.GestureFlingTouchpadScrollLatchingEnabled Review-Url: https://codereview.chromium.org/2625453002 Cr-Commit-Position: refs/heads/master@{#443332} Committed: https://chromium.googlesource.com/chromium/src/+/4ced61f2e1215b847fc726343aec... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4ced61f2e1215b847fc726343aec...
Message was sent while issue was closed.
On 2017/01/12 19:45:22, sahel wrote: > https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_... > File ui/events/blink/input_handler_proxy_unittest.cc (right): > > https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_... > ui/events/blink/input_handler_proxy_unittest.cc:1014: > EXPECT_CALL(mock_input_handler_, ScrollBy(testing::_)).Times(0); > On 2017/01/12 19:33:17, bokan wrote: > > This needs to go above the Animate call to be effective I think. btw, if you > > haven't seen it, reading through at least the beginning of > > https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide is super helpful. > > Thanks for the doc, I will do that. > I think it's effective as it is now, when I change the Times(0) to Timers(1) the > test fails. I would assume that's because there's nothing between the EXPECT and VERIFY macros so Times(0) will always pass since nothing happens between. So if you change it to Times(1), nothing happens between the two calls so it isn't called. If you expected Animation to call ScrollBy 1 times, and you set EXPECT().Times(1) after the Animate call, it would fail even if Animate did call ScrollBy. My understanding of gMock is that you set expectations *prior* to running your code.
Message was sent while issue was closed.
On 2017/01/12 19:57:55, bokan wrote: > On 2017/01/12 19:45:22, sahel wrote: > > > https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_... > > File ui/events/blink/input_handler_proxy_unittest.cc (right): > > > > > https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_... > > ui/events/blink/input_handler_proxy_unittest.cc:1014: > > EXPECT_CALL(mock_input_handler_, ScrollBy(testing::_)).Times(0); > > On 2017/01/12 19:33:17, bokan wrote: > > > This needs to go above the Animate call to be effective I think. btw, if you > > > haven't seen it, reading through at least the beginning of > > > https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide is super > helpful. > > > > Thanks for the doc, I will do that. > > I think it's effective as it is now, when I change the Times(0) to Timers(1) > the > > test fails. > > I would assume that's because there's nothing between the EXPECT and VERIFY > macros so Times(0) will always pass since nothing happens between. So if you > change it to Times(1), nothing happens between the two calls so it isn't called. > If you expected Animation to call ScrollBy 1 times, and you set > EXPECT().Times(1) after the Animate call, it would fail even if Animate did call > ScrollBy. My understanding of gMock is that you set expectations *prior* to > running your code. Ah, found it in here: https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#Using_Mocks_in_Tests The specific quote: "Important note: gMock requires expectations to be set before the mock functions are called, otherwise the behavior is undefined. In particular, you mustn't interleave EXPECT_CALL()s and calls to the mock functions."
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:100001) has been created in https://codereview.chromium.org/2630603002/ by sahel@chromium.org. The reason for reverting is: EXPECT_CALL(mock_input_handler_, ScrollBy(testing::_)).Times(0); line at the end of the unittest won't do anything. It doesn't break anything but it doesn't do the intended check either. I have to call Animate() after this line to do the checking..
Message was sent while issue was closed.
On 2017/01/12 20:30:50, sahel wrote: > A revert of this CL (patchset #3 id:100001) has been created in > https://codereview.chromium.org/2630603002/ by mailto:sahel@chromium.org. > > The reason for reverting is: EXPECT_CALL(mock_input_handler_, > ScrollBy(testing::_)).Times(0); line at the end of the unittest won't do > anything. It doesn't break anything but it doesn't do the intended check either. > > I have to call Animate() after this line to do the checking.. Sahel canceled the revert and will address the issue in a follow-up patch for cleaner revision history |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
