Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2423143002:
Scrollbar fade animation broken on Android regression resolved. (Closed)
DescriptionScrollbar fade animation broken on Android regression resolved.
ClearCurrentlyScrollingLayer() is always called in scrollEnd(), and in
scrollBegin() if the scroll is in inertial phase, the last scrolled
layer is re-used instead of a new hit testing.
BUG=649122
TEST=LayerTreeHostImplTest.ScrollbarAnimationEndsAfterScrollEnd
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Patch Set 1 #Patch Set 2 : scroll variables cached before reseting, and restored in scrollBegin with inertial phase. #
Total comments: 17
Patch Set 3 : The helper class moved to layer_tree_host_impl.h #Patch Set 4 : unittest added. #
Total comments: 17
Patch Set 5 : scrolling_layer_id cached. #
Total comments: 9
Patch Set 6 : review comments addressed. #
Total comments: 2
Messages
Total messages: 62 (37 generated)
Description was changed from ========== Scrollbar fade animation broken on Android regression resolved. ClearCurrentlyScrollingLayer() is always called in scrollEnd(), and in scrollBegin() if the scroll is in inertial phase, the last scrolled layer is re-used instead of a new hit testing. BUG= ========== to ========== Scrollbar fade animation broken on Android regression resolved. ClearCurrentlyScrollingLayer() is always called in scrollEnd(), and in scrollBegin() if the scroll is in inertial phase, the last scrolled layer is re-used instead of a new hit testing. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_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: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Scrollbar fade animation broken on Android regression resolved. ClearCurrentlyScrollingLayer() is always called in scrollEnd(), and in scrollBegin() if the scroll is in inertial phase, the last scrolled layer is re-used instead of a new hit testing. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Scrollbar fade animation broken on Android regression resolved. ClearCurrentlyScrollingLayer() is always called in scrollEnd(), and in scrollBegin() if the scroll is in inertial phase, the last scrolled layer is re-used instead of a new hit testing. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
sahel@chromium.org changed reviewers: + tdresser@chromium.org
tdresser@chromium.org changed reviewers: + bokan@chromium.org
+bokan for review. Can you add a bug number and a test? https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:162: // coresponding scroll event. corresponding https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:187: ScrollingVariablesState CashedScrollingVariablesState; Variable name should be cached_scrolling_variables_state_. Can we put this on the LTHI? https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:702: // that is in inertial phase (a fling) to lach the event to its corresponding lach -> latch. https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:703: // scroll. scroll -> scroller. https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:704: void CacheScrollingVariablesState(); Can we come up with a more specific name than "ScrollingVariables"? There are other scrolling related variables that aren't included here. https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:752: bool did_lock_scrolling_layer_; Can we put these in the struct here as well?
Overall changes look fine to me, just needs test. https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:187: ScrollingVariablesState CashedScrollingVariablesState; Is there a reason for this variable to be a local global? If not, please just make it a member of LTHI, it simplifies lifetime/threading issues.
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 ========== Scrollbar fade animation broken on Android regression resolved. ClearCurrentlyScrollingLayer() is always called in scrollEnd(), and in scrollBegin() if the scroll is in inertial phase, the last scrolled layer is re-used instead of a new hit testing. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Scrollbar fade animation broken on Android regression resolved. ClearCurrentlyScrollingLayer() is always called in scrollEnd(), and in scrollBegin() if the scroll is in inertial phase, the last scrolled layer is re-used instead of a new hit testing. BUG=649122 TEST=LayerTreeHostImplTest.ScrollbarAnimationEndsAfterScrollEnd CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
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...)
Patchset #4 (id:100001) 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.
https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:162: // coresponding scroll event. On 2016/10/19 15:28:04, tdresser wrote: > corresponding Done. https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:187: ScrollingVariablesState CashedScrollingVariablesState; On 2016/10/19 15:28:04, tdresser wrote: > Variable name should be cached_scrolling_variables_state_. > > Can we put this on the LTHI? Done. https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:187: ScrollingVariablesState CashedScrollingVariablesState; On 2016/10/20 15:35:10, bokan wrote: > Is there a reason for this variable to be a local global? If not, please just > make it a member of LTHI, it simplifies lifetime/threading issues. Acknowledged. https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:702: // that is in inertial phase (a fling) to lach the event to its corresponding On 2016/10/19 15:28:04, tdresser wrote: > lach -> latch. Done. https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:703: // scroll. On 2016/10/19 15:28:04, tdresser wrote: > scroll -> scroller. Done. https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:704: void CacheScrollingVariablesState(); On 2016/10/19 15:28:04, tdresser wrote: > Can we come up with a more specific name than "ScrollingVariables"? There are > other scrolling related variables that aren't included here. You are right, but I couldn't come up with a better name, because the cached variables are not related to each other. I would appreciate any ideas/help here. https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:752: bool did_lock_scrolling_layer_; On 2016/10/19 15:28:04, tdresser wrote: > Can we put these in the struct here as well? I am not sure if it's a good idea to do so, because they are not logically related to each other, and they are accessed in different places/functions.
https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:704: void CacheScrollingVariablesState(); On 2016/10/25 15:34:14, sahel wrote: > On 2016/10/19 15:28:04, tdresser wrote: > > Can we come up with a more specific name than "ScrollingVariables"? There are > > other scrolling related variables that aren't included here. > > You are right, but I couldn't come up with a better name, because the cached > variables are not related to each other. I would appreciate any ideas/help here. How about CacheScrollStateForFling (FlingContinuation? EnsuingFling? SubsequentFling?) and calling the class ScrollStateForFling? https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:752: bool did_lock_scrolling_layer_; On 2016/10/25 15:34:14, sahel wrote: > On 2016/10/19 15:28:04, tdresser wrote: > > Can we put these in the struct here as well? > > I am not sure if it's a good idea to do so, because they are not logically > related to each other, and they are accessed in different places/functions. Yah, wheel_scrolling_ is set in ScrollBeginImpl which we call on a fling and the others are set/reset in mouse moves/up/down so I don't think they're part of "FlingState" https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:267: cached_scrolling_variables_state_ = ScrollingVariablesState( These should all be at their default values anyway so there's no need to initialize here. We're guaranteed to have a ScrollEnd before a fling anyway, right? https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2655: active_tree_->LayerById(active_tree_->LastScrolledLayerId()); I'd prefer if we just store the CurrentlyScrollingLayer's id as part of the CacheScrollingVariablesState https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2700: return ScrollBeginImpl(scroll_state, scrolling_layer_impl, type); Is it intentional that we're now calling ScrollBeginImpl for fling scrolls rather than FlingScrollBegin? If so, the only other place we call FlingScrollBegin is InputHandlerProxy::HandleGestureFlingStart (other than tests) so we should replace all uses. https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:10878: TEST_F(LayerTreeHostImplTest, ScrollbarAnimationEndsAfterScrollEnd) { This tests that the CL fixes the bug, but we also added a new cache/restore functionality which should also be tested. You could either add a new test and make sure a fling restores the state properly, or just add to this test (and change the test name). https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:10914: EXPECT_FALSE(scrollbar_animation_controller->currently_scrolling()); You're basically testing that we call ClearCurrentlyScrollingLayer at ScrollEnd right? I would just check host_impl_->CurrentlyScrollingLayer and make sure it's cleared. That way you don't have to add the testing-only method to the ScrollbarAnimationController, nor setup any of the scrollbar state above. The scrollbars not fading out is just a consequence of the fact the current scrolling layer isn't cleared so it's fine to test the root cause.
https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:267: cached_scrolling_variables_state_ = ScrollingVariablesState( On 2016/10/25 17:29:22, bokan wrote: > These should all be at their default values anyway so there's no need to > initialize here. We're guaranteed to have a ScrollEnd before a fling anyway, > right? I thought it's better for maintainability of the code, in case the default for any of these values changes, if it's not the case, I can remove the initialization. For touchpad scrolling yes, we get a scroll end and handle the fling by calling scroll begin. For touch scroll, no, we get the scroll end only when the scroll event is finished. if the scroll is followed by a fling, we get the scrollend only after the fling. https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2655: active_tree_->LayerById(active_tree_->LastScrolledLayerId()); On 2016/10/25 17:29:22, bokan wrote: > I'd prefer if we just store the CurrentlyScrollingLayer's id as part of the > CacheScrollingVariablesState I preferred to use the one that is already cached. Because it makes the struct smaller, but if you think it's easier to read/follow, I can add it to the struct. https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2700: return ScrollBeginImpl(scroll_state, scrolling_layer_impl, type); On 2016/10/25 17:29:22, bokan wrote: > Is it intentional that we're now calling ScrollBeginImpl for fling scrolls > rather than FlingScrollBegin? If so, the only other place we call > FlingScrollBegin is InputHandlerProxy::HandleGestureFlingStart (other than > tests) so we should replace all uses. On Mac we don't get a fling start and that's why on other platforms while scrolling with touchpad a flingStart is handled by scrollBegin. For touch scroll though, fling is handled by a HandleGestureFlingStart. I think the correct approach isn't changing the HandleGestureFlingStart. It's better to call HandleGestureFlingStart on mac as well, instead of scrollBegin, and make touchpad consistent with touch. Anyway, I'll do some refactoring later on, to make touch and touchpad scrolls follow similar/identical code paths. But I prefer to do it, in a separate patch. https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:10878: TEST_F(LayerTreeHostImplTest, ScrollbarAnimationEndsAfterScrollEnd) { On 2016/10/25 17:29:22, bokan wrote: > This tests that the CL fixes the bug, but we also added a new cache/restore > functionality which should also be tested. You could either add a new test and > make sure a fling restores the state properly, or just add to this test (and > change the test name). I've already written tests for latching. In this patch, I changed the latching logic to restore the state rather than not clearing it. Isn't it enough that it still passes the latching tests, do I need to write a test to show that the states are restored properly?
https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:267: cached_scrolling_variables_state_ = ScrollingVariablesState( On 2016/10/25 17:53:41, sahel wrote: > On 2016/10/25 17:29:22, bokan wrote: > > These should all be at their default values anyway so there's no need to > > initialize here. We're guaranteed to have a ScrollEnd before a fling anyway, > > right? > > I thought it's better for maintainability of the code, in case the default for > any of these values changes, if it's not the case, I can remove the > initialization. > > For touchpad scrolling yes, we get a scroll end and handle the fling by calling > scroll begin. For touch scroll, no, we get the scroll end only when the scroll > event is finished. if the scroll is followed by a fling, we get the scrollend > only after the fling. Ah, that's unfortunate. Ok then, initializing is fine but put this in the initializer list above. https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2655: active_tree_->LayerById(active_tree_->LastScrolledLayerId()); On 2016/10/25 17:53:41, sahel wrote: > On 2016/10/25 17:29:22, bokan wrote: > > I'd prefer if we just store the CurrentlyScrollingLayer's id as part of the > > CacheScrollingVariablesState > > I preferred to use the one that is already cached. Because it makes the struct > smaller, but if you think it's easier to read/follow, I can add it to the > struct. Please do. In general, unless something is known to be performance sensitive, we should err on the side of readibility/maintainability. https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2700: return ScrollBeginImpl(scroll_state, scrolling_layer_impl, type); On 2016/10/25 17:53:41, sahel wrote: > On 2016/10/25 17:29:22, bokan wrote: > > Is it intentional that we're now calling ScrollBeginImpl for fling scrolls > > rather than FlingScrollBegin? If so, the only other place we call > > FlingScrollBegin is InputHandlerProxy::HandleGestureFlingStart (other than > > tests) so we should replace all uses. > > On Mac we don't get a fling start and that's why on other platforms while > scrolling with touchpad a flingStart is handled by scrollBegin. > > For touch scroll though, fling is handled by a HandleGestureFlingStart. > > I think the correct approach isn't changing the HandleGestureFlingStart. It's > better to call HandleGestureFlingStart on mac as well, instead of scrollBegin, > and make touchpad consistent with touch. > > Anyway, I'll do some refactoring later on, to make touch and touchpad scrolls > follow similar/identical code paths. But I prefer to do it, in a separate patch. Ok, sgtm, thanks. https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:10878: TEST_F(LayerTreeHostImplTest, ScrollbarAnimationEndsAfterScrollEnd) { On 2016/10/25 17:53:41, sahel wrote: > On 2016/10/25 17:29:22, bokan wrote: > > This tests that the CL fixes the bug, but we also added a new cache/restore > > functionality which should also be tested. You could either add a new test and > > make sure a fling restores the state properly, or just add to this test (and > > change the test name). > > I've already written tests for latching. In this patch, I changed the latching > logic to restore the state rather than not clearing it. > Isn't it enough that it still passes the latching tests, do I need to write a > test to show that the states are restored properly? Good point, ok, existing tests should be enough.
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...
Patchset #5 (id:140001) has been deleted
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2423143002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:704: void CacheScrollingVariablesState(); On 2016/10/25 17:29:21, bokan wrote: > On 2016/10/25 15:34:14, sahel wrote: > > On 2016/10/19 15:28:04, tdresser wrote: > > > Can we come up with a more specific name than "ScrollingVariables"? There > are > > > other scrolling related variables that aren't included here. > > > > You are right, but I couldn't come up with a better name, because the cached > > variables are not related to each other. I would appreciate any ideas/help > here. > > How about CacheScrollStateForFling (FlingContinuation? EnsuingFling? > SubsequentFling?) and calling the class ScrollStateForFling? Done. https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:267: cached_scrolling_variables_state_ = ScrollingVariablesState( On 2016/10/25 19:19:22, bokan wrote: > On 2016/10/25 17:53:41, sahel wrote: > > On 2016/10/25 17:29:22, bokan wrote: > > > These should all be at their default values anyway so there's no need to > > > initialize here. We're guaranteed to have a ScrollEnd before a fling anyway, > > > right? > > > > I thought it's better for maintainability of the code, in case the default for > > any of these values changes, if it's not the case, I can remove the > > initialization. > > > > For touchpad scrolling yes, we get a scroll end and handle the fling by > calling > > scroll begin. For touch scroll, no, we get the scroll end only when the scroll > > event is finished. if the scroll is followed by a fling, we get the scrollend > > only after the fling. > > Ah, that's unfortunate. Ok then, initializing is fine but put this in the > initializer list above. Done. https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2655: active_tree_->LayerById(active_tree_->LastScrolledLayerId()); On 2016/10/25 19:19:22, bokan wrote: > On 2016/10/25 17:53:41, sahel wrote: > > On 2016/10/25 17:29:22, bokan wrote: > > > I'd prefer if we just store the CurrentlyScrollingLayer's id as part of the > > > CacheScrollingVariablesState > > > > I preferred to use the one that is already cached. Because it makes the struct > > smaller, but if you think it's easier to read/follow, I can add it to the > > struct. > > Please do. In general, unless something is known to be performance sensitive, we > should err on the side of readibility/maintainability. Done. https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:10914: EXPECT_FALSE(scrollbar_animation_controller->currently_scrolling()); On 2016/10/25 17:29:22, bokan wrote: > You're basically testing that we call ClearCurrentlyScrollingLayer at ScrollEnd > right? I would just check host_impl_->CurrentlyScrollingLayer and make sure it's > cleared. That way you don't have to add the testing-only method to the > ScrollbarAnimationController, nor setup any of the scrollbar state above. The > scrollbars not fading out is just a consequence of the fact the current > scrolling layer isn't cleared so it's fine to test the root cause. The name of the ClearCurrentlyScrollingLayer doesn't imply that scrollbar animations are called through it. The regression happened because I thought clearing the currently scrolling layer in scrollBegin is enough and it's not necessary to call it in scrollEnd as well. This unittest shows the dependency of scrollbar animations to clearing/setting the currently scrolling layer. I think it helps to prevent such regressions in future. But I can remove it if it doesn't help. Checking host_impl_->CurrentlyScrollingLayer()==nullptr is already happening in several tests.
nit I missed previously, but lgtm otherwise https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2423143002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:10914: EXPECT_FALSE(scrollbar_animation_controller->currently_scrolling()); On 2016/10/26 14:11:44, sahel wrote: > On 2016/10/25 17:29:22, bokan wrote: > > You're basically testing that we call ClearCurrentlyScrollingLayer at > ScrollEnd > > right? I would just check host_impl_->CurrentlyScrollingLayer and make sure > it's > > cleared. That way you don't have to add the testing-only method to the > > ScrollbarAnimationController, nor setup any of the scrollbar state above. The > > scrollbars not fading out is just a consequence of the fact the current > > scrolling layer isn't cleared so it's fine to test the root cause. > > The name of the ClearCurrentlyScrollingLayer doesn't imply that scrollbar > animations are called through it. The regression happened because I thought > clearing the currently scrolling layer in scrollBegin is enough and it's not > necessary to call it in scrollEnd as well. This unittest shows the dependency of > scrollbar animations to clearing/setting the currently scrolling layer. I think > it helps to prevent such regressions in future. But I can remove it if it > doesn't help. > Checking host_impl_->CurrentlyScrollingLayer()==nullptr is already happening in > several tests. Acknowledged. https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:152: int scrollingLayerId() { return scrolling_layer_id_; } nit: these methods should all be of the style scrolling_layer_id()
LGTM! https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2686: if (scrolling_layer_impl) Use {} https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:4114: LayerImpl* LayerTreeHostImpl::RestoreScrollStateForFling() { It seems a bit weird to me that this returns the currently scrolling layer. Can it just return void, and then we read the currently scrolling layer afterwards? https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:137: // corresponding scroll event. It doesn't latch the fling to the scroll event, it latches the fling to the previously scrolling layer.
https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:4114: LayerImpl* LayerTreeHostImpl::RestoreScrollStateForFling() { On 2016/10/26 15:38:18, tdresser wrote: > It seems a bit weird to me that this returns the currently scrolling layer. > > Can it just return void, and then we read the currently scrolling layer > afterwards? +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: This issue passed the CQ dry run.
https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2686: if (scrolling_layer_impl) On 2016/10/26 15:38:18, tdresser wrote: > Use {} Done. https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:4114: LayerImpl* LayerTreeHostImpl::RestoreScrollStateForFling() { On 2016/10/26 15:38:18, tdresser wrote: > It seems a bit weird to me that this returns the currently scrolling layer. > > Can it just return void, and then we read the currently scrolling layer > afterwards? Done. https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:137: // corresponding scroll event. On 2016/10/26 15:38:18, tdresser wrote: > It doesn't latch the fling to the scroll event, it latches the fling to the > previously scrolling layer. Done. https://codereview.chromium.org/2423143002/diff/160001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:152: int scrollingLayerId() { return scrolling_layer_id_; } On 2016/10/26 14:17:43, bokan wrote: > nit: these methods should all be of the style scrolling_layer_id() Done.
sahel@chromium.org changed reviewers: + aelias@chromium.org
aelias@chromium.org: Please review changes in
https://codereview.chromium.org/2423143002/diff/180001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2423143002/diff/180001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:138: class ScrollStateForFling { What's the event sequence that makes this needed? Is it like ScrollBegin, ScrollEnd, ScrollBegin[ForFling]? If possible, I'd prefer we adjust the event generation so that the ScrollEnd is not produced if it's going to turn into a fling.
https://codereview.chromium.org/2423143002/diff/180001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2423143002/diff/180001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.h:138: class ScrollStateForFling { On 2016/10/27 21:48:11, aelias wrote: > What's the event sequence that makes this needed? Is it like ScrollBegin, > ScrollEnd, ScrollBegin[ForFling]? If possible, I'd prefer we adjust the event > generation so that the ScrollEnd is not produced if it's going to turn into a > fling. Yes, that's the even sequence and it only happens on a touchpad fling. I spent a long while to try to predict if a fling is going to happen after an scrollend or not, and wasn't able to do that efficiently. One way to address this, is keeping scrollends in a queue (debouncing queue or a new queue) and waiting for some timeout to see if a fling event is received or not. After discussions that we had with @tdresser, we decided not to use any queues (to avoid adding extra latency) and eventually we want the touch scrolls to have the same sequence and send the scrollend before fling, rather than not sending it for touchpad.
On 2016/10/27 at 22:57:06, sahel wrote: > https://codereview.chromium.org/2423143002/diff/180001/cc/trees/layer_tree_ho... > File cc/trees/layer_tree_host_impl.h (right): > > https://codereview.chromium.org/2423143002/diff/180001/cc/trees/layer_tree_ho... > cc/trees/layer_tree_host_impl.h:138: class ScrollStateForFling { > On 2016/10/27 21:48:11, aelias wrote: > > What's the event sequence that makes this needed? Is it like ScrollBegin, > > ScrollEnd, ScrollBegin[ForFling]? If possible, I'd prefer we adjust the event > > generation so that the ScrollEnd is not produced if it's going to turn into a > > fling. > > Yes, that's the even sequence and it only happens on a touchpad fling. > I spent a long while to try to predict if a fling is going to happen after an scrollend or not, and wasn't able to do that efficiently. On what platform? ChromeOS or Mac or both? Have you examined exactly how the events are generated in the higher layers of the stack? Unless a platform layer not under our control is involved, I don't think any timeout should be needed. The fling detector synchronously decides at the moment TouchEnd is received whether the sequence amounted to a fling. At that point, it can produce GestureFlingStart instead of sending GestureScrollEnd. (And if your reply will be that Mac touchpad is not sufficiently under our control, then it calls for a bit of investigation to make sure that's really not the case -- often platforms reveal hooks for such information and the current platform behavior is not so set in stone as initially assumed.) > After discussions that we had with @tdresser, we decided not to use any queues (to avoid adding extra latency) and eventually we want the touch scrolls to have the same sequence and send the scrollend before fling, rather than not sending it for touchpad. I don't agree touch scrolls should have this pattern. It seems much more natural for the drag and fling to be considered part of the same scroll and the fact that you need to reuse the hit test result from the first ScrollBegin just proves that. GestureScrollBegin->GestureFlingStart->GestureScrollEnd seems much better.
On 2016/10/27 23:14:23, aelias wrote: > On 2016/10/27 at 22:57:06, sahel wrote: > > > https://codereview.chromium.org/2423143002/diff/180001/cc/trees/layer_tree_ho... > > File cc/trees/layer_tree_host_impl.h (right): > > > > > https://codereview.chromium.org/2423143002/diff/180001/cc/trees/layer_tree_ho... > > cc/trees/layer_tree_host_impl.h:138: class ScrollStateForFling { > > On 2016/10/27 21:48:11, aelias wrote: > > > What's the event sequence that makes this needed? Is it like ScrollBegin, > > > ScrollEnd, ScrollBegin[ForFling]? If possible, I'd prefer we adjust the > event > > > generation so that the ScrollEnd is not produced if it's going to turn into > a > > > fling. > > > > Yes, that's the even sequence and it only happens on a touchpad fling. > > I spent a long while to try to predict if a fling is going to happen after an > scrollend or not, and wasn't able to do that efficiently. > > On what platform? ChromeOS or Mac or both? Have you examined exactly how the > events are generated in the higher layers of the stack? > > Unless a platform layer not under our control is involved, I don't think any > timeout should be needed. The fling detector synchronously decides at the > moment TouchEnd is received whether the sequence amounted to a fling. At that > point, it can produce GestureFlingStart instead of sending GestureScrollEnd. > (And if your reply will be that Mac touchpad is not sufficiently under our > control, then it calls for a bit of investigation to make sure that's really not > the case -- often platforms reveal hooks for such information and the current > platform behavior is not so set in stone as initially assumed.) > > > After discussions that we had with @tdresser, we decided not to use any queues > (to avoid adding extra latency) and eventually we want the touch scrolls to have > the same sequence and send the scrollend before fling, rather than not sending > it for touchpad. > > I don't agree touch scrolls should have this pattern. It seems much more > natural for the drag and fling to be considered part of the same scroll and the > fact that you need to reuse the hit test result from the first ScrollBegin just > proves that. GestureScrollBegin->GestureFlingStart->GestureScrollEnd seems much > better. For touch scroll/fling gestureEvents are generated in gesture_provider and that is why we can easily avoid generating a GSE when a GFS is generated. touchpad, ChrOs: flings and scrollends are in debouncing queue and they get pushed to gesture_queue based on the time out : in this situation we can check the scrollend before sending it to the actual gestureevent. We can either not send it at all (and only send the fling) or we can inject information saying that a fling is happening next and then send the event) In either case, we have to wait to see if we get a fling from os or not, before sending the scrollend. touchpad, mac: we don’t get fling events from os, we get scroll events by phase and momentum phase info, a scroll event by phase=phaseEnded might be followed up by a scroll event with momentumPhase=phaseBegan. Again when we want to send a scrollend, we don’t know that if a fling is going to happen or not. Unless if we wait and see if we will receive a event with momentumPhase=phaseBegan from OS or not. Anyway, for touchpad flings there is no way to see if a fling is going to happen or not, before receiving the fling event from os (receiving an scroll event with momentum phase info on mac). That's why we should wait before sending the scrollend to gesture_event_queue to see if a fling is going to happen or not. As far as I know we cannot get the prediction of a fling from os, and that's why the only possible way that I can think of is to defer sending the scrollEnd or restoring the state in case of a fling. I'd like to discuss about the issue some more and get ideas about what's the better approach for implementing the latching, but I'd prefer to land this cl to solve the regression and change the latching implementation later on, if needed.
On 2016/10/28 at 16:48:46, sahel wrote: > touchpad, ChrOs: flings and scrollends are in debouncing queue and they get pushed to > gesture_queue based on the time out : in this situation we can check the scrollend > before sending it to the actual gestureevent. We can either not send it at all > (and only send the fling) or we can inject information saying that a fling is > happening next and then send the event) In either case, we have to wait to see if we > get a fling from os or not, before sending the scrollend. OK, thanks for doing the research on platform behaviors, I appreciate it. What I'm hearing is that this is a ChromeOS specific issue. On ChromeOS, we have full control to apply the fix at any level of the stack and don't need to resort to any hack. I'd suggest that we change ChromeOS to delay sending the ScrollEnd at the moment when the debounce timeout expires. It sounds like the underlying bug here is that ChromeOS is sending it too early today (at the moment when the finger releases the screen). > I'd like to discuss about the issue some more and get ideas about what's the better approach > for implementing the latching, but I'd prefer to land this cl to solve the regression and > change the latching implementation later on, if needed. Sorry, please instead revert the original regressing patch on ToT instead until we have the proper fix, since http://crbug.com/526463 is nonurgent. I don't think we should land the caching logic -- it's the kind of thing that I think is likely to cause problems in the future.
On 2016/11/01 21:25:01, aelias wrote: > On 2016/10/28 at 16:48:46, sahel wrote: > > touchpad, ChrOs: flings and scrollends are in debouncing queue and they get > pushed to > > gesture_queue based on the time out : in this situation we can check the > scrollend > > before sending it to the actual gestureevent. We can either not send it at all > > (and only send the fling) or we can inject information saying that a fling is > > happening next and then send the event) In either case, we have to wait to see > if we > > get a fling from os or not, before sending the scrollend. > > OK, thanks for doing the research on platform behaviors, I appreciate it. What > I'm hearing is that this is a ChromeOS specific issue. On ChromeOS, we have > full control to apply the fix at any level of the stack and don't need to resort > to any hack. I'd suggest that we change ChromeOS to delay sending the ScrollEnd > at the moment when the debounce timeout expires. It sounds like the underlying > bug here is that ChromeOS is sending it too early today (at the moment when the > finger releases the screen). > > > I'd like to discuss about the issue some more and get ideas about what's the > better approach > > for implementing the latching, but I'd prefer to land this cl to solve the > regression and > > change the latching implementation later on, if needed. > > Sorry, please instead revert the original regressing patch on ToT instead until > we have the proper fix, since http://crbug.com/526463 is nonurgent. I don't > think we should land the caching logic -- it's the kind of thing that I think is > likely to cause problems in the future. I believe the regressing patch is fixed on ToT. I agree that 526463 is non-urgent, though it is important. If I understand correctly, this isn't Chrome OS specific. If it was, like you say, we could just fix ChromeOS. "touchpad, mac: we don’t get fling events from os, we get scroll events by phase and momentum phase info, a scroll event by phase=phaseEnded might be followed up by a scroll event with momentumPhase=phaseBegan. Again when we want to send a scrollend, we don’t know that if a fling is going to happen or not. Unless if we wait and see if we will receive a event with momentumPhase=phaseBegan from OS or not." On Mac, at the time when we receive a scroll event with phase=phaseEnded, we don't know if a fling is coming. Between the event with phase=phaseEnded and momentumPhase=phaseBegin, we need to persist the scroll state somewhere, and if no event with momentumPhase=phaseBegin occurs, we need to clear that state. We definitely want to unify the gesture streams for touch and touchpad eventually, but it seems reasonably likely to me that we'll need to switch to firing separate scroll end and fling start events, in order to support the behavior of all platforms.
On 2016/11/02 12:09:22, tdresser wrote: > On 2016/11/01 21:25:01, aelias wrote: > > On 2016/10/28 at 16:48:46, sahel wrote: > > > touchpad, ChrOs: flings and scrollends are in debouncing queue and they get > > pushed to > > > gesture_queue based on the time out : in this situation we can check the > > scrollend > > > before sending it to the actual gestureevent. We can either not send it at > all > > > (and only send the fling) or we can inject information saying that a fling > is > > > happening next and then send the event) In either case, we have to wait to > see > > if we > > > get a fling from os or not, before sending the scrollend. > > > > OK, thanks for doing the research on platform behaviors, I appreciate it. > What > > I'm hearing is that this is a ChromeOS specific issue. On ChromeOS, we have > > full control to apply the fix at any level of the stack and don't need to > resort > > to any hack. I'd suggest that we change ChromeOS to delay sending the > ScrollEnd > > at the moment when the debounce timeout expires. It sounds like the > underlying > > bug here is that ChromeOS is sending it too early today (at the moment when > the > > finger releases the screen). > > > > > I'd like to discuss about the issue some more and get ideas about what's the > > better approach > > > for implementing the latching, but I'd prefer to land this cl to solve the > > regression and > > > change the latching implementation later on, if needed. > > > > Sorry, please instead revert the original regressing patch on ToT instead > until > > we have the proper fix, since http://crbug.com/526463 is nonurgent. I don't > > think we should land the caching logic -- it's the kind of thing that I think > is > > likely to cause problems in the future. > > I believe the regressing patch is fixed on ToT. I agree that 526463 is > non-urgent, though it is important. > > If I understand correctly, this isn't Chrome OS specific. If it was, like you > say, we could just fix ChromeOS. > > "touchpad, mac: we don’t get fling events from os, we get scroll events by phase > and momentum > phase info, a scroll event by phase=phaseEnded might be followed up by a scroll > event > with momentumPhase=phaseBegan. Again when we want to send a scrollend, we don’t > know > that if a fling is going to happen or not. Unless if we wait and see if we will > receive > a event with momentumPhase=phaseBegan from OS or not." > > On Mac, at the time when we receive a scroll event with phase=phaseEnded, we > don't know if > a fling is coming. Between the event with phase=phaseEnded and > momentumPhase=phaseBegin, > we need to persist the scroll state somewhere, and if no event with > momentumPhase=phaseBegin > occurs, we need to clear that state. > > We definitely want to unify the gesture streams for touch and touchpad > eventually, but > it seems reasonably likely to me that we'll need to switch to firing separate > scroll end > and fling start events, in order to support the behavior of all platforms. My mistake, it isn't fixed on ToT. Agreed that fixing on ToT is our first priority.
On 2016/11/02 at 12:09:22, tdresser wrote: > If I understand correctly, this isn't Chrome OS specific. If it was, like you > say, we could just fix ChromeOS. > > "touchpad, mac: we don’t get fling events from os, we get scroll events by phase and momentum > phase info, a scroll event by phase=phaseEnded might be followed up by a scroll event > with momentumPhase=phaseBegan. Again when we want to send a scrollend, we don’t know > that if a fling is going to happen or not. Unless if we wait and see if we will receive > a event with momentumPhase=phaseBegan from OS or not." > > On Mac, at the time when we receive a scroll event with phase=phaseEnded, we don't know if > a fling is coming. Between the event with phase=phaseEnded and momentumPhase=phaseBegin, > we need to persist the scroll state somewhere, and if no event with momentumPhase=phaseBegin > occurs, we need to clear that state. > > We definitely want to unify the gesture streams for touch and touchpad eventually, but > it seems reasonably likely to me that we'll need to switch to firing separate scroll end > and fling start events, in order to support the behavior of all platforms. OK, I apologize, I didn't read the Mac explanation carefully enough and misunderstood. I'd like to request that sahel@ empirically investigate the Mac behavior further in two ways I think are relevant to the design here: 1) Does Mac *always* send a non-momentum scrollBegin/scrollEnd before the momentum ScrollBegin? If this assumption does not hold, then I think the approach in this CL is broken and we would be forced to use a timeout instead. (The scenario is: drag scroll not followed by a fling. Wait 10 seconds. Now begin a quick fling not preceded by drag scroll.) 2) By any chance, does Mac always post scrollEnd+momentum scrollBegin at the same time to the message loop? If so, there is a neat trick we can do which has the benefits of a timeout approach without being racy: on scrollEnd on the browser UI thread, post an "actually scrollEnd" task back to the browser UI thread message loop; within that task, if a fling has started, do nothing, otherwise scrollEnd. If this works, I would consider that approach the cleanest design to solve this. Here's my decision tree: If the answer to 2) is Yes then let's ship that, else if the answer to 1) is No let's write a millisecond-based timeout approach, else I plan to lgt'm this patch as is.
cc'ing tapted@ for arcane Mac scroll event questions. Do you already know the answer to my questions above by any chance?
On 2016/11/03 01:18:04, aelias wrote: > cc'ing tapted@ for arcane Mac scroll event questions. Do you already know the > answer to my questions above by any chance? There's some stuff for generating trackpad scroll sequences here - https://cs.chromium.org/chromium/src/ui/events/cocoa/events_mac_unittest.mm?q... with a bunch of empirical analysis Feel free to move EventsMacTest::TrackpadScrollSequence out somewhere more useful On 2016/11/03 01:16:50, aelias wrote: > On 2016/11/02 at 12:09:22, tdresser wrote: > > If I understand correctly, this isn't Chrome OS specific. If it was, like you > > say, we could just fix ChromeOS. > > > > "touchpad, mac: we don’t get fling events from os, we get scroll events by > phase and momentum > > phase info, a scroll event by phase=phaseEnded might be followed up by a > scroll event > > with momentumPhase=phaseBegan. Again when we want to send a scrollend, we > don’t know > > that if a fling is going to happen or not. Unless if we wait and see if we > will receive > > a event with momentumPhase=phaseBegan from OS or not." > > > > On Mac, at the time when we receive a scroll event with phase=phaseEnded, we > don't know if > > a fling is coming. Between the event with phase=phaseEnded and > momentumPhase=phaseBegin, > > we need to persist the scroll state somewhere, and if no event with > momentumPhase=phaseBegin > > occurs, we need to clear that state. > > > > We definitely want to unify the gesture streams for touch and touchpad > eventually, but > > it seems reasonably likely to me that we'll need to switch to firing separate > scroll end > > and fling start events, in order to support the behavior of all platforms. > > OK, I apologize, I didn't read the Mac explanation carefully enough and > misunderstood. I'd like to request that sahel@ empirically investigate the Mac > behavior further in two ways I think are relevant to the design here: > 1) Does Mac *always* send a non-momentum scrollBegin/scrollEnd before the > momentum ScrollBegin? If this assumption does not hold, then I think the > approach in this CL is broken and we would be forced to use a timeout instead. > (The scenario is: drag scroll not followed by a fling. Wait 10 seconds. Now > begin a quick fling not preceded by drag scroll.) Yes. I've always observed this to be the case. I think it's impossible to generate the required momentum without some "real" delta movement beforehand, and that will always be wrapped between a scrollBegin and a scrollEnd. > 2) By any chance, does Mac always post scrollEnd+momentum scrollBegin at the > same time to the message loop? If so, there is a neat trick we can do which has > the benefits of a timeout approach without being racy: on scrollEnd on the > browser UI thread, post an "actually scrollEnd" task back to the browser UI > thread message loop; within that task, if a fling has started, do nothing, > otherwise scrollEnd. If this works, I would consider that approach the cleanest > design to solve this. I'm not sure of the answer to this. I'd say it's likely.. and it's probably not a disaster if a momentum component gets ignored because the non-momentum part has been marked complete. > > Here's my decision tree: If the answer to 2) is Yes then let's ship that, else > if the answer to 1) is No let's write a millisecond-based timeout approach, else > I plan to lgt'm this patch as is.
Another thing I realized is that Windows 10 natively supports fling and the only reason Chrome doesn't is because we use a legacy gesture API. We should make sure the design we chose is also a good fit for Windows when we fix http://crbug.com/582193 . This means another blocker for fixing this from my POV would be to investigate the Windows API, see if it has a ScrollEnd before FlingStart, and if so empirically test to answer questions 1) and 2) for Windows as well. Apologies for requesting all this investigation, I realize my requests make this makes this a lot more work for you, but this seems like a very tricky problem and my attitude is that we should make sure we fully understand platform APIs before we commit to workarounds for their quirks. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
