Change LayoutState to be stack-allocated

Change LayoutState to be stack-allocated

LayoutState does 2 fundamentally different things: it acts as a quick
repaint mapping between renderers and their repaint container (currently
only the RenderView crbug.com/363834), and keeps track of various layout
tree information (like the current pagination model).

Because the fast repaint mapping doesn't work for all types of renderers,
there's a concept of LayoutState being disabled, and sometimes not 'pushed'
at all. This CL renames the disabled state, which only applies to the
repaint optimization, to layoutStateCachedOffsetsEnabled, and makes
LayoutState stack-based instead of heap-based. It now will always "push,"
and tracks whether or not the repaint optimization is disabled as part of
itself.

This is one step in the process of teaching LayoutState to work on
renderers that aren't the RenderView.

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

[esprehn, 2014-06-13 23:25:26]

You need to sync, the ASSERT_DISABLED thing is gone.

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

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/La...
Source/core/rendering/LayoutState.cpp:170: root.view()->pushLayoutState(*this);
Why does this sometimes push and sometimes set in the other constructor?

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

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:382: LayoutState statePusher(*this,
isTableRow() ? LayoutSize() : locationOffset());
we should really call these state not statePusher since it's the real state
object now.

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1579: LayoutState statePusher(*this,
locationOffset());
LayoutState state(...)

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

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderInline.cpp:1059: if
(v->canMapUsingLayoutStateForContainer(paintInvalidationContainer)) {
You could probably do this rename separately to make the patch smaller if you
want.

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.cpp:890: // FIXME: This LayoutState
invocation should be fixed for consistency.
Fixed how?

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:956: m_layoutState = &layoutState;
Can we unify the pushLayoutState and setLayoutState paths?

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

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderView.h:244: : m_didDisable(false)
m_didDisable(m_view.layoutState() &&
m_view.layoutState()->cachedOffsetsEnabled())

?

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderView.h:248: m_didDisable =
m_view.layoutState()->cachedOffsetsEnabled();
Why does cachedOffsetEnabled map to m_didDisable?

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderView.h:249:
m_view.layoutState()->m_cachedOffsetsEnabled = false;
This is weird to reaching into the LayotuState with m_cachedOffsetsEnabled.

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderView.h:251: #if !ASSERT_DISABLED
This can't land, abarth removed ASSERT_DISABLED. It's not #if ASSERT_ENABLED

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderView.h:260:
m_view.layoutState()->m_cachedOffsetsEnabled = true;
So cachedOffsetsEnabled means layoutStateEnabled?

[leviw_travelin_and_unemployed, 2014-06-13 23:46:16]

Thanks!

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

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:382: LayoutState statePusher(*this,
isTableRow() ? LayoutSize() : locationOffset());
On 2014/06/13 23:25:26, esprehn wrote:
> we should really call these state not statePusher since it's the real state
> object now.

Sure!

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:956: m_layoutState = &layoutState;
On 2014/06/13 23:25:26, esprehn wrote:
> Can we unify the pushLayoutState and setLayoutState paths?

This is actually leftover from where I broke multicolumn. setLayoutState isn't
used anymore, and shouldn't exist.

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

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderView.h:244: : m_didDisable(false)
On 2014/06/13 23:25:26, esprehn wrote:
> m_didDisable(m_view.layoutState() &&
> m_view.layoutState()->cachedOffsetsEnabled())
> 
> ?

Sure.

https://codereview.chromium.org/335963002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderView.h:260:
m_view.layoutState()->m_cachedOffsetsEnabled = true;
On 2014/06/13 23:25:26, esprehn wrote:
> So cachedOffsetsEnabled means layoutStateEnabled?

Yes, but there is no more layoutStateEnabled. LayoutState is always "enabled,"
but LayoutState::cachedOffsetsEnabled is not always true.

[leviw_travelin_and_unemployed, 2014-06-14 02:12:34]

ptal

[esprehn, 2014-06-14 03:08:27]

lgtm, couple random nits.

https://codereview.chromium.org/335963002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/335963002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1019: LayoutState rootLayoutStateScope(*root);
rootLayoutState

https://codereview.chromium.org/335963002/diff/60001/Source/core/rendering/La...
File Source/core/rendering/LayoutState.cpp (right):

https://codereview.chromium.org/335963002/diff/60001/Source/core/rendering/La...
Source/core/rendering/LayoutState.cpp:141: RenderObject* o = &renderer;
object =

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

https://codereview.chromium.org/335963002/diff/60001/Source/core/rendering/La...
Source/core/rendering/LayoutState.h:122: bool m_layoutDeltaYSaturated:1;
We should switch all these back to real bools instead of bitfields after this
lands. Since we always make these on the stack now using bitfields just makes it
slower.

[leviw_travelin_and_unemployed, 2014-06-16 19:12:18]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-06-16 19:13:04]

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

