Invalidate previous paint rect in RenderObject::paintInvalidationForWholeRenderer()

Source/core/rendering/RenderObject.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Dirk Mueller (mueller@kde.org)
  *           (C) 2004 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2009 Google Inc. All rights reserved.
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 1513 matching lines @@
     if (!isRooted())
         return;
 
     if (view()->document().printing())
         return; // Don't invalidate paints if we're printing.
 
     // FIXME: really, we're in the paint invalidation phase here, and the following queries are legal.
     // 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);
 }
 
 LayoutRect RenderObject::boundsRectForPaintInvalidation(const RenderLayerModelObject* paintInvalidationContainer, const PaintInvalidationState* paintInvalidationState) const
 {
     if (!paintInvalidationContainer)
         return computePaintInvalidationRect(paintInvalidationContainer, paintInvalidationState);
     return RenderLayer::computePaintInvalidationRect(this, paintInvalidationContainer->layer(), paintInvalidationState);
 }
 
@@ skipping 507 matching lines @@
 
     if (diff.transformChanged() && !needsLayout()) {
         if (RenderBlock* container = containingBlock())
             container->setNeedsOverflowRecalcAfterStyleChange();
     }
 
     if (updatedDiff.needsRepaint()) {
         // 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();
+        }
     }
 }
 
 static inline bool rendererHasBackground(const RenderObject* renderer)
 {
     return renderer && renderer->hasBackground();
 }
 
 void RenderObject::styleWillChange(StyleDifference diff, const RenderStyle& newStyle)
 {
@@ skipping 1368 matching lines @@
 {
     if (object1) {
         const WebCore::RenderObject* root = object1;
         while (root->parent())
             root = root->parent();
         root->showRenderTreeAndMark(object1, "*", object2, "-", 0);
     }
 }
 
 #endif