Only update composited scrolling state when necessary

Source/WebCore/rendering/RenderLayer.cpp

Index: Source/WebCore/rendering/RenderLayer.cpp
diff --git a/Source/WebCore/rendering/RenderLayer.cpp b/Source/WebCore/rendering/RenderLayer.cpp
index c4787b98c1b85f14ef32d7096cf2f4c121f39b59..a275518736d6614dcd241889154a516860d6fa10 100644
--- a/Source/WebCore/rendering/RenderLayer.cpp
+++ b/Source/WebCore/rendering/RenderLayer.cpp
@@ -149,6 +149,7 @@ RenderLayer::RenderLayer(RenderLayerModelObject* renderer)
     , m_hasOutOfFlowPositionedDescendantDirty(true)
     , m_needsCompositedScrolling(false)
     , m_descendantsAreContiguousInStackingOrder(false)
+    , m_descendantsAreContiguousInStackingOrderDirty(true)
     , m_isRootLayer(renderer->isRenderView())
     , m_usedTransparency(false)
     , m_paintingInsideReflection(false)
@@ -595,12 +596,9 @@ bool RenderLayer::acceleratedCompositingForOverflowScrollEnabled() const
 //  And we would conclude that C could be promoted.
 void RenderLayer::updateDescendantsAreContiguousInStackingOrder()
 {
-    if (!isStackingContext() || !acceleratedCompositingForOverflowScrollEnabled())
+    if (!m_descendantsAreContiguousInStackingOrderDirty || !isStackingContext() || !acceleratedCompositingForOverflowScrollEnabled())
         return;
 
-    ASSERT(!m_normalFlowListDirty);
-    ASSERT(!m_zOrderListsDirty);

[Julien - ping for review, 2013/04/18 00:42:34]

I would have like to see any explanation why this was removed somewhere. It's
unfortunate as it weakens our pre-conditions.

-
     OwnPtr<Vector<RenderLayer*> > posZOrderList;
     OwnPtr<Vector<RenderLayer*> > negZOrderList;
     rebuildZOrderLists(StopAtStackingContexts, posZOrderList, negZOrderList);
@@ -635,6 +633,8 @@ void RenderLayer::updateDescendantsAreContiguousInStackingOrder()
     int count = 0;
     bool firstIteration = true;
     updateDescendantsAreContiguousInStackingOrderRecursive(lookup, minIndex, maxIndex, count, firstIteration);
+
+    m_descendantsAreContiguousInStackingOrderDirty = false;
 }
 
 void RenderLayer::updateDescendantsAreContiguousInStackingOrderRecursive(const HashMap<const RenderLayer*, int>& lookup, int& minIndex, int& maxIndex, int& count, bool firstIteration)
