Only update composited-scrolling state once after layout.

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 275b6e3a55f520bcb99e072fcf557cafde10ee70..c661b27ca22bdfa98f5026348ce1d2ff58618422 100644
--- a/Source/core/rendering/RenderLayer.cpp
+++ b/Source/core/rendering/RenderLayer.cpp
@@ -144,6 +144,7 @@ RenderLayer::RenderLayer(RenderLayerModelObject* renderer)
     , m_hasSelfPaintingLayerDescendantDirty(false)
     , m_hasOutOfFlowPositionedDescendant(false)
     , m_hasOutOfFlowPositionedDescendantDirty(true)
+    , m_hasUnclippedDescendant(false)
     , m_forceNeedsCompositedScrolling(DoNotForceCompositedScrolling)
     , m_needsCompositedScrolling(false)
     , m_descendantsAreContiguousInStackingOrder(false)
@@ -226,6 +227,9 @@ RenderLayer::~RenderLayer()
             toElement(node)->setSavedLayerScrollOffset(m_scrollOffset);
     }
 
+    if (!m_renderer->documentBeingDestroyed())
+        compositor()->removeOutOfFlowPositionedLayer(this);

[Julien - ping for review, 2013/05/03 21:15:13]

It's weird that you try to remove |this| even if it's not in the out-of-flow
positioned map.

[Ian Vollick, 2013/05/04 03:33:00]

The layer can get destructed when we cease to be out-of-flow positioned. When
this happens, it doesn't seem to get notified in RenderLayer::styleChanged, and
when I check if it's out of flow positioned here, it reports the new value. So
removing the layer from the collection here, just in case, felt like the safest
thing to do.

+
     destroyScrollbar(HorizontalScrollbar);
     destroyScrollbar(VerticalScrollbar);
 
@@ -476,6 +480,35 @@ void RenderLayer::dirtyAncestorChainHasSelfPaintingLayerDescendantStatus()
     }
 }
 
+void RenderLayer::setAncestorChainHasOutOfFlowPositionedDescendant()
+{
+    for (RenderLayer* layer = this; layer; layer = layer->parent()) {
+        if (!layer->m_hasOutOfFlowPositionedDescendantDirty && layer->hasOutOfFlowPositionedDescendant())
+            break;
+
+        layer->m_hasOutOfFlowPositionedDescendantDirty = false;
+        layer->m_hasOutOfFlowPositionedDescendant = true;
+    }
+}
+
+void RenderLayer::dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus()
+{
+    for (RenderLayer* layer = this; layer; layer = layer->parent()) {
+        layer->m_hasOutOfFlowPositionedDescendantDirty = true;
+
+        // We may or may not have an unclipped descendant. If we do, we'll reset
+        // this to true the next time composited scrolling state is updated.
+        layer->m_hasUnclippedDescendant = false;
+
+        // If we have reached an out of flow positioned layer, we know our parent should have an out-of-flow positioned descendant.
+        // In this case, there is no need to dirty our ancestors further.
+        if (layer->renderer()->isOutOfFlowPositioned()) {
+            ASSERT(!parent() || parent()->m_hasOutOfFlowPositionedDescendantDirty || parent()->hasOutOfFlowPositionedDescendant());
+            break;
+        }
+    }
+}
+
 bool RenderLayer::acceleratedCompositingForOverflowScrollEnabled() const
 {
     return renderer()->frame()
@@ -565,9 +598,6 @@ void RenderLayer::updateDescendantsAreContiguousInStackingOrder()
     if (!m_descendantsAreContiguousInStackingOrderDirty || !isStackingContext() || !acceleratedCompositingForOverflowScrollEnabled())
         return;
 
-    ASSERT(!m_normalFlowListDirty);
-    ASSERT(!m_zOrderListsDirty);

[Julien - ping for review, 2013/05/03 21:15:13]

It's the second time I see this ASSERT removed. Could you explain why it's fine
to remove it here? (more ASSERT is always better in my book)

[Ian Vollick, 2013/05/04 03:33:00]

I don't think these ASSERTS were ever useful. This function makes no use of
either the normal flow or z-order lists; it rebuilds new lists on the fly.

-
     OwnPtr<Vector<RenderLayer*> > posZOrderList;
     OwnPtr<Vector<RenderLayer*> > negZOrderList;
     rebuildZOrderLists(StopAtStackingContexts, posZOrderList, negZOrderList);
@@ -629,13 +659,8 @@ void RenderLayer::updateDescendantsAreContiguousInStackingOrderRecursive(const H
         }
     }
 
