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 7c893323a1087c15405daf5a2c22bbbc73c9d405..10cb3f358c347e8602a3bd260dc31a3bf744a154 100644
--- a/Source/core/rendering/RenderLayer.cpp
+++ b/Source/core/rendering/RenderLayer.cpp
@@ -596,17 +596,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, AcceleratedCompositingForOverflowScrollDisabled);
 
     const RenderLayer* positionedAncestor = parent();
     while (positionedAncestor && !isPositionedContainer(positionedAncestor) && !positionedAncestor->isStackingContext())
@@ -637,17 +627,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, AcceleratedCompositingForOverflowScrollAlwaysOn);
 }
 
 // Compute what positive and negative z-order lists would look like before and
@@ -5618,16 +5598,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, 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)
@@ -5669,7 +5649,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, AcceleratedCompositingForOverflowScrollEnabledMode acceleratedCompositingForOverflowScrollEnabledMode)
 {
     if (isInTopLayer())
         return;
@@ -5677,20 +5657,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
+    // 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;
 
@@ -5708,7 +5708,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, acceleratedCompositingForOverflowScrollEnabledMode);
         }
     }
 }
@@ -5775,8 +5775,9 @@ void RenderLayer::repaintIncludingNonCompositingDescendants(RenderLayerModelObje
     }
 }
 
-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()
@@ -5791,7 +5792,7 @@ bool RenderLayer::shouldBeNormalFlowOnly() const
             && !renderer()->hasFilter()
             && !renderer()->hasBlendMode()
             && !isTransparent()
-            && !needsCompositedScrolling()
+            && shouldNeedCompositedScrolling
             && !renderer()->isFloatingWithShapeOutside()
             ;
 }