@@ -1030,6 +1030,9 @@ void RenderLayer::setAncestorChainHasVisibleDescendant()
 void RenderLayer::updateDescendantDependentFlags(HashSet<const RenderObject*>* outOfFlowDescendantContainingBlocks)
 {
     if (m_visibleDescendantStatusDirty || m_hasSelfPaintingLayerDescendantDirty || m_hasOutOfFlowPositionedDescendantDirty) {
+        const bool hadVisibleDescendant = m_hasVisibleDescendant;
+        const bool hadOutOfFlowPositionedDescendant = m_hasOutOfFlowPositionedDescendant;
+
         m_hasVisibleDescendant = false;
         m_hasSelfPaintingLayerDescendant = false;
         m_hasOutOfFlowPositionedDescendant = false;
@@ -1040,6 +1043,7 @@ void RenderLayer::updateDescendantDependentFlags(HashSet<const RenderObject*>* o
             child->updateDescendantDependentFlags(&childOutOfFlowDescendantContainingBlocks);
 
             bool childIsOutOfFlowPositioned = child->renderer() && child->renderer()->isOutOfFlowPositioned();
+

[Julien - ping for review, 2013/04/18 00:42:34]

Nit: Unneeded extra-space.

[Ian Vollick, 2013/04/18 20:55:52]

Done.

             if (childIsOutOfFlowPositioned)
                 childOutOfFlowDescendantContainingBlocks.add(child->renderer()->containingBlock());
 
@@ -1051,7 +1055,7 @@ void RenderLayer::updateDescendantDependentFlags(HashSet<const RenderObject*>* o
 
             bool hasVisibleDescendant = child->m_hasVisibleContent || child->m_hasVisibleDescendant;
             bool hasSelfPaintingLayerDescendant = child->isSelfPaintingLayer() || child->hasSelfPaintingLayerDescendant();
-            bool hasOutOfFlowPositionedDescendant = !childOutOfFlowDescendantContainingBlocks.isEmpty();
+            bool hasOutOfFlowPositionedDescendant = hasVisibleDescendant && (!childOutOfFlowDescendantContainingBlocks.isEmpty() || child->hasOutOfFlowPositionedDescendant());
 
             m_hasVisibleDescendant |= hasVisibleDescendant;
             m_hasSelfPaintingLayerDescendant |= hasSelfPaintingLayerDescendant;
@@ -1066,14 +1070,14 @@ void RenderLayer::updateDescendantDependentFlags(HashSet<const RenderObject*>* o
 
         m_visibleDescendantStatusDirty = false;
         m_hasSelfPaintingLayerDescendantDirty = false;
+        m_hasOutOfFlowPositionedDescendantDirty = false;
 
-        if (m_hasOutOfFlowPositionedDescendantDirty)
+        if (m_hasVisibleDescendant != hadVisibleDescendant || m_hasOutOfFlowPositionedDescendant != hadOutOfFlowPositionedDescendant)
             updateNeedsCompositedScrolling();
-
-        m_hasOutOfFlowPositionedDescendantDirty = false;
     }
 
     if (m_visibleContentStatusDirty) {
+        const bool hadVisibleContent = m_hasVisibleContent;
         if (renderer()->style()->visibility() == VISIBLE)
             m_hasVisibleContent = true;
         else {
@@ -1101,6 +1105,8 @@ void RenderLayer::updateDescendantDependentFlags(HashSet<const RenderObject*>* o
             }
         }    
         m_visibleContentStatusDirty = false; 
+        if (hadVisibleContent != m_hasVisibleContent)
+            updateNeedsCompositedScrolling();
     }
 }
 
@@ -1287,6 +1293,16 @@ RenderLayer* RenderLayer::stackingContainer() const
     return layer;
 }
 
+RenderLayer* RenderLayer::stackingContext() const

[Julien - ping for review, 2013/04/18 00:42:34]

This function should be named stackingContextAncestor(), that would remove the
need for the comment in RenderLayer.h as now your code is self-describing :)

(unfortunately it wouldn't match stackingContainer but we could remove it in the
same way)

[Ian Vollick, 2013/04/18 20:55:52]

Done.

+{
+    RenderLayer* layer = parent();

[Julien - ping for review, 2013/04/18 00:42:34]

Note that this could be implemented (which I would prefer):
RenderLayer* ancestor = parent();
if (!ancestor)
    return 0;
while (!layer->isStackingContext())
   layer = layer->parent();

ASSERT(layer->isStackingContext()); // Not really useful.
return layer;

[Ian Vollick, 2013/04/18 20:55:52]

Done.

+    while (layer && !layer->isStackingContext())
+        layer = layer->parent();
+
+    ASSERT(!layer || layer->isStackingContext());
+    return layer;
+}
+
 static inline bool isPositionedContainer(RenderLayer* layer)
 {
     RenderLayerModelObject* layerRenderer = layer->renderer();
@@ -1677,6 +1693,17 @@ 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();
+        child->updateDescendantsAreContiguousInStackingOrder();
+
+        // 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();
@@ -1710,11 +1737,17 @@ RenderLayer* RenderLayer::removeChild(RenderLayer* oldChild)
 
     if (oldChild->isNormalFlowOnly())
         dirtyNormalFlowList();
-    if (!oldChild->isNormalFlowOnly() || oldChild->firstChild()) { 
+    if (!oldChild->isNormalFlowOnly() || oldChild->firstChild()) {
         // Dirty the z-order list in which we are contained.  When called via the
         // 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();
+        updateDescendantsAreContiguousInStackingOrder();
+
+        // This could affect whether or not a layer has an out of flow
+        // positioned descendant so we need to schedule some updates.
+        for (RenderLayer* layer = this; layer; layer = layer->parent())

[Julien - ping for review, 2013/04/18 00:42:34]

This could probably stop at the oldChild's containing block (see addChild).

[Ian Vollick, 2013/04/18 20:55:52]

Done.

+            layer->updateNeedsCompositedScrolling();
     }
 
     if ((oldChild->renderer() && oldChild->renderer()->isOutOfFlowPositioned()) || oldChild->hasOutOfFlowPositionedDescendant())
@@ -1723,7 +1756,7 @@ RenderLayer* RenderLayer::removeChild(RenderLayer* oldChild)
     oldChild->setPreviousSibling(0);
     oldChild->setNextSibling(0);
     oldChild->setParent(0);
-    
+
     oldChild->updateDescendantDependentFlags();
     if (oldChild->m_hasVisibleContent || oldChild->m_hasVisibleDescendant)
         dirtyAncestorChainVisibleDescendantStatus();
@@ -1944,41 +1977,46 @@ bool RenderLayer::needsCompositedScrolling() const
 
 void RenderLayer::updateNeedsCompositedScrolling()
 {
-    bool oldNeedsCompositedScrolling = m_needsCompositedScrolling;
+    bool needsCompositedScrolling = false;
 
     FrameView* frameView = renderer()->view()->frameView();
-    if (!frameView || !frameView->containsScrollableArea(this))
-        m_needsCompositedScrolling = false;
-    else {
+    if (frameView && frameView->containsScrollableArea(this)) {
+        updateDescendantDependentFlags();
+
         bool forceUseCompositedScrolling = acceleratedCompositingForOverflowScrollEnabled()
             && canBeStackingContainer()
             && !hasOutOfFlowPositionedDescendant();
 
 #if ENABLE(ACCELERATED_OVERFLOW_SCROLLING)
-        m_needsCompositedScrolling = forceUseCompositedScrolling || renderer()->style()->useTouchOverflowScrolling();
+        needsCompositedScrolling = forceUseCompositedScrolling || renderer()->style()->useTouchOverflowScrolling();
 #else
-        m_needsCompositedScrolling = forceUseCompositedScrolling;
+        needsCompositedScrolling = forceUseCompositedScrolling;
 #endif
         // We gather a boolean value for use with Google UMA histograms to
         // quantify the actual effects of a set of patches attempting to
         // relax composited scrolling requirements, thereby increasing the
         // number of composited overflow divs.
         if (acceleratedCompositingForOverflowScrollEnabled())
-            HistogramSupport::histogramEnumeration("Renderer.NeedsCompositedScrolling", m_needsCompositedScrolling, 2);
+            HistogramSupport::histogramEnumeration("Renderer.NeedsCompositedScrolling", needsCompositedScrolling, 2);
     }
 
-    if (oldNeedsCompositedScrolling != m_needsCompositedScrolling) {
-        updateSelfPaintingLayer();
-        if (isStackingContainer())
-            dirtyZOrderLists();
-        else
-            clearZOrderLists();
+    if (m_needsCompositedScrolling == needsCompositedScrolling)
+        return;
 
-        dirtyStackingContainerZOrderLists();
+    m_needsCompositedScrolling = needsCompositedScrolling;
 
-        compositor()->setShouldReevaluateCompositingAfterLayout();
-        compositor()->setCompositingLayersNeedRebuild();
-    }
+    updateIsNormalFlowOnly();
+    updateSelfPaintingLayer();
+
+    if (isStackingContainer())
+        dirtyZOrderLists();
+    else
+        clearZOrderLists();
+
+    dirtyStackingContainerZOrderLists();
+
+    compositor()->setShouldReevaluateCompositingAfterLayout();
+    compositor()->setCompositingLayersNeedRebuild();
 }
 
 static inline int adjustedScrollDelta(int beginningDelta) {
@@ -2273,7 +2311,7 @@ void RenderLayer::updateCompositingLayersAfterScroll()
         // repositioning, so update compositing layers from there.
         if (RenderLayer* compositingAncestor = stackingContainer()->enclosingCompositingLayer()) {
             if (compositor()->compositingConsultsOverlap()) {
-                if (usesCompositedScrolling() && !hasOutOfFlowPositionedDescendant())
+                if (usesCompositedScrolling())
                     compositor()->updateCompositingLayers(CompositingUpdateOnCompositedScroll, compositingAncestor);
                 else
                     compositor()->updateCompositingLayers(CompositingUpdateOnScroll, compositingAncestor);
@@ -5504,6 +5542,9 @@ void RenderLayer::dirtyZOrderLists()
         m_negZOrderList->clear();
     m_zOrderListsDirty = true;
 
+    m_descendantsAreContiguousInStackingOrderDirty = true;
+    updateDescendantsAreContiguousInStackingOrder();
+
     if (!renderer()->documentBeingDestroyed()) {
         compositor()->setCompositingLayersNeedRebuild();
         if (acceleratedCompositingForOverflowScrollEnabled())
@@ -5513,9 +5554,18 @@ void RenderLayer::dirtyZOrderLists()
 
 void RenderLayer::dirtyStackingContainerZOrderLists()
 {
-    RenderLayer* sc = stackingContainer();
-    if (sc)
-        sc->dirtyZOrderLists();
+    RenderLayer* stackingContainer = this->stackingContainer();
+    if (stackingContainer)

[Julien - ping for review, 2013/04/18 00:42:34]

There is only one case where this could be NULL: the RenderView.

The code could be improved based on that:
if (!stackingContainer)
    return 0;

stackingContainer->dirtyZOrderLists();
RenderLayer* stackingContext = this->stackingContext();
ASSERT(stackingContext);
if (stackingContext != stackingContainer)
   stackingContext->dirtyZOrderLists();

[Ian Vollick, 2013/04/18 20:55:52]

Done.

+        stackingContainer->dirtyZOrderLists();
+
+    // Any change that could affect our stacking container's z-order list could
+    // cause other RenderLayers in our stacking context to either opt in or out
+    // of composited scrolling. It is important that we make our stacking
+    // context aware of these z-order changes so the appropriate updating can
+    // happen.
+    RenderLayer* stackingContext = this->stackingContext();
+    if (stackingContext && stackingContext != stackingContainer)
+        stackingContext->dirtyZOrderLists();
 }
 
 void RenderLayer::dirtyNormalFlowList()
@@ -5526,6 +5576,7 @@ void RenderLayer::dirtyNormalFlowList()
         m_normalFlowList->clear();
     m_normalFlowListDirty = true;
 
+    updateDescendantsAreContiguousInStackingOrder();
     if (!renderer()->documentBeingDestroyed()) {
         compositor()->setCompositingLayersNeedRebuild();
         if (acceleratedCompositingForOverflowScrollEnabled())
@@ -5628,7 +5679,7 @@ void RenderLayer::collectLayers(bool includeHiddenLayers, CollectLayersBehavior
 
 void RenderLayer::updateLayerListsIfNeeded()
 {
-    bool shouldUpdateDescendantsAreContiguousInStackingOrder = isStackingContext() && (m_zOrderListsDirty || m_normalFlowListDirty);
+    bool shouldUpdateDescendantsAreContiguousInStackingOrder = acceleratedCompositingForOverflowScrollEnabled() && isStackingContext() && (m_zOrderListsDirty || m_normalFlowListDirty) && m_descendantsAreContiguousInStackingOrderDirty;
     updateZOrderLists();
     updateNormalFlowList();
 
@@ -5724,6 +5775,18 @@ bool RenderLayer::shouldBeNormalFlowOnly() const
             ;
 }
 
+void RenderLayer::updateIsNormalFlowOnly()
+{
+    bool isNormalFlowOnly = shouldBeNormalFlowOnly();
+    if (isNormalFlowOnly == m_isNormalFlowOnly)
+        return;
+
+    m_isNormalFlowOnly = isNormalFlowOnly;
+    if (RenderLayer* p = parent())
+        p->dirtyNormalFlowList();
+    dirtyStackingContainerZOrderLists();
+}
+
 bool RenderLayer::shouldBeSelfPaintingLayer() const
 {
     return !isNormalFlowOnly()
@@ -5806,29 +5869,35 @@ bool RenderLayer::isVisuallyNonEmpty() const
     return false;
 }
 
-void RenderLayer::updateStackingContextsAfterStyleChange(const RenderStyle* oldStyle)
+void RenderLayer::updateVisibilityAfterStyleChange(const RenderStyle* oldStyle)
 {
-    if (!oldStyle)
+    EVisibility oldVisibility = oldStyle ? oldStyle->visibility() : VISIBLE;
+    if (oldVisibility == renderer()->style()->visibility())
         return;
 
-    bool wasStackingContext = isStackingContext(oldStyle);
-    bool isStackingContext = this->isStackingContext();
-    if (isStackingContext != wasStackingContext) {
-        dirtyStackingContainerZOrderLists();
-        if (isStackingContext)
-            dirtyZOrderLists();
-        else
-            clearZOrderLists();
-        return;
-    }
+    dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus();
+}
+
+void RenderLayer::updateStackingContextsAfterStyleChange(const RenderStyle* oldStyle)
+{
+    bool wasStackingContext = oldStyle ? isStackingContext(oldStyle) : false;
+    EVisibility oldVisibility = oldStyle ? oldStyle->visibility() : VISIBLE;
+    int oldZIndex = oldStyle ? oldStyle->zIndex() : 0;
 
     // FIXME: RenderLayer already handles visibility changes through our visiblity dirty bits. This logic could
     // likely be folded along with the rest.
-    if (oldStyle->zIndex() != renderer()->style()->zIndex() || oldStyle->visibility() != renderer()->style()->visibility()) {
-        dirtyStackingContainerZOrderLists();
-        if (isStackingContext)
-            dirtyZOrderLists();
-    }
+    bool isStackingContext = this->isStackingContext();
+    if (isStackingContext == wasStackingContext && oldVisibility == renderer()->style()->visibility() && oldZIndex == renderer()->style()->zIndex())
+        return;
+
+    dirtyStackingContainerZOrderLists();
+
+    if (isStackingContainer())
+        dirtyZOrderLists();
+    else
+        clearZOrderLists();
+
+    updateNeedsCompositedScrolling();
 }
 
 static bool overflowRequiresScrollbar(EOverflow overflow)
@@ -5894,7 +5963,8 @@ void RenderLayer::setAncestorChainHasOutOfFlowPositionedDescendant(RenderObject*
 
 void RenderLayer::dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus()
 {
-    m_hasOutOfFlowPositionedDescendantDirty = true;
+        m_hasOutOfFlowPositionedDescendantDirty = true;
+        updateNeedsCompositedScrolling();

[Julien - ping for review, 2013/04/18 00:42:34]

Wrong indentation for the previous 2 lines.

This also forces a full recomputation of the descendant dependent flags which is
annoying. Do we really need it? (could be a FIXME as it is not really something
we want in the long run)

[Ian Vollick, 2013/04/18 20:55:52]

Until we put the updates into a well defined phase, we have to
updateNeedsCompositedScrolling here, unfortunately. I've added a FIXME. I've
also updated the code so that we don't have to do this update as often (when we
add an out of flow positioned descendant, I now call setAncestorChainBlah rather
than dirtyAncestorChainBlah so we have to dirty/update less frequently).

     if (parent())
         parent()->dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus();
 }
@@ -5902,10 +5972,12 @@ void RenderLayer::dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus()
 void RenderLayer::updateOutOfFlowPositioned(const RenderStyle* oldStyle)
 {
     bool wasOutOfFlowPositioned = oldStyle && (oldStyle->position() == AbsolutePosition || oldStyle->position() == FixedPosition);
-    if (parent() && ((renderer() && renderer()->isOutOfFlowPositioned()) != wasOutOfFlowPositioned)) {
-        parent()->dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus();
-        if (!renderer()->documentBeingDestroyed() && acceleratedCompositingForOverflowScrollEnabled())
-            compositor()->setShouldReevaluateCompositingAfterLayout();
+    bool isOutOfFlowPositioned = renderer()->isOutOfFlowPositioned();
+    if (parent() && isOutOfFlowPositioned != wasOutOfFlowPositioned) {
+        if (isOutOfFlowPositioned)
+            parent()->setAncestorChainHasOutOfFlowPositionedDescendant(renderer()->containingBlock());
+        else
+            parent()->dirtyAncestorChainHasOutOfFlowPositionedDescendantStatus();
     }
 }
 
@@ -5989,15 +6061,6 @@ void RenderLayer::updateFilters(const RenderStyle* oldStyle, const RenderStyle*
 
 void RenderLayer::styleChanged(StyleDifference, const RenderStyle* oldStyle)
 {
-    bool isNormalFlowOnly = shouldBeNormalFlowOnly();
-    if (isNormalFlowOnly != m_isNormalFlowOnly) {
-        m_isNormalFlowOnly = isNormalFlowOnly;
-        RenderLayer* p = parent();
-        if (p)
-            p->dirtyNormalFlowList();
-        dirtyStackingContainerZOrderLists();
-    }
-
     if (renderer()->style()->overflowX() == OMARQUEE && renderer()->style()->marqueeBehavior() != MNONE && renderer()->isBox()) {
         if (!m_marquee)
             m_marquee = adoptPtr(new RenderMarquee(this));
@@ -6010,6 +6073,7 @@ void RenderLayer::styleChanged(StyleDifference, const RenderStyle* oldStyle)
 
     updateScrollbarsAfterStyleChange(oldStyle);
     updateStackingContextsAfterStyleChange(oldStyle);
+    updateVisibilityAfterStyleChange(oldStyle);
     // Overlay scrollbars can make this layer self-painting so we need
     // to recompute the bit once scrollbars have been updated.
     updateSelfPaintingLayer();
@@ -6047,8 +6111,6 @@ void RenderLayer::styleChanged(StyleDifference, const RenderStyle* oldStyle)
     updateFilters(oldStyle, renderer()->style());
 #endif
 
-    updateNeedsCompositedScrolling();
-
     const RenderStyle* newStyle = renderer()->style();
     if (compositor()->updateLayerCompositingState(this)
         || needsCompositingLayersRebuiltForClip(oldStyle, newStyle)
@@ -6073,7 +6135,13 @@ void RenderLayer::updateScrollableAreaSet(bool hasOverflow)
     if (HTMLFrameOwnerElement* owner = frame->ownerElement())
         isVisibleToHitTest &= owner->renderer() && owner->renderer()->visibleToHitTesting();
 
-    if (hasOverflow && isVisibleToHitTest ? frameView->addScrollableArea(this) : frameView->removeScrollableArea(this))
+    bool updatedScrollableAreaSet = false;
+    if (hasOverflow && isVisibleToHitTest)
+        updatedScrollableAreaSet = frameView->addScrollableArea(this);
+    else
+        updatedScrollableAreaSet = frameView->removeScrollableArea(this);
+
+    if (updatedScrollableAreaSet)
         updateNeedsCompositedScrolling();
 }