Don't coerce pointers to compositor layer mappings to booleans.

Source/core/page/scrolling/ScrollingCoordinator.cpp

Index: Source/core/page/scrolling/ScrollingCoordinator.cpp
diff --git a/Source/core/page/scrolling/ScrollingCoordinator.cpp b/Source/core/page/scrolling/ScrollingCoordinator.cpp
index 1905e88b6fa879564e33c7f06b9f5f89072514f2..dbb55ffd48d4084c68e20748e1742dd25f70153e 100644
--- a/Source/core/page/scrolling/ScrollingCoordinator.cpp
+++ b/Source/core/page/scrolling/ScrollingCoordinator.cpp
@@ -172,7 +172,7 @@ static void clearPositionConstraintExceptForLayer(GraphicsLayer* layer, Graphics
 
 static WebLayerPositionConstraint computePositionConstraint(const RenderLayer* layer)
 {
-    ASSERT(layer->compositedLayerMapping());
+    ASSERT(layer->hasCompositedLayerMapping());
     do {
         if (layer->renderer()->style()->position() == FixedPosition) {
             const RenderObject* fixedPositionObject = layer->renderer();
@@ -185,14 +185,42 @@ static WebLayerPositionConstraint computePositionConstraint(const RenderLayer* l
 
         // Composited layers that inherit a fixed position state will be positioned with respect to the nearest compositedLayerMapping's GraphicsLayer.
         // So, once we find a layer that has its own compositedLayerMapping, we can stop searching for a fixed position RenderObject.
-    } while (layer && layer->compositedLayerMapping());
+        //
+        // FIXME: I don't think that this is correct. First, the above comment is wrong: we stop searching
+        // precisely when we find a layer that does *not* have a compositedLayerMapping. Second, I think the
+        // logic is broken, even if we were to update this comment. Consider the following tree:
+        //
+        // + A - composited/fixed-pos
+        //   + B - composited/not-fixed-pos
+        //
+        // If we call computePositionConstraint for B, we skip the 'if' block above (because B is not
+        // fixed-pos), but we will continue marching up to A. Since A is fixed-pos we will return a
+        // position constraint. The result: the chain of composited layers below a composited, fixed-pos
+        // layer will all report that they have a position constraint. This seems wrong. Since composited
+        // layers inherit their positions, if their composited ancestor is fixed, they ought to inherit
+        // that fixed positioning and require no more intervention from the compositor for scroll
+        // compensation, etc.
+        //
+        // And there's another problem. Consider this tree.
+        //
+        // + A - composited
+        //   + B - not-composited/fixed-pos
+        //     + C - composited/not-fixed-pos
+        //
+        // In this case, we will not return a position constraint for C -- its parent is not composited
+        // and so we will not perform a second iteration of the for loop. This seems correct: C should
+        // get positioned correctly via CompositedLayerMapping::updateGraphicsLayerGeometry, and because
+        // we have a non-composited fixed-pos layer, we should not impl-scroll so we can rely on that
+        // main thread positioning logic to sort us out. The problem? This is inconsistent with the case
+        // above.
+    } while (layer && layer->hasCompositedLayerMapping());

[shawnsingh, 2013/11/12 09:55:50]

Actually the code existing before your patch is wrong because I made a mistake
in my compositingState patch. 
https://codereview.chromium.org/26156006/diff/8001/Source/core/page/scrolling...

I accidentally lost the "!" negation operator in the refactoring process.  I
think we need to fix that in a separate patch because there is almost certainly
something we will need to merge back to the m32 branch for this.  It turns out
that there is no difference in correctness (possibly wrong for pinch-zoom on the
impl thread, but not for basic positioning), because either way the layer will
get fixed to something else that is fixed.

So I believe your FIXME is unnecessary - please feel free to discuss if you
think otherwise =)

[Ian Vollick, 2013/11/14 04:12:47]

K, cool :)

     return WebLayerPositionConstraint();
 }
 
 void ScrollingCoordinator::updateLayerPositionConstraint(RenderLayer* layer)
 {
-    ASSERT(layer->compositedLayerMapping());
-    CompositedLayerMapping* compositedLayerMapping = layer->compositedLayerMapping();
+    ASSERT(layer->hasCompositedLayerMapping());
+    CompositedLayerMappingPtr compositedLayerMapping = layer->compositedLayerMapping();
     GraphicsLayer* mainLayer = compositedLayerMapping->childForSuperlayers();
 
     // Avoid unnecessary commits
@@ -510,7 +538,9 @@ void ScrollingCoordinator::setTouchEventTargetRects(const LayerHitTestRects& lay
         WebVector<WebRect> webRects(iter->value.size());
         for (size_t i = 0; i < iter->value.size(); ++i)
             webRects[i] = enclosingIntRect(iter->value[i]);
-        CompositedLayerMapping* compositedLayerMapping = layer->compositedLayerMapping();
+        // This should be ensured by convertLayerRectsToEnclosingCompositedLayer above.
+        ASSERT(layer->hasCompositedLayerMapping());
+        CompositedLayerMappingPtr compositedLayerMapping = layer->compositedLayerMapping();
         // If the layer is using composited scrolling, then it's the contents that these
         // rects apply to.
         GraphicsLayer* graphicsLayer = compositedLayerMapping->scrollingContentsLayer();
@@ -523,10 +553,14 @@ void ScrollingCoordinator::setTouchEventTargetRects(const LayerHitTestRects& lay
 
     // If there are any layers left that we haven't updated, clear them out.
     for (HashSet<const RenderLayer*>::iterator it = oldLayersWithTouchRects.begin(); it != oldLayersWithTouchRects.end(); ++it) {
-        if (CompositedLayerMapping* compositedLayerMapping = (*it)->compositedLayerMapping()) {
-            GraphicsLayer* graphicsLayer = compositedLayerMapping->scrollingContentsLayer();
+        // FIXME: This is a bug. What's happening here is that we're clearing touch regions for
+        // layers that we didn't visit above. That assumes a 1:1 mapping between RenderLayer and
+        // the graphics layer that owns the touch rects. This is false in the case of
+        // HasOwnBackingButPaintsIntoAncestor and will be extra-false in the world of squashing.
+        if ((*it)->hasCompositedLayerMapping()) {
+            GraphicsLayer* graphicsLayer = (*it)->compositedLayerMapping()->scrollingContentsLayer();
             if (!graphicsLayer)
-                graphicsLayer = compositedLayerMapping->mainGraphicsLayer();
+                graphicsLayer = (*it)->compositedLayerMapping()->mainGraphicsLayer();
             graphicsLayer->platformLayer()->setTouchEventHandlerRegion(WebVector<WebRect>());
         }
     }
@@ -553,7 +587,7 @@ void ScrollingCoordinator::touchEventTargetRectsDidChange(const Document*)
 void ScrollingCoordinator::updateScrollParentForGraphicsLayer(GraphicsLayer* child, RenderLayer* parent)
 {
     WebLayer* scrollParentWebLayer = 0;
-    if (parent && parent->compositedLayerMapping())
+    if (parent && parent->hasCompositedLayerMapping())
         scrollParentWebLayer = scrollingWebLayerForGraphicsLayer(parent->compositedLayerMapping()->parentForSublayers());
 
     child->setScrollParent(scrollParentWebLayer);
@@ -562,7 +596,7 @@ void ScrollingCoordinator::updateScrollParentForGraphicsLayer(GraphicsLayer* chi
 void ScrollingCoordinator::updateClipParentForGraphicsLayer(GraphicsLayer* child, RenderLayer* parent)
 {
     WebLayer* clipParentWebLayer = 0;
-    if (parent && parent->compositedLayerMapping())
+    if (parent && parent->hasCompositedLayerMapping())
         clipParentWebLayer = scrollingWebLayerForGraphicsLayer(parent->compositedLayerMapping()->parentForSublayers());
 
     child->setClipParent(clipParentWebLayer);