Improve handling of PaintInvalidationLayer (renamed to PaintInvalidationSubtree)

third_party/WebKit/Source/core/layout/LayoutObject.cpp

Index: third_party/WebKit/Source/core/layout/LayoutObject.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutObject.cpp b/third_party/WebKit/Source/core/layout/LayoutObject.cpp
index f9c613c511a1d71c54ad4f14d7160c8104822cc8..8bd06afe8f4fc22889991dedf655cc20c85df56f 100644
--- a/third_party/WebKit/Source/core/layout/LayoutObject.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutObject.cpp
@@ -1321,10 +1321,7 @@ void LayoutObject::invalidateTreeIfNeeded(const PaintInvalidationState& paintInv
     PaintInvalidationReason reason = invalidatePaintIfNeeded(newPaintInvalidationState);
     clearPaintInvalidationFlags(newPaintInvalidationState);
 
-    if (reason == PaintInvalidationDelayedFull)
-        newPaintInvalidationState.pushDelayedPaintInvalidationTarget(*this);
-
-    newPaintInvalidationState.updateForChildren();
+    newPaintInvalidationState.updateForChildren(reason);
     invalidatePaintOfSubtreesIfNeeded(newPaintInvalidationState);
 }
 
@@ -1439,12 +1436,10 @@ PaintInvalidationReason LayoutObject::invalidatePaintIfNeeded(const PaintInvalid
     if (!RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled())
         setPreviousPositionFromPaintInvalidationBacking(newLocation);
 
-    if (!shouldCheckForPaintInvalidationRegardlessOfPaintInvalidationState() && !paintInvalidationState.forcedSubtreeInvalidationWithinContainer()) {
-        ASSERT(paintInvalidationState.forcedSubtreeInvalidationRectUpdateWithinContainer());
+    if (!shouldCheckForPaintInvalidationRegardlessOfPaintInvalidationState() && paintInvalidationState.forcedSubtreeInvalidationRectUpdateWithinContainerOnly())

[chrishtr, 2016/05/09 22:20:41]

I'm trying to understand what the old and new version of this conditional is
doing. Can you explain?
And also add a comment?

[Xianzhu, 2016/05/10 17:17:12]

The previous condition was an implicit check for
forcedSubtreeInvlaidationRectUpdate *only*, now it's checked explicitly. Added
comment.

         return PaintInvalidationNone;
-    }
 
-    PaintInvalidationReason invalidationReason = getPaintInvalidationReason(paintInvalidationContainer, oldBounds, oldLocation, newBounds, newLocation);
+    PaintInvalidationReason invalidationReason = getPaintInvalidationReason(paintInvalidationState, oldBounds, oldLocation, newBounds, newLocation);
 
     // We need to invalidate the selection before checking for whether we are doing a full invalidation.
     // This is because we need to update the old rect regardless.
@@ -1465,7 +1460,7 @@ PaintInvalidationReason LayoutObject::invalidatePaintIfNeeded(const PaintInvalid
         // mutation, but incurs no pixel difference (i.e. bounds stay the same) so no rect-based
         // invalidation is issued. See crbug.com/508383 and crbug.com/515977.
         // This is a workaround to force display items to update paint offset.
-        if (!RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled() && paintInvalidationState.forcedSubtreeInvalidationWithinContainer())
+        if (!RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled() && paintInvalidationState.forcedSubtreeInvalidationCheckingWithinContainer())
             invalidateDisplayItemClientsWithPaintInvalidationState(paintInvalidationContainer, paintInvalidationState, invalidationReason);
 
         return invalidationReason;
@@ -1480,10 +1475,13 @@ PaintInvalidationReason LayoutObject::invalidatePaintIfNeeded(const PaintInvalid
     return invalidationReason;
 }
 
-PaintInvalidationReason LayoutObject::getPaintInvalidationReason(const LayoutBoxModelObject& paintInvalidationContainer,
+PaintInvalidationReason LayoutObject::getPaintInvalidationReason(const PaintInvalidationState& paintInvalidationState,
     const LayoutRect& oldBounds, const LayoutPoint& oldPositionFromPaintInvalidationBacking,
     const LayoutRect& newBounds, const LayoutPoint& newPositionFromPaintInvalidationBacking) const
 {
+    if (paintInvalidationState.forcedSubtreeFullInvalidationWithinContainer())
+        return PaintInvalidationSubtree;
+
     if (shouldDoFullPaintInvalidation())
         return m_bitfields.fullPaintInvalidationReason();
 
@@ -1790,28 +1788,28 @@ StyleDifference LayoutObject::adjustStyleDifference(StyleDifference diff) const
         // Text nodes share style with their parents but transforms don't apply to them,
         // hence the !isText() check.
         if (!isText() && (!hasLayer() || !toLayoutBoxModelObject(this)->layer()->hasStyleDeterminedDirectCompositingReasons()))
-            diff.setNeedsPaintInvalidationLayer();
+            diff.setNeedsPaintInvalidationSubtree();
     }
 
     // If opacity or zIndex changed, and the layer does not paint into its own separate backing, then we need to invalidate paints (also
     // ignoring text nodes)
     if (diff.opacityChanged() || diff.zIndexChanged()) {
         if (!isText() && (!hasLayer() || !toLayoutBoxModelObject(this)->layer()->hasStyleDeterminedDirectCompositingReasons()))
-            diff.setNeedsPaintInvalidationLayer();
+            diff.setNeedsPaintInvalidationSubtree();
     }
 
     // If filter changed, and the layer does not paint into its own separate backing or it paints with filters, then we need to invalidate paints.
     if (diff.filterChanged() && hasLayer()) {
         PaintLayer* layer = toLayoutBoxModelObject(this)->layer();
         if (!layer->hasStyleDeterminedDirectCompositingReasons() || layer->paintsWithFilters())
-            diff.setNeedsPaintInvalidationLayer();
+            diff.setNeedsPaintInvalidationSubtree();
     }
 
     // If backdrop filter changed, and the layer does not paint into its own separate backing or it paints with filters, then we need to invalidate paints.
     if (diff.backdropFilterChanged() && hasLayer()) {
         PaintLayer* layer = toLayoutBoxModelObject(this)->layer();
         if (!layer->hasStyleDeterminedDirectCompositingReasons() || layer->paintsWithBackdropFilters())
-            diff.setNeedsPaintInvalidationLayer();
+            diff.setNeedsPaintInvalidationSubtree();
     }
 
     // Optimization: for decoration/color property changes, invalidation is only needed if we have style or text affected by these properties.
@@ -1838,12 +1836,6 @@ StyleDifference LayoutObject::adjustStyleDifference(StyleDifference diff) const
             diff.setNeedsFullLayout();
     }
 
-    // If we have no layer(), just treat a PaintInvalidationLayer hint as a normal paint invalidation.
-    if (diff.needsPaintInvalidationLayer() && !hasLayer()) {

[chrishtr, 2016/05/09 22:20:41]

Why can this code be removed?

[Xianzhu, 2016/05/10 17:17:12]

I think if a style change affects the whole subtree, we should invalidate the
whole subtree regardless layer. I'm running
https://codereview.chromium.org/1959423002 on trybots to see if we can actually
hit this condition in tests.

[Xianzhu, 2016/05/10 19:09:19]

Try jobs finished. There are a lot of hits of this condition, in cases that a
subtree-affecting style is just added and the layer haven't been created yet. I
think we have other paths to ensure paint invalidation of the subtree, but as
now the cost of
setShouldDoFullPaintInvalidationIncludingNonCompositedDescendants() call itself
becomes trivial (the resulted paint invalidations are the same if we already did
it right), it does no harm to remove this condition.

-        diff.clearNeedsPaintInvalidation();
-        diff.setNeedsPaintInvalidationObject();
-    }
-
     return diff;
 }
 
@@ -1997,7 +1989,7 @@ void LayoutObject::setStyle(PassRefPtr<ComputedStyle> style)
             setNeedsLayoutAndPrefWidthsRecalc(LayoutInvalidationReason::StyleChange);
     }
 
