[CSS Grid Layout] Support calc() breadth track size

[CSS Grid Layout] Support calc() breadth track size

The RenderGrid support for calc() was already there but we had to add also the CSSComputedStyle support. According to the specs, http://dev.w3.org/csswg/css-grid/#resolved-track-list, we must show the used value. This change also brings several new tests.

BUG=297689
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161787

[Julien - ping for review, 2013-09-24 19:09:16]

lgtm.

@Allan, could you double-check the calc() coverage.

https://codereview.chromium.org/23472008/diff/1/LayoutTests/fast/css-grid-lay...
File LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js
(right):

https://codereview.chromium.org/23472008/diff/1/LayoutTests/fast/css-grid-lay...
LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js:62:
testGridDefinitionsSetJSValues("minmax(25%, calc(30px))", "minmax(calc(75%),
40px)", "minmax(25%, 30px)", "minmax(75%, 40px)");
The expected results are actually wrong here. We should resolve the minmax to
its pixel value.

This is an existing issue and I would argue to fix it upfront but could live
with having some FIXMEs in this code and expected output that points to the
right bug.

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

https://codereview.chromium.org/23472008/diff/1/Source/core/css/CSSComputedSt...
Source/core/css/CSSComputedStyleDeclaration.cpp:1002: if
(!trackBreadth.isLength())
There should be FIXME as this is wrong. We should always return the used value.

https://codereview.chromium.org/23472008/diff/1/Source/core/rendering/RenderG...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/23472008/diff/1/Source/core/rendering/RenderG...
Source/core/rendering/RenderGrid.cpp:414: if (trackLength.isFixed() ||
trackLength.isPercent() || trackLength.isViewportPercentage() ||
trackLength.isCalculated())
trackLength.isSpecified() :)

