Reduce invalidation on children-needs-layout containers

Reduce invalidation on children-needs-layout containers
    
The concept of this patch is to detect cases where containers
don't need to generate invalidation because they don't need
layout themselves (only need layout because of their children)
and don't have anything that warrant an invalidation (e.g. a
order, an outline, a background, basically anything that would
require the difference in size to be invalidated).
    
This change removes a lot of invalidation in our tests (yay!),
a good chunk is due to not invalidating when body's size
change but there are still some room for improvements.
    
The new test is a variation of table-section-repaint.html
that checks that we correctly handle block flow shifts.
    
BUG=313447
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177329

[Julien - ping for review, 2014-03-19 17:35:44]

We discussed this approach with Eric based on an earlier patch
(https://codereview.chromium.org/171843005). The visual results are still
correct and this removes quite some invalidation.

[leviw_travelin_and_unemployed, 2014-03-19 17:47:31]

https://codereview.chromium.org/204843002/diff/1/Source/core/rendering/Layout...
File Source/core/rendering/LayoutRepainter.cpp (right):

https://codereview.chromium.org/204843002/diff/1/Source/core/rendering/Layout...
Source/core/rendering/LayoutRepainter.cpp:58: if
(!m_object.needsLayoutBecauseOfChildren() && m_object.hasNothingToPaint())
Why bother with the hasNothingToPaint bitfield instead of just querying the
style here?

[Julien - ping for review, 2014-03-20 01:30:32]

https://codereview.chromium.org/204843002/diff/1/Source/core/rendering/Layout...
File Source/core/rendering/LayoutRepainter.cpp (right):

https://codereview.chromium.org/204843002/diff/1/Source/core/rendering/Layout...
Source/core/rendering/LayoutRepainter.cpp:58: if
(!m_object.needsLayoutBecauseOfChildren() && m_object.hasNothingToPaint())
On 2014/03/19 17:47:32, Levi wrote:
> Why bother with the hasNothingToPaint bitfield instead of just querying the
> style here?

Very good point, sir! Changed.

[leviw_travelin_and_unemployed, 2014-03-20 21:12:13]

LGTM with one nit

https://codereview.chromium.org/204843002/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/204843002/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:428: crbug.com/313447 [ Mac ]
fast/repaint/table-cell-move.html [ NeedsRebaseline ]
Why just Mac? Font size differences?

https://codereview.chromium.org/204843002/diff/20001/Source/core/rendering/La...
File Source/core/rendering/LayoutRepainter.cpp (right):

https://codereview.chromium.org/204843002/diff/20001/Source/core/rendering/La...
Source/core/rendering/LayoutRepainter.cpp:60: // FIXME: We disable the
optimization for SVG renderers for now.
Looking at this and seeing a FIXME about an optimization doesn't immediately
tell me what the optimization is. Instead I have to parse through what's being
checked. How about a static inline method or something that gives this
optimization a name? :)

[Julien - ping for review, 2014-03-20 22:25:32]

https://codereview.chromium.org/204843002/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/204843002/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:428: crbug.com/313447 [ Mac ]
fast/repaint/table-cell-move.html [ NeedsRebaseline ]
On 2014/03/20 21:12:13, Levi wrote:
> Why just Mac? Font size differences? 

That and theme differences (for form control it should matter). I just based
these on the new baselines (if they are win/linux only, we need one for mac
too).

https://codereview.chromium.org/204843002/diff/20001/Source/core/rendering/La...
File Source/core/rendering/LayoutRepainter.cpp (right):

https://codereview.chromium.org/204843002/diff/20001/Source/core/rendering/La...
Source/core/rendering/LayoutRepainter.cpp:60: // FIXME: We disable the
optimization for SVG renderers for now.
On 2014/03/20 21:12:13, Levi wrote:
> Looking at this and seeing a FIXME about an optimization doesn't immediately
> tell me what the optimization is. Instead I have to parse through what's being
> checked. How about a static inline method or something that gives this
> optimization a name? :)

Done, named it skipInvalidationWhenLayingOutChildren().

[pdr., 2014-03-21 00:25:06]

LGTM 2

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/La...
File Source/core/rendering/LayoutRepainter.cpp (right):

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/La...
Source/core/rendering/LayoutRepainter.cpp:59: // FIXME: We don't skip
invalidations for SVG renderers for now.
This is fine, we'll never need this.

[eseidel, 2014-03-21 00:37:09]

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/La...
File Source/core/rendering/LayoutRepainter.cpp (right):

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/La...
Source/core/rendering/LayoutRepainter.cpp:54: bool
LayoutRepainter::skipInvalidationWhenLayingOutChildren() const
I would have pushed this check onto the RenderObject itself.

I'm also surprised that this is only useful in the children-changed case. A
similar "is self painting" check could be used to avoid repaints for empty divs
changing size, etc.

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

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:530: bool needsLayoutBecauseOfChildren()
const { return needsLayout() && !selfNeedsLayout() &&
!needsPositionedMovementLayout() && !needsSimplifiedNormalFlowLayout(); }
Why the needsLayout() check?  Doesn't that just check all these bools?  Don't
you want to just check the last 3?

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:808: bool hasVisualOverflowingEffect()
const { return boxShadow() || hasBorderImageOutsets() || hasOutline(); }
Love it.  Reflection?  Presumably this is also only self-painting effects, since
you can have visual/layout overflowing kids who paint of course.