-    if (diff.needsPaintInvalidationLayer() || updatedDiff.needsPaintInvalidationLayer())
+    if (diff.needsPaintInvalidationSubtree() || updatedDiff.needsPaintInvalidationSubtree())
         setShouldDoFullPaintInvalidationIncludingNonCompositingDescendants();
     else if (diff.needsPaintInvalidationObject() || updatedDiff.needsPaintInvalidationObject())
         setShouldDoFullPaintInvalidation();
@@ -3464,7 +3456,7 @@ void LayoutObject::clearPaintInvalidationFlags(const PaintInvalidationState& pai
 {
     // paintInvalidationStateIsDirty should be kept in sync with the
     // booleans that are cleared below.
-    ASSERT(paintInvalidationState.forcedSubtreeInvalidationWithinContainer() || paintInvalidationState.forcedSubtreeInvalidationRectUpdateWithinContainer() || paintInvalidationStateIsDirty());
+    ASSERT(!shouldCheckForPaintInvalidationRegardlessOfPaintInvalidationState() || paintInvalidationStateIsDirty());
     clearShouldDoFullPaintInvalidation();
     m_bitfields.setChildShouldCheckForPaintInvalidation(false);
     m_bitfields.setNeededLayoutBecauseOfChildren(false);
@@ -3610,23 +3602,20 @@ void LayoutObject::invalidatePaintIncludingNonCompositingDescendants()
     traverseNonCompositingDescendants(*this, [&paintInvalidationContainer](LayoutObject& object) {
         if (object.hasLayer())
             toLayoutBoxModelObject(object).layer()->setNeedsRepaint();
-        object.invalidatePaintOfPreviousPaintInvalidationRect(paintInvalidationContainer, PaintInvalidationLayer);
+        object.invalidatePaintOfPreviousPaintInvalidationRect(paintInvalidationContainer, PaintInvalidationSubtree);
     });
 }
 
-// FIXME: If we had a flag to force invalidations in a whole subtree, we could get rid of this function (crbug.com/410097).
 void LayoutObject::setShouldDoFullPaintInvalidationIncludingNonCompositingDescendants()
 {
-    // Need to access the current compositing status.
-    DisableCompositingQueryAsserts disabler;
-    traverseNonCompositingDescendants(*this, [](LayoutObject& object) {
-        object.setShouldDoFullPaintInvalidation();
-    });
+    // Clear first because PaintInvalidationSubtree overrides other full paint invalidation reasons.
+    clearShouldDoFullPaintInvalidation();
+    setShouldDoFullPaintInvalidation(PaintInvalidationSubtree);
 }
 
 void LayoutObject::invalidatePaintIncludingNonSelfPaintingLayerDescendantsInternal(const LayoutBoxModelObject& paintInvalidationContainer)
 {
-    invalidatePaintOfPreviousPaintInvalidationRect(paintInvalidationContainer, PaintInvalidationLayer);
+    invalidatePaintOfPreviousPaintInvalidationRect(paintInvalidationContainer, PaintInvalidationSubtree);
     for (LayoutObject* child = slowFirstChild(); child; child = child->nextSibling()) {
         if (child->hasLayer())
             toLayoutBoxModelObject(child)->layer()->setNeedsRepaint();