-    if (!isStackingContext()) {
-        bool newValue = maxIndex - minIndex == count;
-        bool didUpdate = newValue != m_descendantsAreContiguousInStackingOrder;
-        m_descendantsAreContiguousInStackingOrder = newValue;
-        if (didUpdate)
-            updateNeedsCompositedScrolling();
-    }
+    if (!isStackingContext())
+        m_descendantsAreContiguousInStackingOrder = maxIndex - minIndex == count;

[Julien - ping for review, 2013/05/03 21:15:13]

As you touch this line, I would put some parentheses to just to make the order
resolution more obvious.

[Ian Vollick, 2013/05/04 03:33:00]

Done.

 }
 
 static inline bool isPositionedContainer(const RenderLayer* layer)
@@ -1063,6 +1088,7 @@ bool RenderLayer::canBeStackingContainer() const
     if (isStackingContext() || !ancestorStackingContainer())
         return true;
 
+    ASSERT(m_descendantsAreContiguousInStackingOrderDirty);

[Julien - ping for review, 2013/05/03 21:15:13]

Isn't the ASSERT backwards? (I don't see a good argument to ASSERT that you
query a dirty value)

[Ian Vollick, 2013/05/04 03:33:00]

Whoa! Yeah, it's backwards. The typo'd ASSERT was never tripped in the tests
because in the refactoring I removed the code that cleaned the dirty bit. Bleh.
Both have been fixed.

     return m_descendantsAreContiguousInStackingOrder;
 }
 
@@ -1119,56 +1145,61 @@ void RenderLayer::setAncestorChainHasVisibleDescendant()
     }
 }
 
