Make LayoutState always be RAII

Make LayoutState always be RAII

Repaint after layout will eventually do a tree walk that
utilizes LayoutState (or more accurately, something *like*
LayoutState) to quickly compute repaint rects while walking
the tree. In the course of building that functionality, I
got mired in the ugliness of LayoutState.

LayoutStateMaintainer is an RAII that is partially broken
due to the need to explicitly call "pop". RenderView would
explicitly create and initialize a LayoutState on the stack
then clear the pointer after layout. This patch fixes the
LayoutStateMaintainer issue (which necessitated some
refactoring in the rendering code) and introduces a
RootLayoutStateScope that takes care of RenderView's case.

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

[leviw_travelin_and_unemployed, 2014-03-12 23:40:21]

This ends up being a far bigger patch than I expected. Let me know what you
think.

[Julien - ping for review, 2014-03-13 17:44:27]

I like this change overall! The table part is a bit too awkward for my taste but
I can't think of a better solution (unless we just make the
LayoutStateMaintainer scoped like some other classes).

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

https://codereview.chromium.org/196533012/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderDeprecatedFlexibleBox.cpp:301:
updateLayerTransform();
Interestingly updateLayerTransform was out of the LayoutStateMaintainer scope
before this change and still is in some other classes so I would keep it that
way.

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

https://codereview.chromium.org/196533012/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderTable.cpp:574: LayoutUnit movedSectionLogicalTop =
0;
Really not a huge fan of these variables (along with the side effect logic in
layoutContents).

It's a pity we can't just move the repainting logic inside layoutContent as it
would this awkwardness.

[leviw_travelin_and_unemployed, 2014-03-14 22:37:00]

On 2014/03/13 17:44:27, Julien Chaffraix - PST wrote:
> I like this change overall! The table part is a bit too awkward for my taste
but
> I can't think of a better solution (unless we just make the
> LayoutStateMaintainer scoped like some other classes).
> 
>
https://codereview.chromium.org/196533012/diff/40001/Source/core/rendering/Re...
> File Source/core/rendering/RenderDeprecatedFlexibleBox.cpp (right):
> 
>
https://codereview.chromium.org/196533012/diff/40001/Source/core/rendering/Re...
> Source/core/rendering/RenderDeprecatedFlexibleBox.cpp:301:
> updateLayerTransform();
> Interestingly updateLayerTransform was out of the LayoutStateMaintainer scope
> before this change and still is in some other classes so I would keep it that
> way.
> 
>
https://codereview.chromium.org/196533012/diff/40001/Source/core/rendering/Re...
> File Source/core/rendering/RenderTable.cpp (right):
> 
>
https://codereview.chromium.org/196533012/diff/40001/Source/core/rendering/Re...
> Source/core/rendering/RenderTable.cpp:574: LayoutUnit movedSectionLogicalTop =
> 0;
> Really not a huge fan of these variables (along with the side effect logic in
> layoutContents).
> 
> It's a pity we can't just move the repainting logic inside layoutContent as it
> would this awkwardness.

PTAL :)

[leviw_travelin_and_unemployed, 2014-03-17 17:34:24]

On 2014/03/14 22:37:00, Levi wrote:
> PTAL :)

Ping! :)

[eseidel, 2014-03-17 17:45:27]

lgtm

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

https://codereview.chromium.org/196533012/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1645: if (needsPositionedMovementLayout()
&& !tryLayoutDoingPositionedMovementOnly())
Rietveld's diff display here is awful.

[leviw_travelin_and_unemployed, 2014-03-17 17:59:22]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-03-17 17:59:28]

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

[commit-bot: I haz the power, 2014-03-17 18:00:57]

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

[commit-bot: I haz the power, 2014-03-17 18:00:58]

Failed to apply patch for Source/core/rendering/RenderBlockFlow.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderBlockFlow.cpp
  Hunk #5 FAILED at 446.
  1 out of 5 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderBlockFlow.cpp.rej

