Implement calc math functions
Created attachment 436406 [details] Patch
Created attachment 436448 [details] Patch
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));
Created attachment 436674 [details] Patch
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?
Yeah it was not previously observable without the introduction of the one child node functions so I think its fine to fix here.
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.
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?
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()
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?
Created attachment 436966 [details] Patch
Created attachment 436967 [details] Patch
<rdar://problem/82638816>
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?
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) ?
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].
*** Bug 203311 has been marked as a duplicate of this bug. ***