-void RenderLayer::updateDescendantDependentFlags(HashSet<const RenderObject*>* outOfFlowDescendantContainingBlocks)
+// This method is called on all out of flow positioned layers. It simply walks
+// up to its containing block marking every layer it passes as having an
+// 'unclipped' descendant. The map is used to prevent wasted walking. If we
+// have already passed layer A heading towards containing block B, there's no
+// need to do it again; we can early out.
+void RenderLayer::updateHasUnclippedDescendant(RenderLayer::AncestorContainingBlockMap& map)
 {
-    if (m_visibleDescendantStatusDirty || m_hasSelfPaintingLayerDescendantDirty || m_hasOutOfFlowPositionedDescendantDirty) {
-        const bool hadVisibleDescendant = m_hasVisibleDescendant;
-        const bool hadOutOfFlowPositionedDescendant = m_hasOutOfFlowPositionedDescendant;
+    ASSERT(renderer()->isOutOfFlowPositioned());
+    if (!m_hasVisibleContent && !m_hasVisibleDescendant)
+        return;
+
+    const RenderObject* containingBlock = renderer()->containingBlock();
+    for (RenderLayer* ancestor = parent(); ancestor && ancestor->renderer() != containingBlock; ancestor = ancestor->parent()) {
+        if (!map.contains(ancestor)) {

[Julien - ping for review, 2013/05/03 21:15:13]

Ninja trick that avoids 3 hash lookups per call:

AncestorContainingBlockMap::iterator it = map.find(ancestor);
if (ancestor == notFound) {
    // [snipped] Add the new ancestor...
} else
    it->value.add(containingBlock);

[Ian Vollick, 2013/05/04 03:33:00]

Done.

[Ian Vollick, 2013/05/04 03:33:00]

Done.

+            OwnPtr<HashSet<const RenderObject*> > containingBlocks = adoptPtr(new HashSet<const RenderObject*>);
+            containingBlocks->add(containingBlock);
+            map.add(ancestor, containingBlocks.release());
+        } else if (!map.get(ancestor)->contains(containingBlock)) {
+            map.get(ancestor)->add(containingBlock);
+        } else {
+            break;
+        }
+
+        ancestor->m_hasUnclippedDescendant = true;
+    }
+}
 
+void RenderLayer::updateDescendantDependentFlags()
+{
+    if (m_visibleDescendantStatusDirty || m_hasSelfPaintingLayerDescendantDirty || m_hasOutOfFlowPositionedDescendantDirty) {
         m_hasVisibleDescendant = false;
         m_hasSelfPaintingLayerDescendant = false;
         m_hasOutOfFlowPositionedDescendant = false;
 
-        HashSet<const RenderObject*> childOutOfFlowDescendantContainingBlocks;
         for (RenderLayer* child = firstChild(); child; child = child->nextSibling()) {
-            childOutOfFlowDescendantContainingBlocks.clear();
-            child->updateDescendantDependentFlags(&childOutOfFlowDescendantContainingBlocks);
-
-            bool childIsOutOfFlowPositioned = child->renderer() && child->renderer()->isOutOfFlowPositioned();
-            if (childIsOutOfFlowPositioned)
-                childOutOfFlowDescendantContainingBlocks.add(child->renderer()->containingBlock());
-
-            if (outOfFlowDescendantContainingBlocks) {
-                HashSet<const RenderObject*>::const_iterator it = childOutOfFlowDescendantContainingBlocks.begin();
-                for (; it != childOutOfFlowDescendantContainingBlocks.end(); ++it)
-                    outOfFlowDescendantContainingBlocks->add(*it);
-            }
+            child->updateDescendantDependentFlags();
 
             bool hasVisibleDescendant = child->m_hasVisibleContent || child->m_hasVisibleDescendant;
             bool hasSelfPaintingLayerDescendant = child->isSelfPaintingLayer() || child->hasSelfPaintingLayerDescendant();
-            bool hasOutOfFlowPositionedDescendant = hasVisibleDescendant && (!childOutOfFlowDescendantContainingBlocks.isEmpty() || child->hasOutOfFlowPositionedDescendant());
+            bool hasOutOfFlowPositionedDescendant = child->renderer()->isOutOfFlowPositioned() || child->hasOutOfFlowPositionedDescendant();
 
             m_hasVisibleDescendant |= hasVisibleDescendant;
             m_hasSelfPaintingLayerDescendant |= hasSelfPaintingLayerDescendant;
             m_hasOutOfFlowPositionedDescendant |= hasOutOfFlowPositionedDescendant;
 
-            if (m_hasVisibleDescendant && m_hasSelfPaintingLayerDescendant && m_hasOutOfFlowPositionedDescendant)
+            if (m_hasVisibleDescendant && m_hasSelfPaintingLayerDescendant && hasOutOfFlowPositionedDescendant)
                 break;
         }
 
-        if (outOfFlowDescendantContainingBlocks && renderer())
-            outOfFlowDescendantContainingBlocks->remove(renderer());
-
         m_visibleDescendantStatusDirty = false;
         m_hasSelfPaintingLayerDescendantDirty = false;
         m_hasOutOfFlowPositionedDescendantDirty = false;
-
-        if (m_hasVisibleDescendant != hadVisibleDescendant || m_hasOutOfFlowPositionedDescendant != hadOutOfFlowPositionedDescendant)
-            updateNeedsCompositedScrolling();
     }
 
     if (m_visibleContentStatusDirty) {
-        const bool hadVisibleContent = m_hasVisibleContent;
         if (renderer()->style()->visibility() == VISIBLE)
             m_hasVisibleContent = true;
         else {
@@ -1194,10 +1225,8 @@ void RenderLayer::updateDescendantDependentFlags(HashSet<const RenderObject*>* o
                         r = r->nextSibling();
                 }
             }
-        }    
-        m_visibleContentStatusDirty = false; 
-        if (hadVisibleContent != m_hasVisibleContent)
-            updateNeedsCompositedScrolling();
+        }
+        m_visibleContentStatusDirty = false;
     }
 }
 