Patch:       Source/core/rendering/RenderBlockFlow.cpp
Index: Source/core/rendering/RenderBlockFlow.cpp
diff --git a/Source/core/rendering/RenderBlockFlow.cpp
b/Source/core/rendering/RenderBlockFlow.cpp
index
26ea3a01ef3d8cc6dca64649bb1e8fafde074f0e..7c643bd9c47bc9494fc2aa80da921f565d011aea
100644
--- a/Source/core/rendering/RenderBlockFlow.cpp
+++ b/Source/core/rendering/RenderBlockFlow.cpp
@@ -301,14 +301,35 @@ void RenderBlockFlow::layoutBlock(bool relayoutChildren)
     // number of columns.
     bool done = false;
     LayoutUnit pageLogicalHeight = 0;
+    LayoutRepainter repainter(*this, checkForRepaintDuringLayout());
     while (!done)
         done = layoutBlockFlow(relayoutChildren, pageLogicalHeight,
layoutScope);
+
+    fitBorderToLinesIfNeeded();
+
+    RenderView* renderView = view();
+    if (renderView->layoutState()->m_pageLogicalHeight)
+       
setPageLogicalOffset(renderView->layoutState()->pageLogicalOffset(*this,
logicalTop()));
+
+    updateLayerTransform();
+
+    // Update our scroll information if we're overflow:auto/scroll/hidden now
that we know if
+    // we overflow or not.
+    updateScrollInfoAfterLayout();
+
+    // Repaint with our new bounds if they are different from our old bounds.
+    bool didFullRepaint = repainter.repaintAfterLayout();
+    if (!didFullRepaint && m_repaintLogicalTop != m_repaintLogicalBottom &&
(style()->visibility() == VISIBLE || enclosingLayer()->hasVisibleContent())) {
+        if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled())
+            setShouldRepaintOverflowIfNeeded(true);
+        else
+            repaintOverflow();
+    }
+    clearNeedsLayout();
 }
 
 inline bool RenderBlockFlow::layoutBlockFlow(bool relayoutChildren, LayoutUnit
&pageLogicalHeight, SubtreeLayoutScope& layoutScope)
 {
-    LayoutRepainter repainter(*this, checkForRepaintDuringLayout());
-
     LayoutUnit oldLeft = logicalLeft();
     if (updateLogicalWidthAndColumnWidth())
         relayoutChildren = true;
@@ -368,7 +389,6 @@ inline bool RenderBlockFlow::layoutBlockFlow(bool
relayoutChildren, LayoutUnit &
     if (RenderMultiColumnFlowThread* flowThread = multiColumnFlowThread()) {
         if (flowThread->recalculateColumnHeights()) {
             setChildNeedsLayout(MarkOnlyThis);
-            statePusher.pop();
             return false;
         }
     } else if (hasColumns()) {
@@ -381,7 +401,6 @@ inline bool RenderBlockFlow::layoutBlockFlow(bool
relayoutChildren, LayoutUnit &
         m_overflow = savedOverflow.release();
 
         if (!hasSpecifiedPageLogicalHeight &&
shouldRelayoutForPagination(pageLogicalHeight, layoutOverflowLogicalBottom)) {
-            statePusher.pop();
             setEverHadLayout(true);
             return false;
         }
@@ -390,7 +409,6 @@ inline bool RenderBlockFlow::layoutBlockFlow(bool
relayoutChildren, LayoutUnit &
     }
 
     if (shouldBreakAtLineToAvoidWidow()) {
-        statePusher.pop();
         setEverHadLayout(true);
         return false;
     }
@@ -428,30 +446,6 @@ inline bool RenderBlockFlow::layoutBlockFlow(bool
relayoutChildren, LayoutUnit &
     // Add overflow from children (unless we're multi-column, since in that
case all our child overflow is clipped anyway).
     computeOverflow(oldClientAfterEdge);
 
-    statePusher.pop();
-
-    fitBorderToLinesIfNeeded();
-
-    RenderView* renderView = view();
-    if (renderView->layoutState()->m_pageLogicalHeight)
-       
setPageLogicalOffset(renderView->layoutState()->pageLogicalOffset(*this,
logicalTop()));
-
-    updateLayerTransform();
-
-    // Update our scroll information if we're overflow:auto/scroll/hidden now
that we know if
-    // we overflow or not.
-    updateScrollInfoAfterLayout();
-
-    // Repaint with our new bounds if they are different from our old bounds.
-    bool didFullRepaint = repainter.repaintAfterLayout();
-    if (!didFullRepaint && m_repaintLogicalTop != m_repaintLogicalBottom &&
(style()->visibility() == VISIBLE || enclosingLayer()->hasVisibleContent())) {
-        if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled())
-            setShouldRepaintOverflowIfNeeded(true);
-        else
-            repaintOverflow();
-    }
-
-    clearNeedsLayout();
     return true;
 }

[leviw_travelin_and_unemployed, 2014-03-17 18:39:25]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-03-17 18:39:28]

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

[commit-bot: I haz the power, 2014-03-17 18:48:52]

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

[commit-bot: I haz the power, 2014-03-17 18:48:53]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[leviw_travelin_and_unemployed, 2014-03-17 19:20:24]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-03-17 19:20:33]

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

