Fix scroll chaining for non-descendants of root scroller.

Reland: 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.

Note: this issue was previously landed at {#420763} but reverted in @{#422152}

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

[bokan, 2016-09-23 00:01:36]

Description was changed from

==========
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.

Also removed LayerImpl::ApplyScroll as it's unused.

BUG=505516
==========

to

==========
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.

Also removed LayerImpl::ApplyScroll as it's unused.

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

[bokan, 2016-09-23 00:02:31]

Description was changed from

==========
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.

Also removed LayerImpl::ApplyScroll as it's unused.

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

to

==========
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
==========

[bokan, 2016-09-23 01:48:38]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[bokan, 2016-09-23 01:48:50]

bokan@chromium.org changed reviewers:
+ tdresser@chromium.org

[commit-bot: I haz the power, 2016-09-23 01:49:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2016-09-23 01:53:35]

bokan@chromium.org changed reviewers:
+ jaydasika@chromium.org

[bokan, 2016-09-23 01:53:56]

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

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_hos...
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.

[commit-bot: I haz the power, 2016-09-23 02:47:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 02:47:28]

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...)

[tdresser, 2016-09-23 15:59:58]

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

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_hos...
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_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_hos...
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_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_hos...
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_hos...
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_hos...
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?

[bokan, 2016-09-23 19:57:01]

Looking into trybot failures, they pass locally...

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

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_hos...
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_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_hos...
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_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_hos...
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_hos...
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_hos...
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.

[bokan, 2016-09-23 19:57:08]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 19:57:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tdresser, 2016-09-23 20:40:30]

LGTM.

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

https://codereview.chromium.org/2365793002/diff/20001/cc/trees/layer_tree_hos...
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.

[bokan, 2016-09-23 20:57:43]

bokan@chromium.org changed reviewers:
+ aelias@chromium.org

[bokan, 2016-09-23 20:57:43]

+aelias for cc OWNER

[commit-bot: I haz the power, 2016-09-23 21:41:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 21:41:35]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-09-23 22:02:34]

lgtm

[bokan, 2016-09-23 22:02:59]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-09-23 22:03:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 22:53:36]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-09-23 22:53:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-23 22:55:20]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/bc48cb30d9e473b693dc996b7750510d05d50f69
Cr-Commit-Position: refs/heads/master@{#420763}
==========

[commit-bot: I haz the power, 2016-09-23 22:55:21]

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

[bokan, 2016-09-30 16:05:01]

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

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

[bokan, 2016-10-05 00:06:44]

Description was changed from

==========
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

Committed: https://crrev.com/bc48cb30d9e473b693dc996b7750510d05d50f69
Cr-Commit-Position: refs/heads/master@{#420763}
==========

to

==========
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

Committed: https://crrev.com/bc48cb30d9e473b693dc996b7750510d05d50f69
Cr-Commit-Position: refs/heads/master@{#420763}
==========

[bokan, 2016-10-05 00:24:08]

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_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2365793002/diff/80001/cc/trees/layer_tree_hos...
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?

[bokan, 2016-10-05 13:15:32]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 13:15:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 13:59:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 13:59:20]

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...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bokan, 2016-10-05 20:48:05]

aelias@: ping.

[aelias_OOO_until_Jul13, 2016-10-05 21:55:42]

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

https://codereview.chromium.org/2365793002/diff/80001/cc/trees/layer_tree_hos...
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?

[bokan, 2016-10-05 22:50:38]

On 2016/10/05 21:55:42, aelias wrote:
>
https://codereview.chromium.org/2365793002/diff/80001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2365793002/diff/80001/cc/trees/layer_tree_hos...
> 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?

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

[bokan, 2016-10-12 12:29:26]

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.

[bokan, 2016-10-12 12:29:31]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-10-12 12:29:32]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2365793002/#ps120001
(title: "Rebase and remove hack")

[commit-bot: I haz the power, 2016-10-12 12:29:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 13:49:04]

Description was changed from

==========
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

Committed: https://crrev.com/bc48cb30d9e473b693dc996b7750510d05d50f69
Cr-Commit-Position: refs/heads/master@{#420763}
==========

to

==========
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

Committed: https://crrev.com/bc48cb30d9e473b693dc996b7750510d05d50f69
Cr-Commit-Position: refs/heads/master@{#420763}
==========

[commit-bot: I haz the power, 2016-10-12 13:49:06]

Committed patchset #7 (id:120001)

[bokan, 2016-10-12 13:50:39]

Description was changed from

==========
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

Committed: https://crrev.com/bc48cb30d9e473b693dc996b7750510d05d50f69
Cr-Commit-Position: refs/heads/master@{#420763}
==========

to

==========
Reland: 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
==========

[commit-bot: I haz the power, 2016-10-12 13:50:50]

Description was changed from

==========
Reland: 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
==========

to

==========
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

Committed: https://crrev.com/bc48cb30d9e473b693dc996b7750510d05d50f69
Committed: https://crrev.com/5ded2662fcd1ef488aff3b9b5d9374c5635a8128
Cr-Original-Commit-Position: refs/heads/master@{#420763}
Cr-Commit-Position: refs/heads/master@{#424731}
==========

[commit-bot: I haz the power, 2016-10-12 13:50:51]

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

[bokan, 2016-10-12 13:51:18]

Description was changed from

==========
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

Committed: https://crrev.com/bc48cb30d9e473b693dc996b7750510d05d50f69
Committed: https://crrev.com/5ded2662fcd1ef488aff3b9b5d9374c5635a8128
Cr-Original-Commit-Position: refs/heads/master@{#420763}
Cr-Commit-Position: refs/heads/master@{#424731}
==========

to

==========
Reland: 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.

Note: this issue was previously landed at {#420763} but reverted in @{#422152}

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