Clip incremental invalidation rects for RenderBox

Clip incremental invalidation rects for RenderBox

RenderBox::incrementallyInvalidatePaint invalidates according to
border-box delta, not considering the bounds. If the bounds are
clipped by ancestor clipping, the incrementally invalidated rect
may exceed the ancestor clipping.

Clip the incremental invalidation rects by oldBounds and newBounds
to avoid invalidation on unrelated areas.

BUG=411420
TEST=fast/repaint/resize-with-border-clipped.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182081

[Xianzhu, 2014-09-09 16:18:10]

wangxianzhu@chromium.org changed reviewers:
+ esprehn@chromium.org, jchaffraix@chromium.org, leviw@chromium.org

[Xianzhu, 2014-09-10 17:20:09]

ping...

[Xianzhu, 2014-09-11 21:29:49]

On 2014/09/10 17:20:09, Xianzhu wrote:
> ping...

ping...

[Xianzhu, 2014-09-15 16:47:02]

ping...

[dsinclair, 2014-09-16 15:24:00]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org

[dsinclair, 2014-09-16 15:24:01]

https://codereview.chromium.org/548523005/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/548523005/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:4157:
invalidatePaintUsingContainer(&paintInvalidationContainer,
rectClippedByNewBounds, InvalidationIncremental);
This maybe a bit easier to read if the conditions are broken apart.

if (rectClippedByOldBounds == rectClippedByNewBounds) {
  invalidatePaintUsingContainer(..., rectClipedByOldBounds, ...);
  return;
}

if (!rectClippedByNewBounds.contains(rectClippedByOldBounds))
  invalidatePaintUsingContainer(..., rectClippedByOldBounds, ...)

if (!rectClippedByOldBounds.contains(rectClippedByNewBounds))
  invalidatePaintUsingContainer(..., rectClippedByNewBounds))

[Xianzhu, 2014-09-16 15:53:27]

https://codereview.chromium.org/548523005/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/548523005/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:4157:
invalidatePaintUsingContainer(&paintInvalidationContainer,
rectClippedByNewBounds, InvalidationIncremental);
Good idea. Done.

[dsinclair, 2014-09-16 17:30:38]

lgtm

[Julien - ping for review, 2014-09-16 18:22:53]

lgtm.

Let's add some comment in the description that you are concerned about the
*ancestors' clip*.

https://codereview.chromium.org/548523005/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/548523005/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:4153: bool equal = rectClippedByOldBounds ==
rectClippedByNewBounds;
(FYI as it's on the old patch) coding style violation: booleans should include a
verb (usually be or do).

https://codereview.chromium.org/548523005/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/548523005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:3952:
invalidatePaintUsingContainer(&paintInvalidationContainer,
rectClippedByNewBounds, InvalidationIncremental);
I count 3 cases for the non-equal case. Do we have really enough coverage for
all of them?

[Xianzhu, 2014-09-16 18:36:13]

On 2014/09/16 18:22:53, Julien Chaffraix - PST wrote:
> lgtm.
> 
> Let's add some comment in the description that you are concerned about the
> *ancestors' clip*.
> 
Done.

https://codereview.chromium.org/548523005/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/548523005/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:4153: bool equal = rectClippedByOldBounds ==
rectClippedByNewBounds;
On 2014/09/16 18:22:52, Julien Chaffraix - PST wrote:
> (FYI as it's on the old patch) coding style violation: booleans should include
a
> verb (usually be or do).

Acknowledged.

https://codereview.chromium.org/548523005/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/548523005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:3952:
invalidatePaintUsingContainer(&paintInvalidationContainer,
rectClippedByNewBounds, InvalidationIncremental);
On 2014/09/16 18:22:52, Julien Chaffraix - PST wrote:
> I count 3 cases for the non-equal case. Do we have really enough coverage for
> all of them?

Yes. The existing layout tests cover the cases. Actually the conditions were
added after I had noticed duplicated invalidation rects without the conditions
for existing layout tests. The new layout test just cover the clipping behavior.

[Xianzhu, 2014-09-16 18:36:23]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-09-16 18:37:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/548523005/40001

[commit-bot: I haz the power, 2014-09-16 18:42:37]

Committed patchset #3 (id:40001) as 182081

[Xianzhu, 2015-02-04 23:39:25]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/899813004/ by wangxianzhu@chromium.org.

The reason for reverting is: Haven't looked into the issue deeply, but in the
future we won't care about the invalidation rects, so we can just revert this
kind of optimizations.

BUG=453586.