Fix RenderLayer::scrollsWithRespectTo().

Source/core/rendering/RenderLayer.cpp

Index: Source/core/rendering/RenderLayer.cpp
diff --git a/Source/core/rendering/RenderLayer.cpp b/Source/core/rendering/RenderLayer.cpp
index be92d92417e0a84c5a96345eb64f1c16ec420a37..a13176a281125c032501a1bb6053cbb485a8cf6e 100644
--- a/Source/core/rendering/RenderLayer.cpp
+++ b/Source/core/rendering/RenderLayer.cpp
@@ -368,35 +368,25 @@ bool RenderLayer::scrollsWithRespectTo(const RenderLayer* other) const
     if (isRootFixedPos || otherIsRootFixedPos)
         return true;
 
-    if (containingBlock == otherContainingBlock)
+    if (containingBlock->enclosingLayer() == otherContainingBlock->enclosingLayer())

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

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.

         return false;
 
     // Maintain a set of containing blocks between the first layer and its
     // closest scrollable ancestor.
-    HashSet<const RenderObject*> containingBlocks;
+    HashSet<const RenderLayer*> containingBlocks;
     while (containingBlock) {
-        if (containingBlock->enclosingLayer()->scrollsOverflow()) {
+        containingBlocks.add(containingBlock->enclosingLayer());
+        if (containingBlock->enclosingLayer()->scrollsOverflow())
             break;
-        }
-        if (containingBlock->enclosingLayer() == other) {

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

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.

[Ian Vollick, 2014/06/18 18:28:30]

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]

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

-            // This layer does not scroll with respect to the other layer if the other one does not scroll and this one is a child.
-            return false;
-        }
-        containingBlocks.add(containingBlock);
         containingBlock = containingBlock->containingBlock();
     }
 
     // Do the same for the 2nd layer, but if we find a common containing block,
     // it means both layers are contained within a single non-scrolling subtree.
     // Hence, they will not scroll with respect to each other.
-    bool thisLayerScrollsOverflow = scrollsOverflow();
     while (otherContainingBlock) {
-        if (containingBlocks.contains(otherContainingBlock))
+        if (containingBlocks.contains(otherContainingBlock->enclosingLayer()))
             return false;
-        // The other layer scrolls with respect to this one if this one scrolls and it's a child.

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

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]

I buy it.

-        if (!thisLayerScrollsOverflow && otherContainingBlock->enclosingLayer() == this)
-            return false;
-        // The other layer does not scroll with respect to this one if this one does not scroll and it's a child.
         if (otherContainingBlock->enclosingLayer()->scrollsOverflow())
             break;
         otherContainingBlock = otherContainingBlock->containingBlock();