Mark when we may have been invalidated to early out on repaintTreeAfterLayout.

Mark when we may have been invalidated to early out on repaintTreeAfterLayout.

This CL adds in a flag to mark if we may have been invalidated. If the flag is
set we will propagate the flag up to our parents. This allows us to early
out of the repaintTreeAfterLayout walk if at any point the node does not
need to be invalidated.

This improves the performance of the
large-table-with-collapsed-borders-and-colspans-wider-than-table.html test
by ~17% and removes some over-invalidations that were output.

I visually verified the items in TestExpectations to make sure they matched.

BUG=320139, 368739
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173348

[dsinclair, 2014-05-02 16:56:53]

PTAL.

[fs, 2014-05-02 17:21:41]

I was wondering when this change would show up =). Looks a bit "sprinkly"
though... (can't think of a non-sprinkly way ATM though.)

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

https://codereview.chromium.org/264963004/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1523: if
(!child->isOutOfFlowPositioned())
Could the children be "gated" here instead? Won't matter much for non-wide trees
- and probably only micro-effect on wide trees (SVG DOMs are often wide.) It'll
boil down to the cost of the indirect (virtual) call. Just a note, and should
probably be backed up by numbers, so don't make too much of it - just
mentioning.

https://codereview.chromium.org/264963004/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGImage.cpp (right):

https://codereview.chromium.org/264963004/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGImage.cpp:98: setMayNeedInvalidation(true);
When I see "sprinkles" in this RenderObject, I expect corresponding sprinkles in
RenderSVGShape. (With the gating the way it is, not setting this flag to true
will end up not clearing repaint-state so it seems it could be bad to leave it
unset?)

[Julien - ping for review, 2014-05-02 17:27:06]

As discussed this is not cool to add another boolean to be set up for EACH
layout. We should use layoutDidCalled whenever possible and file a bug for each
cases where we need an extra invalidation.

[dsinclair, 2014-05-02 19:18:52]

PTAL. Using layoutDidGet called along with the new flag to make this less
invasive.

https://codereview.chromium.org/264963004/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGImage.cpp (right):

https://codereview.chromium.org/264963004/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGImage.cpp:98: setMayNeedInvalidation(true);
On 2014/05/02 17:21:42, fs wrote:
> When I see "sprinkles" in this RenderObject, I expect corresponding sprinkles
in
> RenderSVGShape. (With the gating the way it is, not setting this flag to true
> will end up not clearing repaint-state so it seems it could be bad to leave it
> unset?)


De-sprinkled and piggy-backed on layoutWasCalled.

[leviw_travelin_and_unemployed, 2014-05-05 18:52:32]

https://codereview.chromium.org/264963004/diff/150001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/box-inline-resize.html (right):

https://codereview.chromium.org/264963004/diff/150001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/box-inline-resize.html:4: <script
src="../../resources/run-after-display.js"></script>
It's odd that this test change is included. At least add a comment about why
you're bundling them in the description.

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:369: if (!checkInvalidations)
What does the call to RenderBox::repaintTreeAfterLayout get us in the case that
checkInvalidations was false? Won't it bail early as well?

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:998: 
Nit: empty line.

[Julien - ping for review, 2014-05-05 19:02:34]

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:998: 
Unneeded space.

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1019: // Make sure our parent is marked as
needing invalidation.
Interesting, the needs layout logic walks up the tree, marking as it goes, until
it find an ancestor needing layout.

Here we only need to mark the parent, which means that we don't forget renderers
deep into the tree. Either that or we don't have coverage for it.

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1024: bool shouldCheckForInvalidations()
I may be better to use shouldCheckForInvalidationAfterLayout() as we still have
the old non-repaint-after-layout code.

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1144: , m_mayNeedInvalidation(false)
I think we should have a FIXME to remove this boolean with either a meta bug
(preferred) or pointing at the issues blocking that now (we want an actionable
FIXME)

[dsinclair, 2014-05-05 19:38:25]

PTAL. The patchset 10 has the try bots running without the TestExpectation
changes so you'll be able to see the result differences when they complete.

https://codereview.chromium.org/264963004/diff/150001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/box-inline-resize.html (right):

https://codereview.chromium.org/264963004/diff/150001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/box-inline-resize.html:4: <script
src="../../resources/run-after-display.js"></script>
On 2014/05/05 18:52:32, leviw wrote:
> It's odd that this test change is included. At least add a comment about why
> you're bundling them in the description.

Removed, isn't relevant to this CL.

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:369: if (!checkInvalidations)
On 2014/05/05 18:52:32, leviw wrote:
> What does the call to RenderBox::repaintTreeAfterLayout get us in the case
that
> checkInvalidations was false? Won't it bail early as well?

Done.

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:998: 
On 2014/05/05 19:02:35, Julien Chaffraix - PST wrote:
> Unneeded space.

Done.

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:998: 
On 2014/05/05 18:52:32, leviw wrote:
> Nit: empty line.

Done.

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1019: // Make sure our parent is marked as
needing invalidation.
On 2014/05/05 19:02:35, Julien Chaffraix - PST wrote:
> Interesting, the needs layout logic walks up the tree, marking as it goes,
until
> it find an ancestor needing layout.
> 
> Here we only need to mark the parent, which means that we don't forget
renderers
> deep into the tree. Either that or we don't have coverage for it.


Not sure what I need to do for this? We mark our parent, and the parent may mark
it's ancestor. But, as soon as we hit an ancestor that is marked we know we're
done.

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1024: bool shouldCheckForInvalidations()
On 2014/05/05 19:02:35, Julien Chaffraix - PST wrote:
> I may be better to use shouldCheckForInvalidationAfterLayout() as we still
have
> the old non-repaint-after-layout code.

Done.

https://codereview.chromium.org/264963004/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1144: , m_mayNeedInvalidation(false)
On 2014/05/05 19:02:35, Julien Chaffraix - PST wrote:
> I think we should have a FIXME to remove this boolean with either a meta bug
> (preferred) or pointing at the issues blocking that now (we want an actionable
> FIXME)

Done.

[leviw_travelin_and_unemployed, 2014-05-05 21:31:43]

This LGTM.

[dsinclair, 2014-05-05 23:14:59]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2014-05-05 23:15:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dsinclair@chromium.org/264963004/190001

[commit-bot: I haz the power, 2014-05-05 23:28:39]

Change committed as 173348