Invalidate previous paint rect in RenderObject::paintInvalidationForWholeRenderer()

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 612025741947dfc4a0327918231b98b6bb27393f..8487a99918a0bfddcba3de23d49cbdb99b4a645a 100644
--- a/Source/core/rendering/RenderObject.cpp
+++ b/Source/core/rendering/RenderObject.cpp
@@ -1531,7 +1531,10 @@ void RenderObject::paintInvalidationForWholeRenderer() const
     // Until those states are fully fledged, I'll just disable the ASSERTS.
     DisableCompositingQueryAsserts disabler;
     const RenderLayerModelObject* paintInvalidationContainer = containerForPaintInvalidation();
-    LayoutRect paintInvalidationRect = boundsRectForPaintInvalidation(paintInvalidationContainer);
+
+    // We should invalidate previousPaintInvalidationRect, but for objects that don't save previousPaintInvalidationRect,
+    // we have to use the current paint invalidation rect.
+    LayoutRect paintInvalidationRect = isBox() || isSVG() ? previousPaintInvalidationRect() : boundsRectForPaintInvalidation(paintInvalidationContainer);

[leviw_travelin_and_unemployed, 2014/07/15 23:41:29]

This will still be broken for inlines and such, no? How about a test showing it
is, and a fixme.


Also, I don't like these magic values to mean "we have a previous paint
invalidation rect." I'd rather there at least be a descriptive function. We
should also have some asserts around this, like asserting that no unexpected
types are having setPreviousPaintInvalidationRect called.

[Xianzhu, 2014/07/16 00:22:36]

Add FIXME with link to crbug.com/394133.
Added bool savesPreviousPaintInvalidationRect() method and ASSERTs in
previousPaintInvalildationRect() and setPreviousPaintInvalidationRect().

     invalidatePaintUsingContainer(paintInvalidationContainer, paintInvalidationRect, InvalidationPaint);
 }
 
@@ -2059,12 +2062,32 @@ void RenderObject::setStyle(PassRefPtr<RenderStyle> style)
         // Invalidate paints with the new style, e.g., for example if we go from not having
         // an outline to having an outline.
 
-        // The paintInvalidationForWholeRenderer() call is needed for non-layout changes to style. See the corresponding
-        // comment in RenderObject::styleWillChange for why.
-        if (needsLayout())
+        if (needsLayout()) {
             setShouldDoFullPaintInvalidationAfterLayout(true);
-        else if (!selfNeedsLayout())
-            paintInvalidationForWholeRenderer();
+        } else {
+            // The paintInvalidationForWholeRenderer() calls are needed for non-layout changes to style. See the corresponding
+            // comment in RenderObject::styleWillChange for why.
+
+            bool shouldInvalidatePaint;
+            if (isBox() || isSVG()) {
+                // Should do paint invalidation if the original diff didn't require repaint.
+                if (!diff.needsRepaint())

[leviw_travelin_and_unemployed, 2014/07/15 23:41:29]

It feels to me like this optimization and the fix above to use
previousPaintInvalidationRect should be different patches. One is correctness,
one is optimization, no?

[Xianzhu, 2014/07/16 00:22:36]

Done.

+                    shouldInvalidatePaint = true;
+                LayoutRect paintInvalidationRect = boundsRectForPaintInvalidation(containerForPaintInvalidation());
+                if (paintInvalidationRect != previousPaintInvalidationRect()) {
+                    shouldInvalidatePaint = true;
+                    // If the original diff didn't require require repaint, invalidate the previous paint rect.
+                    if (!diff.needsRepaint())
+                        paintInvalidationForWholeRenderer();
+                    setPreviousPaintInvalidationRect(paintInvalidationRect);
+                }
+            } else {
+                shouldInvalidatePaint = true;
+            }
+
+            if (shouldInvalidatePaint)
+                paintInvalidationForWholeRenderer();
+        }
     }
 }