Workaround rect-based paint invalidation on ancestor clip change

third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp

Index: third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp b/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
index 4181c785c95ba9080af6ac10e5077d4bfd866908..10985cfa7b88464be2d9f7b8f7b4a913e84e0d45 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
@@ -363,6 +363,8 @@ void LayoutBoxModelObject::invalidateTreeIfNeeded(PaintInvalidationState& paintI
     // FIXME: This assert should be re-enabled when we move paint invalidation to after compositing update. crbug.com/360286
     // ASSERT(&newPaintInvalidationContainer == containerForPaintInvalidation());
 
+    LayoutRect previousPaintInvalidationRect = this->previousPaintInvalidationRect();
+
     PaintInvalidationReason reason = invalidatePaintIfNeeded(paintInvalidationState, newPaintInvalidationContainer);
     clearPaintInvalidationState(paintInvalidationState);
 
@@ -373,10 +375,18 @@ void LayoutBoxModelObject::invalidateTreeIfNeeded(PaintInvalidationState& paintI
     if (reason == PaintInvalidationLocationChange)
         childTreeWalkState.setForceSubtreeInvalidationWithinContainer();
 
-    // Workaround for crbug.com/533277.
+    // TODO(wangxianzhu): This is a workaround for crbug.com/533277. Will remove when we enable paint offset caching.
     if (reason != PaintInvalidationNone && hasPercentageTransform(styleRef()))
         childTreeWalkState.setForceSubtreeInvalidationWithinContainer();
 
+    // TODO(wangxianzhu): This is a workaround for crbug.com/490725. We don't have enough saved information to do accurate check
+    // of clipping change. Will remove when we remove rect-based paint invalidation.
+    if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled()
+        && previousPaintInvalidationRect != this->previousPaintInvalidationRect()

[chrishtr, 2015/09/28 17:38:13]

If the rect changes, why didn't invalidatePaintIfNeeded return a paint
invalidation reason other than None?

[Xianzhu, 2015/09/28 17:49:05]

These condition are for the ancestor whose clip changed. The condition checking
paint invalidation rect is approximate to check for clip change (we don't have
enough information to check for clip change). If an object has clip and its
paint invalidation rect changed, it's likely that its clip changed. This is not
perfect, but catches some bug cases as a workaround. If the conditions are true,
we force descendants to update their paint invalidation rects.

[chrishtr, 2015/09/28 22:16:29]

Ok, but in what situation is invalidatePaintIfNeeded going to return
PaintInvalidationReasonNone yet the conditionals you're adding hold?
If something other than PaintInvalidationReasonNone is returned, why
compare the rects?

[Xianzhu, 2015/09/28 22:56:26]

Let's take the added layout test as an example:

<div id="container" style="position: absolute; top: 100px; width: 100px; height:
10px; overflow: hidden">
  <div style="height: 20px"></div>
  <div style="width: 100px; height: 300px">
    <div id="content" style="width: 100px; height: 300px; background-color:
red"></div>
  </div>
</div>

Previously, when 'container' size changes, 'content' will skip paint
invalidation because neither style nor layout changes so it has no paint
invalidation flag set, leaving its previousPaintInvalidaitonRect incorrect and
causing problems of the next paint invalidation. There is no under-invalidation
of rects or objects.

With the CL, 'container' meets the conditions here, and force all descendants to
update their paint invalidation rects. Because change of the paint invalidation
rects is caused by overflow clip of 'container', the descendants don't need to
invalidate either rect or object. They just need to update
previousPaintInvalidationRect for the next paint invalidation.

This bug can be also fixed with synchronized painting which will remove
rect-based paint invalidation.

[chrishtr, 2015/09/28 23:38:13]

This all sounds great, and makes sense. What I don't understand yet is why you
aren't simply
checking whether the paint invalidation reason here for the container is any
non-None value, and instead trying to compare the rects. Is it because you're
trying to optimize the situation where the container changes box decorations to
not trigger this logic?

[Xianzhu, 2015/09/28 23:51:48]

I see your question now :)

Simple style change (e.g. background) causes non-None paint invalidation reason
but we don't need to traverse the descendants. I'm afraid making this condition
too loose may degrade performance.

+        && !usesCompositedScrolling()
+        && hasOverflowClip())
+        childTreeWalkState.setForceSubtreeInvalidationRectUpdateWithinContainer();
+
     invalidatePaintOfSubtreesIfNeeded(childTreeWalkState);
 }