Bug 229507 – Implement sin, cos, tan, e and pi for calc

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nikos Mouchtaris]

Implement calc math functions

[Nikos Mouchtaris]

Created attachment 436406 [details]
Patch

[Nikos Mouchtaris]

Created attachment 436448 [details]
Patch

[Darin Adler]

Comment on attachment 436448 [details]
Patch

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

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:-682
> -#if ASSERT_ENABLED
> -        for (auto& child : m_children)
> -            ASSERT(child->category() == CalculationCategory::Number);
> -#endif

What’s the rationale for removing this assertion? Change log doesn’t say.

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:31
> -#include <wtf/text/TextStream.h>
> +#include "math.h"
>  
> +#include <wtf/text/TextStream.h>

This isn’t correct WebKit style. The includes should been a single paragraph, not two separate ones. No blank line between them. But blank line after them before "namespace".

Also, this should be <math.h> or <cmath>, not "math.h". I think we should prefer <cmath>.

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:95
> +        double sinValue = sin(m_children[0]->evaluate(maxValue));
> +        return sinValue;

No reason for a local variable here.

    return std::sin(m_children[0]->evaluate(maxValue));

[Nikos Mouchtaris]

Created attachment 436674 [details]
Patch

[Tim Horton]

Comment on attachment 436674 [details]
Patch

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

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:98
> +static const CSSCalcSymbolTable getConstantTable()

style nit: no `get` because it doesn't have out args (which is the only case where we use the `get` prefix in webkit)

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:100
> +    return { {CSSValuePi, CSSUnitType::CSS_NUMBER, piDouble}, {CSSValueE, CSSUnitType::CSS_NUMBER, std::exp(1.0)} };

style nit: spaces inside braces

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:632
> -        if (calcOperationNode.children().size() == 1 && depth)
> +        if (calcOperationNode.children().size() == 1 && !depth)

I wonder if you want to fix this separately with a separate test? Or maybe you said it was previously not observable?

[Nikos Mouchtaris]

Yeah it was not previously observable without the introduction of the one child node functions so I think its fine to fix here.

[Myles C. Maxfield]

Comment on attachment 436674 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +        * css3/calc/simple-trig-functions.html: Added.

No tests that test failing parses? like atan(3px)?

No tests that test things like tan(pi/2) for infinity?

> LayoutTests/css3/calc/simple-trig-functions.html:71
> +        element.innerHTML += " => PASS";

This should be using js-test.js instead.

> LayoutTests/css3/calc/trig-functions-with-constants.html:53
> +        element.innerHTML += " => PASS";

Ditto.

[Myles C. Maxfield]

Comment on attachment 436674 [details]
Patch

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

>> LayoutTests/ChangeLog:12
>> +        * css3/calc/simple-trig-functions.html: Added.
> 
> No tests that test failing parses? like atan(3px)?
> 
> No tests that test things like tan(pi/2) for infinity?

How about testing animations?

[Myles C. Maxfield]

Comment on attachment 436674 [details]
Patch

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

>> LayoutTests/css3/calc/trig-functions-with-constants.html:53
>> +        element.innerHTML += " => PASS";
> 
> Ditto.

You'll also want to test the CSSOM itself, and getComputedStyle()

[Myles C. Maxfield]

Comment on attachment 436674 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Implement calc math functions

Which ones?

[Nikos Mouchtaris]

Created attachment 436966 [details]
Patch

[Nikos Mouchtaris]

Created attachment 436967 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/82638816>

[Jon Lee]

Comment on attachment 436967 [details]
Patch

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

> LayoutTests/css3/calc/trig-functions-with-constants.html:45
> +                testValue('calc(sin(tan(pi/4)*pi/2) * 100px)', '100');

same test case repeated 3 times?

[Simon Fraser (smfr)]

Comment on attachment 436967 [details]
Patch

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

Are there WPT for these trig functions? Can we contribute some?

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:366
> +    return adoptRef(new CSSCalcOperationNode(CalculationCategory::Number, op, WTFMove(values)));

Normally we avoid calling bare 'new' and instead give classes a static create() function. But I see you're following existing code here.

> LayoutTests/css3/calc/trig-functions-with-constants.html:31
> +                testValue('calc(sin(30deg + 1.0471967rad ) * 100px)', '100');

Do you test lack of units  - tan(0.3) ?

[EWS]

Committed r282162 (241454@main): <https://commits.webkit.org/241454@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436967 [details].

[Simon Fraser (smfr)]

*** Bug 203311 has been marked as a duplicate of this bug. ***