[New Multicolumn] Rebalance properly when child content changes.

Source/core/rendering/RenderMultiColumnSet.cpp

Index: Source/core/rendering/RenderMultiColumnSet.cpp
diff --git a/Source/core/rendering/RenderMultiColumnSet.cpp b/Source/core/rendering/RenderMultiColumnSet.cpp
index 9e7602d08e78160db94dffe4e6f1532e0207abae..3896604e2699dbd06f697d76db37e2616e04f238 100644
--- a/Source/core/rendering/RenderMultiColumnSet.cpp
+++ b/Source/core/rendering/RenderMultiColumnSet.cpp
@@ -84,9 +84,8 @@ LayoutUnit RenderMultiColumnSet::heightAdjustedForSetOffset(LayoutUnit height) c
 
 LayoutUnit RenderMultiColumnSet::pageLogicalTopForOffset(LayoutUnit offset) const
 {
-    LayoutUnit portionLogicalTop = (isHorizontalWritingMode() ? flowThreadPortionRect().y() : flowThreadPortionRect().x());
     unsigned columnIndex = columnIndexAtOffset(offset, AssumeNewColumns);
-    return portionLogicalTop + columnIndex * computedColumnHeight();
+    return logicalTopInFlowThread() + columnIndex * computedColumnHeight();
 }
 
 void RenderMultiColumnSet::setAndConstrainColumnHeight(LayoutUnit newHeight)
@@ -206,11 +205,15 @@ bool RenderMultiColumnSet::recalculateColumnHeight(bool initial)
     // determined that the columns need to be as tall as the specified height of the container, we
     // have already laid it out correctly, and there's no need for another pass.
 
+    // We can get rid of the content runs now, if we haven't already done so. They are only needed
+    // to calculate the initial balanced column height. In fact, we have to get rid of them before
+    // the next layout pass, since each pass will rebuild this.
+    m_contentRuns.clear();
+
     if (m_computedColumnHeight == oldColumnHeight)
         return false; // No change. We're done.
 
     m_minSpaceShortage = RenderFlowThread::maxLogicalHeight();
-    m_contentRuns.clear();
     return true; // Need another pass.
 }
 
@@ -248,41 +251,34 @@ void RenderMultiColumnSet::updateLogicalWidth()
     setLogicalWidth(logicalWidth() + delta);
 }
 
-void RenderMultiColumnSet::prepareForLayout()
+void RenderMultiColumnSet::prepareForLayout(bool initial)

[Julien - ping for review, 2014/05/20 17:03:04]

We should replace the first-pass boolean logic with an enum while touching this
code!

/me doesn't like boolean parameter

