Summary: | Implement sin, cos, tan, e and pi for calc | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikos Mouchtaris <nmouchtaris> | ||||||||||||
Component: | New Bugs | Assignee: | Nikos Mouchtaris <nmouchtaris> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jonlee, kevinturner, macpherson, menard, mmaxfield, simon.fraser, thorton, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Nikos Mouchtaris
2021-08-25 11:43:49 PDT
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
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. *** |