Fix RenderLayer::collectLayers logic bug.

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 6d147eb20354ec44726495482ff4a1bbbf678019..134e8e5322e8081404b97974121772483e94a66a 100644
--- a/Source/core/rendering/RenderLayer.cpp
+++ b/Source/core/rendering/RenderLayer.cpp
@@ -709,17 +709,7 @@ static inline bool isPositionedContainer(const RenderLayer* layer)
 
 void RenderLayer::collectBeforePromotionZOrderList(RenderLayer* ancestorStackingContext, OwnPtr<Vector<RenderLayer*> >& posZOrderListBeforePromote, OwnPtr<Vector<RenderLayer*> >& negZOrderListBeforePromote)
 {
-    // FIXME: TemporaryChange should support bit fields.
-    bool oldNeedsCompositedScrolling = m_needsCompositedScrolling;
-    bool oldIsNormalFlowOnly = m_isNormalFlowOnly;
-
-    m_needsCompositedScrolling = false;
-    m_isNormalFlowOnly = shouldBeNormalFlowOnly();
-
-    ancestorStackingContext->rebuildZOrderLists(StopAtStackingContexts, posZOrderListBeforePromote, negZOrderListBeforePromote, 0);
-
-    m_needsCompositedScrolling = oldNeedsCompositedScrolling;
-    m_isNormalFlowOnly = oldIsNormalFlowOnly;
+    ancestorStackingContext->rebuildZOrderLists(posZOrderListBeforePromote, negZOrderListBeforePromote, this, ForceCompositedScrollingOff);
 
     const RenderLayer* positionedAncestor = parent();
     while (positionedAncestor && !isPositionedContainer(positionedAncestor) && !positionedAncestor->isStackingContext())
@@ -750,17 +740,7 @@ void RenderLayer::collectBeforePromotionZOrderList(RenderLayer* ancestorStacking
 
 void RenderLayer::collectAfterPromotionZOrderList(RenderLayer* ancestorStackingContext, OwnPtr<Vector<RenderLayer*> >& posZOrderListAfterPromote, OwnPtr<Vector<RenderLayer*> >& negZOrderListAfterPromote)
 {
-    // FIXME: TemporaryChange should support bit fields.
-    bool oldNeedsCompositedScrolling = m_needsCompositedScrolling;
-    bool oldIsNormalFlowOnly = m_isNormalFlowOnly;
-
-    m_isNormalFlowOnly = false;
-    m_needsCompositedScrolling = true;
-
-    ancestorStackingContext->rebuildZOrderLists(StopAtStackingContexts, posZOrderListAfterPromote, negZOrderListAfterPromote, this);
-
-    m_needsCompositedScrolling = oldNeedsCompositedScrolling;
-    m_isNormalFlowOnly = oldIsNormalFlowOnly;
+    ancestorStackingContext->rebuildZOrderLists(posZOrderListAfterPromote, negZOrderListAfterPromote, this, ForceCompositedScrollingOn);
 }
 
 // Compute what positive and negative z-order lists would look like before and
@@ -5715,16 +5695,16 @@ void RenderLayer::rebuildZOrderLists()
 {
     ASSERT(m_layerListMutationAllowed);
     ASSERT(isDirtyStackingContainer());
-    rebuildZOrderLists(StopAtStackingContainers, m_posZOrderList, m_negZOrderList);
+    rebuildZOrderLists(m_posZOrderList, m_negZOrderList);
     m_zOrderListsDirty = false;
 }
 
-void RenderLayer::rebuildZOrderLists(CollectLayersBehavior behavior, OwnPtr<Vector<RenderLayer*> >& posZOrderList, OwnPtr<Vector<RenderLayer*> >& negZOrderList, const RenderLayer* layerToForceAsStackingContainer)
+void RenderLayer::rebuildZOrderLists(OwnPtr<Vector<RenderLayer*> >& posZOrderList, OwnPtr<Vector<RenderLayer*> >& negZOrderList, const RenderLayer* layerToForceAsStackingContainer, ForceNeedsCompositedScrollingMode forceMode)
 {
     bool includeHiddenLayers = compositor()->inCompositingMode();
     for (RenderLayer* child = firstChild(); child; child = child->nextSibling())
         if (!m_reflection || reflectionLayer() != child)
-            child->collectLayers(includeHiddenLayers, behavior, posZOrderList, negZOrderList, layerToForceAsStackingContainer);
+            child->collectLayers(includeHiddenLayers, posZOrderList, negZOrderList, layerToForceAsStackingContainer, forceMode);
 
     // Sort the two lists.
     if (posZOrderList)
@@ -5766,7 +5746,7 @@ void RenderLayer::updateNormalFlowList()
     m_normalFlowListDirty = false;
 }
 
-void RenderLayer::collectLayers(bool includeHiddenLayers, CollectLayersBehavior behavior, OwnPtr<Vector<RenderLayer*> >& posBuffer, OwnPtr<Vector<RenderLayer*> >& negBuffer, const RenderLayer* layerToForceAsStackingContainer)
+void RenderLayer::collectLayers(bool includeHiddenLayers, OwnPtr<Vector<RenderLayer*> >& posBuffer, OwnPtr<Vector<RenderLayer*> >& negBuffer, const RenderLayer* layerToForceAsStackingContainer, ForceNeedsCompositedScrollingMode forceMode)
 {
     if (isInTopLayer())
         return;
@@ -5774,20 +5754,40 @@ void RenderLayer::collectLayers(bool includeHiddenLayers, CollectLayersBehavior
     updateDescendantDependentFlags();
 
     bool isStacking = false;
-
-    switch (behavior) {
-        case StopAtStackingContexts:
-            isStacking = (this == layerToForceAsStackingContainer) || isStackingContext();
+    bool isNormalFlow = false;
+
+    // To decide whether to treat the current layers as a stacking context or
+    // normal flow only, we have to take the current state of
+    // ForceNeedsCompositedScrollingMode into account.
+    if (this == layerToForceAsStackingContainer) {
+        switch (forceMode) {
+        case ForceCompositedScrollingOn:
+            isStacking = true;
+            isNormalFlow = false;
             break;
-
-        case StopAtStackingContainers:
-            isStacking = (this == layerToForceAsStackingContainer) || isStackingContainer();
+        case ForceCompositedScrollingOff:
+            isStacking = isStackingContext();
+            isNormalFlow = shouldBeNormalFlowOnly(ForceCompositedScrollingOff);
             break;
+        case DoNotForceCompositedScrolling:

[shawnsingh, 2013/05/07 00:47:42]

It might be more robust to future code changes to also add a default: here, and
let the DoNotForceCompositedScrolling fall through to that default case.  I'm
not sure if there's some style reason not to do it, though?

[hartmanng, 2013/05/13 19:27:06]

Based on https://codereview.chromium.org/14458008/#msg5, I get the feeling that
default: clauses are frowned upon in this area of the code, but I could be
interpreting that incorrectly. jchaffraix, can you clarify?

+            // If we've supplied a layer to force, we should either be forcing on or off.
+            ASSERT_NOT_REACHED();
+            break;
+        }
+    } else if (forceMode == DoNotForceCompositedScrolling) {
+        isStacking = isStackingContainer();
+        isNormalFlow = isNormalFlowOnly();
+    } else {
+        // If we get to this case, we need to descend to stacking containers,
+        // and therefore composited-ness should not be taken into account at
+        // all.
+        isStacking = isStackingContext();
+        isNormalFlow = shouldBeNormalFlowOnly(ForceCompositedScrollingOff);
     }
 
     // Overflow layers are just painted by their enclosing layers, so they don't get put in zorder lists.
     bool includeHiddenLayer = includeHiddenLayers || (m_hasVisibleContent || (m_hasVisibleDescendant && isStacking));
-    if (includeHiddenLayer && !isNormalFlowOnly() && !isOutOfFlowRenderFlowThread()) {
+    if (includeHiddenLayer && !isNormalFlow && !isOutOfFlowRenderFlowThread()) {
         // Determine which buffer the child should be in.
         OwnPtr<Vector<RenderLayer*> >& buffer = (zIndex() >= 0) ? posBuffer : negBuffer;
 
@@ -5805,7 +5805,7 @@ void RenderLayer::collectLayers(bool includeHiddenLayers, CollectLayersBehavior
         for (RenderLayer* child = firstChild(); child; child = child->nextSibling()) {
             // Ignore reflections.
             if (!m_reflection || reflectionLayer() != child)
-                child->collectLayers(includeHiddenLayers, behavior, posBuffer, negBuffer, layerToForceAsStackingContainer);
+                child->collectLayers(includeHiddenLayers, posBuffer, negBuffer, layerToForceAsStackingContainer, forceMode);
         }
     }
 }
@@ -5872,8 +5872,9 @@ void RenderLayer::repaintIncludingNonCompositingDescendants(RenderLayerModelObje
     }
 }
 
-bool RenderLayer::shouldBeNormalFlowOnly() const
+bool RenderLayer::shouldBeNormalFlowOnly(ForceNeedsCompositedScrollingMode forceMode) const
 {
+    const bool needsCompositedScrollingGivenForceMode = ((forceMode == ForceCompositedScrollingOff) || !needsCompositedScrolling());

[shawnsingh, 2013/05/07 00:47:42]

I think this variable name is a little unclear -- there's a phrase inside of the
phrase which seems like it could be interpreted as "bool
needsCompositedScrolling" ... at which point total confusion ensues.

[hartmanng, 2013/05/13 19:27:06]

Good point, it is a bit confusing. I was trying to make it specific to
distinguish it from the needsCompositedScrolling() value - the idea is that we
only want the "real" needsCompositedScrolling if we're not forcing composited
scrolling off.

Do you think this would be better as just "bool needsCompositedScrolling"? I may
be adding unnecessary complexity here.

For now I've called it "shouldNeedCompositedScrolling", but I'm not sure that's
quite right either.

     return (renderer()->hasOverflowClip()
                 || renderer()->hasReflection()
                 || renderer()->hasMask()
@@ -5890,7 +5891,7 @@ bool RenderLayer::shouldBeNormalFlowOnly() const
             && !renderer()->hasBlendMode()
 #endif
             && !isTransparent()
-            && !needsCompositedScrolling()
+            && needsCompositedScrollingGivenForceMode
             && !renderer()->isFloatingWithShapeOutside()
             ;
 }