Bug 158063

Summary: [css-grid] Update <fixed-size> syntax
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: CSSAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, jfernandez, rego, svillar
Priority: P2 Keywords: BlinkMergeCandidate
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.chromium.org/p/chromium/issues/detail?id=611412
Bug Depends on: 158021    
Bug Blocks: 60731    
Attachments:
Description Flags
Patch
none
Rebased patch
none
Patch none

Manuel Rego Casasnovas
Reported 2016-05-25 03:36:06 PDT
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/
Attachments
Patch (9.86 KB, patch)
2016-05-25 03:46 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Rebased patch (9.90 KB, patch)
2016-05-25 08:12 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (10.15 KB, patch)
2016-05-25 12:21 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1 2016-05-25 03:46:08 PDT
Created attachment 279755 [details] Patch
Darin Adler
Comment 2 2016-05-25 04:06:02 PDT
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 3 2016-05-25 06:14:42 PDT
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
Comment 4 2016-05-25 08:12:25 PDT
Created attachment 279772 [details] Rebased patch
Darin Adler
Comment 5 2016-05-25 09:19:18 PDT
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 6 2016-05-25 12:20:24 PDT
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
Comment 7 2016-05-25 12:21:50 PDT
Created attachment 279790 [details] Patch
WebKit Commit Bot
Comment 8 2016-05-25 12:51:44 PDT
Comment on attachment 279790 [details] Patch Clearing flags on attachment: 279790 Committed r201399: <http://trac.webkit.org/changeset/201399>
WebKit Commit Bot
Comment 9 2016-05-25 12:51:48 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.