[webkit-reviews] review denied: [Bug 106474] [CSS Grid Layout] Add support for min-content : [Attachment 182808] Proposed implementation 3: Fixed the build for real this time.

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 106474: [CSS Grid Layout] Add support for min-content
https://bugs.webkit.org/show_bug.cgi?id=106474

Attachment 182808: Proposed implementation 3: Fixed the build for real this
time.
https://bugs.webkit.org/attachment.cgi?id=182808&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=182808&action=review


Overall design looks good. Some questions on the details.

> Source/WebCore/rendering/RenderGrid.cpp:209
> +    if (trackLength.isFixed() || trackLength.isPercent() ||
trackLength.isViewportPercentage()) {
> +	   LayoutUnit computedBreadth =
computeUsedBreadthOfSpecifiedLength(direction, trackLength);
> +	   ASSERT(computedBreadth != infinity);
> +	   return computedBreadth;
> +    }
> +
> +    // Per the spec we return Infinity if the track length is min-content,
max-content or a fraction.
> +    return infinity;

FWIW, the way RenderBox handles code like this is to do something like the
following:
LayoutUnit RenderGrid::computeUsedBreadthOfSpecifiedLength(...)
{
    if (trackLength.isFixed() || ...)
	return valueForLength(...);
    return -1;
}

LayoutUnit RenderGrid::computeUsedBreadthOfMaxLength()
{
    LayoutUnit computedBreadth = computeUsedBreadthOfSpecifiedLength(...);
    if (computedBreadth == -1)
	return infinity;
    return computedBreadth;
}

The nice thing about doing it this way is that you share more code and as you
add other value types to computeUsedBreadthOfSpecifiedLength, you don't need to
modify computeUsedBreadthOfMaxLength and computeUsedBreadthOfMinLength.

> Source/WebCore/rendering/RenderGrid.cpp:229
> +	   return LayoutUnit();

Nit: I'd just return 0 here.

> Source/WebCore/rendering/RenderGrid.cpp:232
> +	   return child->minPreferredLogicalWidth();

What should this return if the child has a fixed width/min-width/max-width set
directly on it? If it should ignore them, then you may need to use
minIntrsinsicLogicalWidth here.

> Source/WebCore/rendering/RenderGrid.cpp:239
> +	   // FIXME: We shouldn't unconditionally relayout the child but detect
a size change.
> +	   child->setNeedsLayout(true, MarkOnlyThis);

I don't understand why we need this here. We just did a layout. If we are going
to setNeedsLayout, I think it probably belongs somewhere else in the code (i.e.
the place where we modify the width/height such that the child now needs a
layout).

> Source/WebCore/rendering/RenderGrid.cpp:257
> +	       // FIXME: We should share this logical with the other branches.

This sentence is missing some words. Not sure what's it's trying to say.

> Source/WebCore/rendering/RenderGrid.cpp:259
> +		   size_t cellIndex = resolveGridPosition((direction ==
ForColumns) ? child->style()->gridItemColumn() :
child->style()->gridItemRow());

Can we make a version of resolveGridPosition that takes a direction and a
RenderObject* child? I think that would make this code here and below a bit
more readable.