[leviw_travelin_and_unemployed, 2014-06-16 22:08:02]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-06-16 22:09:07]

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

[commit-bot: I haz the power, 2014-06-16 23:52:36]

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

[commit-bot: I haz the power, 2014-06-16 23:52:37]

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

[leviw_travelin_and_unemployed, 2014-06-17 22:06:22]

PTAL. I had to fix a couple things that somehow passed locally for me on Mac but
were borked on Linux.

[esprehn, 2014-06-17 23:26:34]

lgtm

[leviw_travelin_and_unemployed, 2014-06-17 23:27:04]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-06-17 23:27:56]

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

[commit-bot: I haz the power, 2014-06-17 23:31:21]

Change committed as 176362

[Julien - ping for review, 2014-06-17 23:34:26]

Late at the party but lgtm (with some comments)

https://codereview.chromium.org/335963002/diff/160001/Source/core/rendering/L...
File Source/core/rendering/LayoutState.cpp (right):

https://codereview.chromium.org/335963002/diff/160001/Source/core/rendering/L...
Source/core/rendering/LayoutState.cpp:58: {
Should we add ASSERT(m_renderer.view()->layoutState())? (we are accessing m_next
below so we will definitely see a crash but that should make it obvious what's
wrong)

https://codereview.chromium.org/335963002/diff/160001/Source/core/rendering/L...
Source/core/rendering/LayoutState.cpp:154: ,
m_cachedOffsetsEnabled(shouldDisableLayoutStateForSubtree(root))
That potentially makes the code O(depth_of_tree^2) :(

shouldDisableLayoutStateForSubtree should hit any previous |root| from another
LayoutState, which should prevent this performance bottleneck.

https://codereview.chromium.org/335963002/diff/160001/Source/core/rendering/L...
Source/core/rendering/LayoutState.cpp:164: // FIXME: Why does RenderTableSection
create this wonky LayoutState?
RenderTableSections are definitely not special in any way AFAICT.
RenderTableRows are the ones that are wonky (they don't push their offset to
LayoutState IIRC which is why we need a lot of checks for them).

https://codereview.chromium.org/335963002/diff/160001/Source/core/rendering/L...
Source/core/rendering/LayoutState.cpp:186: if (m_renderer.view()->layoutState())
{
Nit: 
if (LayoutState* currentLayoutState = m_renderer.view()->layoutState()) {
    ASSERT(currentLayoutState == this);
    ...
}

https://codereview.chromium.org/335963002/diff/160001/Source/core/rendering/R...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/335963002/diff/160001/Source/core/rendering/R...
Source/core/rendering/RenderView.cpp:459: LayoutState rootLayoutState(0, false,
*this);
FALSE :(

At a minimum we should say which boolean we set (/* pageLogicalHeightChanged */)

[leviw_travelin_and_unemployed, 2014-06-17 23:37:04]

https://codereview.chromium.org/335963002/diff/160001/Source/core/rendering/L...
File Source/core/rendering/LayoutState.cpp (right):

https://codereview.chromium.org/335963002/diff/160001/Source/core/rendering/L...
Source/core/rendering/LayoutState.cpp:58: {
On 2014/06/17 23:34:25, Julien Chaffraix - PST wrote:
> Should we add ASSERT(m_renderer.view()->layoutState())? (we are accessing
m_next
> below so we will definitely see a crash but that should make it obvious what's
> wrong)

Sure.

https://codereview.chromium.org/335963002/diff/160001/Source/core/rendering/L...
Source/core/rendering/LayoutState.cpp:154: ,
m_cachedOffsetsEnabled(shouldDisableLayoutStateForSubtree(root))
On 2014/06/17 23:34:26, Julien Chaffraix - PST wrote:
> That potentially makes the code O(depth_of_tree^2) :(
> 
> shouldDisableLayoutStateForSubtree should hit any previous |root| from another
> LayoutState, which should prevent this performance bottleneck.

This is only created in FrameView for a sub-tree root. This works exactly like
RootLayoutState used to work.