[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..89bf5b7cb9f01f32d2ff732f5952e3ea8d049010 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -680,13 +680,11 @@ GridTrackSize LayoutGrid::gridTrackSize(GridTrackSizingDirection direction, size
     return GridTrackSize(minTrackBreadth, maxTrackBreadth);
 }
 
-LayoutUnit LayoutGrid::logicalHeightForChild(LayoutBox& child, GridSizingData& sizingData)
+LayoutUnit LayoutGrid::logicalHeightForChild(LayoutBox& child, GridSizingData& sizingData, bool overrideWidthHasChanged)

[jfernandez, 2016/04/04 15:23:24]

We shouldn't need this new argument. We only use to know whether we have to mark
the child for layout. We can do this before calling to logicalHeightForChild.

[svillar, 2016/04/05 08:34:29]

Acknowledged.

 {
     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 || overrideWidthHasChanged)
         layoutScope.setNeedsLayout(&child, LayoutInvalidationReason::GridChanged);
 
     bool hasOverrideHeight = child.hasOverrideLogicalContentHeight();
@@ -694,7 +692,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 +714,26 @@ LayoutUnit LayoutGrid::minSizeForChild(LayoutBox& child, GridTrackSizingDirectio
     if (!childSize.isAuto() || childMinSize.isAuto())
         return minContentForChild(child, direction, sizingData);
 
+    SubtreeLayoutScope layoutScope(child);
+    if (updateOverrideContainingBlockContentLogicalWidthForChild(child, sizingData))
+        layoutScope.setNeedsLayout(&child, LayoutInvalidationReason::GridChanged);
+    child.layoutIfNeeded();
+
+    ASSERT(child.hasOverrideContainingBlockLogicalWidth());

[jfernandez, 2016/04/04 15:23:24]

Why we need this ASSERT ? 
It's already part overrideContainingBlockContentLogicalWidth() function, so I
don't think it's useful. 

I think it's also weird this way of updating the
OverrideContianerBlockLogicalWidth and mark the child for layout. We are doing
the same inside minContentFrChild + logicalHeightForChild. 

I think we should look for a different way of doing this, verifying only once
whether the overrideWidth ahs changed or not and setting the child for layout.
Then, we will call layoutIfNeeded in the specific functions. 

     if (isRowAxis)
-        return child.computeLogicalWidthUsing(MinSize, childMinSize, contentLogicalWidth(), this);
+        return child.computeLogicalWidthUsing(MinSize, childMinSize, child.overrideContainingBlockContentLogicalWidth(), this) + child.marginLogicalWidth();
+
+    return child.computeLogicalHeightUsing(MinSize, childMinSize, child.intrinsicLogicalHeight()) + child.marginLogicalHeight() + child.scrollbarLogicalHeight();
+}
 
-    return child.computeLogicalHeightUsing(MinSize, childMinSize, child.intrinsicLogicalHeight()) + child.scrollbarLogicalHeight();
+bool LayoutGrid::updateOverrideContainingBlockContentLogicalWidthForChild(LayoutBox& child, GridSizingData& sizingData)
+{
+    LayoutUnit overrideWidth = gridAreaBreadthForChild(child, ForColumns, sizingData.columnTracks);
+    if (child.hasOverrideContainingBlockLogicalWidth() && child.overrideContainingBlockContentLogicalWidth() == overrideWidth)
+        return false;
+
+    child.setOverrideContainingBlockContentLogicalWidth(overrideWidth);
+    return true;
 }
 
 LayoutUnit LayoutGrid::minContentForChild(LayoutBox& child, GridTrackSizingDirection direction, GridSizingData& sizingData)
@@ -741,7 +754,8 @@ LayoutUnit LayoutGrid::minContentForChild(LayoutBox& child, GridTrackSizingDirec
         return child.minPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
     }
 
-    return logicalHeightForChild(child, sizingData);
+    bool overrideWidthHasChanged = updateOverrideContainingBlockContentLogicalWidthForChild(child, sizingData);

[jfernandez, 2016/04/01 11:00:46]

Why we can't call this function inside logicalHeightForChild instead of using
the new extra argument ? 

[svillar, 2016/04/05 08:34:29]

Because it's pretty weird that a function that returns the logicalHeight does
modify the width as a side effect. This means that you cannot safely ask for the
height of an element without the risk of the width being modified.

+    return logicalHeightForChild(child, sizingData, overrideWidthHasChanged);
 }
 
 LayoutUnit LayoutGrid::maxContentForChild(LayoutBox& child, GridTrackSizingDirection direction, GridSizingData& sizingData)
@@ -762,7 +776,8 @@ LayoutUnit LayoutGrid::maxContentForChild(LayoutBox& child, GridTrackSizingDirec
         return child.maxPreferredLogicalWidth() + marginIntrinsicLogicalWidthForChild(child);
     }
 
-    return logicalHeightForChild(child, sizingData);
+    bool overrideWidthHasChanged = updateOverrideContainingBlockContentLogicalWidthForChild(child, sizingData);
+    return logicalHeightForChild(child, sizingData, overrideWidthHasChanged);
 }
 
 // We're basically using a class instead of a std::pair because of accessing gridItem() or getGridSpan() is much more
@@ -1568,8 +1583,11 @@ LayoutUnit LayoutGrid::gridAreaBreadthForChild(const LayoutBox& child, GridTrack
 {
     const GridSpan& span = cachedGridSpan(child, direction);
     LayoutUnit gridAreaBreadth;
-    for (const auto& trackPosition : span)
+    for (const auto& trackPosition : span) {
+        if (tracks[trackPosition].baseSize() == infinity)
+            return LayoutUnit(infinity);

[jfernandez, 2016/04/01 11:00:46]

Could you explain this change ? 

[svillar, 2016/04/05 08:34:29]

Sure. This function can now be called just after initializing the tracks' base
and growthLimit sizes. Infinity is represented as -1, so we cannot safely just
sum the sizes of the tracks because infinities will reduce the breadth size
instead of considering that we have an unlimited room.

[svillar, 2016/04/05 08:42:33]

Now that I think about it, base sizes are never initialized to 0, so we should
be safe without that change.

         gridAreaBreadth += tracks[trackPosition].baseSize();
+    }
 
     gridAreaBreadth += guttersSize(direction, span.integerSpan());