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