Use unified invalidation path for repaint-only style changes

Use unified invalidation path for repaint-only style changes

Use setShouldDoFullPaintInvalidation() for repaint-only style change
to avoid paintInvalidationForWholeRenderer() if possible.

Remaining issues:
- crbug.com/394133: for objects that don't invalidate themselves in
  invalidateTreeIfNeeded(), we still need to call
  paintInvalidationForWholeRenderer();

About tests:
- Some previous duplicated repaints are combined.
- Several test (e.g. fast/repaint/vertical-overflow-parent.html) have
  extra invalidations for some scrollbar area. They will be eliminated
  after bug 394833 is fixed.
- Some layer related tests had under-repaint previously. This is fixed
  by this change. 

BUG=394004, 394050
TEST=See "About tests" above.

[Xianzhu, 2014-07-17 18:33:46]

Several test (e.g. fast/repaint/vertical-overflow-parent.html) will have extra
invalidations for some scrollbar area. They will be eliminated after bug 394833
is fixed.

[dsinclair, 2014-07-17 18:36:02]

Looking through the results, it looks like
svg/repaint/remove-background-property-on-root.html maybe a real failure (no
invalidation rects were issued).

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (left):

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1577: savePreviousBorderBoxSizeIfNeeded();
Does the ordering of these two methods matter? In the old version we invalidated
children and then savePreviousBorderBoxSizeIfNeeded().

In the new version we do the opposite.

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:3397: // Make sure our parent is marked
as child needing invalidation.
as child needing full paint invalidation

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:850: // For now RenderBoxes and
RenderSVGModelObjects know how to invalidate themselves in
invalidateTreeIfNeeded.
This makes it sound like we're going to make all objects know how to invalidate
themselves? bug #?

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:852: bool
invalidatesSelfDuringTreeInvalidation() const { return isBox() || isSVG(); }
canSelfInvalidate?

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1037: bool
shouldCheckForSelfPaintInvalidation() const
The naming of these two methods doesn't seem quite clear to me.

It sounds like shoudlCheckForPaintInvalidtion is
shouldCheckforSelfOrChildPaintInvalidation() and
shouldCheckForSelfPaintInvalidation() is just shouldCheckForPaintInvalidation()

[Xianzhu, 2014-07-17 19:12:54]

On 2014/07/17 18:36:02, dsinclair wrote:
> Looking through the results, it looks like
> svg/repaint/remove-background-property-on-root.html maybe a real failure (no
> invalidation rects were issued).
> 

Thanks for catching this! Will look into this asap.

Will address other comments after fixing this regression.

[Xianzhu, 2014-07-17 21:57:35]

PTAL.

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (left):

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1577: savePreviousBorderBoxSizeIfNeeded();
On 2014/07/17 18:36:01, dsinclair wrote:
> Does the ordering of these two methods matter? In the old version we
invalidated
> children and then savePreviousBorderBoxSizeIfNeeded().
> 
> In the new version we do the opposite.

They are actually equivalent. There were two calls to
savePreviousBorderBoxSizeIfNeeded(). This one was needed because of the early
return. I extracted the logic invalidating the object itself into the new
invalidatePaintIfNeeded to avoid the duplicated code in early returns.

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:3397: // Make sure our parent is marked
as child needing invalidation.
On 2014/07/17 18:36:01, dsinclair wrote:
> as child needing full paint invalidation

Updated.

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:850: // For now RenderBoxes and
RenderSVGModelObjects know how to invalidate themselves in
invalidateTreeIfNeeded.
On 2014/07/17 18:36:01, dsinclair wrote:
> This makes it sound like we're going to make all objects know how to
invalidate
> themselves? bug #?

Updated crbug.com/394133 and added link here.

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:852: bool
invalidatesSelfDuringTreeInvalidation() const { return isBox() || isSVG(); }
On 2014/07/17 18:36:01, dsinclair wrote:
> canSelfInvalidate?

As the object just don't self invalidate during invalidateTreeIfNeeded(), but
can self invalidate in paintInvalidationForWholeRenderer(), I'd like to use
canSelfInvalidateDuringTreeInvalidation().

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1037: bool
shouldCheckForSelfPaintInvalidation() const
On 2014/07/17 18:36:01, dsinclair wrote:
> The naming of these two methods doesn't seem quite clear to me.
> 
> It sounds like shoudlCheckForPaintInvalidtion is
> shouldCheckforSelfOrChildPaintInvalidation() and
> shouldCheckForSelfPaintInvalidation() is just
shouldCheckForPaintInvalidation()

