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 578 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, AcceleratedCompositingForOverflowScrollDisabled);
 
     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, AcceleratedCompositingForOverflowScrollAlwaysOn);
 }
 
 // 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 4950 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, AcceleratedCompositingForOverflowScrollEnabledMode acceleratedCompositingForOverflowScrollEnabledMode)
 {
     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, acceleratedCompositingForOverflowScrollEnabledMode);
 
     // 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, AcceleratedCompositingForOverflowScrollEnabledMode acceleratedCompositingForOverflowScrollEnabledMode)
 {
     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
+    // AcceleratedCompositingForOverflowScrollEnabledMode into account.
+    if (this == layerToForceAsStackingContainer) {

[shawnsingh, 2013/05/16 09:47:54]

OK, so I think I'm gradually overcoming the confusion here.  I think the problem
is that the logic here identifies the various scenarios in an indirect way, so
reading the code doesn't quite convey the concept of what's going on. 
Conceptually, I feel there are four scenarios we would expect collectLayers to
be invoked:

1. experiment 1 seeing the z order list with the layer forced as a stacking
container
  (isStacking=true, isNormalFlow=false)

2. experiment 2 seeing the z-order list with the layer forced as NOT a stacking
container.
  (isStacking=onlyWhenStackingContext,
isNormalFlow=overrideTheNeedsCompositingCondition)

3. while building the real z-order list, telling collectLayers that it can
promote overflow scrolls to be stacking containers even if they aren't
officially stacking contexts
  (isStacking=isContextOrOverflow, isNormalFlow=computeNormally)

4. while building the real z-order list, telling collectLayers that we can only
promote this layer if it is officially a stacking context.
  (isStacking=isContext, isNormalFlow=overrideTheNeedsCompositingCondition)

so... I think we should make these four cases the outer if-statements (or
switch) of this logic.  The current if-statement is effectively using
layerToForceAsStackingContainer as the indicator of which scenario we're in,
rather than the enum.

Instead, we should use the enum to indicate which scenario we're in, and
ASSERT(layerToForceAsStackingContainer) when appropriate.

And yes, I think that means we have to add an extra state to the enum.

As for naming, I couldn't find the original definition of this enum.  Which
patch is it?  I feel the word "compositing" is a red herring here. If the enum
really is being used in other places with these names, maybe we're trying to
force two concepts into one enum.   Here is the enum I envision for
collectLayers:

layerCollectionModeEnum {
  ForceLayerToStackingContainer
  PreventLayerToStackingContainer
  OverflowScrollCanBeStackingContainers
  OnlyStackingContextsCanBeStackingContainers
}



Other than this proposed rearrangement, I think I see how the patch addresses
the original problem and it looks reasonable. =)

[hartmanng, 2013/05/20 15:09:10]

https://codereview.chromium.org/14858004/ is where this enum is defined,
specifically in Settings.h. I think you're right - we're trying to force two
concepts into this enum, so I made a new one as you suggested.
I think we actually only need 3 scenarios here, we weren't really using one of
the codepaths (your "experiment 2"). Apart from that, done.

+        switch (acceleratedCompositingForOverflowScrollEnabledMode) {
+        case AcceleratedCompositingForOverflowScrollAlwaysOn:
+            isStacking = true;
+            isNormalFlow = false;
             break;
-
-        case StopAtStackingContainers:
-            isStacking = (this == layerToForceAsStackingContainer) || isStackingContainer();
+        case AcceleratedCompositingForOverflowScrollDisabled:
+            isStacking = isStackingContext();
+            isNormalFlow = shouldBeNormalFlowOnly(AcceleratedCompositingForOverflowScrollDisabled);
             break;
+        case AcceleratedCompositingForOverflowScrollEnabled:
+            // If we've supplied a layer to force, we should either be forcing on or off.
+            ASSERT_NOT_REACHED();
+            break;
+        }
+    } else if (acceleratedCompositingForOverflowScrollEnabledMode == AcceleratedCompositingForOverflowScrollEnabled) {
+        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(AcceleratedCompositingForOverflowScrollDisabled);
     }
 
     // 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, acceleratedCompositingForOverflowScrollEnabledMode);
         }
     }
 }
 
 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(AcceleratedCompositingForOverflowScrollEnabledMode acceleratedCompositingForOverflowScrollEnabledMode) const
 {
+    const bool shouldNeedCompositedScrolling = ((acceleratedCompositingForOverflowScrollEnabledMode == AcceleratedCompositingForOverflowScrollDisabled) || !needsCompositedScrolling());

[shawnsingh, 2013/05/16 09:47:54]

I think we're trying to hide actual conditional logic in an OR statement.
perhaps this should say:

bool doesNotNeedCompositedScrolling;
if ( overflowScrollCompositingMode  == Disabled )
  doesNotNeedCompositedScrolling = true;
else
  doesNotNeedCompositedScrolling = !needsCompositedScrolling();

Personally this seems much more clear and intuitive to me... =)


As a side note, I feel like the logic in this function should be grouped for
more intuitive readability, too:

bool couldBeNormalFlow = /* the first chunk of conditions */
bool hasExceptionThatCannotBeNormalFlow = isPositioned() || hasTransform ||
etc... || needsCompositedScrolling

return couldBeNormalFlow && !hasExceptionThatCannotBeNormalFlow;

[shawnsingh, 2013/05/16 09:53:23]

Hmm, maybe this could be even a bit more intuitive by using the opposite naming:

bool needsCompositedScrolling;
if (overflowScrollCompositingMode != Disabled)
  needsCompositedScrolling = needsCompositedScrolling();
else
  needsCompositedScrolling = false;


and then when all conditions are aggregated, it will be more consistent with the
pattern there.

[hartmanng, 2013/05/20 15:09:10]

Done.

[hartmanng, 2013/05/20 15:09:10]

Done.

     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()
             && !renderer()->hasBlendMode()
             && !isTransparent()
-            && !needsCompositedScrolling()
+            && shouldNeedCompositedScrolling
             && !renderer()->isFloatingWithShapeOutside()
             ;
 }
 
 void RenderLayer::updateIsNormalFlowOnly()
 {
     bool isNormalFlowOnly = shouldBeNormalFlowOnly();
     if (isNormalFlowOnly == m_isNormalFlowOnly)
         return;
 
@@ skipping 604 matching lines @@
     }
 }
 
 void showLayerTree(const WebCore::RenderObject* renderer)
 {
     if (!renderer)
         return;
     showLayerTree(renderer->enclosingLayer());
 }
 #endif