[css-grid] Fix indefinite height detection

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

Index: third_party/WebKit/Source/core/layout/LayoutBox.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
index bd3dd55b2e8ccb76b136b641254925cee03264ec..13afe0331f593041c6a22b435752c0b17fbdf615 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -2698,11 +2698,9 @@ bool LayoutBox::skipContainingBlockForPercentHeightCalculation(const LayoutBox*
 
 LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
 {
-    LayoutUnit availableHeight(-1);
-
-    bool skippedAutoHeightContainingBlock = false;
     LayoutBlock* cb = containingBlock();
     const LayoutBox* containingBlockChild = this;
+    bool skippedAutoHeightContainingBlock = false;
     LayoutUnit rootMarginBorderPaddingHeight;
     while (!cb->isLayoutView() && skipContainingBlockForPercentHeightCalculation(cb)) {
         if (cb->isBody() || cb->isDocumentElement())
@@ -2713,33 +2711,12 @@ LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
     }
     cb->addPercentHeightDescendant(const_cast<LayoutBox*>(this));
 
-    const ComputedStyle& cbstyle = cb->styleRef();
-
-    // A positioned element that specified both top/bottom or that specifies height should be treated as though it has a height
-    // explicitly specified that can be used for any percentage computations.
-    bool isOutOfFlowPositionedWithSpecifiedHeight = cb->isOutOfFlowPositioned() && (!cbstyle.logicalHeight().isAuto() || (!cbstyle.logicalTop().isAuto() && !cbstyle.logicalBottom().isAuto()));
-
-    bool includeBorderPadding = isTable();
-
-    LayoutUnit stretchedFlexHeight(-1);
-    if (cb->isFlexItem())
-        stretchedFlexHeight = toLayoutFlexibleBox(cb->parent())->childLogicalHeightForPercentageResolution(*cb);
-
+    LayoutUnit availableHeight;

[svillar, 2016/07/20 08:24:00]

My main doubt is, what's the criteria you have used to move the code to the new
function? I'm having a hard time trying to know that.

[Manuel Rego, 2016/07/21 09:46:01]

Basically the idea is having a function that I can call from the grid container,
to know if it has a definite or indefinite height.
So I tried to move everything I could to an independent funciton,
so calls to current object like isHorizontalWritingMode() or
hasOverrideContainingBlockLogicalHeight() couldn't be moved.
Also I wanted to avoid to pass weird bool params to that method
(like in previous version).

     if (isHorizontalWritingMode() != cb->isHorizontalWritingMode()) {
         availableHeight = containingBlockChild->containingBlockLogicalWidthForContent();
-    } else if (stretchedFlexHeight != LayoutUnit(-1)) {
-        availableHeight = stretchedFlexHeight;
-    } else if (hasOverrideContainingBlockLogicalHeight() && !isOutOfFlowPositionedWithSpecifiedHeight) {
+    } else if (isGridItem() && hasOverrideContainingBlockLogicalHeight()) {

[svillar, 2016/07/20 08:24:00]

Why just for grid items? What about !isOutOfFlowPositionedWithSpecifiedHeight ?

[Manuel Rego, 2016/07/21 09:46:01]

True the isGridItem() is not needed.

About "!isOutOfFlowPositionedWithSpecifiedHeight" that was wrong.
It was needed for some cases as we were using it to know if the height is
indefinite.
But it was causing bugs too: http://jsbin.com/hisova/1/edit?html,css,output

And the thing is that it's not possible in the current code to just remove
!isOutOfFlowPositionedWithSpecifiedHeight.
As it'll be making other good tests to fail.
The main issue is how bad we're detecting that the height is indefinite,
using the firstInFlowChildBox()->percentageLogicalHeightIsResolvable().

         availableHeight = overrideContainingBlockContentLogicalHeight();
-    } else if (cb->isGridItem() && cb->hasOverrideLogicalContentHeight()) {
-        availableHeight = cb->overrideLogicalContentHeight();
-    } else if (cbstyle.logicalHeight().isFixed()) {
-        LayoutUnit contentBoxHeight = cb->adjustContentBoxLogicalHeightForBoxSizing(cbstyle.logicalHeight().value());
-        availableHeight = cb->constrainContentBoxLogicalHeightByMinMax(
-            contentBoxHeight - cb->scrollbarLogicalHeight(), LayoutUnit(-1)).clampNegativeToZero();
-        if (cb->isTableCell())
-            includeBorderPadding = true;
-    } else if (cb->isTableCell()) {
+    } else if (!cb->styleRef().logicalHeight().isFixed() && cb->isTableCell()) {
         if (!skippedAutoHeightContainingBlock) {
             // Table cells violate what the CSS spec says to do with heights. Basically we
             // don't care if the cell specified a height or not. We just always make ourselves
@@ -2758,28 +2735,9 @@ LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
                 return LayoutUnit(-1);
             }
             availableHeight = cb->overrideLogicalContentHeight();
-            includeBorderPadding = true;
-        }
-    } else if (cbstyle.logicalHeight().hasPercent() && !isOutOfFlowPositionedWithSpecifiedHeight) {
-        // We need to recur and compute the percentage height for our containing block.
-        LayoutUnit heightWithScrollbar = cb->computePercentageLogicalHeight(cbstyle.logicalHeight());
-        if (heightWithScrollbar != -1) {
-            LayoutUnit contentBoxHeightWithScrollbar = cb->adjustContentBoxLogicalHeightForBoxSizing(heightWithScrollbar);
-            // We need to adjust for min/max height because this method does not
-            // handle the min/max of the current block, its caller does. So the
-            // return value from the recursive call will not have been adjusted
-            // yet.
-            LayoutUnit contentBoxHeight = cb->constrainContentBoxLogicalHeightByMinMax(contentBoxHeightWithScrollbar - cb->scrollbarLogicalHeight(), LayoutUnit(-1));
-            availableHeight = std::max(LayoutUnit(), contentBoxHeight);
         }
-    } else if (isOutOfFlowPositionedWithSpecifiedHeight) {
-        // Don't allow this to affect the block' size() member variable, since this
-        // can get called while the block is still laying out its kids.
-        LogicalExtentComputedValues computedValues;
-        cb->computeLogicalHeight(cb->logicalHeight(), LayoutUnit(), computedValues);
-        availableHeight = computedValues.m_extent - cb->borderAndPaddingLogicalHeight() - cb->scrollbarLogicalHeight();
-    } else if (cb->isLayoutView()) {
-        availableHeight = view()->viewLogicalHeightForPercentages();
+    } else {
+        availableHeight = LayoutBlock::availableLogicalHeightForPercentageComputation(*cb);
     }
 
     if (availableHeight == -1)
@@ -2791,6 +2749,7 @@ LayoutUnit LayoutBox::computePercentageLogicalHeight(const Length& height) const
         availableHeight += cb->paddingLogicalHeight();
 
     LayoutUnit result = valueForLength(height, availableHeight);
+    bool includeBorderPadding = isTable() || (cb->isTableCell() && (cb->styleRef().logicalHeight().isFixed() || (!skippedAutoHeightContainingBlock && cb->hasOverrideLogicalContentHeight())));

[svillar, 2016/07/20 08:24:00]

I think this is not correct, for example if cb->isTableCell() and
cb->styleRef().logicalHeight().isFixed() are both true we set this to true, but
in the previous code that was only done whenever 
!skippedAutoHeightContainingBlock.

Apart from that such a complex condition in just 1 line seems a bit too much to
my taste :).

[Manuel Rego, 2016/07/21 09:46:01]

Are you sure? In the previous code we've:
  else if (cbstyle.logicalHeight().isFixed()) {
    ...
    if (cb->isTableCell())
      includeBorderPadding = true;
  }

So that was enough to make it true.
Yeah, I can split it in several lines.

     if (includeBorderPadding) {
         // TODO(rhogan) crbug.com/467378: Doing this for content inside tables cells is wrong, it should fill
         // whatever height the cell makes available.
@@ -2901,6 +2860,7 @@ LayoutUnit LayoutBox::computeReplacedLogicalHeightUsing(SizeType sizeType, const
     case Percent:
     case Calculated:
     {
+        // TODO(rego): Check if we can somehow reuse LayoutBox::computePercentageLogicalHeight() and/or LayoutBlock::availableLogicalHeightForPercentageComputation().
         LayoutObject* cb = isOutOfFlowPositioned() ? container() : containingBlock();
         while (cb->isAnonymous())
             cb = cb->containingBlock();