[webkit-reviews] review denied: [Bug 108403] [CSS Grid Layout] computePreferredLogicalWidths doesn't handle minmax tracks : [Attachment 185859] Proposed fix I: Updated computePreferredLogicalWidths to handle all minmax combination.

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 108403: [CSS Grid Layout] computePreferredLogicalWidths doesn't handle
minmax tracks
https://bugs.webkit.org/show_bug.cgi?id=108403

Attachment 185859: Proposed fix I: Updated computePreferredLogicalWidths to
handle all minmax combination.
https://bugs.webkit.org/attachment.cgi?id=185859&action=review

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


Just a few minor issues.

> Source/WebCore/rendering/RenderGrid.cpp:164
> +LayoutUnit RenderGrid::resolvePreferredLogicalWidth(PreferredWidthType type,
size_t trackIndex) const

Instead of passing in the type, can we just pass in the Length itself? Then we
don't need the enum, which seems better to me.

> Source/WebCore/rendering/RenderGrid.cpp:169
> +    if (length.isFixed())
> +	   return length.intValue();

It's not possible to put border/padding/margin on a grid cell, right? If so, it
might be worth adding a comment here about that being why it's OK to not worry
about them here.

> Source/WebCore/rendering/RenderGrid.cpp:178
> +	       // FIXME: We should include the child's fixed margins like
RenderBlock.

RenderBlock does crazy with margins. You might want to point to
RenderFlexibleBox as a closer example. In fact, I think we probably want to do
exactly what RenderFlexibleBox does, which might involve moving
marginLogicalWidthForChild up into RenderBlock so it can be shared.

> Source/WebCore/rendering/RenderGrid.cpp:198
> +    // FIXME: What should we do for the other values (e.g. <percentage> and
calc())
> +    return 0;

Right now, every other class treats anything other than Fixed the same a Auto.
The sizing spec says to resolve "definite" lengths, but we don't do that
anywhere currently. We should try to and see if we can get away with it without
breaking the web, but I've been putting that off until I'm done reducing code
duplication across computePreferredLogicalWidth overrides.

> LayoutTests/fast/css-grid-layout/grid-preferred-logical-widths.html:23
> +.gridMinMinContent {
> +    -webkit-grid-columns: minmax(-webkit-min-content, 40px)
minmax(-webkit-min-content, 40px);
> +    -webkit-grid-rows: 10px;
> +    font: 10px/1 Ahem;
> +}
> +
> +.gridMinMaxContent {
> +    -webkit-grid-columns: minmax(30px, -webkit-min-content) minmax(30px,
-webkit-min-content);
> +    -webkit-grid-rows: 10px;
> +    font: 10px/1 Ahem;
> +}
> +
> +.gridMaxContent {

Nit: these class names don't seem to use a consistent naming scheme.