Divorce PaintInvalidationState from LayoutState

Divorce PaintInvalidationState from LayoutState

We don't actually benefit from most of the work LayoutState was doing to
keep track of offsets during layout. Instead, we were doing all that
additional work in both invalidation and layout but only using them for
invalidation.

The new PaintInvalidationState inherits the fast paint-offset caching
from LayoutState, but goes the extra mile to identify new repaint containers,
re-enable itself when disabled and an invalidation boundary is crossed, and
work for invalidation containers other than the root.

LayoutState was also improperly accumulating its layoutOffset using things
like scroll offsets, which resulted in incorrect rendering on
fast/multicol/shrink-to-column-height-for-pagination.html. This is fixed
with this patch because we no longer consider paint-specific offsets at
all in LayoutState.

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

[Julien - ping for review, 2014-07-02 00:27:28]

Some early comments.

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InvalidationTreeWalkState.cpp (right):

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.cpp:72: ,
m_cachedOffsetsEnabled(false)
Disabling cachedOffset as part of the constructor is a terrible idiom that's
going to cause us pain (silent slow behavior :(). I thought we had
ForceHorriblySlowRectMapping to avoid such gymnastics?

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.cpp:75: { }
Nit: 1 curly brace per line.

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InvalidationTreeWalkState.h (right):

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.h:65: return
m_cachedOffsetsEnabled && container == &m_paintInvalidationContainer;
Is it common for container != m_paintInvalidationContainer or is it just when
traversing paint container boundaries?

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/La...
File Source/core/rendering/LayoutState.h (right):

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/La...
Source/core/rendering/LayoutState.h:83: // Do not add anything apart from
bitfields until after m_columnInfo. See
https://bugs.webkit.org/show_bug.cgi?id=100173
I think this comment (and its associated review) still applies so why are we
switching from bitfields to booleans?

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

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:387: // FIXME: Teach
InvalidationTreeWalkState to handle this case.
Link to the existing bug for LayoutState.

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:405: InvalidationTreeWalkState
disabledTreeWalkState(childInvalidationTreeWalkState, *this,
repaintContainerForChild);
|disabledTreeWalkState| is unused.

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlowThread.cpp (left):

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlowThread.cpp:168: ForceHorriblySlowRectMapping
slowRectMapping(*this); // We can't use layout state to repaint, since the
regions are somewhere else.
Is this really solved?

[leviw_travelin_and_unemployed, 2014-07-02 00:41:02]

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InvalidationTreeWalkState.cpp (right):

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.cpp:72: ,
m_cachedOffsetsEnabled(false)
On 2014/07/02 00:27:28, Julien Chaffraix - PST wrote:
> Disabling cachedOffset as part of the constructor is a terrible idiom that's
> going to cause us pain (silent slow behavior :(). I thought we had
> ForceHorriblySlowRectMapping to avoid such gymnastics?

Fair enough. I'll add an assert that we never exit the function with
m_cachedOffsetsEnabled with a FIXME for SVG.

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.cpp:75: { }
On 2014/07/02 00:27:28, Julien Chaffraix - PST wrote:
> Nit: 1 curly brace per line.

Really? We have over 1000 examples if this construct in core/, but I can go that
way if you want.

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InvalidationTreeWalkState.h (right):

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.h:65: return
m_cachedOffsetsEnabled && container == &m_paintInvalidationContainer;
On 2014/07/02 00:27:28, Julien Chaffraix - PST wrote:
> Is it common for container != m_paintInvalidationContainer or is it just when
> traversing paint container boundaries?

I'd have to validate.

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/La...
File Source/core/rendering/LayoutState.h (right):

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/La...
Source/core/rendering/LayoutState.h:83: // Do not add anything apart from
bitfields until after m_columnInfo. See
https://bugs.webkit.org/show_bug.cgi?id=100173
LayoutState is no longer on the heap, so we don't really benefit much (esprehn
actually wants the bitfields to go away), but there's no reason for it to be in
this patch.

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlowThread.cpp (left):

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlowThread.cpp:168: ForceHorriblySlowRectMapping
slowRectMapping(*this); // We can't use layout state to repaint, since the
regions are somewhere else.
On 2014/07/02 00:27:28, Julien Chaffraix - PST wrote:
> Is this really solved?

You'll notice from the function prototype that we don't have an
InvalidationTreeWalkState on the stack, so we have nothing to disable.

[dsinclair, 2014-07-02 18:48:24]

https://codereview.chromium.org/360833002/diff/80001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/360833002/diff/80001/Source/core/core.gypi#ne...
Source/core/core.gypi:1505: 'rendering/InvalidationTreeWalkState.h',
Should these be below the rendering/Inline* entries?

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InvalidationTreeWalkState.cpp (right):

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.cpp:57: // m_diabled = true;
remove

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.cpp:76: // FIXME: SVG could
probably benefit from a stack-based optimization like html does.
bug?

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.cpp:151: if
(renderer.hasOverflowClip()) {
if (!renderer.hasOverflowClip())
    return;

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/In...
File Source/core/rendering/InvalidationTreeWalkState.h (right):

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.h:2: * Copyright (C) 2014 Google
Inc. All rights reserved.
There is a new short license you can use on new files.

http://www.chromium.org/developers/coding-style#TOC-File-headers

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.h:52:
InvalidationTreeWalkState(RenderObject&);
Should this have an explicit?

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

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:387: // If it's a new repaint container,
we won't have properly accumulated the offset into the
s/repaint/paint invalidation/

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:398: // a relatively positioend inline
element, we need to account for
I can't spell, can you changed positioend to positioned while you're in here?

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

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderInline.cpp:1566: const RenderLayerModelObject&
newPaintInvalidationContainer =
*adjustCompositedContainerForSpecialAncestors(establishesNewPaintInvalidationContainer
? this : &invalidationTreeWalkState.paintInvalidationContainer());
Is there any benefit to putting this into a newPaintInvalidationContainer()
method? We do it in a bunch of places.

(Not sure if it's worth it given it's 2 lines)

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

https://codereview.chromium.org/360833002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:572: // we need to disable LayoutState for
squashed layers.
Comment is out of date.

(We removed a very similar comment/ForceHorriblySlowRectMapping below, can this
one be similarly removed completely?)

[Julien - ping for review, 2014-07-07 22:10:00]

Much better, lgtm

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
File Source/core/rendering/InvalidationTreeWalkState.cpp (right):

https://codereview.chromium.org/360833002/diff/20001/Source/core/rendering/In...
Source/core/rendering/InvalidationTreeWalkState.cpp:75: { }
On 2014/07/02 00:41:02, leviw wrote:
> On 2014/07/02 00:27:28, Julien Chaffraix - PST wrote:
> > Nit: 1 curly brace per line.
> 
> Really? We have over 1000 examples if this construct in core/, but I can go
that
> way if you want.

Usually I would see that in header files (for inline functions), not in a C++
file. It's an area where our style guide is not consistent with practice anyway
that's why it's a nit.

https://codereview.chromium.org/360833002/diff/140001/Source/core/rendering/P...
File Source/core/rendering/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/360833002/diff/140001/Source/core/rendering/P...
Source/core/rendering/PaintInvalidationState.cpp:32: if
(container->isTableRow())
FIXME: Table rows should behave like everyone else :(

I wonder how many of our rectangles are wrong because very few places account
for that idiosyncrasy.

https://codereview.chromium.org/360833002/diff/140001/Source/core/rendering/P...
Source/core/rendering/PaintInvalidationState.cpp:126: const RenderBox* box =
toRenderBox(&renderer);
We generate const RenderBox& toRenderBox(const RenderObject&) so no need for
some pointer juggling.

https://codereview.chromium.org/360833002/diff/140001/Source/core/rendering/P...
File Source/core/rendering/PaintInvalidationState.h (right):

https://codereview.chromium.org/360833002/diff/140001/Source/core/rendering/P...
Source/core/rendering/PaintInvalidationState.h:47: mutable bool
m_cachedOffsetsEnabled;
Does this field have to be mutable?

https://codereview.chromium.org/360833002/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderInline.cpp (right):

https://codereview.chromium.org/360833002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderInline.cpp:1057:
rect.intersect(paintInvalidationState->clipRect());
This code should be probably factored out into the new PaintInvalidationState as
it's repeated several times (something like
paintInvalidationState->doFastMapRectToPaintInvalidationBacking()).

Or maybe we only share the paintOffset logic.

https://codereview.chromium.org/360833002/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCell.cpp (right):

https://codereview.chromium.org/360833002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderTableCell.cpp:370: // if ((!v ||
!v->canMapUsingLayoutStateForContainer(paintInvalidationContainer)) && parent())
doh'?!

[leviw_travelin_and_unemployed, 2014-07-08 00:53:46]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-07-08 00:54:31]

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

[commit-bot: I haz the power, 2014-07-08 00:58:25]

Change committed as 177621

[Yuta Kitamura, 2014-07-09 02:53:16]

A revert of this CL has been created in
https://codereview.chromium.org/377123002/ by yutak@chromium.org.

The reason for reverting is: Speculative revert -- some change between
177596:177623 made DrMemory bot angry
<http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Content%20...>.

It was not immediately obvious which change was to
blame, but this one seemed most suspicious among
others.

Let me try to revert this change and see how
it goes. If this was not the culprit, I will
revert the revert..

[Yuta Kitamura, 2014-07-09 04:31:38]

Second try of revert:
https://codereview.chromium.org/379073002/