Common invalidateTreeIfNeeded()

Common invalidateTreeIfNeeded()

Extract common part of invalidateTreeIfNeeded() of
RenderLayerModelObject and RenderSVGModelObject.

This is preparation of letting setShouldDoFullPaintInvalidation() work
for RenderText (to avoid invalidatePaintForWholeRenderer from
DocumentMarkerController).

No functional change.

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

[Xianzhu, 2014-09-02 22:31:51]

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

[Xianzhu, 2014-09-02 22:34:20]

Patchset #2 (id:20001) has been deleted

[Xianzhu, 2014-09-02 22:34:26]

Patchset #1 (id:1) has been deleted

[dsinclair, 2014-09-03 14:14:47]

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

https://codereview.chromium.org/535633002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1649: // ASSERT(newBounds ==
boundsRectForPaintInvalidation(&newPaintInvalidationContainer, 0));
Why did the ASSERT change? previously we passed the paintInvalidationState into
boundsRectForPaintInvalidation and it was enabled.

You've removed the invalidation state and commented out the ASSERT?

https://codereview.chromium.org/535633002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/535633002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1145: void invalidateSubtreesIfNeeded(const
PaintInvalidationState&);
invalidatePaintOfSubtreesIfNeeded

[Xianzhu, 2014-09-03 17:11:15]

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

https://codereview.chromium.org/535633002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1649: // ASSERT(newBounds ==
boundsRectForPaintInvalidation(&newPaintInvalidationContainer, 0));
On 2014/09/03 14:14:47, dsinclair wrote:
> Why did the ASSERT change? previously we passed the paintInvalidationState
into
> boundsRectForPaintInvalidation and it was enabled.
> 
> You've removed the invalidation state and commented out the ASSERT?

Based on the FIXME, I think the original intent of the ASSERT was to check if
boundsRectForPaintInvalidation() returns the same value with or without
paintInvalidationState, but for the mentioned reasons, we can't do it for now.

Because newBounds has just been set to
boundsRectForPaintInvalidation(&newPaintInvalidationContainer,
&paintInvalidationState) above in this change, it seems meaningless to assert if
they equal, so I changed the assert to meet its original intent but commented it
out because of the reasons mentioned in the FIXME.

https://codereview.chromium.org/535633002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/535633002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1145: void invalidateSubtreesIfNeeded(const
PaintInvalidationState&);
On 2014/09/03 14:14:47, dsinclair wrote:
> invalidatePaintOfSubtreesIfNeeded

Done.

[dsinclair, 2014-09-03 17:44:30]

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

https://codereview.chromium.org/535633002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1649: // ASSERT(newBounds ==
boundsRectForPaintInvalidation(&newPaintInvalidationContainer, 0));
On 2014/09/03 17:11:15, Xianzhu wrote:
> On 2014/09/03 14:14:47, dsinclair wrote:
> > Why did the ASSERT change? previously we passed the paintInvalidationState
> into
> > boundsRectForPaintInvalidation and it was enabled.
> > 
> > You've removed the invalidation state and commented out the ASSERT?
> 
> Based on the FIXME, I think the original intent of the ASSERT was to check if
> boundsRectForPaintInvalidation() returns the same value with or without
> paintInvalidationState, but for the mentioned reasons, we can't do it for now.
> 
> Because newBounds has just been set to
> boundsRectForPaintInvalidation(&newPaintInvalidationContainer,
> &paintInvalidationState) above in this change, it seems meaningless to assert
if
> they equal, so I changed the assert to meet its original intent but commented
it
> out because of the reasons mentioned in the FIXME.


Ah, yea, previously we weren't setting the bounds in this method. I'll have to
defer to leviw@ on what the correct state of the ASSERT should be (if we change
it, or just remove it at this point)

[Julien - ping for review, 2014-09-04 19:48:04]

lgtm.

+Levi, you should look at the ASSERT.

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

https://codereview.chromium.org/535633002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1649: // ASSERT(newBounds ==
boundsRectForPaintInvalidation(&newPaintInvalidationContainer, 0));
The ASSERT should use paintInvalidationContainer as that's the coordinate system
used for computing the bounds.

I think it's fine to remove it as it was just checking that we did set the new
bounds and now it's guaranteed.

https://codereview.chromium.org/535633002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderText.h (right):

https://codereview.chromium.org/535633002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderText.h:187: // FIXME: This is temporarily empty.
Will let it actually work in following steps.
Is that related to the work around folding RenderText invalidations into
repaint-after-compositing-updates? If that's the case, we should just mention it
along with the bug.

[leviw_travelin_and_unemployed, 2014-09-05 21:37:12]

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

https://codereview.chromium.org/535633002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1649: // ASSERT(newBounds ==
boundsRectForPaintInvalidation(&newPaintInvalidationContainer, 0));
Why newPaintInvalidationContainer? That's not actually a variable in the
function. I'm sad to see this commented out, but don't make it fail to compile.

Other than that nit, this is effectively what we want from the assert -- to test
that we're generating the same invalidation bounds with and without the
paintInvalidationState. I'd lean towards keeping the assert and working towards
fixing it, but I'm also okay with removing it altogether and relying on the bug
to track it.

[Xianzhu, 2014-09-05 22:06:37]

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

https://codereview.chromium.org/535633002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1649: // ASSERT(newBounds ==
boundsRectForPaintInvalidation(&newPaintInvalidationContainer, 0));
On 2014/09/05 21:37:12, leviw wrote:
> Why newPaintInvalidationContainer? That's not actually a variable in the
> function. I'm sad to see this commented out, but don't make it fail to
compile.
> 
> Other than that nit, this is effectively what we want from the assert -- to
test
> that we're generating the same invalidation bounds with and without the
> paintInvalidationState. I'd lean towards keeping the assert and working
towards
> fixing it, but I'm also okay with removing it altogether and relying on the
bug
> to track it.

I removed it.

[Xianzhu, 2014-09-08 16:05:41]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-09-08 16:06:02]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/535633002/100001

[Xianzhu, 2014-09-08 16:46:23]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2014-09-08 16:53:38]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-09-08 16:54:25]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/535633002/100001

[commit-bot: I haz the power, 2014-09-08 19:06:17]

Committed patchset #4 (id:100001) as 181571