Allow defining named grid lines on the grid element

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

[esprehn, 2013-05-03 21:45:58]

https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-lay...
File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):

https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-lay...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:47: function
testCSSValue(gridElementId, _namedGridColumns, _namedGridRows)
What's with the _ ?

https://codereview.chromium.org/14786002/diff/1/Source/core/css/CSSComputedSt...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/14786002/diff/1/Source/core/css/CSSComputedSt...
Source/core/css/CSSComputedStyleDeclaration.cpp:1045: Vector<size_t>
linesIndexes = it->value;
const Vector<size_t>& ? I don't think you want to copy the vector as you go
through the loop.

https://codereview.chromium.org/14786002/diff/1/Source/core/rendering/style/S...
File Source/core/rendering/style/StyleGridData.h (right):

https://codereview.chromium.org/14786002/diff/1/Source/core/rendering/style/S...
Source/core/rendering/style/StyleGridData.h:39: 
NamedGridLinesMap to be consistent with other usage. We don't usually do
"Mapping"

[Julien - ping for review, 2013-05-06 16:22:32]

https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-lay...
File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):

https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-lay...
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:
> What's with the _ ?

I wanted to differentiate between the argument and the global variable. It turns
out that you can pass the argument as-is so the extra variables are unneeded.

https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-lay...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:61:
testCSSValue("gridWithFixedMultiple", "'nav first 10px last'", "'nav first 15px
last'");
I am not super happy with the current output of getComputedStyle because it's
super ambiguous. If you get "auto auto auto", you don't know whether it means:
* "auto" auto "auto" (1 grid tracks, 2 named grid lines)
* auto auto auto (3 grid tracks, no named grid line)
* auto "auto" auto (2 grid tracks, 1 named grid line)

I will see if I can get a better output

https://codereview.chromium.org/14786002/diff/1/Source/core/css/CSSComputedSt...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/14786002/diff/1/Source/core/css/CSSComputedSt...
Source/core/css/CSSComputedStyleDeclaration.cpp:1045: Vector<size_t>
linesIndexes = it->value;
On 2013/05/03 21:45:58, esprehn wrote:
> const Vector<size_t>& ? I don't think you want to copy the vector as you go
> through the loop.

Good catch (though we don't do a deep copy as Vector is smart).

https://codereview.chromium.org/14786002/diff/1/Source/core/rendering/style/S...
File Source/core/rendering/style/StyleGridData.h (right):

https://codereview.chromium.org/14786002/diff/1/Source/core/rendering/style/S...
Source/core/rendering/style/StyleGridData.h:39: 
On 2013/05/03 21:45:58, esprehn wrote:
> NamedGridLinesMap to be consistent with other usage. We don't usually do
> "Mapping"

Will be changed.

[Julien - ping for review, 2013-05-06 16:58:40]

Elliott, updated patch available.

https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-lay...
File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):

https://codereview.chromium.org/14786002/diff/1/LayoutTests/fast/css-grid-lay...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:61:
testCSSValue("gridWithFixedMultiple", "'nav first 10px last'", "'nav first 15px
last'");
On 2013/05/06 16:22:33, Julien Chaffraix wrote:
> I am not super happy with the current output of getComputedStyle because it's
> super ambiguous. If you get "auto auto auto", you don't know whether it means:
> * "auto" auto "auto" (1 grid tracks, 2 named grid lines)
> * auto auto auto (3 grid tracks, no named grid line)
> * auto "auto" auto (2 grid tracks, 1 named grid line)
> 
> I will see if I can get a better output

It turns out that forcing a quote would require hacking CSSPrimitiveValue: we
currently only quote a CSS_STRING if it's not an identifier. I will follow-up
with a bug about that (it's probably deeper than just grid).

[esprehn, 2013-05-06 17:06:23]

LGTM. A bunch of code in that test looks like duplication though, I think you
can just call testCSSValue again. Also you can use shouldBeEqualToString() to
get rid of the quotes in the right hand side.

https://codereview.chromium.org/14786002/diff/9001/LayoutTests/fast/css-grid-...
File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):

https://codereview.chromium.org/14786002/diff/9001/LayoutTests/fast/css-grid-...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:50:
shouldBe("getComputedStyle(gridElement,
'').getPropertyValue('-webkit-grid-columns')", namedGridColumns);
You can use shouldBeEqualToString() to get rid of the extra quotes in the
expected values.

https://codereview.chromium.org/14786002/diff/9001/LayoutTests/fast/css-grid-...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:68:
shouldBe("getComputedStyle(element,
'').getPropertyValue('-webkit-grid-columns')", "'first 18px'");
This should be testCSSvalue right? Seems like the same code.

[Julien - ping for review, 2013-05-06 19:59:01]

https://codereview.chromium.org/14786002/diff/9001/LayoutTests/fast/css-grid-...
File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):

https://codereview.chromium.org/14786002/diff/9001/LayoutTests/fast/css-grid-...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:1: <!DOCTYPE HTML
PUBLIC "-//IETF//DTD HTML//EN">
Mhh, this should have been the HTML5 doctype. Will fix it.

https://codereview.chromium.org/14786002/diff/9001/LayoutTests/fast/css-grid-...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:50:
shouldBe("getComputedStyle(gridElement,
'').getPropertyValue('-webkit-grid-columns')", namedGridColumns);
On 2013/05/06 17:06:23, esprehn wrote:
> You can use shouldBeEqualToString() to get rid of the extra quotes in the
> expected values.

Done.

https://codereview.chromium.org/14786002/diff/9001/LayoutTests/fast/css-grid-...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:68:
shouldBe("getComputedStyle(element,
'').getPropertyValue('-webkit-grid-columns')", "'first 18px'");
On 2013/05/06 17:06:23, esprehn wrote:
> This should be testCSSvalue right? Seems like the same code.

It is very close but not totally identical as testCSSValue gets an id. I have
split testCSSValue so that I could easily reuse the code.

[commit-bot: I haz the power, 2013-05-06 20:52:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/14786002/18001

[commit-bot: I haz the power, 2013-05-06 21:17:31]

Change committed as 149798