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..802f5b8ac6f043b7dbf2e1919dc07b022088903d 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);
+    invalidateSubtreesIfNeeded(paintInvalidationState);
+}
 
+void RenderObject::invalidateSubtreesIfNeeded(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));

[dsinclair, 2014/09/03 14:14:47]

Why did the ASSERT change? previously we passed the paintInvalidationState into
boundsRectForPaintInvalidation and it was enabled.

You've removed the invalidation state and commented out the ASSERT?

[Xianzhu, 2014/09/03 17:11:15]

Based on the FIXME, I think the original intent of the ASSERT was to check if
boundsRectForPaintInvalidation() returns the same value with or without
paintInvalidationState, but for the mentioned reasons, we can't do it for now.

Because newBounds has just been set to
boundsRectForPaintInvalidation(&newPaintInvalidationContainer,
&paintInvalidationState) above in this change, it seems meaningless to assert if
they equal, so I changed the assert to meet its original intent but commented it
out because of the reasons mentioned in the FIXME.

[dsinclair, 2014/09/03 17:44:29]

Ah, yea, previously we weren't setting the bounds in this method. I'll have to
defer to leviw@ on what the correct state of the ASSERT should be (if we change
it, or just remove it at this point)

+
+    // 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(),