Fix scroll chaining for non-descendants of root scroller. (issue 2365793002 by bokan@chromium.org)

[bo...@chromium.org]

Reviewers: tdresser (OOO Sept 16-21)
CL: https://codereview.chromium.org/2365793002/

Description:
Fix scroll chaining for non-descendants of root scroller.

This CL fixes the scroll chaining behavior for elements that aren't descendants
of the root scroller element. This can only happen in the non-document root
scroller proposal.

On the main thread scrolling path, we were previously assuming that all scrolls
would chain through the effective root scroller. This CL fixes the terminating
condition as well as removes an invalid DCHECK.

On the compositor side, the chaining mechanisms would previously use the inner
viewport scroll layer to designate viewport scrolling. Further on, we would
explicitly check for the inner viewport layer and scroll using cc::Viewport.
When scrolling a non-descendant of the root scroller, we chain up to the inner
viewport scroll layer without going through the outer viewport. Scrolling
cc::Viewport causes scrolling in the outer viewport though so we would scroll
it inadvertaintly. I've made changes so that we use the outer viewport to
designate cc::Viewport scrolling. Scrolls will terminate at the outer viewport
scroll layer, but they don't have to chain through it.

Unrelated, but I also removed LayerImpl::ApplyScroll as it's unused.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Affected files (+318, -46 lines):
M cc/layers/layer_impl.h
M cc/layers/layer_impl.cc
M cc/trees/layer_tree_host_impl.cc
M cc/trees/layer_tree_host_impl_unittest.cc
A third_party/WebKit/LayoutTests/fast/scrolling/scroll-non-descendant-of-root-scroller.html
M third_party/WebKit/Source/core/input/ScrollManager.cpp

[bo...@chromium.org]

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode11343
cc/trees/layer_tree_host_impl_unittest.cc:11343:
host_impl_->active_tree()->InnerViewportScrollLayer()->id());
The LastScrolledLayerId is set using SetCurrentlyScrollingLayer which
isn't completely precise w.r.t the viewport. For viewport scrolling, we
always set the currently scrolling layer as the one layer that
represents the viewport. Previously this was inner but this patch
switched it to outer.

In this test, its the inner viewport that gets scrolled and so the last
scrolled was previously correct but breaks now since the below "hacking"
is applied to the wrong layer.

I'm not sure if this is a problem in practice, however, it's already
broken today for the opposite case. So if the user scrolls the outer
viewport, the last scrolled layer id will be set to the inner.
+jaydasika@ as FYI.

https://codereview.chromium.org/2365793002/

[tdre...@chromium.org]

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl.cc#oldcode3243
cc/trees/layer_tree_host_impl.cc:3243: scroll_layer_impl =
OuterViewportScrollLayer();
I'm a bit confused by why this was here before, and why it was removed.

Do you know what this was doing?

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode2577
cc/trees/layer_tree_host_impl.cc:2577: potentially_scrolling_layer_impl
= InnerViewportScrollLayer();
Shouldn't this be OuterViewportScrollLayer?

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode6509
cc/trees/layer_tree_host_impl_unittest.cc:6509: outer_scroll_layer =
scroll.get();
Having a raw pointer point to something owned by a unique_ptr is a bit
scary. I'm not sure why this doesn't crash.

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode6550
cc/trees/layer_tree_host_impl_unittest.cc:6550:
outer_scroll_layer->CurrentScrollOffset());
Shouldn't outer_scroll_layer be deleted by the time you get here?

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode11343
cc/trees/layer_tree_host_impl_unittest.cc:11343:
host_impl_->active_tree()->InnerViewportScrollLayer()->id());

On 2016/09/23 01:53:55, bokan wrote:
> The LastScrolledLayerId is set using SetCurrentlyScrollingLayer which
isn't
> completely precise w.r.t the viewport. For viewport scrolling, we
always set the
> currently scrolling layer as the one layer that represents the
viewport.
> Previously this was inner but this patch switched it to outer.
>
> In this test, its the inner viewport that gets scrolled and so the
last scrolled
> was previously correct but breaks now since the below "hacking" is
applied to
> the wrong layer.
>
> I'm not sure if this is a problem in practice, however, it's already
broken
> today for the opposite case. So if the user scrolls the outer
viewport, the last
> scrolled layer id will be set to the inner. +jaydasika@ as FYI.

