Refactor delegated scrolling in LayerImpl

Refactor delegated scrolling in LayerImpl

When the root layer scroll offset delegate's scroll offset is bigger
than that of the outer viewport scroll layer, the root scroll offset is
distributed between inner and outer viewport scroll layer. However, the
layer that gets assigned a new scroll offset first will immediately
update the root layer delegate scroll offset without waiting for the
rest of the undistributed scrolls to be assigned to other layer. This
causes the associated bug 470360.

This CL simplified the logic around delegated scrolling by removing
LayerImpl::ScrollOffsetDelegate. LayerImpl will always have the most
recent and correct scroll offset.

LTHI and LTI still delegate the root layer scroll to an external source
(Android WebView).
There are 2 cases for a LayerImpl to change its scroll offset:
1. When the root layer scroll delegate scrolls, the new scroll offset will be
synchronously distributed to the inner and outer viewport scroll layer.
2. The LayerImpl scrolls by itself, like ScrollBy() or PushScrollOffset(). In
this case, root layer scroll delegate will be updated with the new scroll
offset.

BUG=470360
Committed: https://crrev.com/33370e19d3c6a43d0cb7c87f90aadb79d44dd741
Cr-Commit-Position: refs/heads/master@{#324012}

[hush (inactive), 2015-03-26 23:25:20]

looks like things are pretty intertwined... a bunch of Android WebView tests are
broken.

[hush (inactive), 2015-03-27 19:57:45]

Patchset #1 (id:1) has been deleted

[hush (inactive), 2015-03-27 19:57:55]

Patchset #1 (id:20001) has been deleted

[hush (inactive), 2015-03-30 22:47:39]

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

[hush (inactive), 2015-03-30 22:50:52]

Hi Alex,
PTAL PS6.
I added some extra logic to make sure the root layer delegate scroll is
distributed to the inner and outer viewport at the same time, without the root
layer delegate scroll being updated in the middle of it.

[aelias_OOO_until_Jul13, 2015-03-30 22:54:40]

https://codereview.chromium.org/1038173002/diff/140001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/1038173002/diff/140001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.h:111: bool
block_updates_;
Nobody ever reads this value, is there some other missing piece to this change?

Also, I don't really like the idea of a "block updates" boolean, we shouldn't
really need a bit of extra state just to maintain consistency on a single
thread.

[hush (inactive), 2015-03-30 23:00:44]

https://codereview.chromium.org/1038173002/diff/140001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/1038173002/diff/140001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.h:111: bool
block_updates_;
On 2015/03/30 22:54:39, aelias wrote:
> Nobody ever reads this value, is there some other missing piece to this
change?
> 
sorry I accidentally removed the usage in one of the patches. It should be used
by UpdateRootLayerState. I just added it back.

> Also, I don't really like the idea of a "block updates" boolean, we shouldn't
> really need a bit of extra state just to maintain consistency on a single
> thread.

I will think about another way to achieve this.

[hush (inactive), 2015-04-06 16:57:40]

Patchset #8 (id:180001) has been deleted

[hush (inactive), 2015-04-06 18:06:28]

Hi Alex!
PS8:
I think it is not necessary for LayerImpl to support delegated scrolling. I
simplified the logic around delegated scrolling by removing
LayerImpl::ScrollOffsetDelegate and replace it with a listener that notifies
LayerTreeImpl when the LayerImpl changes its scroll offset. In this way,
CurrentScrollOffset() of a LayerImpl will always be the most recent and correct
one.
LTHI and LTI still delegate the root layer scroll to an external source (Android
WebView)
There are 2 cases for a LayerImpl to change its scroll offset:
1. When the root layer scroll delegate scrolls, the new scroll offset will be
synchronously distributed to the inner and outer viewport scroll layer.
2. The LayerImpl scrolls by itself, like ScrollBy() or PushScrollOffset(). In
this case, root layer scroll delegate will be updated with the new scroll
offset.

[aelias_OOO_until_Jul13, 2015-04-06 19:47:49]

This looks like a great direction, I like it.  I agree LayerImpl having the
logic is a legacy of when there was only one root scrolling layer.

https://codereview.chromium.org/1038173002/diff/200001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1038173002/diff/200001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:86: class ExternalScrollOffsetListener {
What do you think about going further and get rid of the listener class
entirely?  LayerImpl can just call layer_tree_impl()->IsExternalFlingActive()
and layer_tree_impl()->DidUpdateScrollOffset(id()).  DidUpdateScrollOffset would
take an int layer_id as argument and LayerTreeImpl would compare it to
InnerViewportScrollLayer()->id() and OuterViewportScrollLayer()->id() to know if
it's the root scroll, and ignore it otherwise.

https://codereview.chromium.org/1038173002/diff/200001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:397: // Visible for testing.
It's not clear what methods below this comment refers to, and I think
non-test-code could still legitimately call SetCurrentScrollOffset in the
future, so I'd suggest removing the comment.

[aelias_OOO_until_Jul13, 2015-04-06 19:57:32]

https://codereview.chromium.org/1038173002/diff/200001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1038173002/diff/200001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1081: if (!OuterViewportScrollLayer()) {
You can delete this block and DCHECK that an OuterViewportScrollLayer exists
instead.  The old path of InnerViewportScrollLayer-only is no longer shipping.

[hush (inactive), 2015-04-06 23:08:04]

https://codereview.chromium.org/1038173002/diff/200001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1038173002/diff/200001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:86: class ExternalScrollOffsetListener {
On 2015/04/06 19:47:49, aelias wrote:
> What do you think about going further and get rid of the listener class
> entirely?  LayerImpl can just call layer_tree_impl()->IsExternalFlingActive()
> and layer_tree_impl()->DidUpdateScrollOffset(id()).  DidUpdateScrollOffset
would
> take an int layer_id as argument and LayerTreeImpl would compare it to
> InnerViewportScrollLayer()->id() and OuterViewportScrollLayer()->id() to know
if
> it's the root scroll, and ignore it otherwise.

yes. I added DidUpdateScrollOffset and IsExternalFlingActive in LTI.
I also added an extra state in LTI to indicate the root layer scroll offset is
being distributed to the inner/ outer viewport scroll layers and LTI will ignore
DidUpdateScrollOffset in this case.

https://codereview.chromium.org/1038173002/diff/200001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:397: // Visible for testing.
On 2015/04/06 19:47:49, aelias wrote:
> It's not clear what methods below this comment refers to, and I think
> non-test-code could still legitimately call SetCurrentScrollOffset in the
> future, so I'd suggest removing the comment.

Removed the comment.

https://codereview.chromium.org/1038173002/diff/200001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1038173002/diff/200001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1081: if (!OuterViewportScrollLayer()) {
On 2015/04/06 19:57:32, aelias wrote:
> You can delete this block and DCHECK that an OuterViewportScrollLayer exists
> instead.  The old path of InnerViewportScrollLayer-only is no longer shipping.

The tests are exercising the InnerViewportScrollLayer-only logic a lot.
LayerTreeHostImplTests that invoke CreateScrollAndContentsLayers() will only
create the inner scroll layer.
There are a large number of such tests.. I will just put a TODO here while I am
rewriting these tests.

[aelias_OOO_until_Jul13, 2015-04-07 00:52:20]

Looking good.  Please update the patch description.

https://codereview.chromium.org/1038173002/diff/220001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1038173002/diff/220001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1026: base::AutoReset<bool>
is_distributing_root_scroll_auto_reset(
Could you avoid this by having a new method
LayerImpl::SetCurrentScrollOffsetFromDelegate that avoids the feedback?

[hush (inactive), 2015-04-07 01:29:08]

https://codereview.chromium.org/1038173002/diff/220001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1038173002/diff/220001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1026: base::AutoReset<bool>
is_distributing_root_scroll_auto_reset(
On 2015/04/07 00:52:20, aelias wrote:
> Could you avoid this by having a new method
> LayerImpl::SetCurrentScrollOffsetFromDelegate that avoids the feedback?

Done.

https://codereview.chromium.org/1038173002/diff/220001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1040: if (!OuterViewportScrollLayer()) {
I just removed this block and fixed the unit tests.

[aelias_OOO_until_Jul13, 2015-04-07 02:10:17]

lgtm.  Please update the CL description to reflect the current approach before
you land.

[hush (inactive), 2015-04-07 02:58:36]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2015-04-07 02:58:36]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/1038173002/#ps300001
(title: "Added a new test for Inner/Outer viewport scrolling")

[commit-bot: I haz the power, 2015-04-07 02:58:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1038173002/300001

[commit-bot: I haz the power, 2015-04-07 03:50:02]

Committed patchset #13 (id:300001)

[commit-bot: I haz the power, 2015-04-07 03:50:51]

Patchset 13 (id:??) landed as
https://crrev.com/33370e19d3c6a43d0cb7c87f90aadb79d44dd741
Cr-Commit-Position: refs/heads/master@{#324012}