How about shouldCheckForTreePaintInvalidation() and
shouldCheckForSelfPaintInvalidation()?

[dsinclair, 2014-07-18 14:15:04]

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1037: bool
shouldCheckForSelfPaintInvalidation() const
On 2014/07/17 21:57:34, Xianzhu wrote:
> On 2014/07/17 18:36:01, dsinclair wrote:
> > The naming of these two methods doesn't seem quite clear to me.
> > 
> > It sounds like shoudlCheckForPaintInvalidtion is
> > shouldCheckforSelfOrChildPaintInvalidation() and
> > shouldCheckForSelfPaintInvalidation() is just
> shouldCheckForPaintInvalidation()
> 
> How about shouldCheckForTreePaintInvalidation() and
> shouldCheckForSelfPaintInvalidation()?


My preference would be to get the word Tree out of there. The fact that we check
this during a tree walk is irrelevant to what it's actually doing.

shouldCheckForAncestorOrSelfPaintInvalidation()?

The second case, I think the self is redundant since you're calling the method
on yourself already.

[Xianzhu, 2014-07-18 17:15:22]

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/398343003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:1037: bool
shouldCheckForSelfPaintInvalidation() const
On 2014/07/18 14:15:04, dsinclair wrote:
> On 2014/07/17 21:57:34, Xianzhu wrote:
> > On 2014/07/17 18:36:01, dsinclair wrote:
> > > The naming of these two methods doesn't seem quite clear to me.
> > > 
> > > It sounds like shoudlCheckForPaintInvalidtion is
> > > shouldCheckforSelfOrChildPaintInvalidation() and
> > > shouldCheckForSelfPaintInvalidation() is just
> > shouldCheckForPaintInvalidation()
> > 
> > How about shouldCheckForTreePaintInvalidation() and
> > shouldCheckForSelfPaintInvalidation()?
> 
> 
> My preference would be to get the word Tree out of there. The fact that we
check
> this during a tree walk is irrelevant to what it's actually doing.
> 
> shouldCheckForAncestorOrSelfPaintInvalidation()?
> 
> The second case, I think the self is redundant since you're calling the method
> on yourself already.

Agreed. Done.

[dsinclair, 2014-07-18 17:16:08]

lgtm

[Julien - ping for review, 2014-07-18 18:49:44]

I don't think this change - or anything that makes our invalidation code more
complex - should land before we enforce that we correctly clear the
paintInvalidation bits (https://codereview.chromium.org/389573008).

This change looks awfully complicated and seems to fix several more bugs in our
code but doesn't mention them so not lgtm. This should be split.

https://codereview.chromium.org/398343003/diff/50001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/398343003/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:3401: void
RenderObject::setShouldDoFullPaintInvalidation(bool b)
I really think we should split the setting from the clearing, which would remove
this boolean.

https://codereview.chromium.org/398343003/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:3408: for (RenderObject* p = parent(); p
&& !p->shouldCheckForSelfOrChildPaintInvalidation(); p = p->parent())
This is wrong for 2 reasons:
- We shouldn't walk the parent tree to invalidate, we should walk the containing
block chain :(
- We may end up calling that function during paintInvalidation (see
RenderBox::invalidateTreeIfNeeded), which will leave renderers marked for
invalidation for the next phase.

[Xianzhu, 2014-07-18 19:05:57]

On 2014/07/18 18:49:44, Julien Chaffraix - PST wrote:
> I don't think this change - or anything that makes our invalidation code more
> complex - should land before we enforce that we correctly clear the
> paintInvalidation bits (https://codereview.chromium.org/389573008).
>

Thank for the link. I should have noticed that change earlier. Definitely this
change should be based on that one.

[Xianzhu, 2014-07-23 17:47:31]

Uploaded Patch Set 5 which is based on the following split changes:
- https://codereview.chromium.org/403033002/ (submitted)
- https://codereview.chromium.org/401883004/ (submitted)
- https://codereview.chromium.org/408273003/ (pending review)
- https://codereview.chromium.org/403593005/ (pending review)

PTAL.

[Xianzhu, 2014-08-06 19:21:56]

Replaced by https://codereview.chromium.org/443123003/ and
https://codereview.chromium.org/448793002/.