This seems a bit scary – it should be a pretty simple fix, shouldn't it?

https://codereview.chromium.org/2365793002/

[bo...@chromium.org]

Looking into trybot failures, they pass locally...

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl.cc#oldcode3243
cc/trees/layer_tree_host_impl.cc:3243: scroll_layer_impl =
OuterViewportScrollLayer();

On 2016/09/23 15:59:58, tdresser (OOO Sept 26-28) wrote:
> I'm a bit confused by why this was here before, and why it was
removed.
>
> Do you know what this was doing?

I believe it's needed because the ScrollbarAnimationController for the
viewport is registered on the outer viewport scroll layer below. It's
removed since we now return the outer scroll layer from
FindScrollLayerForDeviceViewportPoint (and there's no way we can get
back the inner).

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode2577
cc/trees/layer_tree_host_impl.cc:2577: potentially_scrolling_layer_impl
= InnerViewportScrollLayer();

On 2016/09/23 15:59:58, tdresser (OOO Sept 26-28) wrote:
> Shouldn't this be OuterViewportScrollLayer?

Yah, I wasn't sure since changing it broke some tests. Taking a closer
look now, it does seem like we should return outer and the tests were
just overfitting. Changed to Outer and fixed tests.

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode6509
cc/trees/layer_tree_host_impl_unittest.cc:6509: outer_scroll_layer =
scroll.get();

On 2016/09/23 15:59:58, tdresser (OOO Sept 26-28) wrote:
> Having a raw pointer point to something owned by a unique_ptr is a bit
scary.
> I'm not sure why this doesn't crash.

It's because we std::move them into the layer tree below, the
LayerTreeImpl maintains a unique_ptr to each layer so I think using a
raw ptr here should be ok since the LayerTreeImpl is long lived.

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode6550
cc/trees/layer_tree_host_impl_unittest.cc:6550:
outer_scroll_layer->CurrentScrollOffset());

On 2016/09/23 15:59:58, tdresser (OOO Sept 26-28) wrote:
> Shouldn't outer_scroll_layer be deleted by the time you get here?

The unique_ptr's are std::moved into the layer tree so it's kept around.

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode11343
cc/trees/layer_tree_host_impl_unittest.cc:11343:
host_impl_->active_tree()->InnerViewportScrollLayer()->id());

On 2016/09/23 15:59:58, tdresser (OOO Sept 26-28) wrote:
> On 2016/09/23 01:53:55, bokan wrote:
> > The LastScrolledLayerId is set using SetCurrentlyScrollingLayer
which isn't
> > completely precise w.r.t the viewport. For viewport scrolling, we
always set
> the
> > currently scrolling layer as the one layer that represents the
viewport.
> > Previously this was inner but this patch switched it to outer.
> >
> > In this test, its the inner viewport that gets scrolled and so the
last
> scrolled
> > was previously correct but breaks now since the below "hacking" is
applied to
> > the wrong layer.
> >
> > I'm not sure if this is a problem in practice, however, it's already
broken
> > today for the opposite case. So if the user scrolls the outer
viewport, the
> last
> > scrolled layer id will be set to the inner. +jaydasika@ as FYI.
>
> This seems a bit scary – it should be a pretty simple fix, shouldn't
it?

It's not as straightforward as it seems. This currently gets set during
SetCurrentlyScrollingLayer but, to correctly scroll the viewport,
currently scrolling layer needs to be set to the "representative"
viewport layer rather than the actually scrolled layer. So we'd have to
split this out from the currently scrolling layer. I suspect that it
might be best to set this from the ScrollTree itself whenever a layer's
scroll offset changes but I'm not up to speed on all the details there.
This is only ever used to calculate the "jitter" metric so I'm not sure
what the impact is.

I could make cc::Viewport keep track of its last_scrolled_layer and then
query that from SetCurrentlyScrollingLayer but that seems like extra
machinery and it's not clear what that should do when we scroll both
layers in one scroll update. jaydasika@ added this so he might have some
insight in the right way forward here.

https://codereview.chromium.org/2365793002/

[tdre...@chromium.org]

LGTM.

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode6509
cc/trees/layer_tree_host_impl_unittest.cc:6509: outer_scroll_layer =
scroll.get();

