calc expressions should support time, angle and frequency values.

calc expressions should support time, angle and frequency values.

According to the CSS3 standard calc() expressions can contain time,
angle and frequency values. (http://www.w3.org/TR/css3-values/#calc).
The patch implements this feature.

The patch is partially based on http://trac.webkit.org/changeset/168685
and http://trac.webkit.org/changeset/170544 (Patch by Martin Hodovan,
reviewers were: Darin Adler and Simon Fraser)

R=mikelawther@chromium.org,alancutter@chromium.org
BUG=390566
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179101

[alancutter (OOO until 2018), 2014-07-02 05:56:11]

https://codereview.chromium.org/345903005/diff/1/LayoutTests/css3/calc/calc-e...
File LayoutTests/css3/calc/calc-errors.html (left):

https://codereview.chromium.org/345903005/diff/1/LayoutTests/css3/calc/calc-e...
LayoutTests/css3/calc/calc-errors.html:23: <div style="width: 100px; width:
calc(5dpi + 5dpi);">dpi + dpi</div>
Shouldn't these remain? This patch does not add support for dpi units.

https://codereview.chromium.org/345903005/diff/1/LayoutTests/css3/calc/calc-e...
LayoutTests/css3/calc/calc-errors.html:28: <!-- wrong combination -->
There should be tests that check we don't accept mixing the new unit types with
others.

https://codereview.chromium.org/345903005/diff/1/LayoutTests/css3/calc/calc-w...
File LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html
(right):

https://codereview.chromium.org/345903005/diff/1/LayoutTests/css3/calc/calc-w...
LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html:4: <script
src="../../resources/js-test.js"></script>
New tests should use testharness.js instead of js-test.js.

https://codereview.chromium.org/345903005/diff/1/LayoutTests/css3/calc/calc-w...
LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html:23:
</script>
Our LayoutTests typically put DOM before script so we don't need to use onload
events.

https://codereview.chromium.org/345903005/diff/1/LayoutTests/css3/calc/calc-w...
LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html:28: <div
id="div3" style="transform:rotate(calc(45deg + 45deg));"></div>
We should be testing combinations of different units eg. calc(45deg + 50grad).
We should also test all the unit types; ms, rad and turn are currently untested.

