Respect main thread scrolling reasons for fallback to root layer scrolling. (issue 2118773002 by eseckler@chromium.org)

1 view
Skip to first unread message

esec...@chromium.org

unread,
Jul 1, 2016, 6:30:26 AM7/1/16
to bo...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org
Reviewers: bokan
CL: https://codereview.chromium.org/2118773002/

Message:
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? :)

Description:
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

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+11, -6 lines):
M cc/trees/layer_tree_host_impl.cc
M third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp


Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index 053ae8028000cff6ff1c0d18d8a862866f76b094..190716778a92be08b827ad10e4c7f2993ae88e12 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -2548,6 +2548,12 @@ LayerImpl* LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint(
*main_thread_scrolling_reasons =
MainThreadScrollingReason::kNotScrollingOnMain;

+ // Falling back to the root scroll layer ensures generation of root overscroll
+ // notifications. The inner viewport layer represents the viewport during
+ // scrolling.
+ if (!layer_impl)
+ layer_impl = InnerViewportScrollLayer();
+
// Walk up the hierarchy and look for a scrollable layer.
LayerImpl* potentially_scrolling_layer_impl = NULL;
if (layer_impl) {
@@ -2580,12 +2586,6 @@ LayerImpl* LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint(
}
}
}
- // Falling back to the root scroll layer ensures generation of root overscroll
- // notifications while preventing scroll updates from being unintentionally
- // forwarded to the main thread. The inner viewport layer represents the
- // viewport during scrolling.
- if (!potentially_scrolling_layer_impl)
- potentially_scrolling_layer_impl = InnerViewportScrollLayer();

// The inner viewport layer represents the viewport.
if (potentially_scrolling_layer_impl == OuterViewportScrollLayer())
Index: third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
diff --git a/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp b/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
index 4b444f9b9237fa328f1dcbec4003f9b4d6c1f077..98788a98b0c73df40eaa0ba0f384a2d9d5be4d19 100644
--- a/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
+++ b/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
@@ -671,6 +671,7 @@ void ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThread(MainTh
if (!m_page->mainFrame()->isLocalFrame() || !m_page->deprecatedLocalMainFrame()->view())
return;

+ WebLayer* visualViewportScrollLayer = toWebLayer(m_page->frameHost().visualViewport().scrollLayer());
GraphicsLayer* layer = m_page->deprecatedLocalMainFrame()->view()->layerForScrolling();
if (WebLayer* scrollLayer = toWebLayer(layer)) {
m_lastMainThreadScrollingReasons = mainThreadScrollingReasons;
@@ -680,12 +681,16 @@ void ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThread(MainTh
scrollAnimator->takeOverCompositorAnimation();
}
scrollLayer->addMainThreadScrollingReasons(mainThreadScrollingReasons);
+ if (visualViewportScrollLayer)
+ visualViewportScrollLayer->addMainThreadScrollingReasons(mainThreadScrollingReasons);
} else {
// Clear all main thread scrolling reasons except the one that's set
// if there is a running scroll animation.
uint32_t mainThreadScrollingReasonsToClear = ~0u;
mainThreadScrollingReasonsToClear &= ~MainThreadScrollingReason::kAnimatingScrollOnMainThread;
scrollLayer->clearMainThreadScrollingReasons(mainThreadScrollingReasonsToClear);
+ if (visualViewportScrollLayer)
+ visualViewportScrollLayer->clearMainThreadScrollingReasons(mainThreadScrollingReasonsToClear);
}
}
}


esec...@chromium.org

unread,
Jul 1, 2016, 11:55:56 AM7/1/16
to bo...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org
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.

https://codereview.chromium.org/2118773002/

bo...@chromium.org

unread,
Jul 1, 2016, 3:02:29 PM7/1/16
to esec...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org
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_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_host_impl.cc#newcode2554
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_host_impl_unittest.cc
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_host_impl_unittest.cc#newcode6278
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_host_impl_unittest.cc#newcode6283
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_host_impl_unittest.cc#newcode6299
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/Source/core/page/scrolling/ScrollingCoordinator.cpp
File
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2118773002/diff/40001/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp#newcode685
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

https://codereview.chromium.org/2118773002/

bo...@chromium.org

unread,
Jul 1, 2016, 3:44:42 PM7/1/16
to esec...@chromium.org, yma...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org
oops. Actually +ymalik@, see animation question above

https://codereview.chromium.org/2118773002/

esec...@chromium.org

unread,
Jul 4, 2016, 4:33:26 AM7/4/16
to bo...@chromium.org, yma...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org
Thanks for the speedy review, David!
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_host_impl_unittest.cc
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2118773002/diff/40001/cc/trees/layer_tree_host_impl_unittest.cc#newcode6278
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_host_impl_unittest.cc#newcode6283
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_host_impl_unittest.cc#newcode6299
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/Source/core/page/scrolling/ScrollingCoordinator.cpp
File
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2118773002/diff/40001/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp#newcode685
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/

yma...@chromium.org

unread,
Jul 4, 2016, 9:52:08 AM7/4/16
to esec...@chromium.org, bo...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org

https://codereview.chromium.org/2118773002/diff/40001/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
File
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2118773002/diff/40001/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp#newcode685
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.

https://codereview.chromium.org/2118773002/

bo...@chromium.org

unread,
Jul 4, 2016, 5:20:07 PM7/4/16
to esec...@chromium.org, yma...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org

bo...@chromium.org

unread,
Jul 4, 2016, 5:21:31 PM7/4/16
to esec...@chromium.org, yma...@chromium.org, ajuma+r...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org

aj...@chromium.org

unread,
Jul 4, 2016, 5:36:07 PM7/4/16
to esec...@chromium.org, bo...@chromium.org, yma...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 5, 2016, 4:36:21 AM7/5/16
to esec...@chromium.org, bo...@chromium.org, yma...@chromium.org, ajuma+r...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 5, 2016, 6:45:31 AM7/5/16
to esec...@chromium.org, bo...@chromium.org, yma...@chromium.org, ajuma+r...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org
Committed patchset #4 (id:60001)

https://codereview.chromium.org/2118773002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 5, 2016, 6:47:41 AM7/5/16
to esec...@chromium.org, bo...@chromium.org, yma...@chromium.org, ajuma+r...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-lay...@chromium.org, kenneth.ch...@gmail.com, cc-...@chromium.org, blink-...@chromium.org, skyo...@chromium.org
Patchset 4 (id:??) landed as
https://crrev.com/59eeb0282913e575a4409c257ce13f6de8e03d69
Cr-Commit-Position: refs/heads/master@{#403765}

https://codereview.chromium.org/2118773002/
Reply all
Reply to author
Forward
0 new messages