Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(286)

Issue 149373004: [CSS Grid Layout] Implementation of the grid-template shorthand. (Closed)

Created:
6 years, 10 months ago by jfernandez
Modified:
6 years, 5 months ago
CC:
blink-reviews, dglazkov+blink, apavlov+blink_chromium.org, ed+blinkwatch_opera.com, darktears, Manuel Rego
Base URL:
https://chromium.googlesource.com/chromium/blink.git@grid-template-working
Visibility:
Public.

Description

[CSS Grid Layout] Implementation of the grid-template shorthand. This shorthand sets the values for the grid-template-columns, grid-template-rows and grid-template-areas, so the implementation tries to reuse as much available parsing functions as possible. The "parsingGridTrackList" was refactored to return a CSSValue and let the "parseValue" function to assign the property value. The "forwardSlash" operator is now valid when the track-list clause is part of a shorthand. The "parseValue" function checkouts that only additional clauses are allowed when processing shorthands; the grid-columns-rows-get-set.html tests was modified to verify this. The "parseGridTemplateAreas" was refactored too, in order to process single areas's rows. This is very useful for the gris-template secondary syntax, which mixes areas and rows values. Finally, the "parseGirdLineNames" function was modified as well by defining an new argument to concatenate head/tail custom-ident elements and ensure the identList is at the heading index, since it's now possible the parseList was rewound. The implementation of the grid-template shorthand tries first to match the <grid-template-columns> / <grid-template-rows> syntax, failing back to the secondary syntax if needed. This approach requires to rewind the parseList but it produces a clearer code. TEST=fast/css-grid-layout/grid-template-shorthand-get-set.html BUG=79180, 234196 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170552

Patch Set 1 #

Total comments: 10

Patch Set 2 : grid-template shorthand implementation. #

Patch Set 3 : minor issues in the layout tests. #

Patch Set 4 : Adding checks and layout tests to verify misplaced 'none' arguments. #

Total comments: 7

Patch Set 5 : New approach: simpler code, reusing track-list parsing and allow rewinding. #

Total comments: 11

Patch Set 6 : rebased and applied suggested changes. #

Total comments: 10

Patch Set 7 : Applied last suggested changes. #

Patch Set 8 : Patch rebased and adapted to the new parsing approach. #

Total comments: 6

Patch Set 9 : Applied suggested changes. #

Total comments: 8

Patch Set 10 : Patch rebased and applied suggested changes. #

Patch Set 11 : Fixed compilation error (warnings). #

