[webkit-reviews] review denied: [Bug 108397] [CSS Grid Layout] Add parsing for grid-auto-flow : [Attachment 185610] Proposed change I.

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 108397: [CSS Grid Layout] Add parsing for grid-auto-flow
https://bugs.webkit.org/show_bug.cgi?id=108397

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

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


> Source/WebCore/css/CSSParser.cpp:2520
> +    case CSSPropertyWebkitGridAutoFlow:
> +	   if (id == CSSValueNone || id == CSSValueRows || id ==
CSSValueColumns)
> +	       validPrimitive = true;
> +	   break;

I think this should go in isValidKeywordPropertyAndValue.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:4117
> +

Remove this extra blank line.

> Source/WebCore/css/CSSValueKeywords.in:1005
> +// -webkit-grid-auto-flow
> +// none
> +columns
> +rows

Can you ask on www-style if we can make these column/row to match flex-flow? 
It's OK to check this in, but we shouldn't make web devs have to think whether
the values need to be plural or not.

> Source/WebCore/rendering/style/RenderStyleConstants.h:494
> +enum GridAutoFlow {
> +    AutoFlowNone,
> +    AutoFlowColumns,
> +    AutoFlowRows
> +};

Nit: Should this be on one line like many of the other enums?

> LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html:19
> +.gridAutoFlowNone {
> +    -webkit-grid-auto-flow: none;
> +}
> +.gridAutoFlowColumns {
> +    -webkit-grid-auto-flow: columns;
> +}
> +.gridAutoFlowRows {
> +    -webkit-grid-auto-flow: rows;
> +}
> +.gridAutoFlowInherit {

Can we move these into resources/grid.css?