[commit-bot: I haz the power, 2014-03-17 19:51:38]

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

[commit-bot: I haz the power, 2014-03-17 19:51:39]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[leviw_travelin_and_unemployed, 2014-03-17 21:54:00]

On 2014/03/17 19:51:39, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.blink on win_blink_rel

LayoutState scoping was wrong on FlexBox... Tests caught it (yay!). Should be
good now. PTAL.

[leviw_travelin_and_unemployed, 2014-03-20 01:06:23]

Ping :)

[esprehn, 2014-03-20 01:10:54]

lgtm, some of these nested brace scopes you added are really long. We should
break them up into smaller function calls in the future.

https://codereview.chromium.org/196533012/diff/130001/Source/core/rendering/R...
File Source/core/rendering/RenderDeprecatedFlexibleBox.cpp (right):

https://codereview.chromium.org/196533012/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderDeprecatedFlexibleBox.cpp:263: //
LayoutStateMaintainer needs this deliberate scope to pop before repaint
We might try refactoring these functions so that we don't need these big scope
blocks.

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

https://codereview.chromium.org/196533012/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderTable.cpp:440: 
This scope is soooo long. This really needs to be made into smaller functions.

[leviw_travelin_and_unemployed, 2014-03-20 17:51:17]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-03-20 17:51:22]

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

[commit-bot: I haz the power, 2014-03-20 17:51:34]

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

[commit-bot: I haz the power, 2014-03-20 17:51:35]

Failed to apply patch for Source/core/rendering/RenderBox.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderBox.cpp
  Hunk #1 FAILED at 314.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/rendering/RenderBox.cpp.rej

Patch:       Source/core/rendering/RenderBox.cpp
Index: Source/core/rendering/RenderBox.cpp
diff --git a/Source/core/rendering/RenderBox.cpp
b/Source/core/rendering/RenderBox.cpp
index
52f8ba5ec029f413402c967e5a32e526e52294ca..c97cc6eae13924abbd05eab63e2861b6c3806aa6
100644
--- a/Source/core/rendering/RenderBox.cpp
+++ b/Source/core/rendering/RenderBox.cpp
@@ -314,7 +314,6 @@ void RenderBox::layout()
         ASSERT(!child->needsLayout());
         child = child->nextSibling();
     }
-    statePusher.pop();
     invalidateBackgroundObscurationStatus();
     clearNeedsLayout();
 }

[leviw_travelin_and_unemployed, 2014-03-20 18:12:55]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:13:10]

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

[commit-bot: I haz the power, 2014-03-20 19:16:38]

Change committed as 169676