[webkit-reviews] review granted: [Bug 134057] [CSS Grid Layout] Update grid-auto-flow to the new syntax : [Attachment 234706] Patch

[bugzilla-daemon at webkit.org]

Sergio Villar Senin <svillar at igalia.com> has granted Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 134057: [CSS Grid Layout] Update grid-auto-flow to the new syntax
https://bugs.webkit.org/show_bug.cgi?id=134057

Attachment 234706: Patch
https://bugs.webkit.org/attachment.cgi?id=234706&action=review

------- Additional Comments from Sergio Villar Senin <svillar at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234706&action=review


Awesome! Thanks for the patch.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2057
> +	       else if (style->isGridAutoFlowDirectionColumn())

Is there any other possibility? I mean the direction is either row or column so
we can use a simple "else". If we want to be ultra-sure we could always add an
ASSERT(isColumn() || isRow()).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2063
> +		  
list->append(cssValuePool().createIdentifierValue(CSSValueStack));

Same comment here.

> Source/WebCore/css/CSSParser.cpp:5191
> +    // Second parameter, if any.

We must improve this comment mentioning that this can be used by the grid
shorthand. It isn't obvious from the code why we don't bail out in some,
apparently erroneous cases.