Bug 138356 – Converting time, angle and frequency units in CSS calc() function

• Component: CSS
• Status: RESOLVED FIXED

[Tibor Mészáros]

According to the standard, calc() should be able to handle angle, time and frequency values as well: http://www.w3.org/TR/css3-values/#calc

[Tibor Mészáros]

Created attachment 240930 [details]
Patch

This patch will merge Blink commit https://codereview.chromium.org/345903005/ (Patch by Renata Hodovan) which is based on http://trac.webkit.org/changeset/168685, http://trac.webkit.org/changeset/170544 (Patch by Martin Hodovan).

[Tibor Mészáros]

Could you possibly have a little bit of free time to review this patch?

[Tibor Mészáros]

ping?

[Tibor Mészáros]

Could you have a little free time to review this patch?

[Chris Dumez]

Comment on attachment 240930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240930&action=review

> LayoutTests/ChangeLog:11
> +        * css3/calc/calc-errors-expected.txt:

Most of these tests are passing without your change. This only one that doesn't is:
FAIL Tests calc() with time units. assert_equals: calc(4s + 1s) should equal to 5s expected "5s" but got "0.005s"

This does look like a bug. But otherwise, converting time, angle and frequency units in CSS calc() function is already working. Right now, it is not really clear what this patch is doing.
I would focus on fixing what actually doesn't work (e.g. the case above) instead of refactoring.

> LayoutTests/css3/calc/calc-errors-expected.txt:1
> +unclosed calc

Could you explain what changes you did to this test and why? It is not in the Changelog and not obvious to me at least.

> Source/WebCore/css/CSSCalculationValue.h:97
> +    unsigned short primitiveType() const { return m_expression->primitiveType(); }

Do we really need this? CSSPrimitiveValue::primitiveType() already handles calculated values.

> Source/WebCore/css/CSSPrimitiveValue.cpp:540
> +    switch (isCalculated() ? cssCalcValue()->primitiveType() : m_primitiveUnitType) {

Can't you call CSSPrimitiveValue::primitiveType() here?

[Chris Dumez]

Comment on attachment 240930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240930&action=review

> LayoutTests/css3/calc/calc-with-time-angle-and-frequency-values-expected.txt:2
> +PASS Tests calc() with time units. 

We need more example that are currently failing. As I said before, most of these seem to be passing without fix.

>> Source/WebCore/css/CSSCalculationValue.h:97
>> +    unsigned short primitiveType() const { return m_expression->primitiveType(); }
> 
> Do we really need this? CSSPrimitiveValue::primitiveType() already handles calculated values.

It seems this is needed after call, but it should probably be called from CSSPrimitiveValue::primitiveType().

>> Source/WebCore/css/CSSPrimitiveValue.cpp:540
>> +    switch (isCalculated() ? cssCalcValue()->primitiveType() : m_primitiveUnitType) {
> 
> Can't you call CSSPrimitiveValue::primitiveType() here?

Hmm, ok, so CSSPrimitiveValue::primitiveType()'s handling of calculated values is actually not great. It does not really check the unit inside the calculated expression. Instead, it relies on the category and returns CSS_MS if category is CalcTime. We should fix CSSPrimitiveValue::primitiveType().

> Source/WebCore/css/CSSPrimitiveValue.h:-262
> -        if (timeUnit == Seconds && m_primitiveUnitType == CSS_S)

We could potentially keep the same implementation if we call primitiveType() instead of accessing m_primitiveUnitType directly.

[Chris Dumez]

Comment on attachment 240930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240930&action=review

> LayoutTests/ChangeLog:3
> +        Converting time, angle and frequency units in CSS calc() function

What part of the change fixes frequency unit?

[Chris Dumez]

Created attachment 244818 [details]
Alternative patch

As there was no activity for a while, I am uploading another patch with more failing test cases and that updates CSSPrimitiveValue::primitiveType() instead of updating call sites to use CSSCalcValue::primitiveType() directly (more robust this way).

[WebKit Commit Bot]

Comment on attachment 244818 [details]
Alternative patch

Clearing flags on attachment: 244818

Committed r178627: <http://trac.webkit.org/changeset/178627>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.