Update scroll parent correctly for squashing layers.

Update scroll parent correctly for squashing layers.

Currently, we only set a scroll parent for 
"localRootForOwningLayer".I.e., the highest
GraphicsLayer in the CLM-managed hierarchy, excluding
the squashing containment layer. But for "scroll
buddies", we need the scroll child to be the very top
of the hierarchy.

We also need to ensure that scroll buddies are not
prevented from squashing together because of the
additional "I've got a scroll parent" compositing
reason.

This cl also disables chicken-egg asserts that trip
w/ universal accelerated overflow scroll and changes
how we compute composited bounds for squashed layers
(we currently pull the bounds from the clipper, but
it picks the wrong "clipping root" in this case).

With this CL, universal accelerated overflow scrolling
and squashing work nearly perfectly modulo known hit
testing bugs (which can prevent touch scrolling) and
repaint issues (separate issue reported here
https://code.google.com/p/chromium/issues/detail?id=351790).

BUG=328156
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169776

[shawnsingh, 2014-02-10 20:28:52]

Just wondering, does this patch have any real effect on squashing by itself? 
Maybe we should create a test or two for cases where things can be reasonably
squashed because of scroll parent, not because of overlap.

Also we should wait until Adam's layout tests land so that we can sanity check
that those tests still test what we expect - with scroll squashing, I'm not sure
if some of the scrolling tests that exist will loose the points that they're
trying to cover.

https://codereview.chromium.org/131543013/diff/1/Source/platform/graphics/Com...
File Source/platform/graphics/CompositingReasons.h (right):

https://codereview.chromium.org/131543013/diff/1/Source/platform/graphics/Com...
Source/platform/graphics/CompositingReasons.h:64: const uint64_t
CompositingReasonOverlapsWithoutSquashingTarget         = UINT64_C(1) << 36;
I think we should rename this one too.  -->
CompositingReasonSquashableWithoutSquashingTarget

[shawnsingh, 2014-02-10 20:29:39]

On 2014/02/10 20:28:52, shawnsingh wrote:
> Also we should wait until Adam's layout tests land so that we can sanity check
> that those tests still test what we expect - with scroll squashing, I'm not
sure
> if some of the scrolling tests that exist will loose the points that they're
> trying to cover.

to clarify - the patch is here - https://codereview.chromium.org/137143009/ and
should land very soon

[Ian Vollick, 2014-03-12 18:12:16]

I think this is ready for review. PTAL!

[ojan, 2014-03-12 18:30:24]

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:196: // updateLayerCompositingState will
query compositingReasons for accelerated overflow scrolling.
Is there a test that hits this? It's useful to list the test in the comment so
that the person who tries to fix this can easily tell if this still reproduces.

[abarth-chromium, 2014-03-12 18:45:40]

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:197: DisableCompositingQueryAsserts
disabler;
It's too bad that we're regressing this chicken-egg issue.  We recently removed
the assert disabler from this function.

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:873:
updateScrollParent(m_owningLayer.scrollParent());
This function walks up the RenderLayer tree.  Instead, we should remember this
information as we walk down the RenderLayer tree.

[Ian Vollick, 2014-03-12 20:03:31]

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:196: // updateLayerCompositingState will
query compositingReasons for accelerated overflow scrolling.
On 2014/03/12 18:30:25, ojan wrote:
> Is there a test that hits this? It's useful to list the test in the comment so
> that the person who tries to fix this can easily tell if this still
reproduces.

Added a test for this.

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:197: DisableCompositingQueryAsserts
disabler;
On 2014/03/12 18:45:40, abarth wrote:
> It's too bad that we're regressing this chicken-egg issue.  We recently
removed
> the assert disabler from this function.

Not a regression, just a gap in coverage. We were always asserting here, we just
didn't know it.

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:873:
updateScrollParent(m_owningLayer.scrollParent());
On 2014/03/12 18:45:40, abarth wrote:
> This function walks up the RenderLayer tree.  Instead, we should remember this
> information as we walk down the RenderLayer tree.

Agreed, though it's a little tricky since a layer's scroll parent depends on its
containing block. We'll need to pass down a collection of scroll parents for the
variously positioned descendants (like we do for RenderGeometryMap). This is a
biggish refactor and I'm hoping I can address it in a separate patch. I've
created a bug (http://crbug.com/351851) and added FIXMES if you're ok with that.

[abarth-chromium, 2014-03-12 21:20:13]

On 2014/03/12 20:03:31, Ian Vollick wrote:
>
https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
> Source/core/rendering/RenderLayer.cpp:196: // updateLayerCompositingState will
> query compositingReasons for accelerated overflow scrolling.
> On 2014/03/12 18:30:25, ojan wrote:
> > Is there a test that hits this? It's useful to list the test in the comment
so
> > that the person who tries to fix this can easily tell if this still
> reproduces.
> 
> Added a test for this.

Thanks.  It looks like can-changed-chicken-egg.html is missing an expected
results file.  Maybe you forgot to "git add" it?

> Not a regression, just a gap in coverage. We were always asserting here, we
just
> didn't know it.

Ah, makes sense.

> Agreed, though it's a little tricky since a layer's scroll parent depends on
its
> containing block. We'll need to pass down a collection of scroll parents for
the
> variously positioned descendants (like we do for RenderGeometryMap). This is a
> biggish refactor and I'm hoping I can address it in a separate patch. I've
> created a bug (http://crbug.com/351851) and added FIXMES if you're ok with
that.

IMHO, we should do the refactoring first.  This CL adds more tree walking to the
shipping code path in service of a code path that isn't shipping.  We need to
straighten out this code to not walk the tree everywhere.  We might as well do
it now before going further into technical debt.

[Ian Vollick, 2014-03-13 03:14:06]

On 2014/03/12 21:20:13, abarth wrote:
> On 2014/03/12 20:03:31, Ian Vollick wrote:
> >
>
https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
> > File Source/core/rendering/RenderLayer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
> > Source/core/rendering/RenderLayer.cpp:196: // updateLayerCompositingState
will
> > query compositingReasons for accelerated overflow scrolling.
> > On 2014/03/12 18:30:25, ojan wrote:
> > > Is there a test that hits this? It's useful to list the test in the
comment
> so
> > > that the person who tries to fix this can easily tell if this still
> > reproduces.
> > 
> > Added a test for this.
> 
> Thanks.  It looks like can-changed-chicken-egg.html is missing an expected
> results file.  Maybe you forgot to "git add" it?

You're right, I did forget to add it. I see that I've also forgotten to update
the comment as Ojan suggested. Will fix both tomorrow.

> 
> > Not a regression, just a gap in coverage. We were always asserting here, we
> just
> > didn't know it.
> 
> Ah, makes sense.
> 
> > Agreed, though it's a little tricky since a layer's scroll parent depends on
> its
> > containing block. We'll need to pass down a collection of scroll parents for
> the
> > variously positioned descendants (like we do for RenderGeometryMap). This is
a
> > biggish refactor and I'm hoping I can address it in a separate patch. I've
> > created a bug (http://crbug.com/351851) and added FIXMES if you're ok with
> that.
> 
> IMHO, we should do the refactoring first.  This CL adds more tree walking to
the
> shipping code path in service of a code path that isn't shipping.  We need to
> straighten out this code to not walk the tree everywhere.  We might as well do
> it now before going further into technical debt.

K, I'll look at the refactoring tomorrow, but I wanted to mention that we won't
actually do the walks unless this code path is enabled; we early out in
RenderLayer::scrollParent if we're not on the
universal-accelerated-overflow-scroll path. Not that this does us any good if we
_do_ have the setting enabled, but at least this change wouldn't slow down
shipping code. :)

[Ian Vollick, 2014-03-14 19:28:23]

On 2014/03/13 03:14:06, Ian Vollick wrote:
> On 2014/03/12 21:20:13, abarth wrote:
> > On 2014/03/12 20:03:31, Ian Vollick wrote:
> > >
> >
>
https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
> > > File Source/core/rendering/RenderLayer.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
> > > Source/core/rendering/RenderLayer.cpp:196: // updateLayerCompositingState
> will
> > > query compositingReasons for accelerated overflow scrolling.
> > > On 2014/03/12 18:30:25, ojan wrote:
> > > > Is there a test that hits this? It's useful to list the test in the
> comment
> > so
> > > > that the person who tries to fix this can easily tell if this still
> > > reproduces.
> > > 
> > > Added a test for this.
> > 
> > Thanks.  It looks like can-changed-chicken-egg.html is missing an expected
> > results file.  Maybe you forgot to "git add" it?
> 
> You're right, I did forget to add it. I see that I've also forgotten to update
> the comment as Ojan suggested. Will fix both tomorrow.
> 
> > 
> > > Not a regression, just a gap in coverage. We were always asserting here,
we
> > just
> > > didn't know it.
> > 
> > Ah, makes sense.
> > 
> > > Agreed, though it's a little tricky since a layer's scroll parent depends
on
> > its
> > > containing block. We'll need to pass down a collection of scroll parents
for
> > the
> > > variously positioned descendants (like we do for RenderGeometryMap). This
is
> a
> > > biggish refactor and I'm hoping I can address it in a separate patch. I've
> > > created a bug (http://crbug.com/351851) and added FIXMES if you're ok with
> > that.
> > 
> > IMHO, we should do the refactoring first.  This CL adds more tree walking to
> the
> > shipping code path in service of a code path that isn't shipping.  We need
to
> > straighten out this code to not walk the tree everywhere.  We might as well
do
> > it now before going further into technical debt.
> 
> K, I'll look at the refactoring tomorrow, but I wanted to mention that we
won't
> actually do the walks unless this code path is enabled; we early out in
> RenderLayer::scrollParent if we're not on the
> universal-accelerated-overflow-scroll path. Not that this does us any good if
we
> _do_ have the setting enabled, but at least this change wouldn't slow down
> shipping code. :)

+eseidel and leviw whom I've been chatting with about approaches to deal with
the unnecessary walking.

[Ian Vollick, 2014-03-21 14:28:21]

On 2014/03/14 19:28:23, Ian Vollick wrote:
> On 2014/03/13 03:14:06, Ian Vollick wrote:
> > On 2014/03/12 21:20:13, abarth wrote:
> > > On 2014/03/12 20:03:31, Ian Vollick wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
> > > > File Source/core/rendering/RenderLayer.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/131543013/diff/160001/Source/core/rendering/R...
> > > > Source/core/rendering/RenderLayer.cpp:196: //
updateLayerCompositingState
> > will
> > > > query compositingReasons for accelerated overflow scrolling.
> > > > On 2014/03/12 18:30:25, ojan wrote:
> > > > > Is there a test that hits this? It's useful to list the test in the
> > comment
> > > so
> > > > > that the person who tries to fix this can easily tell if this still
> > > > reproduces.
> > > > 
> > > > Added a test for this.
> > > 
> > > Thanks.  It looks like can-changed-chicken-egg.html is missing an expected
> > > results file.  Maybe you forgot to "git add" it?
> > 
> > You're right, I did forget to add it. I see that I've also forgotten to
update
> > the comment as Ojan suggested. Will fix both tomorrow.
> > 
> > > 
> > > > Not a regression, just a gap in coverage. We were always asserting here,
> we
> > > just
> > > > didn't know it.
> > > 
> > > Ah, makes sense.
> > > 
> > > > Agreed, though it's a little tricky since a layer's scroll parent
depends
> on
> > > its
> > > > containing block. We'll need to pass down a collection of scroll parents
> for
> > > the
> > > > variously positioned descendants (like we do for RenderGeometryMap).
This
> is
> > a
> > > > biggish refactor and I'm hoping I can address it in a separate patch.
I've
> > > > created a bug (http://crbug.com/351851) and added FIXMES if you're ok
with
> > > that.
> > > 
> > > IMHO, we should do the refactoring first.  This CL adds more tree walking
to
> > the
> > > shipping code path in service of a code path that isn't shipping.  We need
> to
> > > straighten out this code to not walk the tree everywhere.  We might as
well
> do
> > > it now before going further into technical debt.
> > 
> > K, I'll look at the refactoring tomorrow, but I wanted to mention that we
> won't
> > actually do the walks unless this code path is enabled; we early out in
> > RenderLayer::scrollParent if we're not on the
> > universal-accelerated-overflow-scroll path. Not that this does us any good
if
> we
> > _do_ have the setting enabled, but at least this change wouldn't slow down
> > shipping code. :)
> 
> +eseidel and leviw whom I've been chatting with about approaches to deal with
> the unnecessary walking.

Now that the refactoring has landed, I think this change is unblocked. Adam, can
you PTAL?

[abarth-chromium, 2014-03-21 15:49:50]

lgtm

https://codereview.chromium.org/131543013/diff/230001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/131543013/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:469: // See
http://crbug.com/351851
Is this comments still accurate?

https://codereview.chromium.org/131543013/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:894: // See
http://crbug.com/351851
ditto

[Ian Vollick, 2014-03-21 18:23:24]

https://codereview.chromium.org/131543013/diff/230001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/131543013/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:469: // See
http://crbug.com/351851
On 2014/03/21 15:49:51, abarth wrote:
> Is this comments still accurate?

Nope. Removed.

https://codereview.chromium.org/131543013/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:894: // See
http://crbug.com/351851
On 2014/03/21 15:49:51, abarth wrote:
> ditto

ditto

[Ian Vollick, 2014-03-21 18:23:33]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-03-21 18:23:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/131543013/240001

[commit-bot: I haz the power, 2014-03-21 18:25:26]

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

[commit-bot: I haz the power, 2014-03-21 18:25:27]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[pdr., 2014-03-21 18:26:07]

On 2014/03/21 18:25:27, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.blink on win_blink_rel

flake flake flake flake flake flake flake flake flake

[pdr., 2014-03-21 18:26:11]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-21 18:26:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/131543013/240001

[commit-bot: I haz the power, 2014-03-21 19:24:45]

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

[commit-bot: I haz the power, 2014-03-21 19:24:45]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Ian Vollick, 2014-03-21 20:50:19]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-03-21 20:50:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/131543013/240001

[commit-bot: I haz the power, 2014-03-22 04:11:22]

Change committed as 169776