Fix RenderLayer::scrollsWithRespectTo().

Source/core/rendering/RenderLayer.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights reserved.
  *
  * Portions are Copyright (C) 1998 Netscape Communications Corporation.
  *
  * Other contributors:
  *   Robert O'Callahan <roc+@cs.cmu.edu>
  *   David Baron <dbaron@fas.harvard.edu>
  *   Christian Biesinger <cbiesinger@web.de>
  *   Randall Jesup <rjesup@wgate.com>
@@ skipping 344 matching lines @@
     // (including having a real, non-viewport containing block).
     //
     // Below, a "root" fixed position element is defined to be one whose
     // containing block is the root. These root-fixed-position elements are
     // the only ones that need this special case code - other fixed position
     // elements, as well as all absolute, relative, and static elements use the
     // logic below.
     const bool isRootFixedPos = position == FixedPosition && containingBlock->enclosingLayer() == rootLayer;
     const bool otherIsRootFixedPos = otherPosition == FixedPosition && otherContainingBlock->enclosingLayer() == rootLayer;
 
     // FIXME: some of these cases don't look quite right.

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

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?

     if (isRootFixedPos && otherIsRootFixedPos)
         return false;
     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();
     }
 
     return true;
 }
 
 void RenderLayer::updateLayerPositionsAfterDocumentScroll()
 {
@@ skipping 3451 matching lines @@
     }
 }
 
 void showLayerTree(const WebCore::RenderObject* renderer)
 {
     if (!renderer)
         return;
     showLayerTree(renderer->enclosingLayer());
 }
 #endif