Bug 158063 – [css-grid] Update <fixed-size> syntax

• Component: CSS
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

The syntax for <fixed-size> has been updated on the spec:
https://drafts.csswg.org/css-grid/#typedef-fixed-size

New syntax is:
  <fixed-size> =
    <fixed-breadth> |
    minmax( <fixed-breadth> , <track-breadth> ) |
    minmax( <inflexible-breadth> , <fixed-breadth> )

This has been already fixed on Blink: https://codereview.chromium.org/2001113002/

[Manuel Rego Casasnovas]

Created attachment 279755 [details]
Patch

[Darin Adler]

Please see the comments in bug 158021. Assertions are firing on the bots right now. We shouldn’t make more change here until this is resolved.

[Manuel Rego Casasnovas]

Comment on attachment 279755 [details]
Patch

(In reply to comment #2)
> Please see the comments in bug 158021. Assertions are firing on the bots
> right now. We shouldn’t make more change here until this is resolved.

Yeah sure, I'll upload a new version once the other bug is fixed.

[Manuel Rego Casasnovas]

Created attachment 279772 [details]
Rebased patch

[Darin Adler]

Comment on attachment 279772 [details]
Rebased patch

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

> Source/WebCore/css/CSSParser.cpp:5820
> -static bool isGridTrackFixedSized(const CSSValue& value)
> +static bool isGridTrackFixedSized(const CSSPrimitiveValue& primitiveValue)

I don’t think the argument name has to change here. Should just be value. No need to constantly restate the fact that it’s a primitiveValue.

> Source/WebCore/css/CSSParser.cpp:5832
> +    ASSERT(value.isPrimitiveValue() || (value.isFunctionValue() && downcast<CSSFunctionValue>(value).arguments()));

Cleaner to assert *after* the isPrimitiveValue check and then would not need the || here.

> Source/WebCore/css/CSSParser.cpp:5838
> +    const auto& minPrimitiveValue = downcast<CSSPrimitiveValue>(*downcast<CSSFunctionValue>(value).arguments()->item(0));
> +    const auto& maxPrimitiveValue = downcast<CSSPrimitiveValue>(*downcast<CSSFunctionValue>(value).arguments()->item(1));

Is there something that guarantees we have two arguments?

I suggest using auto&, not const auto&. Not all that helpful to add the const. I would name the local variables just min and max; no need to state the type in the variable name.

[Manuel Rego Casasnovas]

Comment on attachment 279772 [details]
Rebased patch

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

Thanks for the review.

>> Source/WebCore/css/CSSParser.cpp:5820
>> +static bool isGridTrackFixedSized(const CSSPrimitiveValue& primitiveValue)
> 
> I don’t think the argument name has to change here. Should just be value. No need to constantly restate the fact that it’s a primitiveValue.

ACK.

>> Source/WebCore/css/CSSParser.cpp:5832
>> +    ASSERT(value.isPrimitiveValue() || (value.isFunctionValue() && downcast<CSSFunctionValue>(value).arguments()));
> 
> Cleaner to assert *after* the isPrimitiveValue check and then would not need the || here.

Done.

>> Source/WebCore/css/CSSParser.cpp:5838
>> +    const auto& maxPrimitiveValue = downcast<CSSPrimitiveValue>(*downcast<CSSFunctionValue>(value).arguments()->item(1));
> 
> Is there something that guarantees we have two arguments?
> 
> I suggest using auto&, not const auto&. Not all that helpful to add the const. I would name the local variables just min and max; no need to state the type in the variable name.

It's a minmax() function, so we always have 2 arguments. Anyway I've added another ASSERT to be sure that we've 2 arguments.

Changed to "auto&" and renamed variables too.

[Manuel Rego Casasnovas]

Created attachment 279790 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 279790 [details]
Patch

Clearing flags on attachment: 279790

Committed r201399: <http://trac.webkit.org/changeset/201399>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.