https://codereview.chromium.org/345903005/diff/1/LayoutTests/css3/calc/calc-w...
LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html:33: <div
id="div8" style="pitch:(12Hz + 3Hz);"></div>
Should these have calc() around them?

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSCalculati...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSCalculati...
Source/core/css/CSSCalculationValue.cpp:297: static const CalculationCategory
addSubtractResult[CalcAngle][CalcAngle] = {
It's probably better to use CalcPercentLength + 1 instead of depending on the
ordering of CalcAngle.

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSCalculati...
Source/core/css/CSSCalculationValue.cpp:317: if (leftCategory < CalcAngle ||
rightCategory < CalcAngle)
<= CalcPercentLength

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSCalculati...
File Source/core/css/CSSCalculationValue.h (right):

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSCalculati...
Source/core/css/CSSCalculationValue.h:62: CalcPercentLength,
Please add a comment stating that these first few enum values must remain at the
top of the enum and why.

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSPrimitive...
File Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSPrimitive...
Source/core/css/CSSPrimitiveValue.cpp:554: case CSS_CALC:
Similar comments as for computeTime(), this should ASSERT that the calc holds an
angle.

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSPrimitive...
File Source/core/css/CSSPrimitiveValue.h (right):

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSPrimitive...
Source/core/css/CSSPrimitiveValue.h:289: return getValue<T>();
This should be returning a value relative to timeUnit.
This should also ASSERT that the calc holds a time value.

[Timothy Loh, 2014-07-02 07:06:13]

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSPrimitive...
File Source/core/css/CSSPrimitiveValue.h (right):

https://codereview.chromium.org/345903005/diff/1/Source/core/css/CSSPrimitive...
Source/core/css/CSSPrimitiveValue.h:289: return getValue<T>();
On 2014/07/02 05:56:11, alancutter wrote:
> This should be returning a value relative to timeUnit.
> This should also ASSERT that the calc holds a time value.

Alternatively we can just simplify this function to something like "double
computeSeconds()"; the function is only used in a couple of places anyway.

[alancutter (OOO until 2018), 2014-07-02 07:41:10]

I don't think the conversion between units happens for calcs with single values.
Better add a test to check that calc(50grad) is equivalent to calc(45deg) and
similar tests for time units.

[reni, 2014-07-03 17:29:32]

@timloh, @alancutter: thanks for the reviews! I tried to address all the
requests however while I was creating these bunch of new tests I caught a few
bugs in the previous code. These bugs are fixed now but because of this I had to
differ from the reviews at some points.

The main differences in the current patch are:
* The addSubtractResult matrix is extended to handle the new units. Because of
this modification the related comments (enum comment, matrix size refinement)
were not addressed the way they were asked.
* computeTime() is refactored into computeSeconds() as asked.
* calc-errors.html was completely rebased to be able to cover "all" the invalid
calc combinations of the old and new units without typing all the ~700
combinations. "All" is in quotes since <length>s are only represented with px
values, otherwise the test would be insanely and needlessly slow. Besides, the
new version contains all the previous checks too, ofc.
* calc-with-time-angle-and-frequency-values.html was  extended with mixed (but
legal) unit operations. It also checks unit conversions on calcs with single
values.
One more note here: in the cases where we have mixed operations with radian unit
I used the explicit matrix value as reference. The reason for this is that
converting rad to any other unit needs calculation with PI and if the reference
is given as a floating point number + unit then the test could easily fail
because of rounding problems.

[reni, 2014-07-03 17:30:02]

Note: Windows build fail is unrelated to this patch.

[Mike Lawther (Google), 2014-07-04 04:36:41]

https://codereview.chromium.org/345903005/diff/20001/LayoutTests/css3/calc/ca...
File LayoutTests/css3/calc/calc-errors.html (left):

https://codereview.chromium.org/345903005/diff/20001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-errors.html:29: <div style="width: 100px; width:
calc(200);">non length</div>
It looks like this test for a single unitless value has been lost?

https://codereview.chromium.org/345903005/diff/20001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-errors.html:45: <div style="width: 100px; width:
calc(100/10px);">number / length</div>
It also looks like the tests for 'number / length|percent' have been lost.
Please check your test generator so we don't lose these tests.

https://codereview.chromium.org/345903005/diff/20001/LayoutTests/css3/calc/ca...
File LayoutTests/css3/calc/calc-errors.html (right):

https://codereview.chromium.org/345903005/diff/20001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-errors.html:145: // Generating anc checking wrong
combinations.
typo: 'anc' -> 'and'

https://codereview.chromium.org/345903005/diff/20001/LayoutTests/css3/calc/ca...
File LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html
(right):

https://codereview.chromium.org/345903005/diff/20001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html:9: <div
id="div1" style="-webkit-transition-delay:calc(4s + 1s);"></div>
transition-delay is unprefixed now in Blink. Please only use the unprefixed
version in new tests.

https://codereview.chromium.org/345903005/diff/20001/Source/core/css/CSSCalcu...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/345903005/diff/20001/Source/core/css/CSSCalcu...
Source/core/css/CSSCalculationValue.cpp:85: return CalcOther;
Down on line 654 you removed the test for CalcOther. Is it the case that this
function should no longer return CalcOther? If so, please ASSERT_NOT_REACHED()
for the default case.

[reni, 2014-07-04 11:04:36]

@Mike: thanks again! Missing expressions and ASSERT_NOT_REACHED are added.

[reni, 2014-07-21 08:58:18]

On 2014/07/04 11:04:36, reni wrote:
> @Mike: thanks again! Missing expressions and ASSERT_NOT_REACHED are added.

I'm back from holiday and ready for the reviews :) Could someone take a look at
this?

[alancutter (OOO until 2018), 2014-07-21 14:03:47]

Thanks for the update.

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
File LayoutTests/css3/calc/calc-errors.html (right):

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-errors.html:5: width: 100px;
The inline 'width: 100px' can be removed now that it's in this style rule.

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-errors.html:63: var divOperators = ["/", "mod"];
I think it's okay to drop tests that involve mod since it's not implemented nor
is it in the CSS Calc spec.
http://dev.w3.org/csswg/css-values/#calc-syntax

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-errors.html:162: checkWrongCombinations(calcsForTime,
"animation-delay", "100s");
The coverage provided here is great but this is far too much logic for a test.
Tests should not reimplement the functionality they're testing for or they're at
risk of having bugs themselves.

The tests found in calc-with-time-angle-and-frequency-values.html are in a much
more suitable format, they check concrete input/output expectations in a way
that's easy to comprehend and hard to get wrong. This format should be used
here.

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
File LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html
(right):

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html:7: <body>
HTML, HEAD and BODY tags are omitted in LayoutTests.
http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h...

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html:102:
<script>
Scripts are usually not indented, nor are other root level DOM elements.

Example:
<div>
    <p>This still gets indented.</p>
</div>
<script>
// But no indentation here.
</script>

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html:106: }
Consider reusing a single div target:
<div id="target"></div>
function compareValues(property, calcValue, expectedValue) {
    target.style[property] = '';
    target.style[property] = calcValue;
    var computedCalcValue = getComputedStyle(target)[property];
    ... (same for computedExpectedValue)
    assert_equals(computedCalcValue, computedExpectedValue, calcValue + ' should
equal ' + expectedValue);
}

This should make the tests much easier to read and verify as the input and the
output will be on the same line instead of multiple places.
Example:
compareValues('transform', 'rotate(calc(90deg / 2))', 'rotate(45deg)');

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values.html:161: },
"Tests that calc() can contain angle, time and frequency values.");
You should split this test() function up in the same way you split the divs into
time, angle, frequency and conversion sections, each section should be it's own
test() call.

