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/
Created attachment 279755 [details] Patch
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.
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.
Created attachment 279772 [details] Rebased patch
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.
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.
Created attachment 279790 [details] Patch
Comment on attachment 279790 [details] Patch Clearing flags on attachment: 279790 Committed r201399: <http://trac.webkit.org/changeset/201399>
All reviewed patches have been landed. Closing bug.