[New Multicolumn] Actual support for layout of column-span:all.

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 f2451de20aa11709dfab46211f224c4a2514395a..5c4687c47627151a460ef4c5904aa56486a8af7a 100644
--- a/Source/core/rendering/RenderMultiColumnSet.cpp
+++ b/Source/core/rendering/RenderMultiColumnSet.cpp
@@ -71,6 +71,42 @@ RenderMultiColumnSet* RenderMultiColumnSet::previousSiblingMultiColumnSet() cons
     return 0;
 }
 
+void RenderMultiColumnSet::setLogicalTopInFlowThread(LayoutUnit logicalTop)
+{
+    LayoutRect rect = flowThreadPortionRect();
+    if (isHorizontalWritingMode())
+        rect.setY(logicalTop);
+    else
+        rect.setX(logicalTop);

[Julien - ping for review, 2014/11/19 17:54:34]

It seems like the flowThreadPortionRect() is in logical coordinate as it is
inferred from the visual overflow. Could you explain the need for this code?

[mstensho (USE GERRIT), 2014/11/21 15:39:24]

flowThreadPortionRect() is in the same coordinate system as
RenderBox::m_frameRect, i.e. rather physical. (but both m_frameRect and the flow
thread portion rect have their block direction coordinate (either x or y)
flipped for reverse block direction mode (vertical-rl and horizontal-bt),
though). Visual overflow is in the same coordinate system, isn't it?

The logicalTop that we have here, on the other hand, is super-logical. It's a
block offset from the start of the flow thread, which starts at 0, and increases
as you add children, regardless of writing-mode.

[Julien - ping for review, 2014/11/26 21:09:00]

Visual overflow is in the same coordinate system indeed (which is what most of
the layout code manipulate).
OK but in this case, shouldn't we also check for isFlippingWritingMode() as this
is the primary determinant of whether the need to flip?

+    setFlowThreadPortionRect(rect);
+}
+
+void RenderMultiColumnSet::setLogicalBottomInFlowThread(LayoutUnit logicalBottom)
+{
+    LayoutRect rect = flowThreadPortionRect();
+    if (isHorizontalWritingMode())
+        rect.shiftMaxYEdgeTo(logicalBottom);
+    else
+        rect.shiftMaxXEdgeTo(logicalBottom);
+    setFlowThreadPortionRect(rect);
+}
+
+bool RenderMultiColumnSet::heightIsAuto() const
+{
+    RenderMultiColumnFlowThread* flowThread = multiColumnFlowThread();
+    if (!flowThread->isRenderPagedFlowThread()) {
+        if (multiColumnBlockFlow()->style()->columnFill() == ColumnFillBalance)
+            return true;
+        if (RenderMultiColumnSet* next = nextSiblingMultiColumnSet()) {
+            if (next->isRenderMultiColumnSpannerSet()) {
+                // If we're followed by a spanner, we need to balance.

[Julien - ping for review, 2014/11/19 17:54:34]

This comment (and the rest of the logic) shows that the name is not well done.
It seems like it should just be called needsBalancing() or something similar.

It's also super weird to have the only difference between this function and
requiresBalancing() is a type check. Can't we just squash them together?

[mstensho (USE GERRIT), 2014/11/21 15:39:24]

Callers of this method should really only want to know if the set height is
auto, and not worry about balancing. I found one call site now that (harmlessly)
assumed that auto height meant balancing. I changed it to calling
requiresBalancing(). That will result in an additional virtual method call, but
I suppose we can afford that for the sake of clarity.
That's a crucial type check. A spanner shouldn't be balanced, but its height may
still be auto.

This can be considered to be a result of letting RenderMultiColumnSpannerSet
inherit from RenderMultiColumnSet (see FIXME in RenderMultiColumnSpannerSet.h).
I think the two classes would be better off inheriting from some common base
class. That would be RenderRegion in the current code, but I'd rather get rid of
/ clean up / rename that one before cleaning up this.

Asking a spanner if it requires balancing is meaningless, and we'd probably be
better off avoiding calling requiresBalancing() on something that may
potentially be a spanner set.

+                return true;
+            }
+        }
+    }
+    return !flowThread->columnHeightAvailable();
+}
+
 LayoutSize RenderMultiColumnSet::flowThreadTranslationAtOffset(LayoutUnit blockOffset) const
 {
     unsigned columnIndex = columnIndexAtOffset(blockOffset);
@@ -194,7 +230,7 @@ LayoutUnit RenderMultiColumnSet::calculateColumnHeight(BalancedHeightCalculation
 
 void RenderMultiColumnSet::addContentRun(LayoutUnit endOffsetFromFirstPage)
 {
-    if (!multiColumnFlowThread()->heightIsAuto())
+    if (!requiresBalancing())
         return;
     if (!m_contentRuns.isEmpty() && endOffsetFromFirstPage <= m_contentRuns.last().breakOffset())
         return;
@@ -206,27 +242,27 @@ void RenderMultiColumnSet::addContentRun(LayoutUnit endOffsetFromFirstPage)
 
 bool RenderMultiColumnSet::recalculateColumnHeight(BalancedHeightCalculation calculationMode)
 {
-    if (previousSiblingMultiColumnSet()) {
-        // FIXME: column spanner layout is not yet implemented. Until it's in place, we only operate
-        // on the first set during layout. We need to ignore the others here, or assertions will
-        // fail.
-        return false;
-    }
-    ASSERT(multiColumnFlowThread()->heightIsAuto());
-
     LayoutUnit oldColumnHeight = m_columnHeight;
-    if (calculationMode == GuessFromFlowThreadPortion) {
-        // Post-process the content runs and find out where the implicit breaks will occur.
-        distributeImplicitBreaks();
-    }
-    LayoutUnit newColumnHeight = calculateColumnHeight(calculationMode);
-    setAndConstrainColumnHeight(newColumnHeight);
 
-    // After having calculated an initial column height, the multicol container typically needs at
-    // least one more layout pass with a new column height, but if a height was specified, we only
-    // need to do this if we think that we need less space than specified. Conversely, if we
-    // 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.
+    m_maxColumnHeight = calculateMaxColumnHeight();
+
+    if (heightIsAuto()) {
+        if (calculationMode == GuessFromFlowThreadPortion) {
+            // Post-process the content runs and find out where the implicit breaks will occur.
+            distributeImplicitBreaks();
+        }
+        LayoutUnit newColumnHeight = calculateColumnHeight(calculationMode);
+        setAndConstrainColumnHeight(newColumnHeight);
+        // After having calculated an initial column height, the multicol container typically needs at
+        // least one more layout pass with a new column height, but if a height was specified, we only
+        // need to do this if we think that we need less space than specified. Conversely, if we
+        // 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.
+    } else {
+        // The position of the column set may have changed, in which case height available for
+        // columns may have changed as well.
+        setAndConstrainColumnHeight(m_columnHeight);
+    }
 
     // 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
@@ -261,7 +297,7 @@ void RenderMultiColumnSet::resetColumnHeight()
 
     LayoutUnit oldColumnHeight = pageLogicalHeight();
 
-    if (multiColumnFlowThread()->heightIsAuto())
+    if (heightIsAuto())
         m_columnHeight = 0;
     else
         setAndConstrainColumnHeight(heightAdjustedForSetOffset(multiColumnFlowThread()->columnHeightAvailable()));