https://codereview.chromium.org/345903005/diff/60001/Source/core/css/CSSCalcu...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/345903005/diff/60001/Source/core/css/CSSCalcu...
Source/core/css/CSSCalculationValue.cpp:655: result->value =
CSSCalcPrimitiveValue::create(toCSSPrimitiveValue(value.get()),
parserValue->isInt);
The unitCategory check could be turned into an ASSERT that we don't get
CalcOther here.
We should add tests for calc(42fr) and calc(auto).

https://codereview.chromium.org/345903005/diff/60001/Source/core/css/CSSPrimi...
File Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/345903005/diff/60001/Source/core/css/CSSPrimi...
Source/core/css/CSSPrimitiveValue.cpp:151: return CSS_UNKNOWN;
This function should only get called if we are a calc so the null check can be
skipped. We'd rather catch bugs early via crashes than to let the program get
into an error state that's hard to debug.
Without the null check this function would be a single line and can probably
just be inlined in the two places it gets used.

https://codereview.chromium.org/345903005/diff/60001/Source/core/css/CSSPrimi...
File Source/core/css/CSSPrimitiveValue.h (right):

https://codereview.chromium.org/345903005/diff/60001/Source/core/css/CSSPrimi...
Source/core/css/CSSPrimitiveValue.h:273: enum TimeUnit { Seconds, Milliseconds
};
Is TimeUnit used anywhere? Can it be removed?

https://codereview.chromium.org/345903005/diff/60001/Source/core/css/CSSPrimi...
Source/core/css/CSSPrimitiveValue.h:275: UnitType calcUnitType() const;
This function should be able to live in the CPP file instead of being exposed in
the header file.

[reni, 2014-07-21 20:21:33]

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
File LayoutTests/css3/calc/calc-errors.html (right):

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-errors.html:162: checkWrongCombinations(calcsForTime,
"animation-delay", "100s");
On 2014/07/21 14:03:46, alancutter wrote:
> The coverage provided here is great but this is far too much logic for a test.
> Tests should not reimplement the functionality they're testing for or they're
at
> risk of having bugs themselves.
> 
> The tests found in calc-with-time-angle-and-frequency-values.html are in a
much
> more suitable format, they check concrete input/output expectations in a way
> that's easy to comprehend and hard to get wrong. This format should be used
> here.

Do I have to keep the current coverage in the "manual" version too? (it means
~700 checks)

