Workaround rect-based paint invalidation on ancestor clip change

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  *           (C) 2005, 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 345 matching lines @@
     ASSERT(!needsLayout());
 
     if (!shouldCheckForPaintInvalidation(paintInvalidationState))
         return;
 
     bool establishesNewPaintInvalidationContainer = isPaintInvalidationContainer();
     const LayoutBoxModelObject& newPaintInvalidationContainer = *adjustCompositedContainerForSpecialAncestors(establishesNewPaintInvalidationContainer ? this : &paintInvalidationState.paintInvalidationContainer());
     // 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);
 
     if (reason == PaintInvalidationDelayedFull)
         paintInvalidationState.pushDelayedPaintInvalidationTarget(*this);
 
     PaintInvalidationState childTreeWalkState(paintInvalidationState, *this, newPaintInvalidationContainer);
     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);
 }
 
 void LayoutBoxModelObject::setBackingNeedsPaintInvalidationInRect(const LayoutRect& r, PaintInvalidationReason invalidationReason) const
 {
     // https://bugs.webkit.org/show_bug.cgi?id=61159 describes an unreproducible crash here,
     // so assert but check that the layer is composited.
     ASSERT(compositingState() != NotComposited);
 
     // FIXME: generalize accessors to backing GraphicsLayers so that this code is squashing-agnostic.
@@ skipping 657 matching lines @@
     if (rootElementStyle->hasBackground())
         return false;
 
     if (node() != document().firstBodyElement())
         return false;
 
     return true;
 }
 
 } // namespace blink