[New Multicolumn] Rebalance properly when child content changes.

Source/core/rendering/RenderMultiColumnFlowThread.cpp

Index: Source/core/rendering/RenderMultiColumnFlowThread.cpp
diff --git a/Source/core/rendering/RenderMultiColumnFlowThread.cpp b/Source/core/rendering/RenderMultiColumnFlowThread.cpp
index 487dc477fae4780aa4e5a356e9ea1205e6dda420..1422651cf9e6253720d2cbaa6d9771deb10ec219 100644
--- a/Source/core/rendering/RenderMultiColumnFlowThread.cpp
+++ b/Source/core/rendering/RenderMultiColumnFlowThread.cpp
@@ -139,37 +139,29 @@ LayoutSize RenderMultiColumnFlowThread::columnOffset(const LayoutPoint& point) c
 
 void RenderMultiColumnFlowThread::layoutColumns(bool relayoutChildren, SubtreeLayoutScope& layoutScope)
 {
-    // Update the dimensions of our regions before we lay out the flow thread.
-    // FIXME: Eventually this is going to get way more complicated, and we will be destroying regions
-    // instead of trying to keep them around.
-    bool shouldInvalidateRegions = false;
-    for (RenderMultiColumnSet* columnSet = firstMultiColumnSet(); columnSet; columnSet = columnSet->nextSiblingMultiColumnSet()) {
-        if (relayoutChildren || columnSet->needsLayout()) {
-            if (!m_inBalancingPass)
-                columnSet->prepareForLayout();
-            shouldInvalidateRegions = true;
-        }
-    }
-
-    if (shouldInvalidateRegions)
-        invalidateRegions();
-
     if (relayoutChildren)
         layoutScope.setChildNeedsLayout(this);
 
-    if (requiresBalancing()) {
-        // At the end of multicol layout, relayoutForPagination() is called unconditionally, but if
-        // no children are to be laid out (e.g. fixed width with layout already being up-to-date),
-        // we want to prevent it from doing any work, so that the column balancing machinery doesn't
-        // kick in and trigger additional unnecessary layout passes. Actually, it's not just a good
-        // idea in general to not waste time on balancing content that hasn't been re-laid out; we
-        // are actually required to guarantee this. The calculation of implicit breaks needs to be
-        // preceded by a proper layout pass, since it's layout that sets up content runs, and the
-        // runs get deleted right after every pass.
-        m_needsColumnHeightsRecalculation = shouldInvalidateRegions || needsLayout();
+    if (!needsLayout()) {
+        // At the end of multicol container (our parent RenderBlockFlow) layout, it will call
+        // recalculateColumnHeights() on us unconditionally, but if we won't lay out (e.g. because
+        // we have fixed width with layout already being up-to-date), we want to prevent
+        // recalculateColumnHeights() from doing any work, so that the column balancing machinery
+        // doesn't kick in and trigger additional unnecessary layout passes. Actually, it's not just
+        // a good idea in general to not waste time on balancing content that hasn't been re-laid
+        // out; we are actually required to guarantee this. The calculation of implicit breaks needs
+        // to be preceded by a proper layout pass, since it's layout that sets up content runs, and
+        // the runs get deleted right after every pass.
+        m_needsColumnHeightsRecalculation = false;
+        return;
     }
 
-    layoutIfNeeded();
+    for (RenderMultiColumnSet* columnSet = firstMultiColumnSet(); columnSet; columnSet = columnSet->nextSiblingMultiColumnSet())
+        columnSet->prepareForLayout(!m_inBalancingPass);

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

We should rename this function to something better as we touch this code. It's
unclear to me what name would encompass what this function is doing though.

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

This function prepares a column set for flow thread layout.
The flow thread is laid out first (and it's working on the sets during this
process, before they have actually been laid out). Afterwards, the sets are laid
out.
prepareForFlowThreadLayout() is more descriptive, but maybe we could come up
with something more cohesive than that?

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

Done.

Ended up with resetColumnHeight(), which is only called in the initial pass.

+
+    invalidateRegions();
+    m_needsColumnHeightsRecalculation = requiresBalancing();
+    layout();
 }
 
 bool RenderMultiColumnFlowThread::computeColumnCountAndWidth()
@@ -205,6 +197,9 @@ bool RenderMultiColumnFlowThread::computeColumnCountAndWidth()
 
 bool RenderMultiColumnFlowThread::recalculateColumnHeights()
 {
+    // All column sets that needed layout have now been laid out, so we can finally validate them.
+    validateRegions();
+
     if (!m_needsColumnHeightsRecalculation)
         return false;
 
@@ -215,9 +210,14 @@ bool RenderMultiColumnFlowThread::recalculateColumnHeights()
     // columns, unless we have a bug.
     bool needsRelayout = false;
     for (RenderMultiColumnSet* multicolSet = firstMultiColumnSet(); multicolSet; multicolSet = multicolSet->nextSiblingMultiColumnSet()) {
-        if (multicolSet->recalculateColumnHeight(!m_inBalancingPass)) {
-            multicolSet->setChildNeedsLayout(MarkOnlyThis);
+        if (multicolSet->recalculateColumnHeight(!m_inBalancingPass))
             needsRelayout = true;
+        if (needsRelayout) {
+            // Once a column set gets a new column height, that column set and all successive column
+            // sets need to be laid out over again, since their logical top will be affected by
+            // this, and therefore their column heights may change as well, at least if the multicol
+            // height is constrained.
+            multicolSet->setChildNeedsLayout(MarkOnlyThis);
         }
     }