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 503 matching lines @@
             setNeedsLayoutAndPrefWidthsRecalc();
         else if (updatedDiff.needsPositionedMovementLayout())
             setNeedsPositionedMovementLayout();
     }
 
     if (diff.transformChanged() && !needsLayout()) {
         if (RenderBlock* container = containingBlock())
             container->setNeedsOverflowRecalcAfterStyleChange();
     }
 
     if (updatedDiff.needsRepaint()) {

[chrishtr, 2014/07/16 17:56:25]

Just discussed some more with Levi. I think you can go directly to the desired
solution of setting bits only and getting rid of the call to
paintInvalidationForWholeRenderer() entirely. Just do this:

if (updatedDiff.needsRepaint())
    setShouldDoFullPaintInvalidationAfterLayout(true);


and that's it. This will cause an invalidation to happen after style and/or
layout updates have happened.

[Xianzhu, 2014/07/16 18:05:39]

OK will do. I have tried that way, but it seems to need a much bigger change
than just setting shouldDoFullPaintInvalidationAfterLayout here. Will continue.

I think we'll still need paintInvalidationForWholeRenderer() when a RenderObject
is removed.

         // 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 {
+            if (savesPreviousPaintInvalidationRect())
+                setPreviousPaintInvalidationRect(boundsRectForPaintInvalidation(containerForPaintInvalidation()));

[leviw_travelin_and_unemployed, 2014/07/16 23:06:48]

Isn't this redundant with the change you're making to
paintInvalidationForWholeRenderer()?

[Xianzhu, 2014/07/16 23:47:52]

No. Actually this is paired with the change in paintInvalidationForWholeRenderer
because here the caller wants to invalidate the new location.

With unified invalidation path (I'm working on), the above code won't be needed.

[leviw_travelin_and_unemployed, 2014/07/22 21:42:29]

If we've ever put up a frame with the thing at the new location, we should have
the right location saved, no? Can you help me understand why this case would be
different and we'd want the "new" location?

[Xianzhu, 2014/07/22 21:53:52]

This change has been abandoned. The above code was only needed before we go
unified invalidation path (https://codereview.chromium.org/398343003). Will
continue the new change after landing of depended changes.

             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