Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2486673008:
Touchpad scroll latching enabled for Mac behind flag. (Closed)
|
Created:
4 years, 1 month ago by sahel Modified:
4 years 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, tdresser+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTouchpad scroll latching enabled for Mac behind flag.
The cl predicts if a GSE is going to be followed by a fling or not.
If a fling is going to happen the scrolling layer is not cleared while
processing the GSE.
To predict a fling, GSE is not sent when a wheel event with phase ==
phaseEnded arrives. Instead the send_scroll_end_ timer starts.
If the next wheel event is in momentumPhase, the timer stops and
the GSE with |mayPrecedeFling = true| is sent immediately.
Otherwise, it will be sent with |mayPrecedeFling = false| when the
timer callback is triggered.
To test it, use the --enable-features=TouchpadAndWheelScrollLatching
runtime flag.
BUG=526463
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/0ba6cbb28ab4cc17996711132565b1c4ae896b4e
Cr-Commit-Position: refs/heads/master@{#438520}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Use of may_precede_fling in everywhere. #Patch Set 3 : merge conflicts resolved. #
Total comments: 9
Patch Set 4 : Mouse_wheel_event_queue.cc polished. #
Total comments: 4
Patch Set 5 : mayPrecedeFling->precedeFling #
Total comments: 5
Patch Set 6 : nit fixed #
Total comments: 6
Patch Set 7 : GSE/GSB are not sent when a scroll in inertial phase begins. #
Total comments: 1
Patch Set 8 : nit fixed: assignment instead of condition #
Messages
Total messages: 78 (52 generated)
Description was changed from ========== Touchpad scroll latching enabled for Mac behind flag. The cl predicts if a GSE is going to be followed by a fling or not. If a fling is going to happen the scrolling layer is not cleared while processing the GSE. To predict a fling, GSE is not sent a mouse wheel with phase == phaseEnded arrives. Instead the send_scroll_end_ timer starts. If the next wheel event is in momentumPhase, the timer stops ands the GSE with |flingMightHappenNext = true| is sent immediately. Otherwise, it will be sent with |flingMightHappenNext = false| when the timer callback is triggered. To test it, use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 ========== to ========== Touchpad scroll latching enabled for Mac behind flag. The cl predicts if a GSE is going to be followed by a fling or not. If a fling is going to happen the scrolling layer is not cleared while processing the GSE. To predict a fling, GSE is not sent a mouse wheel with phase == phaseEnded arrives. Instead the send_scroll_end_ timer starts. If the next wheel event is in momentumPhase, the timer stops ands the GSE with |flingMightHappenNext = true| is sent immediately. Otherwise, it will be sent with |flingMightHappenNext = false| when the timer callback is triggered. To test it, use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Touchpad scroll latching enabled for Mac behind flag. The cl predicts if a GSE is going to be followed by a fling or not. If a fling is going to happen the scrolling layer is not cleared while processing the GSE. To predict a fling, GSE is not sent a mouse wheel with phase == phaseEnded arrives. Instead the send_scroll_end_ timer starts. If the next wheel event is in momentumPhase, the timer stops ands the GSE with |flingMightHappenNext = true| is sent immediately. Otherwise, it will be sent with |flingMightHappenNext = false| when the timer callback is triggered. To test it, use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Touchpad scroll latching enabled for Mac behind flag. The cl predicts if a GSE is going to be followed by a fling or not. If a fling is going to happen the scrolling layer is not cleared while processing the GSE. To predict a fling, GSE is not sent when a wheel event with phase == phaseEnded arrives. Instead the send_scroll_end_ timer starts. If the next wheel event is in momentumPhase, the timer stops ands the GSE with |flingMightHappenNext = true| is sent immediately. Otherwise, it will be sent with |flingMightHappenNext = false| when the timer callback is triggered. To test it, use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_TIMED_OUT, no build URL)
sahel@chromium.org changed reviewers: + tdresser@chromium.org
I think I may have missed some discussion here. Where did the discussion with aelias@ end up? Last I saw https://codereview.chromium.org/2423143002/ we were going to take a slightly closer look at the Mac event dispatch. The solution I'm gravitating towards is to cache the data on ScrollEnd, and grab it again in FlingStart. Why should we pick this solution over the other possible solutions? https://codereview.chromium.org/2486673008/diff/40001/cc/input/scroll_state_d... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2486673008/diff/40001/cc/input/scroll_state_d... cc/input/scroll_state_data.h:37: bool may_precede_fling; It might be worth using a three valued enum instead of two bools here, to avoid the risk of invalid state, and to make it clearer that you can't have "may_precede_fling" and "!is_ending". https://codereview.chromium.org/2486673008/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2486673008/diff/40001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.h:85: bool fling_might_happen_next); Can we be consistent, and call this may_precede_fling as well? https://codereview.chromium.org/2486673008/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2486673008/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:583: bool flingMightHappenNext; mayPrecedeFling? Or use flingMightHappenNext throughout.
https://codereview.chromium.org/2486673008/diff/40001/cc/input/scroll_state_d... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2486673008/diff/40001/cc/input/scroll_state_d... cc/input/scroll_state_data.h:37: bool may_precede_fling; On 2016/11/14 16:02:29, tdresser wrote: > It might be worth using a three valued enum instead of two bools here, to avoid > the risk of invalid state, and to make it clearer that you can't have > "may_precede_fling" and "!is_ending". is_ending now is handled very similarly to is_beginning (there is kind of a symmetry.) and, it's accessed in more than 10 different files. I'll change it if you still prefer to have the 3 valued enum. https://codereview.chromium.org/2486673008/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2486673008/diff/40001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.h:85: bool fling_might_happen_next); On 2016/11/14 16:02:29, tdresser wrote: > Can we be consistent, and call this may_precede_fling as well? Done. https://codereview.chromium.org/2486673008/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebInputEvent.h (right): https://codereview.chromium.org/2486673008/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:583: bool flingMightHappenNext; On 2016/11/14 16:02:29, tdresser wrote: > mayPrecedeFling? Or use flingMightHappenNext throughout. Done.
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sahel@chromium.org changed reviewers: + aelias@chromium.org
aelias@chromium.org: Please review changes in cc/* third_party/*
Description was changed from ========== Touchpad scroll latching enabled for Mac behind flag. The cl predicts if a GSE is going to be followed by a fling or not. If a fling is going to happen the scrolling layer is not cleared while processing the GSE. To predict a fling, GSE is not sent when a wheel event with phase == phaseEnded arrives. Instead the send_scroll_end_ timer starts. If the next wheel event is in momentumPhase, the timer stops ands the GSE with |flingMightHappenNext = true| is sent immediately. Otherwise, it will be sent with |flingMightHappenNext = false| when the timer callback is triggered. To test it, use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Touchpad scroll latching enabled for Mac behind flag. The cl predicts if a GSE is going to be followed by a fling or not. If a fling is going to happen the scrolling layer is not cleared while processing the GSE. To predict a fling, GSE is not sent when a wheel event with phase == phaseEnded arrives. Instead the send_scroll_end_ timer starts. If the next wheel event is in momentumPhase, the timer stops and the GSE with |mayPrecedeFling = true| is sent immediately. Otherwise, it will be sent with |mayPrecedeFling = false| when the timer callback is triggered. To test it, use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2486673008/diff/40001/cc/input/scroll_state_d... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2486673008/diff/40001/cc/input/scroll_state_d... cc/input/scroll_state_data.h:37: bool may_precede_fling; On 2016/11/18 16:13:58, sahel wrote: > On 2016/11/14 16:02:29, tdresser wrote: > > It might be worth using a three valued enum instead of two bools here, to > avoid > > the risk of invalid state, and to make it clearer that you can't have > > "may_precede_fling" and "!is_ending". > > is_ending now is handled very similarly to is_beginning (there is kind of a > symmetry.) and, it's accessed in more than 10 different files. I'll change it if > you still prefer to have the 3 valued enum. Acknowledged. https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... cc/input/scroll_state_data.h:37: bool may_precede_fling; Shouldn't this be |precedes_fling| now? Don't we only dispatch it when we're positive it precedes a fling? https://codereview.chromium.org/2486673008/diff/80001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2486673008/diff/80001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:185: blink::WebMouseWheelEvent::PhaseBegan) { Should we combine the two nested if's into a single if? https://codereview.chromium.org/2486673008/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebInputEvent.h (left): https://codereview.chromium.org/2486673008/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:460: Was this intentional?
tdresser@chromium.org changed reviewers: + ccameron@chromium.org, dtapuska@chromium.org
aelias@ is out until the beginning of December. dtapuska & ccameron, can you take a look? aelias@ agreed that this approach (using a timer) is reasonable.
Does this work for main thread fling?
https://codereview.chromium.org/2486673008/diff/80001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (left): https://codereview.chromium.org/2486673008/diff/80001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:199: if (current_phase_ended) { Can these blocks of code be reduced a bit to be: if ((enable_scroll_latching && momentum_phase_ended) || (!enabled_scroll_latching && current_phase_ended)) { SendScrollEnd(...); } else if (has_phase_info && !enable_scroll_latching!) { SendScrolLEnd(...); } else { Schedule... }
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... cc/input/scroll_state_data.h:37: bool may_precede_fling; On 2016/11/22 21:44:40, tdresser wrote: > Shouldn't this be |precedes_fling| now? Don't we only dispatch it when we're > positive it precedes a fling? In case of a touchpad fling, the scrollBegin/scrollEnd are called in InputHandlerProxy::ScrollByMouseWheel. The scrollEnd state has may_precede_fling = true. Because the next scrollBegin might be in inertial phase. Other than this case, we can accurately tell if a fling is going to happen after a scrollEnd or not. https://codereview.chromium.org/2486673008/diff/80001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (left): https://codereview.chromium.org/2486673008/diff/80001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:199: if (current_phase_ended) { On 2016/11/22 22:10:34, dtapuska wrote: > Can these blocks of code be reduced a bit to be: > > if ((enable_scroll_latching && momentum_phase_ended) || > (!enabled_scroll_latching && current_phase_ended)) { > SendScrollEnd(...); > } else if (has_phase_info && !enable_scroll_latching!) { > SendScrolLEnd(...); > } else { > Schedule... > } Done. https://codereview.chromium.org/2486673008/diff/80001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2486673008/diff/80001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:185: blink::WebMouseWheelEvent::PhaseBegan) { On 2016/11/22 21:44:40, tdresser wrote: > Should we combine the two nested if's into a single if? Done. https://codereview.chromium.org/2486673008/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebInputEvent.h (left): https://codereview.chromium.org/2486673008/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebInputEvent.h:460: On 2016/11/22 21:44:40, tdresser wrote: > Was this intentional? Sorry it happened when I resolved the merge conflicts, I'll fix it.
https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... cc/input/scroll_state_data.h:37: bool may_precede_fling; On 2016/11/24 15:28:55, sahel wrote: > On 2016/11/22 21:44:40, tdresser wrote: > > Shouldn't this be |precedes_fling| now? Don't we only dispatch it when we're > > positive it precedes a fling? > > In case of a touchpad fling, the scrollBegin/scrollEnd are called in > InputHandlerProxy::ScrollByMouseWheel. The scrollEnd state has > may_precede_fling = true. Because the next scrollBegin might be in inertial > phase. Other than this case, we can accurately tell if a fling is going to > happen after a scrollEnd or not. I'm not quite following this. You're saying the only case we don't know may_precede_fling for sure is in ScrollByMouseWheel? Isn't ScrollByMouseWheel only used for inertial scroll updates, so we're never going to be followed by a fling start? https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:214: SendScrollEnd(scroll_update, false, false); I think a comment here would be useful. https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:221: !has_phase_info) { A comment might also be useful here.
On 2016/11/28 15:25:17, tdresser wrote: > https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... > File cc/input/scroll_state_data.h (right): > > https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... > cc/input/scroll_state_data.h:37: bool may_precede_fling; > On 2016/11/24 15:28:55, sahel wrote: > > On 2016/11/22 21:44:40, tdresser wrote: > > > Shouldn't this be |precedes_fling| now? Don't we only dispatch it when we're > > > positive it precedes a fling? > > > > In case of a touchpad fling, the scrollBegin/scrollEnd are called in > > InputHandlerProxy::ScrollByMouseWheel. The scrollEnd state has > > may_precede_fling = true. Because the next scrollBegin might be in inertial > > phase. Other than this case, we can accurately tell if a fling is going to > > happen after a scrollEnd or not. > > I'm not quite following this. > > You're saying the only case we don't know may_precede_fling for sure is in > ScrollByMouseWheel? Isn't ScrollByMouseWheel only used for inertial scroll > updates, so we're never going to be followed by a fling start? > > https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): > > https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:214: > SendScrollEnd(scroll_update, false, false); > I think a comment here would be useful. > > https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:221: > !has_phase_info) { > A comment might also be useful here. In TouchpadFlingScroll, ScrollByMouseWheel calls ScrollBegin/ScrollBy/ScrollEnd for every synthetic wheel (rather than calling scrollBy only). ScrollBegin state is always in inertial phase. For scrollEnds in this function, we know that if a scrollBegin happens next, it will be in inertial phase, but we don't know that if a scrollBegin will happen next or this synthetic wheel event was the last one. It's confusing because scrollBegin in inertial phase gets called more than once during a scroll sequence. Once it's called to handle a GFS. Then for every Synthetic wheel event it's called with state in inertial phase as well.
On 2016/11/28 15:51:40, sahel wrote: > On 2016/11/28 15:25:17, tdresser wrote: > > > https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... > > File cc/input/scroll_state_data.h (right): > > > > > https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... > > cc/input/scroll_state_data.h:37: bool may_precede_fling; > > On 2016/11/24 15:28:55, sahel wrote: > > > On 2016/11/22 21:44:40, tdresser wrote: > > > > Shouldn't this be |precedes_fling| now? Don't we only dispatch it when > we're > > > > positive it precedes a fling? > > > > > > In case of a touchpad fling, the scrollBegin/scrollEnd are called in > > > InputHandlerProxy::ScrollByMouseWheel. The scrollEnd state has > > > may_precede_fling = true. Because the next scrollBegin might be in inertial > > > phase. Other than this case, we can accurately tell if a fling is going to > > > happen after a scrollEnd or not. > > > > I'm not quite following this. > > > > You're saying the only case we don't know may_precede_fling for sure is in > > ScrollByMouseWheel? Isn't ScrollByMouseWheel only used for inertial scroll > > updates, so we're never going to be followed by a fling start? > > > > > https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... > > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): > > > > > https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:214: > > SendScrollEnd(scroll_update, false, false); > > I think a comment here would be useful. > > > > > https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:221: > > !has_phase_info) { > > A comment might also be useful here. > > In TouchpadFlingScroll, ScrollByMouseWheel calls > ScrollBegin/ScrollBy/ScrollEnd for every synthetic wheel (rather than calling > scrollBy only). ScrollBegin state is always in inertial phase. For scrollEnds > in this function, we know that if a scrollBegin happens next, it will be in > inertial phase, but we don't know that if a scrollBegin will happen next or > this synthetic wheel event was the last one. It's confusing because scrollBegin > in inertial phase gets called more than once during a scroll sequence. > Once it's called to handle a GFS. Then for every Synthetic wheel event > it's called with state in inertial phase as well. We have control over that fling though, right? So we should be able to know when it ends? There's logic where we deal with ending it here: https://cs.chromium.org/chromium/src/ui/events/blink/input_handler_proxy.cc?r... You'd have to plumb a bit of extra information through, but I think it would be worth it. Does that sound doable?
Patchset #5 (id:120001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #5 (id:140001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/28 15:57:23, tdresser wrote: > On 2016/11/28 15:51:40, sahel wrote: > > On 2016/11/28 15:25:17, tdresser wrote: > > > > > > https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... > > > File cc/input/scroll_state_data.h (right): > > > > > > > > > https://codereview.chromium.org/2486673008/diff/80001/cc/input/scroll_state_d... > > > cc/input/scroll_state_data.h:37: bool may_precede_fling; > > > On 2016/11/24 15:28:55, sahel wrote: > > > > On 2016/11/22 21:44:40, tdresser wrote: > > > > > Shouldn't this be |precedes_fling| now? Don't we only dispatch it when > > we're > > > > > positive it precedes a fling? > > > > > > > > In case of a touchpad fling, the scrollBegin/scrollEnd are called in > > > > InputHandlerProxy::ScrollByMouseWheel. The scrollEnd state has > > > > may_precede_fling = true. Because the next scrollBegin might be in > inertial > > > > phase. Other than this case, we can accurately tell if a fling is going to > > > > happen after a scrollEnd or not. > > > > > > I'm not quite following this. > > > > > > You're saying the only case we don't know may_precede_fling for sure is in > > > ScrollByMouseWheel? Isn't ScrollByMouseWheel only used for inertial scroll > > > updates, so we're never going to be followed by a fling start? > > > > > > > > > https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... > > > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): > > > > > > > > > https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... > > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:214: > > > SendScrollEnd(scroll_update, false, false); > > > I think a comment here would be useful. > > > > > > > > > https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... > > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:221: > > > !has_phase_info) { > > > A comment might also be useful here. > > > > In TouchpadFlingScroll, ScrollByMouseWheel calls > > ScrollBegin/ScrollBy/ScrollEnd for every synthetic wheel (rather than calling > > scrollBy only). ScrollBegin state is always in inertial phase. For scrollEnds > > in this function, we know that if a scrollBegin happens next, it will be in > > inertial phase, but we don't know that if a scrollBegin will happen next or > > this synthetic wheel event was the last one. It's confusing because > scrollBegin > > in inertial phase gets called more than once during a scroll sequence. > > Once it's called to handle a GFS. Then for every Synthetic wheel event > > it's called with state in inertial phase as well. > > We have control over that fling though, right? So we should be able to know when > it ends? There's logic where we deal with ending it here: > https://cs.chromium.org/chromium/src/ui/events/blink/input_handler_proxy.cc?r... > > You'd have to plumb a bit of extra information through, but I think it would be > worth it. > Does that sound doable? I changed the variable name to precede_fling. I'll do the plumbing in ChromeOS patch.
https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:214: SendScrollEnd(scroll_update, false, false); On 2016/11/28 15:25:17, tdresser wrote: > I think a comment here would be useful. Done. https://codereview.chromium.org/2486673008/diff/100001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:221: !has_phase_info) { On 2016/11/28 15:25:17, tdresser wrote: > A comment might also be useful here. Done.
tdresser@chromium.org changed reviewers: - ccameron@chromium.org
LGTM -ccameron, as he hasn't given any feedback, and aelias@ will be back shortly. https://codereview.chromium.org/2486673008/diff/160001/cc/input/scroll_state_... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2486673008/diff/160001/cc/input/scroll_state_... cc/input/scroll_state_data.h:37: bool precede_fling; nit: precedes_fling? https://codereview.chromium.org/2486673008/diff/160001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2486673008/diff/160001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.h:85: bool precede_fling); Nit: It might be worth using enums for synthetic and precedes_fling, to avoid having to keep track of the parameter order.
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.
https://codereview.chromium.org/2486673008/diff/160001/cc/input/scroll_state_... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2486673008/diff/160001/cc/input/scroll_state_... cc/input/scroll_state_data.h:37: bool precede_fling; On 2016/11/29 19:06:54, tdresser wrote: > nit: precedes_fling? Sorry, English is not my first language., Done. https://codereview.chromium.org/2486673008/diff/160001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2486673008/diff/160001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.h:85: bool precede_fling); On 2016/11/29 19:06:54, tdresser wrote: > Nit: It might be worth using enums for synthetic and precedes_fling, to avoid > having to keep track of the parameter order. Synthetic is a temporary variable, once scroll latching is enabled on all platforms by default, this won't exist anymore.
https://codereview.chromium.org/2486673008/diff/160001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right): https://codereview.chromium.org/2486673008/diff/160001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.h:85: bool precede_fling); On 2016/11/30 19:41:54, sahel wrote: > On 2016/11/29 19:06:54, tdresser wrote: > > Nit: It might be worth using enums for synthetic and precedes_fling, to avoid > > having to keep track of the parameter order. > > Synthetic is a temporary variable, once scroll latching is enabled on all > platforms by default, this won't exist anymore. Acknowledged.
https://codereview.chromium.org/2486673008/diff/180001/cc/input/scroll_state_... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2486673008/diff/180001/cc/input/scroll_state_... cc/input/scroll_state_data.h:37: bool precedes_fling; It's confusing to add a special boolean to the scroll end meaning "don't actually end". I'd prefer having a single flingstart event, or perhaps no event at all since the first update event with is_in_inertial_phase = true is self-explanatory. As for the scroll deltas that may be attached to ScrollEnd, I'm not sure whether that ever actually happens, but if it does, then it's a bug to delay them for 100ms anyway. So either send a ScrollUpdate message for them separately, or confirm that it never happens and don't worry about it. https://codereview.chromium.org/2486673008/diff/180001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2486673008/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:212: if ((enable_scroll_latching_ && momentum_phase_ended) || All these enable_scroll_latching_ if statements are hard to read and prove control flow correctness for. Please structure this into one big if statements: if (enable_scroll_latching_) { ... } else { ... } And please move the other blocks you added/changed above into this big omni-if-statement, so there's exactly one use of enable_scroll_latching_ in this method. It's fine if this causes a bit of code duplication particularly since the non-latching path is slated for deletion after launch (and my suggestion will make this deletion easier). https://codereview.chromium.org/2486673008/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:234: last_scroll_update_ = scroll_update; If there's a delta here (which I'm not sure ever happens), then it's a bug to delay it. In the non-fling case, users would experience this as a 100ms jank before the last scroll tick. So I suggest sending a semi-synthetic ScrollUpdate event for this if it's nonzero.
OK, anyway, I realized today I've been acting as a long-distance bottleneck on your work, touchpad isn't my area of ownership, and I should be letting input-team drive this, so rubberstamp lgtm and you can land when tdresser@ is satisfied. I trust that my opinion was taken into consideration and you don't need to ask for my permission even if you find the need to go against some of my opinions in the future.
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.
https://codereview.chromium.org/2486673008/diff/180001/cc/input/scroll_state_... File cc/input/scroll_state_data.h (right): https://codereview.chromium.org/2486673008/diff/180001/cc/input/scroll_state_... cc/input/scroll_state_data.h:37: bool precedes_fling; On 2016/12/02 05:17:56, aelias_OOO_until_Dec1 wrote: > It's confusing to add a special boolean to the scroll end meaning "don't > actually end". I'd prefer having a single flingstart event, or perhaps no event > at all since the first update event with is_in_inertial_phase = true is > self-explanatory. > > As for the scroll deltas that may be attached to ScrollEnd, I'm not sure whether > that ever actually happens, but if it does, then it's a bug to delay them for > 100ms anyway. So either send a ScrollUpdate message for them separately, or > confirm that it never happens and don't worry about it. -Done. -GSE don't have any scroll delta. I confirm that this case won't happen ever. https://codereview.chromium.org/2486673008/diff/180001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/2486673008/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:212: if ((enable_scroll_latching_ && momentum_phase_ended) || On 2016/12/02 05:17:56, aelias_OOO_until_Dec1 wrote: > All these enable_scroll_latching_ if statements are hard to read and prove > control flow correctness for. Please structure this into one big if statements: > > if (enable_scroll_latching_) { > ... > } else { > ... > } > > And please move the other blocks you added/changed above into this big > omni-if-statement, so there's exactly one use of enable_scroll_latching_ in this > method. > > It's fine if this causes a bit of code duplication particularly since the > non-latching path is slated for deletion after launch (and my suggestion will > make this deletion easier). Done. https://codereview.chromium.org/2486673008/diff/180001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:234: last_scroll_update_ = scroll_update; On 2016/12/02 05:17:56, aelias_OOO_until_Dec1 wrote: > If there's a delta here (which I'm not sure ever happens), then it's a bug to > delay it. In the non-fling case, users would experience this as a 100ms jank > before the last scroll tick. So I suggest sending a semi-synthetic ScrollUpdate > event for this if it's nonzero. It is possible to get a mouse wheel event from OS X with delta and phaseEnded. In this case a GSU and a GSE will be generated. GSE won't have and deltas.
LGTM - let's get dtapuska@ to take a look before we land this though.
lgtm % nit https://codereview.chromium.org/2486673008/diff/200001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2486673008/diff/200001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:147: scroll_state_data.is_in_inertial_phase = true; Can we short hand this to: scroll_state_data.is_in_inertial_phase = (event.data.scrollBegin.inertialPhase == WebGestureEvent::MomentumPhase);
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 aelias@chromium.org, dtapuska@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2486673008/#ps220001 (title: "nit fixed: assignment instead of condition")
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": 220001, "attempt_start_ts": 1481728590813290,
"parent_rev": "65a585fda203543b99376304398fdf67c75b0f76", "commit_rev":
"b33b2531f925299aa920b60a260b0408aa4ecbe6"}
Message was sent while issue was closed.
Description was changed from ========== Touchpad scroll latching enabled for Mac behind flag. The cl predicts if a GSE is going to be followed by a fling or not. If a fling is going to happen the scrolling layer is not cleared while processing the GSE. To predict a fling, GSE is not sent when a wheel event with phase == phaseEnded arrives. Instead the send_scroll_end_ timer starts. If the next wheel event is in momentumPhase, the timer stops and the GSE with |mayPrecedeFling = true| is sent immediately. Otherwise, it will be sent with |mayPrecedeFling = false| when the timer callback is triggered. To test it, use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Touchpad scroll latching enabled for Mac behind flag. The cl predicts if a GSE is going to be followed by a fling or not. If a fling is going to happen the scrolling layer is not cleared while processing the GSE. To predict a fling, GSE is not sent when a wheel event with phase == phaseEnded arrives. Instead the send_scroll_end_ timer starts. If the next wheel event is in momentumPhase, the timer stops and the GSE with |mayPrecedeFling = true| is sent immediately. Otherwise, it will be sent with |mayPrecedeFling = false| when the timer callback is triggered. To test it, use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2486673008 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Touchpad scroll latching enabled for Mac behind flag. The cl predicts if a GSE is going to be followed by a fling or not. If a fling is going to happen the scrolling layer is not cleared while processing the GSE. To predict a fling, GSE is not sent when a wheel event with phase == phaseEnded arrives. Instead the send_scroll_end_ timer starts. If the next wheel event is in momentumPhase, the timer stops and the GSE with |mayPrecedeFling = true| is sent immediately. Otherwise, it will be sent with |mayPrecedeFling = false| when the timer callback is triggered. To test it, use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2486673008 ========== to ========== Touchpad scroll latching enabled for Mac behind flag. The cl predicts if a GSE is going to be followed by a fling or not. If a fling is going to happen the scrolling layer is not cleared while processing the GSE. To predict a fling, GSE is not sent when a wheel event with phase == phaseEnded arrives. Instead the send_scroll_end_ timer starts. If the next wheel event is in momentumPhase, the timer stops and the GSE with |mayPrecedeFling = true| is sent immediately. Otherwise, it will be sent with |mayPrecedeFling = false| when the timer callback is triggered. To test it, use the --enable-features=TouchpadAndWheelScrollLatching runtime flag. BUG=526463 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/0ba6cbb28ab4cc17996711132565b1c4ae896b4e Cr-Commit-Position: refs/heads/master@{#438520} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0ba6cbb28ab4cc17996711132565b1c4ae896b4e Cr-Commit-Position: refs/heads/master@{#438520} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
