[webkit-reviews] review granted: [Bug 103799] [CSS Grid Layout] Implement CSS parsing and handling for <track-minmax> : [Attachment 177283] Proposed change.

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 103799: [CSS Grid Layout] Implement CSS parsing and handling for
<track-minmax>
https://bugs.webkit.org/show_bug.cgi?id=103799

Attachment 177283: Proposed change.
https://bugs.webkit.org/attachment.cgi?id=177283&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=177283&action=review


Please add some more test cases before landing.

> Source/WebCore/css/CSSFunctionValue.h:54
> +    explicit CSSFunctionValue(String, PassRefPtr<CSSValueList>);

Nit: No explicit.

> Source/WebCore/css/CSSParser.cpp:4538
> +	   // We only accept the following grammar: minmax( <track-breadth> ,
<track-breadth> )

It's not clear to me what 'we' refers to here. I would probably say, "The spec
defines the following grammar:".

> Source/WebCore/rendering/style/GridTrackSize.h:64
> +	   m_minTrackBreadth = length;
> +	   m_maxTrackBreadth = length;

I see, you set both min and max so you can simplify operator==?  I would make
operator== more complex instead.

> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt:18
> +PASS getComputedStyle(gridWithMinMax,
'').getPropertyValue('-webkit-grid-columns') is 'minmax(10%, 15px)'
> +PASS getComputedStyle(gridWithMinMax,
'').getPropertyValue('-webkit-grid-rows') is 'minmax(20px, 50%)'

Can you add some test cases with 'auto', '-webkit-min-content' and
'-webkit-calc' as arguments to minmax?.