Fix RenderLayer::collectLayers logic bug.

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 691 matching lines @@
 {
     // FIXME: This is not in sync with containingBlock.
     // RenderObject::canContainFixedPositionedObject() should probably be used
     // instead.
     RenderLayerModelObject* layerRenderer = layer->renderer();
     return layer->isRootLayer() || layerRenderer->isPositioned() || layer->hasTransform();
 }
 
 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())
         positionedAncestor = positionedAncestor->parent();
     if (positionedAncestor && (!isPositionedContainer(positionedAncestor) || positionedAncestor->isStackingContext()))
         positionedAncestor = 0;
 
     if (!posZOrderListBeforePromote)
         posZOrderListBeforePromote = adoptPtr(new Vector<RenderLayer*>());
     else if (posZOrderListBeforePromote->find(this) != notFound)
@@ skipping 10 matching lines @@
     for (size_t index = 0; index < posZOrderListBeforePromote->size(); index++) {
         if (posZOrderListBeforePromote->at(index) == positionedAncestor) {
             posZOrderListBeforePromote->insert(index + 1, this);
             return;
         }
     }
 }
 
 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
 // after promotion, so we can later ensure that proper stacking order is
 // preserved between the two sets of lists.
 //
 // A few examples:
 // c = currentLayer
 // - = negative z-order child of currentLayer
 // + = positive z-order child of currentLayer
@@ skipping 4934 matching lines @@
         compositor()->setCompositingLayersNeedRebuild();
         if (acceleratedCompositingForOverflowScrollEnabled())
             compositor()->setShouldReevaluateCompositingAfterLayout();
     }
 }
 
 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)
         std::stable_sort(posZOrderList->begin(), posZOrderList->end(), compareZIndex);
 
     if (negZOrderList)
         std::stable_sort(negZOrderList->begin(), negZOrderList->end(), compareZIndex);
 
     // Append layers for top layer elements after normal layer collection, to ensure they are on top regardless of z-indexes.
     // The renderers of top layer elements are children of the view, sorted in top layer stacking order.
@@ skipping 21 matching lines @@
         if (child->isNormalFlowOnly() && (!m_reflection || reflectionLayer() != child)) {
             if (!m_normalFlowList)
                 m_normalFlowList = adoptPtr(new Vector<RenderLayer*>);
             m_normalFlowList->append(child);
         }
     }
     
     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;
 
     updateDescendantDependentFlags();
 
     bool isStacking = false;
+    bool isNormalFlow = false;
 
-    switch (behavior) {
-        case StopAtStackingContexts:
-            isStacking = (this == layerToForceAsStackingContainer) || isStackingContext();
+    // 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;
 
         // Create the buffer if it doesn't exist yet.
         if (!buffer)
             buffer = adoptPtr(new Vector<RenderLayer*>);
         
         // Append ourselves at the end of the appropriate buffer.
         buffer->append(this);
     }
 
     // Recur into our children to collect more layers, but only if we don't establish
     // a stacking context/container.
     if ((includeHiddenLayers || m_hasVisibleDescendant) && !isStacking) {
         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);
         }
     }
 }
 
 void RenderLayer::updateLayerListsIfNeeded()
 {
     updateZOrderLists();
     updateNormalFlowList();
 
     if (RenderLayer* reflectionLayer = this->reflectionLayer()) {
@@ skipping 46 matching lines @@
 void RenderLayer::repaintIncludingNonCompositingDescendants(RenderLayerModelObject* repaintContainer)
 {
     renderer()->repaintUsingContainer(repaintContainer, pixelSnappedIntRect(renderer()->clippedOverflowRectForRepaint(repaintContainer)));
 
     for (RenderLayer* curr = firstChild(); curr; curr = curr->nextSibling()) {
         if (!curr->isComposited())
             curr->repaintIncludingNonCompositingDescendants(repaintContainer);
     }
 }
 
-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()
                 || renderer()->isCanvas()
                 || renderer()->isVideo()
                 || renderer()->isEmbeddedObject()
                 || renderer()->isRenderIFrame()
                 || (renderer()->style()->specifiesColumns() && !isRootLayer()))
             && !renderer()->isPositioned()
             && !renderer()->hasTransform()
             && !renderer()->hasClipPath()
             && !renderer()->hasFilter()
 #if ENABLE(CSS_COMPOSITING)
             && !renderer()->hasBlendMode()
 #endif
             && !isTransparent()
-            && !needsCompositedScrolling()
+            && needsCompositedScrollingGivenForceMode
             && !renderer()->isFloatingWithShapeOutside()
             ;
 }
 
 void RenderLayer::updateIsNormalFlowOnly()
 {
     bool isNormalFlowOnly = shouldBeNormalFlowOnly();
     if (isNormalFlowOnly == m_isNormalFlowOnly)
         return;
 
@@ skipping 589 matching lines @@
     }
 }
 
 void showLayerTree(const WebCore::RenderObject* renderer)
 {
     if (!renderer)
         return;
     showLayerTree(renderer->enclosingLayer());
 }
 #endif