https://codereview.chromium.org/23472008/diff/1/Source/core/rendering/RenderG...
Source/core/rendering/RenderGrid.cpp:428: if (trackLength.isFixed() ||
trackLength.isPercent() || trackLength.isViewportPercentage() ||
trackLength.isCalculated()) {
Ditto.

https://codereview.chromium.org/23472008/diff/1/Source/core/rendering/RenderG...
Source/core/rendering/RenderGrid.cpp:440: ASSERT(trackLength.isFixed() ||
trackLength.isPercent() || trackLength.isViewportPercentage() ||
trackLength.isCalculated());
Ditto.

[svillar, 2013-09-25 08:31:45]

https://codereview.chromium.org/23472008/diff/1/LayoutTests/fast/css-grid-lay...
File LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js
(right):

https://codereview.chromium.org/23472008/diff/1/LayoutTests/fast/css-grid-lay...
LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js:62:
testGridDefinitionsSetJSValues("minmax(25%, calc(30px))", "minmax(calc(75%),
40px)", "minmax(25%, 30px)", "minmax(75%, 40px)");
On 2013/09/24 19:09:16, Julien Chaffraix wrote:
> The expected results are actually wrong here. We should resolve the minmax to
> its pixel value.
> 
> This is an existing issue and I would argue to fix it upfront but could live
> with having some FIXMEs in this code and expected output that points to the
> right bug.

Yes you're right, but I decided not to add those FIXMEs there because these
files have some other examples of bad results using minmax, it'd be weird to
have some results with the FIXMEs and some others without them.

I guess we can land this without adding all the FIXMEs and then fix them all in
the patch for http://crbug.com/297158

[commit-bot: I haz the power, 2013-09-27 06:25:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/svillar@igalia.com/23472008/7001

[commit-bot: I haz the power, 2013-09-27 07:51:40]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[svillar, 2013-09-27 11:43:27]

On 2013/09/27 07:51:40, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

So the thing is that I was not considering grid-auto-{columns|rows} in my patch
and it turns out that the sizingBox() call crashes for those properties in some
inspector tests.

I'm going to delay a bit the fix for this patch because I think the sizingBox()
is likely not the best one for several reasons, the renderer object doesn't need
to be the RenderGrid for example and we're also using that maximum length as the
maximum length of every track which is not correct either.

[alancutter (OOO until 2018), 2013-09-30 00:24:59]

Sorry for the delay, I've just gotten back from travelling.

Calc coverage lgtm with nits.

Please add a link to the relevant spec in the change description and the
tracking bug.

https://codereview.chromium.org/23472008/diff/7001/LayoutTests/fast/css-grid-...
File LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html
(right):

https://codereview.chromium.org/23472008/diff/7001/LayoutTests/fast/css-grid-...
LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-multiple.html:117:
<div class="grid gridWithCalcInsideMinMax"
id="gridWithCalcInsideMinMaxElement"></div>
Any reason the ids need Element appended to the end? This seems inconsistent
with the rest of the test cases.

https://codereview.chromium.org/23472008/diff/7001/LayoutTests/fast/css-grid-...
File LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html (right):

https://codereview.chromium.org/23472008/diff/7001/LayoutTests/fast/css-grid-...
LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html:94: <div
class="grid gridWithCalcComplexInsideMinMax"
id="gridWithCalcComplexInsideMinMaxElement"></div>
Ditto.

[svillar, 2013-11-05 12:17:37]

This new version mimics the behaviour of IE, i.e., for grid-auto-{*} and for
non-grid grid-definition-{*} the output of getComputedStyle() shows the
specified values

[svillar, 2013-11-05 16:42:11]

BTW it'd be nice to have another round of reviews as the patch changed
significantly after all the recent changes in CSSComputedStyle.

[Julien - ping for review, 2013-11-12 06:58:49]

lgtm

[commit-bot: I haz the power, 2013-11-12 07:00:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/svillar@igalia.com/23472008/107001

[commit-bot: I haz the power, 2013-11-12 07:00:50]

Failed to apply patch for
LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt
  Hunk #1 FAILED at 32.
  1 out of 3 hunks FAILED -- saving rejects to file
LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt.rej

Patch:      
LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt
Index: LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt
diff --git
a/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt
b/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt
index
b05ee2700cee86fa055118128fa85c72440b4b77..a885bd83a9faab4bd7e28565b99d465c89b24be0
100644
--- a/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt
+++ b/LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt
@@ -32,6 +32,14 @@ PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-colu
 PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "11px"
 PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-columns') is "800px"
 PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "600px"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-columns') is "150px"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "75px"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-columns') is "550px"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "465px"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-columns') is "80px"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "300px"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-columns') is "415px"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "300px"
 
 Test getting wrong values for grid-definition-columns and grid-definition-rows
through CSS (they should resolve to the default: 'none')
 PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-columns') is "none"
@@ -105,6 +113,24 @@ PASS element.style.gridDefinitionColumns is "3fr"
 PASS getComputedStyle(element, '').getPropertyValue('grid-definition-rows') is
"600px"
 PASS element.style.gridDefinitionRows is "4fr"
 
+Test getting and setting grid-definition-columns and grid-definition-rows to
calc() values through JS
+PASS getComputedStyle(element, '').getPropertyValue('grid-definition-columns')
is "150px"
+PASS element.style.gridDefinitionColumns is "calc(150px)"
+PASS getComputedStyle(element, '').getPropertyValue('grid-definition-rows') is
"75px"
+PASS element.style.gridDefinitionRows is "calc(75px)"
+PASS getComputedStyle(element, '').getPropertyValue('grid-definition-columns')
is "370px"
+PASS element.style.gridDefinitionColumns is "calc(50% - 30px)"
+PASS getComputedStyle(element, '').getPropertyValue('grid-definition-rows') is
"135px"
+PASS element.style.gridDefinitionRows is "calc(75px + 10%)"
+PASS getComputedStyle(element, '').getPropertyValue('grid-definition-columns')
is "200px"
+PASS element.style.gridDefinitionColumns is "minmax(25%, calc(30px))"
+PASS getComputedStyle(element, '').getPropertyValue('grid-definition-rows') is
"450px"
+PASS element.style.gridDefinitionRows is "minmax(calc(75%), 40px)"
+PASS getComputedStyle(element, '').getPropertyValue('grid-definition-columns')
is "110px"
+PASS element.style.gridDefinitionColumns is "minmax(10%, calc(30px + 10%))"
+PASS getComputedStyle(element, '').getPropertyValue('grid-definition-rows') is
"200px"
+PASS element.style.gridDefinitionRows is "minmax(calc(25% - 50px), 200px)"
+
 Test setting grid-definition-columns and grid-definition-rows to bad values
through JS
 PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-columns') is "none"
 PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "none"
@@ -128,6 +154,10 @@ PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-colu
 PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "none"
 PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-columns') is "none"
 PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "none"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-columns') is "none"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "none"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-columns') is "none"
+PASS window.getComputedStyle(element,
'').getPropertyValue('grid-definition-rows') is "none"
 
 Test setting grid-definition-columns and grid-definition-rows back to 'none'
through JS
 PASS getComputedStyle(element, '').getPropertyValue('grid-definition-columns')
is "18px"

[commit-bot: I haz the power, 2013-11-12 08:36:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/svillar@igalia.com/23472008/197001

[commit-bot: I haz the power, 2013-11-12 09:42:20]

Change committed as 161787