Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(21)

Issue 14786002: Allow defining named grid lines on the grid element (Closed)

Created:
7 years, 5 months ago by Julien - ping for review
Modified:
7 years, 4 months ago
Reviewers:
esprehn
CC:
blink-reviews, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, ojan, eseidel
Visibility:
Public.

Description

Allow defining named grid lines on the grid element This change adds parsing, style resolution and getComputedStyle support for named grid lines at the grid element level (ie extends our <track-list> support). Per the specification, we allow multiple grid lines with the same name. To fully support resolving the grid lines to a position on our grid, we need to add the parsing at the grid item's level (which means extending our <grid-line> support). This will be done in a follow-up change. BUG=234192 TEST=fast/css-grid-layout/named-grid-line-get-set.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149798

Patch Set 1 #

Total comments: 8

Patch Set 2 : Patch updated after Elliott's comments #

Total comments: 5

Patch Set 3 : Updated patch for landing #

Patch Set 4 : Rebaselined patch for try job / landing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+280 lines, -9 lines) Patch
A LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html View 1 2 1 chunk +153 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-grid-layout/named-grid-line-get-set-expected.txt View 1 2 1 chunk +52 lines, -0 lines 0 comments Download
M Source/core/css/CSSComputedStyleDeclaration.cpp View 1 2 chunks +29 lines, -5 lines 0 comments Download
M Source/core/css/CSSParser.cpp View 1 1 chunk +11 lines, -0 lines 0 comments Download
M Source/core/css/StyleResolver.cpp View 1 2 3 3 chunks +22 lines, -3 lines 0 comments Download
M Source/core/rendering/style/RenderStyle.h View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M Source/core/rendering/style/StyleGridData.h View 1 3 chunks +7 lines, -1 line 0 comments Download
M Source/core/rendering/style/StyleGridData.cpp View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
Julien - ping for review
7 years, 4 months ago (2013-05-01 23:20:10 UTC) #1
esprehn
https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right): https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html#newcode47 LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:47: function testCSSValue(gridElementId, _namedGridColumns, _namedGridRows) What's with the _ ? ...
7 years, 4 months ago (2013-05-03 21:45:58 UTC) #2
Julien - ping for review
https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right): https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html#newcode47 LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:47: function testCSSValue(gridElementId, _namedGridColumns, _namedGridRows) On 2013/05/03 21:45:58, esprehn wrote: ...
7 years, 4 months ago (2013-05-06 16:22:32 UTC) #3
Julien - ping for review
Elliott, updated patch available. https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right): https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html#newcode61 LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:61: testCSSValue("gridWithFixedMultiple", "'nav first 10px last'", ...
7 years, 4 months ago (2013-05-06 16:58:40 UTC) #4
esprehn
LGTM. A bunch of code in that test looks like duplication though, I think you ...
7 years, 4 months ago (2013-05-06 17:06:23 UTC) #5
Julien - ping for review
https://codereview.chromium.org/14786002/diff/9001/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right): https://codereview.chromium.org/14786002/diff/9001/LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html#newcode1 LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Mhh, this should have ...
7 years, 4 months ago (2013-05-06 19:59:01 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/14786002/18001
7 years, 4 months ago (2013-05-06 20:52:55 UTC) #7
commit-bot: I haz the power
7 years, 4 months ago (2013-05-06 21:17:31 UTC) #8
Message was sent while issue was closed.
Change committed as 149798

Powered by Google App Engine
This is Rietveld 408576698