[css-grid] Use the margin box for non auto minimum sizes

third_party/WebKit/Source/core/layout/LayoutGrid.cpp

Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index 58a94f8034fed8e205f959f5efea3e4dcc9c1c2c..a4882db7de57470d4c3cef0d269172401d7d5c74 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -247,6 +247,9 @@ public:
 
     LayoutUnit& freeSpaceForDirection(GridTrackSizingDirection direction) { return direction == ForColumns ? freeSpaceForColumns : freeSpaceForRows; }
 
+    enum SizingOperation { TrackSizing, IntrinsicSizeComputation };
+    SizingOperation sizingOperation { TrackSizing };
+
 private:
     LayoutUnit freeSpaceForColumns { };
     LayoutUnit freeSpaceForRows { };
@@ -330,6 +333,7 @@ void LayoutGrid::computeTrackSizesForDirection(GridTrackSizingDirection directio
 {
     ASSERT(freeSpace >= 0);
     sizingData.freeSpaceForDirection(direction) = freeSpace - guttersSize(direction, direction == ForRows ? gridRowCount() : gridColumnCount());
+    sizingData.sizingOperation = GridSizingData::TrackSizing;
 
     LayoutUnit baseSizes, growthLimits;
     computeUsedBreadthOfGridTracks(direction, sizingData, baseSizes, growthLimits, AvailableSpaceDefinite);
@@ -419,6 +423,7 @@ void LayoutGrid::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, Layo
 
     GridSizingData sizingData(gridColumnCount(), gridRowCount());
     sizingData.freeSpaceForDirection(ForColumns) = LayoutUnit();
+    sizingData.sizingOperation = GridSizingData::IntrinsicSizeComputation;
     const_cast<LayoutGrid*>(this)->computeUsedBreadthOfGridTracks(ForColumns, sizingData, minLogicalWidth, maxLogicalWidth, AvailableSpaceIndefinite);
 
     LayoutUnit totalGuttersSize = guttersSize(ForColumns, sizingData.columnTracks.size());
@@ -434,6 +439,7 @@ void LayoutGrid::computeIntrinsicLogicalHeight(GridSizingData& sizingData)
 {
     ASSERT(tracksAreWiderThanMinTrackBreadth(ForColumns, sizingData));
     sizingData.freeSpaceForDirection(ForRows) = LayoutUnit();
+    sizingData.sizingOperation = GridSizingData::IntrinsicSizeComputation;
     computeUsedBreadthOfGridTracks(ForRows, sizingData, m_minContentHeight, m_maxContentHeight, AvailableSpaceIndefinite);
 
     LayoutUnit totalGuttersSize = guttersSize(ForRows, gridRowCount());
@@ -683,10 +689,8 @@ GridTrackSize LayoutGrid::gridTrackSize(GridTrackSizingDirection direction, size
 LayoutUnit LayoutGrid::logicalHeightForChild(LayoutBox& child, GridSizingData& sizingData)
 {
     SubtreeLayoutScope layoutScope(child);
-    LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child.hasOverrideContainingBlockLogicalWidth() ? child.overrideContainingBlockContentLogicalWidth() : LayoutUnit();
-    LayoutUnit overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChild(child, ForColumns, sizingData.columnTracks);
     bool shouldClearContainingBlockLogicalHeight = child.hasRelativeLogicalHeight() || child.styleRef().logicalHeight().isIntrinsicOrAuto();
-    if (shouldClearContainingBlockLogicalHeight || oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth)
+    if (shouldClearContainingBlockLogicalHeight)

[jfernandez, 2016/04/06 12:22:23]

We only use the  TreeScope for the setNeedsForLayout in case of having to clear
the containing block's logical height.  In the old code, we were doing it as
well when the containing block's logical width had changed. 

Now we deal with containing block's logical width changes outsize this function,
which makes sense, indeed. However, we need to be sure that not using the scope
in the cases were we mark the child for layout before computing its logical
height is equivalent.

[svillar, 2016/04/06 14:55:09]

As you can see in the SubtreeLayoutScope, that class is a helper to ensure that
once a subtree is marked for layout, we do indeed lay it out. As you can see, in
this patch the setNeedsLayout() we add is followed by a layoutIfNeeded() in one
case and by a logicalHeightForChild() in the other (which internally calls
layoutIfNeeded()). That's why I think we're safe here.

[jfernandez, 2016/04/07 07:49:39]

Yes, you're right. The change is equivalent to the original code.

         layoutScope.setNeedsLayout(&child, LayoutInvalidationReason::GridChanged);
 
     bool hasOverrideHeight = child.hasOverrideLogicalContentHeight();
@@ -694,7 +698,6 @@ LayoutUnit LayoutGrid::logicalHeightForChild(LayoutBox& child, GridSizingData& s
     if (hasOverrideHeight && child.needsLayout())
         child.clearOverrideLogicalContentHeight();
 
-    child.setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
     // If |child| has a relative logical height, we shouldn't let it override its intrinsic height, which is
     // what we are interested in here. Thus we need to set the override logical height to -1 (no possible resolution).
     if (shouldClearContainingBlockLogicalHeight)
@@ -717,10 +720,26 @@ LayoutUnit LayoutGrid::minSizeForChild(LayoutBox& child, GridTrackSizingDirectio
     if (!childSize.isAuto() || childMinSize.isAuto())
         return minContentForChild(child, direction, sizingData);
 
-    if (isRowAxis)
-        return child.computeLogicalWidthUsing(MinSize, childMinSize, contentLogicalWidth(), this);
+    bool overrideLogicalWidthHasChanged = updateOverrideContainingBlockContentLogicalWidthForChild(child, sizingData);
+    if (isRowAxis) {
+        LayoutUnit marginLogicalWidth = sizingData.sizingOperation == GridSizingData::TrackSizing ? computeMarginLogicalSizeForChild(InlineDirection, child) : marginIntrinsicLogicalWidthForChild(child);
+        return child.computeLogicalWidthUsing(MinSize, childMinSize, child.overrideContainingBlockContentLogicalWidth(), this) + marginLogicalWidth;
+    }
+
+    if (overrideLogicalWidthHasChanged)
+        child.setNeedsLayout(LayoutInvalidationReason::GridChanged);
+    child.layoutIfNeeded();
+    return child.computeLogicalHeightUsing(MinSize, childMinSize, child.intrinsicLogicalHeight()) + child.marginLogicalHeight() + child.scrollbarLogicalHeight();
+}
+
+bool LayoutGrid::updateOverrideContainingBlockContentLogicalWidthForChild(LayoutBox& child, GridSizingData& sizingData)
+{
+    LayoutUnit overrideWidth = gridAreaBreadthForChild(child, ForColumns, sizingData.columnTracks);
+    if (child.hasOverrideContainingBlockLogicalWidth() && child.overrideContainingBlockContentLogicalWidth() == overrideWidth)
+        return false;
 
-    return child.computeLogicalHeightUsing(MinSize, childMinSize, child.intrinsicLogicalHeight()) + child.scrollbarLogicalHeight();
+    child.setOverrideContainingBlockContentLogicalWidth(overrideWidth);
+    return true;
 }
 
 LayoutUnit LayoutGrid::minContentForChild(LayoutBox& child, GridTrackSizingDirection direction, GridSizingData& sizingData)
@@ -741,6 +760,8 @@ LayoutUnit LayoutGrid::minContentForChild(LayoutBox& child, GridTrackSizingDirec
         return child.minPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
     }
 
+    if (updateOverrideContainingBlockContentLogicalWidthForChild(child, sizingData))
+        child.setNeedsLayout(LayoutInvalidationReason::GridChanged);

[jfernandez, 2016/04/06 12:22:23]

Shouldn't we use the SubtreeLayoutScope here ?

[svillar, 2016/04/06 14:55:09]

We know that logicalHeightForChild() is going to call layoutIfNeeded(), but I
can change it so that it will assert if we ever remove that from the
logicalHeightForChild() method.

[jfernandez, 2016/04/07 07:49:39]

The SubtreeLAyoutScope approach is used in other places of the grid layout
logic,
so perhaps we can still use it here. 

     return logicalHeightForChild(child, sizingData);
 }
 
@@ -762,6 +783,8 @@ LayoutUnit LayoutGrid::maxContentForChild(LayoutBox& child, GridTrackSizingDirec
         return child.maxPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
     }
 
+    if (updateOverrideContainingBlockContentLogicalWidthForChild(child, sizingData))
+        child.setNeedsLayout(LayoutInvalidationReason::GridChanged);

[jfernandez, 2016/04/06 12:22:23]

Shouldn't we use the SubtreeLayoutScope here ?

     return logicalHeightForChild(child, sizingData);
 }
 
@@ -1648,18 +1671,21 @@ LayoutUnit LayoutGrid::marginLogicalHeightForChild(const LayoutBox& child) const
     return isHorizontalWritingMode() ? child.marginHeight() : child.marginWidth();
 }
 
-LayoutUnit LayoutGrid::computeMarginLogicalHeightForChild(const LayoutBox& child) const
+LayoutUnit LayoutGrid::computeMarginLogicalSizeForChild(MarginDirection forDirection, const LayoutBox& child) const
 {
     if (!child.styleRef().hasMargin())
         return LayoutUnit();
 
-    LayoutUnit marginBefore;
-    LayoutUnit marginAfter;
-    child.computeMarginsForDirection(BlockDirection, this, child.containingBlockLogicalWidthForContent(), child.logicalHeight(), marginBefore, marginAfter,
-        child.style()->marginBeforeUsing(style()),
-        child.style()->marginAfterUsing(style()));
+    bool isRowAxis = forDirection == InlineDirection;
+    LayoutUnit marginStart;
+    LayoutUnit marginEnd;
+    LayoutUnit logicalSize = isRowAxis ? child.logicalWidth() : child.logicalHeight();
+    Length marginStartLength = isRowAxis ? child.styleRef().marginStart() : child.styleRef().marginBeforeUsing(style());
+    Length marginEndLength = isRowAxis ? child.styleRef().marginEnd() : child.styleRef().marginAfterUsing(style());
+    child.computeMarginsForDirection(forDirection, this, child.containingBlockLogicalWidthForContent(), logicalSize,
+        marginStart, marginEnd, marginStartLength, marginEndLength);
 
-    return marginBefore + marginAfter;
+    return marginStart + marginEnd;
 }
 
 LayoutUnit LayoutGrid::availableAlignmentSpaceForChildBeforeStretching(LayoutUnit gridAreaBreadthForChild, const LayoutBox& child) const
@@ -1667,7 +1693,7 @@ LayoutUnit LayoutGrid::availableAlignmentSpaceForChildBeforeStretching(LayoutUni
     // Because we want to avoid multiple layouts, stretching logic might be performed before
     // children are laid out, so we can't use the child cached values. Hence, we need to
     // compute margins in order to determine the available height before stretching.
-    return gridAreaBreadthForChild - (child.needsLayout() ? computeMarginLogicalHeightForChild(child) : marginLogicalHeightForChild(child));
+    return gridAreaBreadthForChild - (child.needsLayout() ? computeMarginLogicalSizeForChild(BlockDirection, child) : marginLogicalHeightForChild(child));
 }
 
 // FIXME: This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.