@@ -1768,16 +1797,6 @@ void RenderLayer::addChild(RenderLayer* child, RenderLayer* beforeChild)
         // case where we're building up generated content layers. This is ok, since the lists will start
         // off dirty in that case anyway.
         child->dirtyStackingContainerZOrderLists();
-
-        // Adding an out of flow positioned descendant can only affect
-        // the opt-in decision for layers beneath and including our
-        // containing block.
-        RenderObject* containingBlock = child->renderer()->containingBlock();
-        for (RenderLayer* layer = child; layer; layer = layer->parent()) {
-            layer->updateNeedsCompositedScrolling();
-            if (layer->renderer() == containingBlock)
-                break;
-        }
     }
 
     child->updateDescendantDependentFlags();
@@ -1787,8 +1806,14 @@ void RenderLayer::addChild(RenderLayer* child, RenderLayer* beforeChild)
     if (child->isSelfPaintingLayer() || child->hasSelfPaintingLayerDescendant())
         setAncestorChainHasSelfPaintingLayerDescendant();
 
-    if (child->renderer() && (child->renderer()->isOutOfFlowPositioned() || child->hasOutOfFlowPositionedDescendant()))
-        setAncestorChainHasOutOfFlowPositionedDescendant(child->renderer()->containingBlock());
+    if (child->renderer() && (child->renderer()->isOutOfFlowPositioned() || child->hasOutOfFlowPositionedDescendant())) {
+        // Now that the out of flow positioned descendant is in the tree, we
+        // need to tell the compositor to reevaluate the compositing
+        // requirements since we may be able to mark more layers as having
+        // an 'unclipped' descendant.
+        compositor()->setNeedsUpdateCompositingRequirementsState();
+        setAncestorChainHasOutOfFlowPositionedDescendant();
+    }
 
     compositor()->layerWasAdded(this, child);
 }
@@ -1816,28 +1841,21 @@ RenderLayer* RenderLayer::removeChild(RenderLayer* oldChild)
         // reattachment process in removeOnlyThisLayer, the layer may already be disconnected
         // from the main layer tree, so we need to null-check the |stackingContainer| value.
         oldChild->dirtyStackingContainerZOrderLists();
-
-        // This could affect whether or not a layer has an out of flow
-        // positioned descendant so we need to schedule some updates.
-        // Removing an out of flow positioned descendant can only affect
-        // the opt-in decision for layers beneath and including the old child's
-        // containing block.
-        RenderObject* containingBlock = oldChild->renderer()->containingBlock();
-        for (RenderLayer* layer = this; layer; layer = layer->parent()) {
-            layer->updateNeedsCompositedScrolling();
-            if (layer->renderer() == containingBlock)
-                break;
-        }
     }
 
-    if ((oldChild->renderer() && oldChild->renderer()->isOutOfFlowPositioned()) || oldChild->hasOutOfFlowPositionedDescendant())
-        dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus();
-
     oldChild->setPreviousSibling(0);
     oldChild->setNextSibling(0);
     oldChild->setParent(0);
 
     oldChild->updateDescendantDependentFlags();
+    if ((oldChild->renderer() && oldChild->renderer()->isOutOfFlowPositioned()) || oldChild->hasOutOfFlowPositionedDescendant()) {
+        // It may now be the case that a layer no longer has an unclipped
+        // descendant. Let the compositor know that it needs to reevaluate
+        // its compositing requirements to check this.
+        compositor()->setNeedsUpdateCompositingRequirementsState();
+        dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus();
+    }
+
     if (oldChild->m_hasVisibleContent || oldChild->m_hasVisibleDescendant)
         dirtyAncestorChainVisibleDescendantStatus();
 
