[webkit-reviews] review granted: [Bug 234430] [css-grid] Fix grid shorthand expansion of initial values : [Attachment 447449] Patch

[bugzilla-daemon at webkit.org]

Sergio Villar Senin <svillar at igalia.com> has granted zsun at igalia.com's request
for review:
Bug 234430: [css-grid] Fix grid shorthand expansion of initial values
https://bugs.webkit.org/show_bug.cgi?id=234430

Attachment 447449: Patch

https://bugs.webkit.org/attachment.cgi?id=447449&action=review




--- Comment #5 from Sergio Villar Senin <svillar at igalia.com> ---
Comment on attachment 447449
  --> https://bugs.webkit.org/attachment.cgi?id=447449
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447449&action=review

> Source/WebCore/style/StyleBuilderConverter.h:1166
> +	   ASSERT(downcast<CSSPrimitiveValue>(value).isValueID());

A bit weird to have a if for just an ASSERT. I'd better do

ASSERT(!is<CSSPrimitiveValue>(value) ||
downcast<CSSPrimitiveValue>(value).isValueID());

> Source/WebCore/style/StyleBuilderConverter.h:1171
>	   return RenderStyle::initialGridAutoFlow();

Seems like you have to indent this return

> Source/WebCore/style/StyleBuilderConverter.h:1175
> +    auto* second = downcast<CSSPrimitiveValue>(is<CSSValueList>(value) &&
downcast<CSSValueList>(value).length() == 2 ?
downcast<CSSValueList>(value).item(1) : nullptr);

Looks like we could store is<CSSValueList>(value) in a boolean since it's also
used in the if block above.