[Julien - ping for review, 2014-03-24 18:00:01]

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/La...
File Source/core/rendering/LayoutRepainter.cpp (right):

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/La...
Source/core/rendering/LayoutRepainter.cpp:54: bool
LayoutRepainter::skipInvalidationWhenLayingOutChildren() const
On 2014/03/21 00:37:09, eseidel wrote:
> I would have pushed this check onto the RenderObject itself.
> 
> I'm also surprised that this is only useful in the children-changed case. A
> similar "is self painting" check could be used to avoid repaints for empty
divs
> changing size, etc.

That wouldn't work currently as changing size triggers a full layout
(selfNeedsLayout() == true) so we have to be conservative. This is one of the
reasons I am advocating for more granular layout phases.

If we had a way of detecting this case, then yes this code should be reused as
you mentioned. I will add a FIXME about that.

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/La...
Source/core/rendering/LayoutRepainter.cpp:59: // FIXME: We don't skip
invalidations for SVG renderers for now.
On 2014/03/21 00:25:06, pdr wrote:
> This is fine, we'll never need this.

Done.

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

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:530: bool needsLayoutBecauseOfChildren()
const { return needsLayout() && !selfNeedsLayout() &&
!needsPositionedMovementLayout() && !needsSimplifiedNormalFlowLayout(); }
On 2014/03/21 00:37:09, eseidel wrote:
> Why the needsLayout() check?  Doesn't that just check all these bools?  Don't
> you want to just check the last 3?

The logic is fine AFAICT. This is similar to needsPositionedMovementLayoutOnly()
: you check that a renderer needs layout but not for the it-needs-layout-itself
cases. I could make this more explicit by replacing needsLayout() with:
normalChildNeedsLayout() || posChildNeedsLayout().

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/204843002/diff/40001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:808: bool hasVisualOverflowingEffect()
const { return boxShadow() || hasBorderImageOutsets() || hasOutline(); }
On 2014/03/21 00:37:09, eseidel wrote:
> Love it.  Reflection?  Presumably this is also only self-painting effects,
since
> you can have visual/layout overflowing kids who paint of course.

Currently no renderer knows about reflections as they are handled in
self-painting layers (ie out of the normal overflow tracking logic). Your
suggestion makes total sense but it doesn't work with the current code.

[eseidel, 2014-05-30 00:19:44]

Do you still want this change Julien?  Levi is probably your best reviewer atm.

[Julien - ping for review, 2014-06-12 17:31:53]

On 2014/05/30 at 00:19:44, eseidel wrote:
> Do you still want this change Julien?  Levi is probably your best reviewer
atm.

Sorry for the slowness, the answer is yes. However it needs to be updated to
work on top of repaint-after-layout as this is the way forward.

[leviw_travelin_and_unemployed, 2014-06-13 00:03:43]

LGTM! \o/ )'( \o/

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

https://codereview.chromium.org/204843002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1362: // FIXME: Branch?
Can you be a *little* more descriptive here? Are you only wondering if this
branch is necessary?

[dsinclair, 2014-06-25 12:51:16]

lgtm

https://codereview.chromium.org/204843002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderTable.cpp (right):

https://codereview.chromium.org/204843002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTable.cpp:385: if
(caption->checkForPaintInvalidationDuringLayout())
This branch can go away as checkForPaintInvalidationDuringLayout would always
return false with RAL enabled.

[Julien - ping for review, 2014-06-30 22:36:30]

Finally got to rebaseline this beast, thanks for the help folks!

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

https://codereview.chromium.org/204843002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1362: // FIXME: Branch?
On 2014/06/13 00:03:42, leviw wrote:
> Can you be a *little* more descriptive here? Are you only wondering if this
> branch is necessary?

This wasn't meant for public consumption but I forgot about this FIXME :(

https://codereview.chromium.org/204843002/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderTable.cpp (right):

https://codereview.chromium.org/204843002/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderTable.cpp:385: if
(caption->checkForPaintInvalidationDuringLayout())
On 2014/06/25 12:51:16, dsinclair wrote:
> This branch can go away as checkForPaintInvalidationDuringLayout would always
> return false with RAL enabled.

done!

[Julien - ping for review, 2014-06-30 22:38:19]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-06-30 22:38:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/204843002/120001

[commit-bot: I haz the power, 2014-06-30 23:57:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13662)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/14688)

[commit-bot: I haz the power, 2014-06-30 23:58:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-30 23:58:50]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13673)

[Julien - ping for review, 2014-07-01 00:59:40]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-07-01 01:00:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/204843002/140001

[commit-bot: I haz the power, 2014-07-01 01:47:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8980)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13680)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/14715)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/23451)

[commit-bot: I haz the power, 2014-07-01 01:49:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-01 01:49:22]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13688)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/23469)

[Julien - ping for review, 2014-07-01 16:54:47]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-07-01 16:55:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/204843002/160001

[Julien - ping for review, 2014-07-01 17:59:33]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-07-01 17:59:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/204843002/180001

[commit-bot: I haz the power, 2014-07-01 19:07:35]

Change committed as 177329