On 2016/09/23 19:57:01, bokan wrote:
> On 2016/09/23 15:59:58, tdresser (OOO Sept 26-28) wrote:
> > Having a raw pointer point to something owned by a unique_ptr is a
bit scary.
> > I'm not sure why this doesn't crash.
>
> It's because we std::move them into the layer tree below, the
LayerTreeImpl
> maintains a unique_ptr to each layer so I think using a raw ptr here
should be
> ok since the LayerTreeImpl is long lived.

D'oh, sure, that's fine.

https://codereview.chromium.org/2365793002/

[bo...@chromium.org]

+aelias for cc OWNER

https://codereview.chromium.org/2365793002/

[ael...@chromium.org]

lgtm



https://codereview.chromium.org/2365793002/

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

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2365793002/60001


https://codereview.chromium.org/2365793002/

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

Committed patchset #4 (id:60001)

https://codereview.chromium.org/2365793002/

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

Patchset 4 (id:??) landed as
https://crrev.com/bc48cb30d9e473b693dc996b7750510d05d50f69
Cr-Commit-Position: refs/heads/master@{#420763}

https://codereview.chromium.org/2365793002/

[bo...@chromium.org]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2382913003/ by bo...@chromium.org.

The reason for reverting is: Broke WebView scrolling. See crbug.com/651515.

https://codereview.chromium.org/2365793002/

[bo...@chromium.org]

I've put up the same patch rebased over my previously landed fix in
https://codereview.chromium.org/2388563002/ so this should fix the issue with
WebView scrolling.

I've also had to add a minor hack to keep telemetry happy as this patch
previously regressed at least one metric.

Alexandre, ptal.


https://codereview.chromium.org/2365793002/diff/80001/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2365793002/diff/80001/cc/trees/layer_tree_host_impl.cc#newcode1971
cc/trees/layer_tree_host_impl.cc:1971: bool
restore_currently_scrolling_viewport = IsCurrentlyScrollingViewport();
I needed to add this hack to preserver behavior telemetry was relying
on. Currently, some benchmarks (sometimes, it's flaky) start a scroll
before the new outer viewport layer is synced to the active tree. With
this change, the CurrentlyScrollingLayer will be null'd and we'll drop
subsequent ScrollBys. This is in line with what would happen if we
removed a scrolling plain layer. The old behavior targeted the inner
viewport which doesn't get replaced on a load so a scroll will work
between page loads which is kind of weird. I'd like to fix this in
telemetry and remove this after. WDYT?

https://codereview.chromium.org/2365793002/

[bo...@chromium.org]

aelias@: ping.

https://codereview.chromium.org/2365793002/

[ael...@chromium.org]

https://codereview.chromium.org/2365793002/diff/80001/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2365793002/diff/80001/cc/trees/layer_tree_host_impl.cc#newcode1971
cc/trees/layer_tree_host_impl.cc:1971: bool
restore_currently_scrolling_viewport = IsCurrentlyScrollingViewport();

On 2016/10/05 at 00:24:08, bokan wrote:
> I needed to add this hack to preserver behavior telemetry was relying
on. Currently, some benchmarks (sometimes, it's flaky) start a scroll
before the new outer viewport layer is synced to the active tree. With
this change, the CurrentlyScrollingLayer will be null'd and we'll drop
subsequent ScrollBys. This is in line with what would happen if we
removed a scrolling plain layer. The old behavior targeted the inner
viewport which doesn't get replaced on a load so a scroll will work
between page loads which is kind of weird. I'd like to fix this in
telemetry and remove this after. WDYT?

This patch is nonurgent, so I'm not sure why we should introduce a
short-term workaround. How about fixing telemetry and then landing this
without the hack after that's rolled?

https://codereview.chromium.org/2365793002/

[bo...@chromium.org]

I was keen on getting this into M55, but on reflection there's really no reason
to rush. Will do.

https://codereview.chromium.org/2365793002/

[bo...@chromium.org]

Relanding patch now that I've landed the fix to telemetry and the original
WebView issue. The relanding is essentially unchanged from what was l-g-t-m'd
here.

https://codereview.chromium.org/2365793002/

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

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2365793002/120001


https://codereview.chromium.org/2365793002/

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

Committed patchset #7 (id:120001)

https://codereview.chromium.org/2365793002/

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

Patchset 7 (id:??) landed as
https://crrev.com/5ded2662fcd1ef488aff3b9b5d9374c5635a8128
Cr-Commit-Position: refs/heads/master@{#424731}

https://codereview.chromium.org/2365793002/