Summary: | [CSS Grid Layout] Implementation of the grid-template shorthand. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Javier Fernandez <jfernandez> | ||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, jfernandez, kling, macpherson, menard, rego, svillar, syoichi | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 129041 | ||||||||
Bug Blocks: | 127987, 132122 | ||||||||
Attachments: |
|
Description
Javier Fernandez
2014-02-18 10:02:37 PST
Created attachment 228512 [details]
Implementation of the grid-template shorthand.
Attachment 228512 [details] did not pass style-queue:
ERROR: Source/WebCore/css/CSSParser.cpp:5266: Wrong number of spaces before statement. (expected: 24) [whitespace/indent] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > Attachment 228512 [details] did not pass style-queue: > > > ERROR: Source/WebCore/css/CSSParser.cpp:5266: Wrong number of spaces before statement. (expected: 24) [whitespace/indent] [4] > Total errors found: 1 in 12 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. I think this issue is already addressed in bug #128751 Comment on attachment 228512 [details] Implementation of the grid-template shorthand. View in context: https://bugs.webkit.org/attachment.cgi?id=228512&action=review > Source/WebCore/css/CSSParser.cpp:4809 > + size_t rowCount = 0; > + size_t columnCount = 0; size_t seems like overkill for these. I don’t think we handle those kinds of huge row and column counts. Why not just “unsigned”? > Source/WebCore/css/CSSParser.cpp:4811 > + RefPtr<CSSValueList> templateRows = CSSValueList::createSpaceSeparated(); Why create this just before an early return? I suggest moving this declaration down. > Source/WebCore/css/CSSParser.cpp:4817 > + while (m_valueList->current()) { could write this as a do/while to avoid the redundant check at the start of the loop > Source/WebCore/css/CSSParser.cpp:4837 > + templateRows->append(value); value.release() would avoid a bit of reference count churn > Source/WebCore/css/CSSParser.cpp:4853 > + addProperty(CSSPropertyWebkitGridTemplateColumns, cssValuePool().createIdentifierValue(CSSValueNone), important); Extra space here after the comma. > Source/WebCore/css/CSSParser.cpp:4895 > + addProperty(CSSPropertyWebkitGridTemplateColumns, columnsValue, important); columnsValue.release() > Source/WebCore/css/CSSParser.cpp:4896 > + addProperty(CSSPropertyWebkitGridTemplateRows, rowsValue, important); rowsValue.release() > Source/WebCore/css/CSSParser.cpp:5029 > + return values; values.release() > Source/WebCore/css/CSSParser.cpp:5273 > + if (!gridRowNames.length()) isEmpty is preferred over !length > Source/WebCore/css/CSSParser.cpp:5296 > + for (lookAheadColumn = currentColumn; lookAheadColumn < (columnCount - 1); ++lookAheadColumn) { Parentheses here don’t make the code clearer. I suggest omitting them. > Source/WebCore/css/CSSParser.h:50 > + > + Extra space here. Just one blank line will do. > Source/WebCore/css/CSSParser.h:77 > +#if ENABLE(CSS_GRID_LAYOUT) > +class CSSGridLineNamesValue; > +#endif Not really helpful to put forward declarations inside an #if. I suggest just declaring this unconditionally. > Source/WebCore/css/CSSParser.h:178 > + void parseGridLineNames(CSSParserValueList&, CSSValueList&, CSSGridLineNamesValue* = 0); nullptr Created attachment 230158 [details]
Applied suggested changes.
Attachment 230158 [details] did not pass style-queue:
ERROR: Source/WebCore/css/CSSParser.cpp:5265: Wrong number of spaces before statement. (expected: 24) [whitespace/indent] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > Attachment 230158 [details] did not pass style-queue: > > > ERROR: Source/WebCore/css/CSSParser.cpp:5265: Wrong number of spaces before statement. (expected: 24) [whitespace/indent] [4] > Total errors found: 1 in 12 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. I think this issue is already addressed in bug #128751 Comment on attachment 230158 [details] Applied suggested changes. Clearing flags on attachment: 230158 Committed r167799: <http://trac.webkit.org/changeset/167799> All reviewed patches have been landed. Closing bug. This broke the Debug build. fixing it in bug #132194. This broke the Debug build. fixing it in bug #132197 |