RESOLVED FIXED 203312
Implement the CSS exponent functions: pow(), sqrt(), hypot()
https://bugs.webkit.org/show_bug.cgi?id=203312
Summary Implement the CSS exponent functions: pow(), sqrt(), hypot()
Simon Fraser (smfr)
Reported 2019-10-23 13:25:09 PDT
Attachments
Patch (22.16 KB, patch)
2021-01-15 11:16 PST, Kevin Turner
no flags
Patch (22.59 KB, patch)
2021-01-15 13:27 PST, Kevin Turner
no flags
Patch (22.11 KB, patch)
2021-01-15 14:18 PST, Kevin Turner
no flags
Patch (22.01 KB, patch)
2021-01-19 15:54 PST, Kevin Turner
no flags
Patch (23.13 KB, patch)
2021-01-25 21:58 PST, Kevin Turner
no flags
Patch (24.12 KB, patch)
2021-08-27 15:14 PDT, Kevin Turner
no flags
Patch (29.40 KB, patch)
2021-08-30 09:57 PDT, Kevin Turner
no flags
Patch (30.02 KB, patch)
2021-08-30 11:51 PDT, Kevin Turner
no flags
Patch (30.04 KB, patch)
2021-08-30 17:15 PDT, Kevin Turner
no flags
Patch (112.53 KB, patch)
2021-08-30 17:52 PDT, Kevin Turner
no flags
Patch (30.04 KB, patch)
2021-08-30 17:55 PDT, Kevin Turner
no flags
Patch (142.55 KB, patch)
2021-08-30 18:01 PDT, Kevin Turner
no flags
Patch (30.04 KB, patch)
2021-08-31 09:57 PDT, Kevin Turner
no flags
Patch (30.42 KB, patch)
2021-08-31 16:52 PDT, Kevin Turner
no flags
Patch (31.04 KB, patch)
2021-09-02 11:43 PDT, Kevin Turner
no flags
Patch (32.67 KB, patch)
2021-09-10 10:30 PDT, Kevin Turner
no flags
Patch (32.55 KB, patch)
2021-09-10 12:45 PDT, Kevin Turner
no flags
Patch (32.44 KB, patch)
2021-09-12 19:03 PDT, Kevin Turner
no flags
Patch (32.37 KB, patch)
2021-09-13 19:11 PDT, Kevin Turner
no flags
Patch (32.31 KB, patch)
2021-09-16 12:26 PDT, Kevin Turner
no flags
Patch (32.45 KB, patch)
2021-09-24 17:39 PDT, Kevin Turner
ews-feeder: commit-queue-
Patch (32.09 KB, patch)
2021-09-24 17:57 PDT, Kevin Turner
no flags
Patch (32.55 KB, patch)
2021-09-27 13:41 PDT, Kevin Turner
no flags
Patch (35.09 KB, patch)
2021-09-27 22:36 PDT, Kevin Turner
no flags
Patch (36.29 KB, patch)
2021-09-28 10:29 PDT, Kevin Turner
no flags
Patch (35.63 KB, patch)
2021-09-28 14:50 PDT, Kevin Turner
no flags
Patch (35.48 KB, patch)
2021-09-29 19:09 PDT, Kevin Turner
no flags
Patch (35.48 KB, patch)
2021-09-29 20:29 PDT, Kevin Turner
simon.fraser: review+
Patch (35.43 KB, patch)
2021-09-30 15:14 PDT, Kevin Turner
no flags
Patch (35.97 KB, patch)
2021-09-30 16:44 PDT, Kevin Turner
ews-feeder: commit-queue-
Patch (35.50 KB, patch)
2021-09-30 22:51 PDT, Kevin Turner
no flags
Kevin Turner
Comment 1 2021-01-12 17:44:42 PST
Giving this a look!
Kevin Turner
Comment 2 2021-01-15 11:16:34 PST
Darin Adler
Comment 3 2021-01-15 11:23:29 PST
Comment on attachment 417716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417716&action=review Thanks for contributing this. Looks generally good. Preliminary comments before EWS run is done, so not setting review+ or review- yet. Not sure we have covered enough of the edge cases in our tests. > Source/WebCore/css/CSSCalculationValue.cpp:914 > + Extra blank line here. > Source/WebCore/css/CSSCalculationValue.cpp:916 > + for (auto child : values) { Should be auto& instead of auto so we don’t churn reference counts. > Source/WebCore/css/CSSCalculationValue.cpp:930 > + if (expectedCategory != CalculationCategory::Number && expectedCategory != CalculationCategory::Length && expectedCategory != CalculationCategory::Percent) Helper function so we don’t include this twice, here and in the loop. > Source/WebCore/css/CSSCalculationValue.cpp:1679 > + double base = children[0]; > + double exp = children[1]; > + return std::pow(base, exp); I don’t think the local variables help us here. > Source/WebCore/css/CSSCalculationValue.cpp:1685 > + double val = children[0]; WebKit coding style says to call this "value", not "val". > Source/WebCore/css/CSSCalculationValue.cpp:1833 > - > + Please don’t add trailing whitespace. > Source/WebCore/platform/CalculationValue.cpp:183 > + float val = m_children[0]->evaluate(maxValue); WebKit coding style says to call this "value", not "val". > LayoutTests/fast/css/calc-parsing.html:61 > + testExpression('calc(sqrt(100px)', '999px', '999px'); How is this working given we forgot the end parenthesis?
Simon Fraser (smfr)
Comment 4 2021-01-15 11:51:14 PST
Comment on attachment 417716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417716&action=review > Source/WebCore/css/CSSCalculationValue.cpp:785 > + bool isExpFuncNode() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt || m_operator == CalcOperator::Pow; } What is "exp" a contraction for here? > Source/WebCore/css/CSSCalculationValue.cpp:1810 > + // TODO: clamp, sin, cos, tan, asin, acos, atan, atan2. Remove clamp > Source/WebCore/css/CSSCalculationValue.cpp:1863 > + // TODO: clamp, sin, cos, tan, asin, acos, atan, atan2. Remove clamp
Kevin Turner
Comment 5 2021-01-15 12:24:53 PST
Comment on attachment 417716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417716&action=review >> Source/WebCore/css/CSSCalculationValue.cpp:785 >> + bool isExpFuncNode() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt || m_operator == CalcOperator::Pow; } > > What is "exp" a contraction for here? "Exponential" - I derived the naming from the title on the CSS spec for these functions: "11.5. Exponential Functions: pow(), sqrt(), hypot(), log(), exp()".
Kevin Turner
Comment 6 2021-01-15 12:27:29 PST
Comment on attachment 417716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417716&action=review >> LayoutTests/fast/css/calc-parsing.html:61 >> + testExpression('calc(sqrt(100px)', '999px', '999px'); > > How is this working given we forgot the end parenthesis? I believe a mismatch in parentheses would cause us to fail silently (leaving the width set to 999px), but the original intention of this test case was to ensure we fail when trying to take the square root of a dimension type since sqrt() operates purely on numbers according to the spec. Will fix this in an updated patch shortly.
Kevin Turner
Comment 7 2021-01-15 13:27:34 PST
Darin Adler
Comment 8 2021-01-15 14:00:15 PST
Comment on attachment 417728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417728&action=review > Source/WebCore/css/CSSCalculationValue.cpp:943 > + > + if (!isValidHypotType(currentCategory)) > + return nullptr; Just realized this is dead code. We’e already checked expectedCategory and currentCategory is == it. So we don’t need this and thus don’t really need the helper function.
Kevin Turner
Comment 9 2021-01-15 14:18:30 PST
Simon Fraser (smfr)
Comment 10 2021-01-15 14:35:59 PST
Comment on attachment 417738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417738&action=review > Source/WebCore/css/CSSCalculationValue.cpp:785 > + bool isExpFuncNode() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt || m_operator == CalcOperator::Pow; } isExponentialFunctionNode() > Source/WebCore/css/CSSCalculationValue.cpp:792 > + bool canHaveOnlyChild() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt; } This name is a bit ambiguous. Is it "can have only one child"? Maybe allowsSingleChild()? > Source/WebCore/css/CSSCalculationValue.cpp:1267 > + // Don't simplify out sqrt() before its evaluated its -> it is > Source/WebCore/css/CSSCalculationValue.cpp:1690 > + sum += std::pow(child, 2); (child * child) probably slightly better.
Darin Adler
Comment 11 2021-01-15 15:00:54 PST
Comment on attachment 417738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417738&action=review A few other minor comments. >> Source/WebCore/css/CSSCalculationValue.cpp:792 >> + bool canHaveOnlyChild() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt; } > > This name is a bit ambiguous. Is it "can have only one child"? Maybe allowsSingleChild()? I think this name isn’t quite right. This returns true for operators that are not identity functions when they have a single child. Not, as the title implies, the only operators that allow a single child. So we should use a different name. Maybe we should also reverse the sense of the function. It’s about an optimization in how we implement the other functions like min and max. > Source/WebCore/css/CSSCalculationValue.cpp:935 > + auto currentCategory = values[i]->category(); > + if (currentCategory != expectedCategory) > + return nullptr; Probably reads slightly better without the local variable. > Source/WebCore/css/CSSCalculationValue.cpp:1670 > + case CalcOperator::Pow: { The items above don’t use braces when they don’t have local variables. This could omit braces to match those. > Source/WebCore/css/CSSCalculationValue.cpp:1676 > + } > + > + case CalcOperator::Sqrt: { These blank lines are different than the formatting above. Let’s try to be consistent. I don’t mind the blank lines, but I would like the entire switch statement to stay consistent. > Source/WebCore/css/CSSCalculationValue.cpp:1685 > + } > + > + case CalcOperator::Hypot: { Ditto.
Kevin Turner
Comment 12 2021-01-17 19:22:04 PST
Comment on attachment 417738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417738&action=review >>> Source/WebCore/css/CSSCalculationValue.cpp:792 >>> + bool canHaveOnlyChild() const { return m_operator == CalcOperator::Hypot || m_operator == CalcOperator::Sqrt; } >> >> This name is a bit ambiguous. Is it "can have only one child"? Maybe allowsSingleChild()? > > I think this name isn’t quite right. This returns true for operators that are not identity functions when they have a single child. Not, as the title implies, the only operators that allow a single child. So we should use a different name. Maybe we should also reverse the sense of the function. It’s about an optimization in how we implement the other functions like min and max. Perhaps something like 'canBehaveAsIdentity()'? hypot() and sqrt() can not behave as identity functions under any circumstance whereas min/max() can when it has one parameter. Would a direct check like 'isIdentity()' be inappropriate here? We could check that the m_children vector is of size 1 and the operator either min or max.
Darin Adler
Comment 13 2021-01-19 09:35:02 PST
(In reply to Kevin Turner from comment #12) > Perhaps something like 'canBehaveAsIdentity()'? hypot() and sqrt() can not > behave as identity functions under any circumstance whereas min/max() can > when it has one parameter. Would a direct check like 'isIdentity()' be > inappropriate here? We could check that the m_children vector is of size 1 > and the operator either min or max. Yes, those both sound good.
Kevin Turner
Comment 14 2021-01-19 15:54:33 PST
Kevin Turner
Comment 15 2021-01-25 21:58:06 PST
Kevin Turner
Comment 16 2021-01-25 22:06:12 PST
Cleaned up spacing in switch statements, added check for hypot() with no children, and added additional non-parsing test cases.
Kevin Turner
Comment 17 2021-08-27 15:14:41 PDT
Myles C. Maxfield
Comment 18 2021-08-27 16:34:11 PDT
Comment on attachment 436678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436678&action=review > LayoutTests/fast/css/calc-parsing.html:68 > + testExpression('hypot()', '999px', '999px'); Can we add tests for very large values? Like exp(1000)?
Kevin Turner
Comment 19 2021-08-30 09:57:14 PDT
Simon Fraser (smfr)
Comment 20 2021-08-30 10:11:36 PDT
Comment on attachment 436786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436786&action=review Would be nice to make Web Platform Tests for these. > Source/WebCore/ChangeLog:2 > + Add support for pow(), sqrt() and hypot() per https://drafts.csswg.org/css-values-4/#exponent-funcs. Missing blank line > Source/WebCore/ChangeLog:7 > + Test: fast/css/calc-parsing.html Normally we write some words here to say what the change does. The spec link would be better here. > Source/WebCore/ChangeLog:9 > + > + Extra. blank line. > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:154 > + // The type of a min(), max(), clamp() or hypot() expression is the result of adding the types of its comma-separated calculations > + return CalculationCategory::Other; I can't tell if the line you copied above with the comment represents a TODO or correct behavior.
Kevin Turner
Comment 21 2021-08-30 11:51:59 PDT
Kevin Turner
Comment 22 2021-08-30 16:25:13 PDT
(In reply to Simon Fraser (smfr) from comment #20) > > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:154 > > + // The type of a min(), max(), clamp() or hypot() expression is the result of adding the types of its comma-separated calculations > > + return CalculationCategory::Other; > > I can't tell if the line you copied above with the comment represents a TODO > or correct behavior. If you are referring to the comment of "The type of a min(), max()...", I was attempting to keep this comment up to date since I am adding 'hypot()' with this commit. Do you think we should remove this comment?
Simon Fraser (smfr)
Comment 23 2021-08-30 16:34:33 PDT
(In reply to Kevin Turner from comment #22) > (In reply to Simon Fraser (smfr) from comment #20) > > > > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:154 > > > + // The type of a min(), max(), clamp() or hypot() expression is the result of adding the types of its comma-separated calculations > > > + return CalculationCategory::Other; > > > > I can't tell if the line you copied above with the comment represents a TODO > > or correct behavior. > > If you are referring to the comment of "The type of a min(), max()...", I > was attempting to keep this comment up to date since I am adding 'hypot()' > with this commit. Do you think we should remove this comment? I don't know! Code above does more complex things to figure out the category. Do min max, clamp etc need to also do something more complex?
Kevin Turner
Comment 24 2021-08-30 17:12:54 PDT
(In reply to Simon Fraser (smfr) from comment #23) > (In reply to Kevin Turner from comment #22) > > (In reply to Simon Fraser (smfr) from comment #20) > > > > > > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:154 > > > > + // The type of a min(), max(), clamp() or hypot() expression is the result of adding the types of its comma-separated calculations > > > > + return CalculationCategory::Other; > > > > > > I can't tell if the line you copied above with the comment represents a TODO > > > or correct behavior. > > > > If you are referring to the comment of "The type of a min(), max()...", I > > was attempting to keep this comment up to date since I am adding 'hypot()' > > with this commit. Do you think we should remove this comment? > > I don't know! Code above does more complex things to figure out the > category. Do min max, clamp etc need to also do something more complex? From my understanding of the spec for these functions is that hypot() should have the same category as min, max and clamp and pow() and sqrt() should behave purely off of numbers. I realize now though that in rebasing this the case statements for ::other are repeated needlessly. Updated patch incoming.
Kevin Turner
Comment 25 2021-08-30 17:15:02 PDT
Kevin Turner
Comment 26 2021-08-30 17:52:40 PDT
EWS Watchlist
Comment 27 2021-08-30 17:53:59 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Kevin Turner
Comment 28 2021-08-30 17:55:33 PDT
Kevin Turner
Comment 29 2021-08-30 18:01:38 PDT
Kevin Turner
Comment 30 2021-08-30 18:23:47 PDT
An update of css-values WPT is being tracked here https://bugs.webkit.org/show_bug.cgi?id=229696
Kevin Turner
Comment 31 2021-08-31 09:57:18 PDT
Darin Adler
Comment 32 2021-08-31 13:20:43 PDT
Comment on attachment 436898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436898&action=review Unclear on how good the test coverage is for all this code. There are a lot of subtle things here to get right or wrong, and the tests are critical. > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:131 > + // TODO: sin, cos, tan, asin, acos, atan, atan2. While modifying this, should change from the non-WebKit-style TODO to a WebKit FIXME > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:181 > + // TODO: sin, cos, tan, asin, acos, atan, atan2. Ditto. > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:151 > + // The type of a min(), max(), clamp() or hypot() expression is the result of adding the types of its comma-separated calculations Not new wording, but what does "adding the types" mean here? > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:332 > + for (auto& child : values) { > + if (child->category() != CalculationCategory::Number) > + return nullptr; > + } I think we should make a helper function that returns whether all children have the same category, and returns that category or nullopt if they aren’t all the same category (or if the vector is empty). We could use this to share the code for PowOrSqrt and Hypot easily and read it carefully. if (commonCategory(values) != CalculationCategory::Number) return nullptr; static std::optional<CalculationCategory> commonCategory(const Vector<Ref<CSSCalcExpressionNode>>& vector) { // ... } > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:346 > + for (unsigned i = 1; i < values.size(); ++i) { Strange to use "unsigned" here when Vector indices are size_t. > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:643 > + // Don't simplify out sqrt() or hypot() before they are evaluated. This comment is nice and short, but sadly it says *what* we do, but not why. Our primary job in comments is to answer the question, "Why?" > Source/WebCore/css/calc/CSSCalcValue.cpp:186 > + case CalcOperator::Sqrt: { > + auto children = createCSS(operationChildren, style); > + if (children.isEmpty()) > + return nullptr; > + return CSSCalcOperationNode::createPowOrSqrt(CalcOperator::Sqrt, WTFMove(children)); > + } > + case CalcOperator::Pow: { > + auto children = createCSS(operationChildren, style); > + if (children.isEmpty()) > + return nullptr; > + return CSSCalcOperationNode::createPowOrSqrt(CalcOperator::Pow, WTFMove(children)); > + } Please merge this into a single case like the case above for Min/Max/Clamp. > Source/WebCore/platform/calc/CalcExpressionOperation.cpp:89 > + case CalcOperator::Pow: { Really confused about how CSSCalcValue uses double, and CalcExpressionOperation uses float.
Kevin Turner
Comment 33 2021-08-31 16:52:15 PDT
Kevin Turner
Comment 34 2021-08-31 17:12:42 PDT
(In reply to Darin Adler from comment #32) > Comment on attachment 436898 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=436898&action=review > > Unclear on how good the test coverage is for all this code. There are a lot > of subtle things here to get right or wrong, and the tests are critical. > I will look at expanding the tests included, I've been thinking through more edge cases and extreme cases to account for. > > Really confused about how CSSCalcValue uses double, and > CalcExpressionOperation uses float. Not sure about this incongruence either, I can't think of a reason why they shouldn't be the same.
Radar WebKit Bug Importer
Comment 35 2021-09-01 12:29:03 PDT
Kevin Turner
Comment 36 2021-09-02 11:43:35 PDT
Kevin Turner
Comment 37 2021-09-02 14:08:09 PDT
(In reply to Kevin Turner from comment #34) > (In reply to Darin Adler from comment #32) > > Comment on attachment 436898 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=436898&action=review > > > > Unclear on how good the test coverage is for all this code. There are a lot > > of subtle things here to get right or wrong, and the tests are critical. > > > I will look at expanding the tests included, I've been thinking through more > edge cases and extreme cases to account for. > > > > > Really confused about how CSSCalcValue uses double, and > > CalcExpressionOperation uses float. > > Not sure about this incongruence either, I can't think of a reason why they > shouldn't be the same. It seems the discrepancy between CSSCalcValue using double, and CalcExpressionOperation using float is a result of CalcExpressionOperation specifically being used with percentage (%) based calculations and using float in service to that. Not familiar with the background in this area however.
Simon Fraser (smfr)
Comment 38 2021-09-02 14:09:22 PDT
We should probably use double everywhere.
Kevin Turner
Comment 39 2021-09-10 10:30:33 PDT
Kevin Turner
Comment 40 2021-09-10 10:39:01 PDT
Rebased to include trig implementations landed with https://bugs.webkit.org/show_bug.cgi?id=229507.
Kevin Turner
Comment 41 2021-09-10 10:41:04 PDT
I've filed a separate bug to track standardizing on using double everywhere: https://bugs.webkit.org/show_bug.cgi?id=230161
Darin Adler
Comment 42 2021-09-10 11:18:08 PDT
Comment on attachment 437888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437888&action=review > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:607 > + if ((isMinOrMaxNode() || isExponentialFuncNode()) && canCombineAllChildren()) { Not obvious why this is min/max/exponential; what makes that set of node types special? > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:677 > + // Don't simplify out sqrt(), hypot() or trig functions before they are evaluated because they do not behave as an identity function with one argument. > + if (calcOperationNode.isIdentity() && depth) This comment seems a little backwards and doesn’t really match the code below. If we need a comment explaining why some functions aren’t identity functions, that probably belongs inside the implementation of isIdentity. Here, the code is almost self-explanatory given that function’s name. With this new refactoring, the non-obvious thing about this code is now the fact that isIdentity guarantees that there is only one child node. To me seems not super-obvious so might be worth mentioning in a comment. I would write a different comment.
Kevin Turner
Comment 43 2021-09-10 11:56:10 PDT
Comment on attachment 437888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437888&action=review >> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:607 >> + if ((isMinOrMaxNode() || isExponentialFuncNode()) && canCombineAllChildren()) { > > Not obvious why this is min/max/exponential; what makes that set of node types special? The checks may not be strictly necessary - I added `isExponentialFuncNode()` to the conditional since the combination-of-children logic here is what we want for these new functions. It seems like we're just enumerating which functions could have this combining of children logic applied in this way, so perhaps this enumeration of types would be better served as part of `canCombineAllChildren()` since it is only called from here.
Kevin Turner
Comment 44 2021-09-10 12:33:42 PDT
W(In reply to Kevin Turner from comment #43) > Comment on attachment 437888 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=437888&action=review > > >> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:607 > >> + if ((isMinOrMaxNode() || isExponentialFuncNode()) && canCombineAllChildren()) { > > > > Not obvious why this is min/max/exponential; what makes that set of node types special? > > The checks may not be strictly necessary - I added `isExponentialFuncNode()` > to the conditional since the combination-of-children logic here is what we > want for these new functions. It seems like we're just enumerating which > functions could have this combining of children logic applied in this way, > so perhaps this enumeration of types would be better served as part of > `canCombineAllChildren()` since it is only called from here. Actually, we already check for specific node types before calling combineChildren() in simplyNode() so these checks at the end here are surely redundant. We can just check canCombineAllChildren() to make sure the types of the children are correct for combining.
Kevin Turner
Comment 45 2021-09-10 12:45:00 PDT
Darin Adler
Comment 46 2021-09-10 16:54:09 PDT
Comment on attachment 437900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437900&action=review > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:198 > + // FIXME: implement asin, acos, atan, atan2. Could put this FIXME after all the cases (as it was before) instead of in the middle of the cases (as it is now). > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:1100 > + if (value < 0) > + return std::numeric_limits<double>::quiet_NaN(); This isn’t needed, since std::sqrt already does this when passed a negative number.
Kevin Turner
Comment 47 2021-09-12 19:03:38 PDT
Darin Adler
Comment 48 2021-09-13 09:07:05 PDT
Comment on attachment 438004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438004&action=review > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:676 > + // Identity nodes have only one child and perform no operation on their child WebKit coding style puts periods at the ends of sentence comments like this one.
Kevin Turner
Comment 49 2021-09-13 19:11:44 PDT
Darin Adler
Comment 50 2021-09-14 16:46:36 PDT
Comment on attachment 438097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438097&action=review > Source/WebCore/platform/calc/CalcExpressionOperation.cpp:102 > + if (value < 0) > + return std::numeric_limits<float>::quiet_NaN(); This isn’t needed, since std::sqrt already does this when passed a negative number.
Kevin Turner
Comment 51 2021-09-16 12:26:31 PDT
Kevin Turner
Comment 52 2021-09-24 17:39:40 PDT
Kevin Turner
Comment 53 2021-09-24 17:57:17 PDT
Kevin Turner
Comment 54 2021-09-27 13:41:10 PDT
Nikos Mouchtaris
Comment 55 2021-09-27 16:57:12 PDT
Comment on attachment 439392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439392&action=review > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:472 > + if (commonCategory(values) != CalculationCategory::Number) Should check that pow has exactly 2 children and that Sqrt has exactly 1 child. > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:679 > + if (m_children.size() < 2 && !isPowOrSqrt() && !isHypot()) { Is including isPowOrSqrt/isHypot here necessary? > Source/WebCore/platform/calc/CalcExpressionOperation.cpp:101 > + float base = m_children[0]->evaluate(maxValue); Don't need local variables. > LayoutTests/ChangeLog:10 > + * fast/css/calc-parsing.html: I would add a couple parsing tests to fast/css/calc-parsing.html. This would have caught the issue with pow/sqrt.
Kevin Turner
Comment 56 2021-09-27 22:36:56 PDT
Kevin Turner
Comment 57 2021-09-28 10:29:24 PDT
Simon Fraser (smfr)
Comment 58 2021-09-28 10:50:34 PDT
Comment on attachment 439494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439494&action=review > Source/WebCore/ChangeLog:11 > + Test: fast/css/calc-parsing.html > + > + Implements pow(), sqrt() and hypot() functions as specified by https://drafts.csswg.org/css-values-4/#exponent-funcs. Test line goes after the comments (with a blank line between) > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:352 > + for (size_t i = 1; i < values.size(); ++i) { for (const auto& value : values) > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:487 > + if (expectedCategory != CalculationCategory::Number && expectedCategory != CalculationCategory::Length && expectedCategory != CalculationCategory::Percent) This needs to allow Time, Frequency, and maybe PercentNumber/PercentLength? Needs tests that validate this. > LayoutTests/ChangeLog:10 > + * fast/css/calc-parsing-expected.txt: > + * fast/css/calc-parsing.html: Need some WPT here.
Darin Adler
Comment 59 2021-09-28 11:45:32 PDT
Comment on attachment 439494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439494&action=review >> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:352 >> + for (size_t i = 1; i < values.size(); ++i) { > > for (const auto& value : values) That idiom is not ideal here, because it doesn’t skip the first item in the vector. Or we could write it this way and redundantly check the category of item 0; wouldn’t hurt correctness.
Simon Fraser (smfr)
Comment 60 2021-09-28 11:51:13 PDT
(In reply to Darin Adler from comment #59) > Comment on attachment 439494 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=439494&action=review > > >> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:352 > >> + for (size_t i = 1; i < values.size(); ++i) { > > > > for (const auto& value : values) > > That idiom is not ideal here, because it doesn’t skip the first item in the > vector. Or we could write it this way and redundantly check the category of > item 0; wouldn’t hurt correctness. Oh I missed the 1.
Kevin Turner
Comment 61 2021-09-28 14:50:15 PDT
Kevin Turner
Comment 62 2021-09-28 14:51:12 PDT
Nikos (nmouchtaris@apple.com) has agreed to assist me in getting WPT tests for this up in a separate patch that, to land before this one.
Kevin Turner
Comment 63 2021-09-29 19:09:13 PDT
Kevin Turner
Comment 64 2021-09-29 20:29:58 PDT
Simon Fraser (smfr)
Comment 65 2021-09-30 12:03:03 PDT
Comment on attachment 439694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439694&action=review Have we imported WPT? Normally we'd import those as failing, then a patch like this would mark them as passing. > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:980 > + if (calcOperationNode.isHypotNode()) Not clear why hypot doesn't have the depth check. > Source/WebCore/css/calc/CSSCalcOperationNode.h:76 > + bool isIdentity() const { return m_children.size() == 1 && (m_operator == CalcOperator::Min || m_operator == CalcOperator::Max); } isIdentity() is a confusing name here, hiding the fact that this only applies to min and max.
Kevin Turner
Comment 66 2021-09-30 12:25:48 PDT
(In reply to Simon Fraser (smfr) from comment #65) > Comment on attachment 439694 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=439694&action=review > > Have we imported WPT? Normally we'd import those as failing, then a patch > like this would mark them as passing. I will check with Nikos for the WPT status. > > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:980 > > + if (calcOperationNode.isHypotNode()) > > Not clear why hypot doesn't have the depth check. This is due to hypot() being able to take units directly like min() and max(), instead of having to be nested in a calc() function without units like the nodes with the depth check. > > Source/WebCore/css/calc/CSSCalcOperationNode.h:76 > > + bool isIdentity() const { return m_children.size() == 1 && (m_operator == CalcOperator::Min || m_operator == CalcOperator::Max); } > > isIdentity() is a confusing name here, hiding the fact that this only > applies to min and max. I believe the discussion that landed us on this naming was to have a generic way to determine if a given node would apply any operation to its child, which does only applies to min/max() with one child. Would you recommend we inline this check and do away with the potential confusion?
Darin Adler
Comment 67 2021-09-30 12:33:28 PDT
Comment on attachment 439694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439694&action=review >>> Source/WebCore/css/calc/CSSCalcOperationNode.h:76 >>> + bool isIdentity() const { return m_children.size() == 1 && (m_operator == CalcOperator::Min || m_operator == CalcOperator::Max); } >> >> isIdentity() is a confusing name here, hiding the fact that this only applies to min and max. > > I believe the discussion that landed us on this naming was to have a generic way to determine if a given node would apply any operation to its child, which does only applies to min/max() with one child. Would you recommend we inline this check and do away with the potential confusion? Simon, I’m not sure this is confusing. This function should return true for any node that is an identity operation, making no change to value from the single child it has. The implementation reflects that this is true only for min and max. But the function doesn’t only "apply" to min and max. It correctly returns false for other types of nodes. Or perhaps this should also return true for sum or product with exactly one child?
Simon Fraser (smfr)
Comment 68 2021-09-30 13:37:12 PDT
(In reply to Darin Adler from comment #67) > Comment on attachment 439694 [details] > Or perhaps this should also return true for sum or product with exactly one > child? It was exactly this kind of question I had in mind.
Kevin Turner
Comment 69 2021-09-30 15:14:29 PDT
Kevin Turner
Comment 70 2021-09-30 16:09:11 PDT
(In reply to Simon Fraser (smfr) from comment #68) > (In reply to Darin Adler from comment #67) > > Comment on attachment 439694 [details] > > > Or perhaps this should also return true for sum or product with exactly one > > child? > > It was exactly this kind of question I had in mind. I think that makes sense to do. I expanded isIdentity() to include Add and Multiply operations with one child and I see no regressions. Will post an updated patch.
Kevin Turner
Comment 71 2021-09-30 16:44:44 PDT
EWS
Comment 72 2021-09-30 22:18:56 PDT
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Kevin Turner
Comment 73 2021-09-30 22:51:08 PDT
EWS
Comment 74 2021-10-01 00:58:18 PDT
Committed r283359 (242368@main): <https://commits.webkit.org/242368@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439825 [details].
Note You need to log in before you can comment on or make changes to this bug.