Patch Set 12 : Added the new property to the ones runtime enabled only. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+665 lines, -76 lines) Patch
M LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-template-shorthand-get-set.html View 1 2 3 4 5 6 7 8 9 1 chunk +217 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/grid-template-shorthand-get-set-expected.txt View 1 2 3 4 5 1 chunk +170 lines, -0 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/resources/grid-template-shorthand-parsing-utils.js View 1 2 3 4 5 1 chunk +51 lines, -0 lines 0 comments Download
M LayoutTests/platform/android/webexposed/css-properties-as-js-properties-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/webexposed/css-properties-as-js-properties-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSComputedStyleDeclaration.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/css/CSSParserValues.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/css/CSSProperty.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSPropertyNames.in View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSShorthands.in View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/RuntimeCSSEnabled.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/parser/CSSPropertyParser.h View 1 2 3 4 5 6 7 8 9 3 chunks +7 lines, -2 lines 0 comments Download
M Source/core/css/parser/CSSPropertyParser.cpp View 1 2 3 4 5 6 7 8 9 7 chunks +200 lines, -73 lines 0 comments Download
M Source/core/css/resolver/StyleBuilderCustom.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/frame/UseCounter.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 32 (0 generated)
svillar
Looking good, it needs some more work but I understand it's a complex syntax. BTW ...
6 years, 10 months ago (2014-02-07 12:02:49 UTC) #1
jfernandez
Applied changes suggested by svillar in the informal review and added new layout tests.
6 years, 10 months ago (2014-02-17 22:46:47 UTC) #2
jfernandez
Added suggested OWNERS.
6 years, 10 months ago (2014-02-17 22:51:05 UTC) #3
jfernandez
Fixed some mistakes in the layout tests.
6 years, 10 months ago (2014-02-20 11:53:22 UTC) #4
jfernandez
Additional checks and layout tests.
6 years, 10 months ago (2014-02-21 12:27:13 UTC) #5
Julien - ping for review
Sorry for the long delay in the review. I haven't looked much at the new ...
6 years, 10 months ago (2014-02-21 20:04:50 UTC) #6
jfernandez
On 2014/02/21 20:04:50, Julien Chaffraix - PST wrote: > Sorry for the long delay in ...
6 years, 10 months ago (2014-02-21 20:52:46 UTC) #7
Julien - ping for review
On 2014/02/21 20:52:46, jfernandez wrote: > On 2014/02/21 20:04:50, Julien Chaffraix - PST wrote: > ...
6 years, 10 months ago (2014-02-21 22:49:15 UTC) #8
jfernandez
Implemented the suggested approach of matching first the <grid-template-columns> / <grid-template-rows> syntax and failing back ...
6 years, 10 months ago (2014-02-24 12:02:26 UTC) #9
Julien - ping for review
Some comments but I like the direction this is going! https://codereview.chromium.org/149373004/diff/390001/Source/core/css/parser/BisonCSSParser-in.cpp File Source/core/css/parser/BisonCSSParser-in.cpp (right): https://codereview.chromium.org/149373004/diff/390001/Source/core/css/parser/BisonCSSParser-in.cpp#newcode4656 ...
6 years, 10 months ago (2014-02-26 21:53:37 UTC) #10
jfernandez
https://codereview.chromium.org/149373004/diff/390001/Source/core/css/parser/BisonCSSParser-in.cpp File Source/core/css/parser/BisonCSSParser-in.cpp (right): https://codereview.chromium.org/149373004/diff/390001/Source/core/css/parser/BisonCSSParser-in.cpp#newcode4656 Source/core/css/parser/BisonCSSParser-in.cpp:4656: addProperty(CSSPropertyGridTemplateRows, templateRows.release(), important); On 2014/02/26 21:53:38, Julien Chaffraix - ...
6 years, 9 months ago (2014-02-27 11:18:14 UTC) #11
jfernandez
I had the change to adapt the patch to the new CSSPropertyParser class, which required ...
6 years, 9 months ago (2014-02-28 17:20:14 UTC) #12
Julien - ping for review
https://codereview.chromium.org/149373004/diff/430001/Source/core/css/parser/BisonCSSParser-in.cpp File Source/core/css/parser/BisonCSSParser-in.cpp (right): https://codereview.chromium.org/149373004/diff/430001/Source/core/css/parser/BisonCSSParser-in.cpp#newcode4634 Source/core/css/parser/BisonCSSParser-in.cpp:4634: parseGridLineNames(m_valueList.get(), *templateRows, static_cast<CSSGridLineNamesValue*>(templateRows->item(templateRows->length() - 1))); I don't think I ...
6 years, 9 months ago (2014-03-01 01:56:11 UTC) #13
jfernandez
https://codereview.chromium.org/149373004/diff/430001/Source/core/css/parser/BisonCSSParser-in.cpp File Source/core/css/parser/BisonCSSParser-in.cpp (right): https://codereview.chromium.org/149373004/diff/430001/Source/core/css/parser/BisonCSSParser-in.cpp#newcode4634 Source/core/css/parser/BisonCSSParser-in.cpp:4634: parseGridLineNames(m_valueList.get(), *templateRows, static_cast<CSSGridLineNamesValue*>(templateRows->item(templateRows->length() - 1))); On 2014/03/01 01:56:12, Julien ...
6 years, 9 months ago (2014-03-03 13:02:08 UTC) #14
jfernandez
Patch rebased, since the CSS parsing code changed considerably.
6 years, 9 months ago (2014-03-20 14:56:50 UTC) #15
Julien - ping for review
https://codereview.chromium.org/149373004/diff/470001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/149373004/diff/470001/Source/core/css/parser/CSSPropertyParser.cpp#newcode3457 Source/core/css/parser/CSSPropertyParser.cpp:3457: bool traillingIdentAdded = false; This name is not English ...
6 years, 9 months ago (2014-03-21 19:25:54 UTC) #16
jfernandez
https://codereview.chromium.org/149373004/diff/470001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/149373004/diff/470001/Source/core/css/parser/CSSPropertyParser.cpp#newcode3457 Source/core/css/parser/CSSPropertyParser.cpp:3457: bool traillingIdentAdded = false; On 2014/03/21 19:25:55, Julien Chaffraix ...
6 years, 9 months ago (2014-03-22 00:30:45 UTC) #17
svillar
I don't know if you took this into account but check what they say about ...
6 years, 9 months ago (2014-03-24 08:58:59 UTC) #18
jfernandez
On 2014/03/24 08:58:59, svillar wrote: > I don't know if you took this into account ...
6 years, 9 months ago (2014-03-24 09:39:48 UTC) #19
Julien - ping for review
lgtm https://codereview.chromium.org/149373004/diff/490001/LayoutTests/fast/css-grid-layout/grid-template-shorthand-get-set.html File LayoutTests/fast/css-grid-layout/grid-template-shorthand-get-set.html (right): https://codereview.chromium.org/149373004/diff/490001/LayoutTests/fast/css-grid-layout/grid-template-shorthand-get-set.html#newcode6 LayoutTests/fast/css-grid-layout/grid-template-shorthand-get-set.html:6: testRunner.overridePreference("WebKitCSSGridLayoutEnabled", 1); This line is unneeded. https://codereview.chromium.org/149373004/diff/490001/Source/core/css/CSSComputedStyleDeclaration.cpp File ...
6 years, 8 months ago (2014-03-31 17:56:29 UTC) #20
jfernandez
Applied most of the suggested changes. I've also simplified the "none" simple case so it's ...
6 years, 8 months ago (2014-03-31 23:02:13 UTC) #21
jfernandez
The CQ bit was checked by jfernandez@igalia.com
6 years, 8 months ago (2014-03-31 23:04:17 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfernandez@igalia.com/149373004/510001
6 years, 8 months ago (2014-03-31 23:04:22 UTC) #23
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-03-31 23:42:06 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on mac_blink_rel
6 years, 8 months ago (2014-03-31 23:42:08 UTC) #25
jfernandez
The CQ bit was checked by jfernandez@igalia.com
6 years, 8 months ago (2014-04-01 09:03:46 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfernandez@igalia.com/149373004/530001
6 years, 8 months ago (2014-04-01 09:03:53 UTC) #27
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-01 09:39:30 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on linux_blink_rel tryserver.blink on mac_blink_rel
6 years, 8 months ago (2014-04-01 09:39:31 UTC) #29
jfernandez
The CQ bit was checked by jfernandez@igalia.com
6 years, 8 months ago (2014-04-01 10:40:24 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfernandez@igalia.com/149373004/550001
6 years, 8 months ago (2014-04-01 10:40:34 UTC) #31
commit-bot: I haz the power
6 years, 8 months ago (2014-04-01 11:48:57 UTC) #32
Message was sent while issue was closed.
Change committed as 170552

Powered by Google App Engine
This is Rietveld 408576698