Fix RenderLayer::scrollsWithRespectTo().

Fix RenderLayer::scrollsWithRespectTo().

scrollsWithRespectTo() currently tries to match up
containing blocks by keeping track of RenderObjects.
However, when it goes to check if a containing block
is a scrolling div, it actually checks the enclosing
RenderLayer. It doesn't make sense to do this since
RenderLayers and RenderObjects don't line up
one-to-one.

Since this function only cares about scrollability,
and the RenderLayer is the object that handles an
elements overflow-scrollability, it makes more
sense to keep track of containing blocks by storing
the enclosingLayer() instead of the RenderObject
itself.

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

[hartmanng, 2014-06-18 17:54:01]

Ian, Chris, please take a look.

https://codereview.chromium.org/343623003/diff/1/LayoutTests/compositing/over...
File LayoutTests/compositing/overflow/scrolls-with-respect-to-expected.txt
(left):

https://codereview.chromium.org/343623003/diff/1/LayoutTests/compositing/over...
LayoutTests/compositing/overflow/scrolls-with-respect-to-expected.txt:39:
scrolled2 scrolls with respect to descendant
I think these were actually incorrect. Based on visual inspection of the test
page, scrolled2 appears to move with descendant. Same with scrolled3 below.

This conveniently serves as a test of the bug this CL fixes.

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:381: if (containingBlock->enclosingLayer()
== other) {
I don't think these special cases are required anymore (the extra test cases
added in https://codereview.chromium.org/268813005 still pass without them).
They were likely specific symptoms of the underlying problem this CL fixes.

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:365: // FIXME: some of these cases don't
look quite right.
Chris, I believe you added this FIXME. Can you explain a bit more what doesn't
seem right about them? I think I've convinced myself that they're ok. As
explained in the large comment above, what we want to test here is

1) are both elements root-fixed-pos? If so, they both anchor to the viewport,
and therefore don't scroll relative to each other.
2) is exactly one element root-fixed-pos? If one is root-fixed-pos and the other
isn't, then _only_ the root-fixed-pos one will anchor to the viewport, and the
other will scroll normally (ie, it will scroll relative to the viewport).

I agree that the second if statement looks a bit weird on first glance, since
the inclusive or seems to imply that if _both_ elements are root-fixed-pos, then
they scroll with respect to each other, but it's impossible to get into that
situation without already having returned false on the previous line.

Does that make sense, or am I missing the part you didn't like?

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:371: if (containingBlock->enclosingLayer()
== otherContainingBlock->enclosingLayer())
We need to compare enclosingLayers, not just RenderObjects. We can get into the
scenario that containingBlock != otherContainingBlock, but
containingBlock->enclosingLayer() == otherContainingBlock->enclosingLayer() (and
similar below). This should return false, since the enclosingLayer is the part
that actually handles overflow scrolling.

[Ian Vollick, 2014-06-18 18:28:31]

This is really exciting. I just have one question about killing the early-out.

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:381: if (containingBlock->enclosingLayer()
== other) {
On 2014/06/18 17:54:01, hartmanng wrote:
> I don't think these special cases are required anymore (the extra test cases
> added in https://codereview.chromium.org/268813005 still pass without them).
> They were likely specific symptoms of the underlying problem this CL fixes.

I think we were also looking to early out for efficiency's sake. Is it still
valid to return false if we visit the enclosing layer of other or its containing
block on our way up here?

[hartmanng, 2014-06-18 18:46:12]

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:381: if (containingBlock->enclosingLayer()
== other) {
On 2014/06/18 18:28:30, Ian Vollick wrote:
> On 2014/06/18 17:54:01, hartmanng wrote:
> > I don't think these special cases are required anymore (the extra test cases
> > added in https://codereview.chromium.org/268813005 still pass without them).
> > They were likely specific symptoms of the underlying problem this CL fixes.
> 
> I think we were also looking to early out for efficiency's sake. Is it still
> valid to return false if we visit the enclosing layer of other or its
containing
> block on our way up here?

It's still valid, I just thought it was unnecessary. Added back in.

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:396: // The other layer scrolls with
respect to this one if this one scrolls and it's a child.
I don't think this early-out will gain us much in terms of efficiency. At most,
it could save 1 loop iteration. Ie, if otherContainingBlock->enclosingLayer() ==
this, then otherContainingBlock->containingBlock() == this->containingBlock(),
which is exactly what the next iteration would check for.

[Ian Vollick, 2014-06-18 20:40:02]

lgtm.

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/343623003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:396: // The other layer scrolls with
respect to this one if this one scrolls and it's a child.
On 2014/06/18 18:46:12, hartmanng wrote:
> I don't think this early-out will gain us much in terms of efficiency. At
most,
> it could save 1 loop iteration. Ie, if otherContainingBlock->enclosingLayer()
==
> this, then otherContainingBlock->containingBlock() == this->containingBlock(),
> which is exactly what the next iteration would check for.

I buy it.

[hartmanng, 2014-06-18 20:45:50]

thanks!

[hartmanng, 2014-06-18 20:46:08]

The CQ bit was checked by hartmanng@chromium.org

[commit-bot: I haz the power, 2014-06-18 20:47:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hartmanng@chromium.org/343623003/20001

[commit-bot: I haz the power, 2014-06-18 20:51:28]

Change committed as 176452