@@ -2066,6 +2084,9 @@ bool RenderLayer::needsCompositedScrolling() const
 
 void RenderLayer::updateNeedsCompositedScrolling()
 {
+    if (RenderLayer* ancestor = ancestorStackingContext())
+        ancestor->updateDescendantsAreContiguousInStackingOrder();
+
     bool needsCompositedScrolling = false;
 
     FrameView* frameView = renderer()->view()->frameView();
@@ -2074,7 +2095,7 @@ void RenderLayer::updateNeedsCompositedScrolling()
 
         bool forceUseCompositedScrolling = acceleratedCompositingForOverflowScrollEnabled()
             && canBeStackingContainer()
-            && !hasOutOfFlowPositionedDescendant();
+            && !hasUnclippedDescendant();
 
 #if ENABLE(ACCELERATED_OVERFLOW_SCROLLING)
         needsCompositedScrolling = forceUseCompositedScrolling || renderer()->style()->useTouchOverflowScrolling();
@@ -2089,6 +2110,11 @@ void RenderLayer::updateNeedsCompositedScrolling()
             HistogramSupport::histogramEnumeration("Renderer.NeedsCompositedScrolling", needsCompositedScrolling, 2);
     }
 
+    setNeedsCompositedScrolling(needsCompositedScrolling);
+}
+
+void RenderLayer::setNeedsCompositedScrolling(bool needsCompositedScrolling)
+{
     if (m_needsCompositedScrolling == needsCompositedScrolling)
         return;
 
@@ -5616,6 +5642,7 @@ void RenderLayer::dirtyZOrderLists()
     m_descendantsAreContiguousInStackingOrderDirty = true;
 
     if (!renderer()->documentBeingDestroyed()) {
+        compositor()->setNeedsUpdateCompositingRequirementsState();
         compositor()->setCompositingLayersNeedRebuild();
         if (acceleratedCompositingForOverflowScrollEnabled())
             compositor()->setShouldReevaluateCompositingAfterLayout();
@@ -5754,7 +5781,6 @@ void RenderLayer::collectLayers(bool includeHiddenLayers, CollectLayersBehavior
 
 void RenderLayer::updateLayerListsIfNeeded()
 {
-    bool shouldUpdateDescendantsAreContiguousInStackingOrder = acceleratedCompositingForOverflowScrollEnabled() && isStackingContext() && (m_zOrderListsDirty || m_normalFlowListDirty) && m_descendantsAreContiguousInStackingOrderDirty;
     updateZOrderLists();
     updateNormalFlowList();
 
@@ -5762,14 +5788,6 @@ void RenderLayer::updateLayerListsIfNeeded()
         reflectionLayer->updateZOrderLists();
         reflectionLayer->updateNormalFlowList();
     }
-
-    if (shouldUpdateDescendantsAreContiguousInStackingOrder) {
-        updateDescendantsAreContiguousInStackingOrder();
-        // The above function can cause us to update m_needsCompositedScrolling
-        // and dirty our layer lists. Refresh them if necessary.
-        updateZOrderLists();
-        updateNormalFlowList();
-    }
 }
 
 void RenderLayer::repaintIncludingDescendants()
@@ -5942,14 +5960,8 @@ bool RenderLayer::isVisuallyNonEmpty() const
 
 void RenderLayer::updateVisibilityAfterStyleChange(const RenderStyle* oldStyle)
 {
-    EVisibility oldVisibility = oldStyle ? oldStyle->visibility() : VISIBLE;
-    if (oldVisibility == renderer()->style()->visibility() || !renderer()->isOutOfFlowPositioned())
-        return;
-
-    if (renderer()->style()->visibility() == VISIBLE)
-        setAncestorChainHasOutOfFlowPositionedDescendant(renderer()->containingBlock());
-    else
-        dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus();
+    if (!oldStyle || (oldStyle->visibility() != renderer()->style()->visibility()))
+        compositor()->setNeedsUpdateCompositingRequirementsState();
 }
 
 void RenderLayer::updateStackingContextsAfterStyleChange(const RenderStyle* oldStyle)
@@ -5971,7 +5983,7 @@ void RenderLayer::updateStackingContextsAfterStyleChange(const RenderStyle* oldS
     else
         clearZOrderLists();
 
-    updateNeedsCompositedScrolling();
+    compositor()->setNeedsUpdateCompositingRequirementsState();
 }
 
 static bool overflowRequiresScrollbar(EOverflow overflow)
@@ -6020,45 +6032,34 @@ void RenderLayer::updateScrollbarsAfterStyleChange(const RenderStyle* oldStyle)
         updateScrollableAreaSet(hasScrollableHorizontalOverflow() || hasScrollableVerticalOverflow());
 }
 
-void RenderLayer::setAncestorChainHasOutOfFlowPositionedDescendant(RenderObject* containingBlock)
+void RenderLayer::updateOutOfFlowPositioned(const RenderStyle* oldStyle)
 {
-    for (RenderLayer* layer = this; layer; layer = layer->parent()) {
-        if (!layer->m_hasOutOfFlowPositionedDescendantDirty && layer->hasOutOfFlowPositionedDescendant())
-            break;
+    if (oldStyle && (renderer()->style()->position() == oldStyle->position()))
+        return;
 
-        layer->m_hasOutOfFlowPositionedDescendantDirty = false;
-        layer->m_hasOutOfFlowPositionedDescendant = true;
-        layer->updateNeedsCompositedScrolling();
+    bool wasOutOfFlowPositioned = oldStyle && (oldStyle->position() == AbsolutePosition || oldStyle->position() == FixedPosition);
+    bool isOutOfFlowPositioned = renderer()->isOutOfFlowPositioned();
+    if (!wasOutOfFlowPositioned && !isOutOfFlowPositioned)
+        return;
 
-        if (layer->renderer() && layer->renderer() == containingBlock)
-            break;
-    }
-}
+    // Even if the layer remains out-of-flow, a change to this property
+    // will likely change its containing block. We must clear these bits
+    // so that they can be set properly by the RenderLayerCompositor.
+    for (RenderLayer* ancestor = parent(); ancestor; ancestor = ancestor->parent())
+        ancestor->m_hasUnclippedDescendant = false;
 
-void RenderLayer::dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus()
-{
-    if (m_hasOutOfFlowPositionedDescendant) {
-        m_hasOutOfFlowPositionedDescendantDirty = true;
-        // FIXME It would be nice to avoid this when we clean up render layer
-        // updating. We shouldn't have to update the composited scrolling state
-        // nearly as frequently if all the updates happen in a single, well
-        // defined phase.
-        updateNeedsCompositedScrolling();
-    }
+    // Ensures that we reset the above bits correctly.
+    compositor()->setNeedsUpdateCompositingRequirementsState();
 
-    if (parent())
-        parent()->dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus();
-}
+    if (wasOutOfFlowPositioned && isOutOfFlowPositioned)
+        return;
 
-void RenderLayer::updateOutOfFlowPositioned(const RenderStyle* oldStyle)
-{
-    bool wasOutOfFlowPositioned = oldStyle && (oldStyle->position() == AbsolutePosition || oldStyle->position() == FixedPosition);
-    bool isOutOfFlowPositioned = renderer()->isOutOfFlowPositioned();
-    if (parent() && isOutOfFlowPositioned != wasOutOfFlowPositioned) {
-        if (isOutOfFlowPositioned)
-            parent()->setAncestorChainHasOutOfFlowPositionedDescendant(renderer()->containingBlock());
-        else
-            parent()->dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus();
+    if (isOutOfFlowPositioned) {
+        setAncestorChainHasOutOfFlowPositionedDescendant();
+        compositor()->addOutOfFlowPositionedLayer(this);
+    } else {
+        dirtyAncestorChainHasSelfPaintingLayerDescendantStatus();
+        compositor()->removeOutOfFlowPositionedLayer(this);
     }
 }
 
@@ -6211,13 +6212,13 @@ void RenderLayer::updateScrollableAreaSet(bool hasOverflow)
         isVisibleToHitTest &= owner->renderer() && owner->renderer()->visibleToHitTesting();
 
     bool updatedScrollableAreaSet = false;
-    if (hasOverflow && isVisibleToHitTest)
+    if (hasOverflow && isVisibleToHitTest) {
         updatedScrollableAreaSet = frameView->addScrollableArea(this);
-    else
+        compositor()->setNeedsUpdateCompositingRequirementsState();
+    } else {
         updatedScrollableAreaSet = frameView->removeScrollableArea(this);
-
-    if (updatedScrollableAreaSet)
-        updateNeedsCompositedScrolling();
+        setNeedsCompositedScrolling(false);
+    }
 }
 
 void RenderLayer::updateScrollCornerStyle()