(Also this is a coding style violation, boolean parameters should have a verb in
them (the coding style says 'Precede boolean values with words like "is" and
"did".'))

[mstensho (USE GERRIT), 2014/05/22 15:07:18]

Done.

I did the second best thing - removed the parameter. :)
It's now resetColumnHeight(), and it's only called in the initial layout phase,
so no need for the parameter.

 {
     RenderBlockFlow* multicolBlock = multiColumnBlockFlow();
-    RenderStyle* multicolStyle = multicolBlock->style();
 
     // Set box logical top.
     ASSERT(!previousSiblingBox() || !previousSiblingBox()->isRenderMultiColumnSet()); // FIXME: multiple set not implemented; need to examine previous set to calculate the correct logical top.
-    setLogicalTop(multicolBlock->borderBefore() + multicolBlock->paddingBefore());
-
-    // Set box width.
-    updateLogicalWidth();
+    setLogicalTop(multicolBlock->borderAndPaddingBefore());
 
+    if (initial) {
+        m_maxColumnHeight = calculateMaxColumnHeight();
+        updateLogicalWidth();

[Julien - ping for review, 2014/05/20 17:03:04]

It's weird that prepareForLayout actually does what layout requires...
Especially since this is only run for the initial layout.

[mstensho (USE GERRIT), 2014/05/22 15:07:18]

Done.

All it really needed from updateLogicalWidth() was to update the column count
and width. It's now done from the flow thread rather than here (since the method
is now named resetColumnHeight()). I got rid of the updateLogicalWidth()
override while I was at it.

+    }
     if (multiColumnFlowThread()->requiresBalancing()) {
-        // Set maximum column height. We will not stretch beyond this.
-        m_maxColumnHeight = RenderFlowThread::maxLogicalHeight();
-        if (!multicolStyle->logicalHeight().isAuto()) {
-            m_maxColumnHeight = multicolBlock->computeContentLogicalHeight(multicolStyle->logicalHeight(), -1);
-            if (m_maxColumnHeight == -1)
-                m_maxColumnHeight = RenderFlowThread::maxLogicalHeight();
-        }
-        if (!multicolStyle->logicalMaxHeight().isUndefined()) {
-            LayoutUnit logicalMaxHeight = multicolBlock->computeContentLogicalHeight(multicolStyle->logicalMaxHeight(), -1);
-            if (logicalMaxHeight != -1 && m_maxColumnHeight > logicalMaxHeight)
-                m_maxColumnHeight = logicalMaxHeight;
-        }
-        m_maxColumnHeight = heightAdjustedForSetOffset(m_maxColumnHeight);
-        m_computedColumnHeight = 0; // Restart balancing.
+        if (initial)
+            m_computedColumnHeight = 0;
     } else {
         setAndConstrainColumnHeight(heightAdjustedForSetOffset(multiColumnFlowThread()->columnHeightAvailable()));
     }
 
-    m_contentRuns.clear();
+    // Content runs are only needed in the initial layout pass, in order to find an initial column
+    // height, and should have been deleted afterwards. We're about to rebuild the content runs, so
+    // the list needs to be empty.
+    ASSERT(m_contentRuns.isEmpty());
 
     // Nuke previously stored minimum column height. Contents may have changed for all we know.
     m_minimumColumnHeight = 0;
+
+    setNeedsLayoutAndFullRepaint();

[Julien - ping for review, 2014/05/20 17:03:04]

There is no branch in prepareForLayout, which means that we will always force a
layout on all multi-column set. Do we really need that?

[mstensho (USE GERRIT), 2014/05/21 09:02:33]

No, that looks very unnecessary, apart from being a nice way to hide repaint
bugs, of course. :)

I must have been somewhat careless when I did this. Now that it's called
blablablaAndFullRepaint(), it's more obvious that it's not cheap.

We can probably even get away with using the somewhat milder
setChildNeedsLayout(MarkOnlyThis).

[mstensho (USE GERRIT), 2014/05/22 15:07:18]

Done.

We now call setChildNeedsLayout(MarkOnlyThis), and only in the initial layout
phase, and even then only if column height changed.

 }
 
 void RenderMultiColumnSet::expandToEncompassFlowThreadContentsIfNeeded()
@@ -306,10 +302,9 @@ void RenderMultiColumnSet::layout()
 {
     RenderRegion::layout();
 
-    if (!nextSiblingMultiColumnSet()) {
-        // This is the last set, i.e. the last region. Seize the opportunity to validate them.
-        multiColumnFlowThread()->validateRegions();
-    }
+    // At this point the logical top of the column set is known. Update maximum column height
+    // (multicol height may be constrained, and in order to calculate it, we need to know our top).
+    m_maxColumnHeight = calculateMaxColumnHeight();
 }
 
 void RenderMultiColumnSet::computeLogicalHeight(LayoutUnit, LayoutUnit logicalTop, LogicalExtentComputedValues& computedValues) const
@@ -318,6 +313,20 @@ void RenderMultiColumnSet::computeLogicalHeight(LayoutUnit, LayoutUnit logicalTo
     computedValues.m_position = logicalTop;
 }
 
+LayoutUnit RenderMultiColumnSet::calculateMaxColumnHeight() const
+{
+    RenderBlockFlow* multicolBlock = multiColumnBlockFlow();
+    RenderStyle* multicolStyle = multicolBlock->style();
+    LayoutUnit availableHeight = multiColumnFlowThread()->columnHeightAvailable();
+    LayoutUnit maxColumnHeight = availableHeight ? availableHeight : RenderFlowThread::maxLogicalHeight();
+    if (!multicolStyle->logicalMaxHeight().isUndefined()) {
+        LayoutUnit logicalMaxHeight = multicolBlock->computeContentLogicalHeight(multicolStyle->logicalMaxHeight(), -1);
+        if (logicalMaxHeight != -1 && maxColumnHeight > logicalMaxHeight)
+            maxColumnHeight = logicalMaxHeight;
+    }
+    return heightAdjustedForSetOffset(maxColumnHeight);
+}
+
 LayoutUnit RenderMultiColumnSet::columnGap() const
 {
     RenderBlockFlow* parentBlock = multiColumnBlockFlow();