Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(10)

Issue 2625453002: Touchpad and wheel scroll latching for ChromeOS behind flag. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 2 weeks ago by sahel
Modified:
5 months, 1 week ago
Reviewers:
bokan, tdresser
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.

Description

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/+/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
Unified diffs Side-by-side diffs Delta from patch set Stats (+155 lines, -44 lines) Patch
M content/browser/renderer_host/input/mouse_wheel_event_queue.cc View 1 chunk +11 lines, -2 lines 0 comments Download
M content/renderer/input/input_handler_wrapper.cc View 2 chunks +3 lines, -0 lines 0 comments Download
M ui/events/blink/input_handler_proxy.h View 3 chunks +5 lines, -1 line 0 comments Download
M ui/events/blink/input_handler_proxy.cc View 1 2 5 chunks +38 lines, -41 lines 0 comments Download
M ui/events/blink/input_handler_proxy_unittest.cc View 1 2 2 chunks +98 lines, -0 lines 2 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 36 (19 generated)
sahel
5 months, 2 weeks ago (2017-01-09 17:50:27 UTC) #6
tdresser
LGTM. +bokan to take a look as well. https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_host/input/mouse_wheel_event_queue.cc File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_host/input/mouse_wheel_event_queue.cc#newcode210 content/browser/renderer_host/input/mouse_wheel_event_queue.cc:210: // ...
5 months, 2 weeks ago (2017-01-09 18:25:19 UTC) #8
sahel
https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_host/input/mouse_wheel_event_queue.cc File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_host/input/mouse_wheel_event_queue.cc#newcode210 content/browser/renderer_host/input/mouse_wheel_event_queue.cc:210: // no phase info exists, start the scroll_end_timer_. On ...
5 months, 2 weeks ago (2017-01-09 20:02:01 UTC) #9
tdresser
https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_host/input/mouse_wheel_event_queue.cc File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_host/input/mouse_wheel_event_queue.cc#newcode281 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: > ...
5 months, 2 weeks ago (2017-01-10 16:19:02 UTC) #10
bokan
Cool, mostly questions for my own benefit and some suggested tweaks to the test but ...
5 months, 2 weeks ago (2017-01-10 18:53:59 UTC) #11
sahel
https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_host/input/mouse_wheel_event_queue.cc File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2625453002/diff/1/content/browser/renderer_host/input/mouse_wheel_event_queue.cc#newcode210 content/browser/renderer_host/input/mouse_wheel_event_queue.cc:210: // no phase info exists, start the scroll_end_timer_. On ...
5 months, 2 weeks ago (2017-01-10 21:10:06 UTC) #12
bokan
Ok, still have some more questions but you don't need to wait to land. lgtm. ...
5 months, 2 weeks ago (2017-01-10 21:25:06 UTC) #13
sahel
On 2017/01/10 21:25:06, bokan wrote: > Ok, still have some more questions but you don't ...
5 months, 1 week ago (2017-01-12 18:53:25 UTC) #23
sahel
https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handler_proxy.cc File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2625453002/diff/1/ui/events/blink/input_handler_proxy.cc#newcode639 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: > ...
5 months, 1 week ago (2017-01-12 18:53:44 UTC) #24
bokan
lgtm % comment https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_handler_proxy_unittest.cc File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_handler_proxy_unittest.cc#newcode1014 ui/events/blink/input_handler_proxy_unittest.cc:1014: EXPECT_CALL(mock_input_handler_, ScrollBy(testing::_)).Times(0); This needs to go ...
5 months, 1 week ago (2017-01-12 19:33:17 UTC) #25
sahel
https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_handler_proxy_unittest.cc File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_handler_proxy_unittest.cc#newcode1014 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 ...
5 months, 1 week ago (2017-01-12 19:45:22 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2625453002/100001
5 months, 1 week ago (2017-01-12 19:46:33 UTC) #29
commit-bot: I haz the power
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4ced61f2e1215b847fc726343aecd410db03da5c
5 months, 1 week ago (2017-01-12 19:54:44 UTC) #32
bokan
On 2017/01/12 19:45:22, sahel wrote: > https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_handler_proxy_unittest.cc > File ui/events/blink/input_handler_proxy_unittest.cc (right): > > https://codereview.chromium.org/2625453002/diff/100001/ui/events/blink/input_handler_proxy_unittest.cc#newcode1014 > ...
5 months, 1 week ago (2017-01-12 19:57:55 UTC) #33
bokan
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_handler_proxy_unittest.cc ...
5 months, 1 week ago (2017-01-12 19:59:24 UTC) #34
sahel
A revert of this CL (patchset #3 id:100001) has been created in https://codereview.chromium.org/2630603002/ by sahel@chromium.org. ...
5 months, 1 week ago (2017-01-12 20:30:50 UTC) #35
dtapuska
5 months, 1 week ago (2017-01-12 20:39:29 UTC) #36
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318