[alancutter (OOO until 2018), 2014-07-22 00:16:36]

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
File LayoutTests/css3/calc/calc-errors.html (right):

https://codereview.chromium.org/345903005/diff/60001/LayoutTests/css3/calc/ca...
LayoutTests/css3/calc/calc-errors.html:162: checkWrongCombinations(calcsForTime,
"animation-delay", "100s");
On 2014/07/21 20:21:33, reni wrote:
> On 2014/07/21 14:03:46, alancutter wrote:
> > The coverage provided here is great but this is far too much logic for a
test.
> > Tests should not reimplement the functionality they're testing for or
they're
> at
> > risk of having bugs themselves.
> > 
> > The tests found in calc-with-time-angle-and-frequency-values.html are in a
> much
> > more suitable format, they check concrete input/output expectations in a way
> > that's easy to comprehend and hard to get wrong. This format should be used
> > here.
> 
> Do I have to keep the current coverage in the "manual" version too? (it means
> ~700 checks)

It's better to have a smaller set of tests that achieve a similar level of
coverage. 700 tests is quite excessive and very noisy if a change causes 200 of
them to fail. Ideally each test would be unique such that a bug could cause them
to fail in isolation from all the others.

That being said I'm okay with less coverage than what's currently here.

[reni, 2014-07-25 08:44:43]

https://codereview.chromium.org/345903005/diff/60001/Source/core/css/CSSCalcu...
File Source/core/css/CSSCalculationValue.cpp (right):

https://codereview.chromium.org/345903005/diff/60001/Source/core/css/CSSCalcu...
Source/core/css/CSSCalculationValue.cpp:655: result->value =
CSSCalcPrimitiveValue::create(toCSSPrimitiveValue(value.get()),
parserValue->isInt);
On 2014/07/21 14:03:47, alancutter wrote:
> The unitCategory check could be turned into an ASSERT that we don't get
> CalcOther here.
> We should add tests for calc(42fr) and calc(auto).

Good point! calc(42fr) showed that we have to count on getting any valid CSS
unit types in unitCategory() (not just valid calc units), since they are
supplied by CSSParserValues without validating against calc restrictions. Hence
we have to keep the unitCategory check here after all.

[alancutter (OOO until 2018), 2014-07-28 14:31:32]

lgtm

[reni, 2014-07-28 14:46:40]

The CQ bit was checked by rhodovan.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2014-07-28 14:46:56]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rhodovan.u-szeged@partner.samsung.com/...

[reni, 2014-07-28 15:04:53]

On 2014/07/28 14:31:32, alancutter wrote:
> lgtm

Thank you! However it seems that I need another lgtm too:

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    Source/core/css/CSSCalculationValue.cpp
    Source/core/css/CSSCalculationValue.h
    Source/core/css/CSSPrimitiveValue.cpp
    Source/core/css/CSSPrimitiveValue.h
    Source/core/css/parser/CSSPropertyParser.cpp
    Source/core/css/resolver/CSSToStyleMap.cpp
    Source/core/css/resolver/StyleBuilderCustom.cpp

[commit-bot: I haz the power, 2014-07-28 15:36:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19237)

[commit-bot: I haz the power, 2014-07-28 15:38:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-28 15:38:42]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[Timothy Loh, 2014-07-29 02:45:19]

On 2014/07/28 15:04:53, reni wrote:
> On 2014/07/28 14:31:32, alancutter wrote:
> > lgtm
> 
> Thank you! However it seems that I need another lgtm too:
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     Source/core/css/CSSCalculationValue.cpp
>     Source/core/css/CSSCalculationValue.h
>     Source/core/css/CSSPrimitiveValue.cpp
>     Source/core/css/CSSPrimitiveValue.h
>     Source/core/css/parser/CSSPropertyParser.cpp
>     Source/core/css/resolver/CSSToStyleMap.cpp
>     Source/core/css/resolver/StyleBuilderCustom.cpp

rslgtm

[reni, 2014-07-29 06:21:55]

The CQ bit was checked by rhodovan.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2014-07-29 06:22:06]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rhodovan.u-szeged@partner.samsung.com/...

[commit-bot: I haz the power, 2014-07-29 07:26:45]

Change committed as 179101