Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118773002:
Respect main thread scrolling reasons for fallback to root layer scrolling. (Closed)
|
Created:
4 years, 5 months ago by Eric Seckler Modified:
4 years, 5 months ago CC:
chromium-reviews, blink-layers+watch_chromium.org, kenneth.christiansen, cc-bugs_chromium.org, blink-reviews, Sami Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRespect main thread scrolling reasons for fallback to root layer scrolling.
For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can thus simply check for main thread scrolling reasons on the final layer selected for scrolling in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint.
BUG=625100
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/59eeb0282913e575a4409c257ce13f6de8e03d69
Cr-Commit-Position: refs/heads/master@{#403765}
Patch Set 1 #Patch Set 2 : Fix test failures, add additional unit tests. #Patch Set 3 : gclient sync to fix compile issue. #
Total comments: 11
Patch Set 4 : Address reviewer comments. #
Dependent Patchsets: Messages
Total messages: 29 (13 generated)
Description was changed from ========== Respect main thread scrolling reasons for fallback to root layer scrolling. For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can then simply perform the same scroll tree traversal for the fallback to the root layer in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint. BUG=625100 ========== to ========== Respect main thread scrolling reasons for fallback to root layer scrolling. For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can then simply perform the same scroll tree traversal for the fallback to the root layer in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint. BUG=625100 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
eseckler@chromium.org changed reviewers: + bokan@chromium.org
Here's a first try. I think an alternative would be to fall back to the outer viewport scroll layer instead of the inner one (which is later replaced by the inner one), in which case we don't need to add the main thread scrolling reasons to the inner scroll layer in ScrollCoordinator. WDYT? :)
On 2016/07/01 10:30:25, Eric Seckler wrote: > Here's a first try. I think an alternative would be to fall back to the outer > viewport scroll layer instead of the inner one (which is later replaced by the > inner one), in which case we don't need to add the main thread scrolling reasons > to the inner scroll layer in ScrollCoordinator. WDYT? :) Second try - This time (hopefully) without test failures. Moving the fallback before the traversal alone does not suffic, e.g. for the case where the inner layer is not scrollable and should be overscrolled on IMPL. Thus, I moved the fallback back to the original location and added another main-thread-scrolling check before returning the final scroll layer. I also added unit tests that check the behavior in LayerTreeHostImpl and ScrollingCoordinator separately. Let me know if you think we should add an integration test that checks both of them in unison as well.
The CQ bit was checked by eseckler@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by eseckler@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.
This approach looks fine to me. Re: integration test, unfortunately we don't have a good way of doing integration between Blink and the compositor other than browser tests which are quite heavy weight so two unit tests are fine I think. https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2554: *scroll_on_main_thread = true; Please remove scroll_on_main_thread and main_thread_scrolling_reasons as output params here and just set them based on the if statements below. (it's not clear from the call site these are output params). https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:6278: LayerImpl* scroll_layer = SetupScrollAndContentsLayers(gfx::Size(50, 50)); Use CreateBasicVirtualViewportLayers to build the layer tree instead. That creates a setup that reflects our reality better. Note that it returns the content layer, not the inner scroll layer so you'll have to set scroll_layer using host_impl_->active_tree()->InnerViewportScrollLayer() https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:6283: scroll_layer->test_properties()->parent->test_properties()->parent; Use active_tree()->InnerViewportContainerLayer() https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:6299: // Overscroll initiated inside layers will be handled by the main thread. Could you add a similar test that doesn't set the main thread scrolling reason and makes sure that overscroll is still handled on the impl thread in that case? https://codereview.chromium.org/2118773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2118773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:685: visualViewportScrollLayer->addMainThreadScrollingReasons(mainThreadScrollingReasons); I think we need to do the above animation takeover for the VisualViewport but I don't understand this code well. +ymalik@ to confirm
Description was changed from ========== Respect main thread scrolling reasons for fallback to root layer scrolling. For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can then simply perform the same scroll tree traversal for the fallback to the root layer in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint. BUG=625100 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Respect main thread scrolling reasons for fallback to root layer scrolling. For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can then simply perform the same scroll tree traversal for the fallback to the root layer in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint. BUG=625100 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
bokan@chromium.org changed reviewers: + ymalik@chromium.org
oops. Actually +ymalik@, see animation question above
Description was changed from ========== Respect main thread scrolling reasons for fallback to root layer scrolling. For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can then simply perform the same scroll tree traversal for the fallback to the root layer in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint. BUG=625100 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Respect main thread scrolling reasons for fallback to root layer scrolling. For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can thus simply check for main thread scrolling reasons on the final layer selected for scrolling in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint. BUG=625100 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Thanks for the speedy review, David! https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:2554: *scroll_on_main_thread = true; On 2016/07/01 19:02:28, bokan wrote: > Please remove scroll_on_main_thread and main_thread_scrolling_reasons as output > params here and just set them based on the if statements below. (it's not clear > from the call site these are output params). Done. https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:6278: LayerImpl* scroll_layer = SetupScrollAndContentsLayers(gfx::Size(50, 50)); On 2016/07/01 19:02:28, bokan wrote: > Use CreateBasicVirtualViewportLayers to build the layer tree instead. That > creates a setup that reflects our reality better. Note that it returns the > content layer, not the inner scroll layer so you'll have to set scroll_layer > using host_impl_->active_tree()->InnerViewportScrollLayer() Done. https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:6283: scroll_layer->test_properties()->parent->test_properties()->parent; On 2016/07/01 19:02:28, bokan wrote: > Use active_tree()->InnerViewportContainerLayer() Removed this part, should now be handled by CreateBasicVirtualViewportLayers(). https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:6299: // Overscroll initiated inside layers will be handled by the main thread. On 2016/07/01 19:02:28, bokan wrote: > Could you add a similar test that doesn't set the main thread scrolling reason > and makes sure that overscroll is still handled on the impl thread in that case? Done. https://codereview.chromium.org/2118773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2118773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:685: visualViewportScrollLayer->addMainThreadScrollingReasons(mainThreadScrollingReasons); On 2016/07/01 19:02:28, bokan wrote: > I think we need to do the above animation takeover for the VisualViewport but I > don't understand this code well. +ymalik@ to confirm Seems like you're right. Added the take-over, but will wait for feedback from ymalik@ before committing :)
https://codereview.chromium.org/2118773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2118773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:685: visualViewportScrollLayer->addMainThreadScrollingReasons(mainThreadScrollingReasons); On 2016/07/04 08:33:26, Eric Seckler wrote: > On 2016/07/01 19:02:28, bokan wrote: > > I think we need to do the above animation takeover for the VisualViewport but > I > > don't understand this code well. +ymalik@ to confirm > > Seems like you're right. Added the take-over, but will wait for feedback from > ymalik@ before committing :) Yes, the takeover for the visual viewport will ensure that any impl-only scroll offset animation is completed on main thread.
lgtm
bokan@chromium.org changed reviewers: + ajuma@chromium.org
+ajuma@ for CC OWNER
cc lgtm
The CQ bit was checked by eseckler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Respect main thread scrolling reasons for fallback to root layer scrolling. For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can thus simply check for main thread scrolling reasons on the final layer selected for scrolling in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint. BUG=625100 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Respect main thread scrolling reasons for fallback to root layer scrolling. For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can thus simply check for main thread scrolling reasons on the final layer selected for scrolling in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint. BUG=625100 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Respect main thread scrolling reasons for fallback to root layer scrolling. For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can thus simply check for main thread scrolling reasons on the final layer selected for scrolling in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint. BUG=625100 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Respect main thread scrolling reasons for fallback to root layer scrolling. For this purpose, ScrollingCoordinator now also commits the main thread scrolling reasons to the visual viewport scroll layer. We can thus simply check for main thread scrolling reasons on the final layer selected for scrolling in LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint. BUG=625100 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/59eeb0282913e575a4409c257ce13f6de8e03d69 Cr-Commit-Position: refs/heads/master@{#403765} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/59eeb0282913e575a4409c257ce13f6de8e03d69 Cr-Commit-Position: refs/heads/master@{#403765} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
