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..fbd8f6add1395d999a446f1f5ee2a88a69dc3a29 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);
+
+    // FIXME: We should invalidate previousPaintInvalidationRect, but for objects that don't save previousPaintInvalidationRect,
+    // we have to use the current paint invalidation rect. crbug.com/394133.
+    LayoutRect paintInvalidationRect = savesPreviousPaintInvalidationRect() ? previousPaintInvalidationRect() : boundsRectForPaintInvalidation(paintInvalidationContainer);
     invalidatePaintUsingContainer(paintInvalidationContainer, paintInvalidationRect, InvalidationPaint);
 }
 
@@ -2059,12 +2062,23 @@ 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())
+        } else {
+            // The paintInvalidationForWholeRenderer() calls are needed for non-layout changes to style. See the corresponding
+            // comment in RenderObject::styleWillChange for why.
+            if (savesPreviousPaintInvalidationRect()) {
+                LayoutRect paintInvalidationRect = boundsRectForPaintInvalidation(containerForPaintInvalidation());
+                if (paintInvalidationRect != previousPaintInvalidationRect()) {
+                    // If the original diff didn't require repaint, invalidate previousPaintInvalidationRect
+                    // before we save a new previousPaintInvalidationRect.
+                    if (!diff.needsRepaint())

[chrishtr, 2014/07/16 00:49:22]

Why the !diff.needsRepaint() conditional?

Why don't we also need to invalidate at the new location?

Is it really critical to submit a fix for just this? I'd rather see us go
directly for setting the dirty bit on
paint invalidation for style-only updates and do invalidation in one place. This
code looks complicated.

[Xianzhu, 2014/07/16 01:59:49]

'!diff.needsRepaint() && updatedDiff.needsRepaint())' might be possible in
theory. As we are changing previousPaintInvalidationRect, if we don't call
previousPaintInvalidationRect() here, the previous previousPaintInvalidationRect
will be lost and be never invalidated.

if diff.needsRepaint(), we have already invalidated the old location in
styleWillChange(), so we don't need do it again here.

However, based on RenderObject::adjustStyleDifference(), the cases causing
'!diff.needsRepaint() && updatedDiff.needsRepaint())' seem all related to layers
and we have other code paths for invalidation of them, so I think I can delete
these lines (line 2073-2076).
With this patch, the new location is invalidated at line 2080 after
previousPaintInvalidationRect is set to the new location at line 2077. In the
future, we don't need to invalidate the new location.
 
I planned multiple incremental small changes, first to resolve the P1 bug with
this change. With line 2073-2076 deleted, I think this patch will be small
enough.

+                        paintInvalidationForWholeRenderer();
+                    setPreviousPaintInvalidationRect(paintInvalidationRect);
+                }
+            }
             paintInvalidationForWholeRenderer();
+        }
     }
 }