Common invalidateTreeIfNeeded()

Source/core/rendering/RenderObject.cpp

Index: Source/core/rendering/RenderObject.cpp
diff --git a/Source/core/rendering/RenderObject.cpp b/Source/core/rendering/RenderObject.cpp
index 2772d053cb54e5454e14fc5d323de93a3ff39d7f..2c34c684c26fa6b2826f99d774c2f492361547a8 100644
--- a/Source/core/rendering/RenderObject.cpp
+++ b/Source/core/rendering/RenderObject.cpp
@@ -1602,13 +1602,20 @@ const char* RenderObject::invalidationReasonToString(InvalidationReason reason)
 
 void RenderObject::invalidateTreeIfNeeded(const PaintInvalidationState& paintInvalidationState)
 {
+    ASSERT(!needsLayout());
+
     // If we didn't need paint invalidation then our children don't need as well.
     // Skip walking down the tree as everything should be fine below us.
     if (!shouldCheckForPaintInvalidation(paintInvalidationState))
         return;
 
+    invalidatePaintIfNeeded(paintInvalidationState, paintInvalidationState.paintInvalidationContainer());
     clearPaintInvalidationState(paintInvalidationState);
+    invalidatePaintOfSubtreesIfNeeded(paintInvalidationState);
+}
 
+void RenderObject::invalidatePaintOfSubtreesIfNeeded(const PaintInvalidationState& paintInvalidationState)
+{
     for (RenderObject* child = slowFirstChild(); child; child = child->nextSibling()) {
         if (!child->isOutOfFlowPositioned())
             child->invalidateTreeIfNeeded(paintInvalidationState);
@@ -1623,19 +1630,29 @@ static PassRefPtr<TraceEvent::ConvertableToTraceFormat> jsonObjectForOldAndNewRe
     return value;
 }
 
-InvalidationReason RenderObject::invalidatePaintIfNeeded(const RenderLayerModelObject& paintInvalidationContainer, const LayoutRect& oldBounds, const LayoutPoint& oldLocation, const PaintInvalidationState& paintInvalidationState)
+InvalidationReason RenderObject::invalidatePaintIfNeeded(const PaintInvalidationState& paintInvalidationState, const RenderLayerModelObject& paintInvalidationContainer)
 {
     RenderView* v = view();
     if (v->document().printing())
         return InvalidationNone; // Don't invalidate paints if we're printing.
 
-    const LayoutRect& newBounds = previousPaintInvalidationRect();
-    const LayoutPoint& newLocation = previousPositionFromPaintInvalidationContainer();
+    const LayoutRect oldBounds = previousPaintInvalidationRect();
+    const LayoutPoint oldLocation = previousPositionFromPaintInvalidationContainer();
+    const LayoutRect newBounds = boundsRectForPaintInvalidation(&paintInvalidationContainer, &paintInvalidationState);
+    const LayoutPoint newLocation = RenderLayer::positionFromPaintInvalidationContainer(this, &paintInvalidationContainer, &paintInvalidationState);
+    setPreviousPaintInvalidationRect(newBounds);
+    setPreviousPositionFromPaintInvalidationContainer(newLocation);
 
-    // FIXME: PaintInvalidationState should not be required here, but the call to flipForWritingMode
-    // in mapRectToPaintInvalidationBacking will give us the wrong results with it disabled.
+    // FIXME: The call to flipForWritingMode in mapRectToPaintInvalidationBacking
+    // will give us the wrong results without paintInvalidationState.
     // crbug.com/393762
-    ASSERT(newBounds == boundsRectForPaintInvalidation(&paintInvalidationContainer, &paintInvalidationState));
+    // ASSERT(newBounds == boundsRectForPaintInvalidation(&newPaintInvalidationContainer, 0));

[Julien - ping for review, 2014/09/04 19:48:04]

The ASSERT should use paintInvalidationContainer as that's the coordinate system
used for computing the bounds.

I think it's fine to remove it as it was just checking that we did set the new
bounds and now it's guaranteed.

[leviw_travelin_and_unemployed, 2014/09/05 21:37:12]

Why newPaintInvalidationContainer? That's not actually a variable in the
function. I'm sad to see this commented out, but don't make it fail to compile.

Other than that nit, this is effectively what we want from the assert -- to test
that we're generating the same invalidation bounds with and without the
paintInvalidationState. I'd lean towards keeping the assert and working towards
fixing it, but I'm also okay with removing it altogether and relying on the bug
to track it.

[Xianzhu, 2014/09/05 22:06:37]

I removed it.

+
+    // If we are set to do a full paint invalidation that means the RenderView will issue
+    // paint invalidations. We can then skip issuing of paint invalidations for the child
+    // renderers as they'll be covered by the RenderView.
+    if (view()->doingFullPaintInvalidation())
+        return InvalidationNone;
 
     TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("blink.invalidation"), "RenderObject::invalidatePaintIfNeeded()",
         "object", this->debugName().ascii(),