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

Issue 1168453002: [CSSGridLayout] Switch from parentheses to brackets for grid line names (Closed)

Created:
5 years, 6 months ago by svillar
Modified:
5 years, 6 months ago
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, dglazkov+blink, jfernandez, Manuel Rego, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

[CSSGridLayout] Switch from parentheses to brackets for grid line names Grid line names are now wrapped in square brackets instead of parentheses as agreed in the latest F2F meeting of the CSSWG, see https://lists.w3.org/Archives/Public/www-style/2015May/0175.html. Apparently they're easier to read and compatible with SASS. BUG=487995 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196568

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fixed test and parsing #

Total comments: 14

Patch Set 3 : Patch for landing #

Patch Set 4 : Patch for landing v2 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+540 lines, -519 lines) Patch
M LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt View 1 chunk +4 lines, -4 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple-expected.txt View 1 chunk +6 lines, -6 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-container-change-named-grid-lines-recompute-child.html View 1 chunk +4 lines, -4 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-element-repeat-get-set.html View 3 chunks +18 lines, -18 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-element-repeat-get-set-expected.txt View 1 chunk +11 lines, -11 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-item-auto-placement-automatic-span.html View 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-item-bad-resolution-double-span.html View 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-item-column-row-get-set.html View 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-item-named-grid-area-resolution.html View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-item-named-grid-line-resolution.html View 1 chunk +5 lines, -5 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-item-negative-position-resolution.html View 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-item-position-changed-dynamic.html View 1 chunk +4 lines, -4 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-shorthand-get-set-expected.txt View 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-template-shorthand-get-set.html View 6 chunks +27 lines, -27 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/grid-template-shorthand-get-set-expected.txt View 4 chunks +10 lines, -10 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html View 1 2 2 chunks +77 lines, -70 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/named-grid-line-get-set-expected.txt View 1 2 2 chunks +88 lines, -84 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks.html View 2 chunks +12 lines, -12 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks-expected.txt View 1 chunk +16 lines, -16 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/named-grid-lines-with-named-grid-areas-dynamic-get-set.html View 1 chunk +6 lines, -6 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/named-grid-lines-with-named-grid-areas-get-set.html View 1 chunk +10 lines, -10 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/named-grid-lines-with-named-grid-areas-resolution.html View 1 chunk +12 lines, -12 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/non-grid-columns-rows-get-set-expected.txt View 1 chunk +4 lines, -4 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/non-grid-columns-rows-get-set-multiple-expected.txt View 1 chunk +6 lines, -6 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/non-grid-element-repeat-get-set.html View 2 chunks +17 lines, -17 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/non-grid-element-repeat-get-set-expected.txt View 1 chunk +11 lines, -11 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/non-named-grid-line-get-set.html View 2 chunks +38 lines, -38 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/non-named-grid-line-get-set-expected.txt View 1 chunk +64 lines, -64 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js View 3 chunks +8 lines, -8 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set-multiple.js View 2 chunks +7 lines, -7 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/resources/non-grid-columns-rows-get-set.js View 2 chunks +8 lines, -8 lines 0 comments Download
M LayoutTests/fast/css-grid-layout/resources/non-grid-columns-rows-get-set-multiple.js View 2 chunks +7 lines, -7 lines 0 comments Download
M Source/core/css/CSSGridLineNamesValue.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/parser/CSSPropertyParser.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/parser/CSSPropertyParser.cpp View 1 2 9 chunks +42 lines, -32 lines 0 comments Download
M Source/core/css/parser/CSSPropertyParserTest.cpp View 1 2 3 1 chunk +4 lines, -4 lines 0 comments Download

Messages

Total messages: 22 (8 generated)
svillar
Sending for review. Should be easy.
5 years, 6 months ago (2015-06-02 09:04:16 UTC) #2
Manuel Rego
The patch seems good to me. Just some small suggestions in the tests. Regarding the ...
5 years, 6 months ago (2015-06-04 11:38:39 UTC) #4
svillar
On 2015/06/04 11:38:39, Manuel Rego wrote: > The patch seems good to me. Just some ...
5 years, 6 months ago (2015-06-04 14:28:11 UTC) #5
svillar
Apart from the test I also had to update the parsing code, as with the ...
5 years, 6 months ago (2015-06-04 16:30:52 UTC) #6
jfernandez
lgtm https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/CSSPropertyParser.cpp#newcode3231 Source/core/css/parser/CSSPropertyParser.cpp:3231: Unneeded white line. https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/CSSPropertyParser.cpp#newcode3442 Source/core/css/parser/CSSPropertyParser.cpp:3442: static inline bool ...
5 years, 6 months ago (2015-06-04 22:49:57 UTC) #8
Julien - ping for review
lgtm, the link in the description is wrong and should be: https://lists.w3.org/Archives/Public/www-style/2015May/0175.html https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp ...
5 years, 6 months ago (2015-06-04 23:17:58 UTC) #9
svillar
Thanks for the reviews! https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/CSSPropertyParser.cpp#newcode3233 Source/core/css/parser/CSSPropertyParser.cpp:3233: return false; On 2015/06/04 23:17:58, ...
5 years, 6 months ago (2015-06-05 07:25:43 UTC) #10
svillar
Applied the suggested changes. I also took the chance to use an already existing function ...
5 years, 6 months ago (2015-06-05 08:38:44 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1168453002/40001
5 years, 6 months ago (2015-06-05 08:50:37 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/57814)
5 years, 6 months ago (2015-06-05 09:11:10 UTC) #16
svillar
Now with unit tests also ported to the new syntax
5 years, 6 months ago (2015-06-05 09:31:35 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1168453002/60001
5 years, 6 months ago (2015-06-05 09:32:23 UTC) #20
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196568
5 years, 6 months ago (2015-06-05 11:00:42 UTC) #21
Julien - ping for review
5 years, 6 months ago (2015-06-05 16:51:20 UTC) #22
Message was sent while issue was closed.
https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3551: if
(!maybeParseGridLineNames(*arguments, *repeatedValues))
On 2015/06/05 at 07:25:43, svillar wrote:
> On 2015/06/04 23:17:58, Julien Chaffraix - PST wrote:
> > This function is not 'maybe' parsing anymore if we make all the caller
reject
> > the value list when it returns false. It should be rename to
parseGridLineNames.
> 
> Sure, I also thought about that.
> 
> > I am concerned that we implemented too complicated a logic due to those
maybe
> > function.
> 
> Not sure what you exactly mean. With the old parser it made total sense,
because we were getting a proper CSSParserValueList from the parser in the event
of having a valid ident list. Right now that has totally changed and we have to
do that parsing on our own.

The comment was basically that with the new parser, maybe a lot of the
complexity in the code is not needed anymore (as embodied by the maybe*
functions) and we could follow with a simplification. Again this was me thinking
out loud.

Powered by Google App Engine
This is Rietveld 408576698