WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
https://drafts.csswg.org/css-values-4/#exponent-funcs
Attachments
Patch
(22.16 KB, patch)
2021-01-15 11:16 PST
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(22.59 KB, patch)
2021-01-15 13:27 PST
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(22.11 KB, patch)
2021-01-15 14:18 PST
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(22.01 KB, patch)
2021-01-19 15:54 PST
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(23.13 KB, patch)
2021-01-25 21:58 PST
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(24.12 KB, patch)
2021-08-27 15:14 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(29.40 KB, patch)
2021-08-30 09:57 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(30.02 KB, patch)
2021-08-30 11:51 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(30.04 KB, patch)
2021-08-30 17:15 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(112.53 KB, patch)
2021-08-30 17:52 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(30.04 KB, patch)
2021-08-30 17:55 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(142.55 KB, patch)
2021-08-30 18:01 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(30.04 KB, patch)
2021-08-31 09:57 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(30.42 KB, patch)
2021-08-31 16:52 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(31.04 KB, patch)
2021-09-02 11:43 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(32.67 KB, patch)
2021-09-10 10:30 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(32.55 KB, patch)
2021-09-10 12:45 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(32.44 KB, patch)
2021-09-12 19:03 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(32.37 KB, patch)
2021-09-13 19:11 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(32.31 KB, patch)
2021-09-16 12:26 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(32.45 KB, patch)
2021-09-24 17:39 PDT
,
Kevin Turner
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(32.09 KB, patch)
2021-09-24 17:57 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(32.55 KB, patch)
2021-09-27 13:41 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(35.09 KB, patch)
2021-09-27 22:36 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(36.29 KB, patch)
2021-09-28 10:29 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(35.63 KB, patch)
2021-09-28 14:50 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(35.48 KB, patch)
2021-09-29 19:09 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(35.48 KB, patch)
2021-09-29 20:29 PDT
,
Kevin Turner
simon.fraser
: review+
Details
Formatted Diff
Diff
Patch
(35.43 KB, patch)
2021-09-30 15:14 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Patch
(35.97 KB, patch)
2021-09-30 16:44 PDT
,
Kevin Turner
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(35.50 KB, patch)
2021-09-30 22:51 PDT
,
Kevin Turner
no flags
Details
Formatted Diff
Diff
Show Obsolete
(27)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 417716
[details]
Patch
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
Created
attachment 417728
[details]
Patch
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
Created
attachment 417738
[details]
Patch
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
Created
attachment 417920
[details]
Patch
Kevin Turner
Comment 15
2021-01-25 21:58:06 PST
Created
attachment 418371
[details]
Patch
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
Created
attachment 436678
[details]
Patch
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
Created
attachment 436786
[details]
Patch
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
Created
attachment 436797
[details]
Patch
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
Created
attachment 436830
[details]
Patch
Kevin Turner
Comment 26
2021-08-30 17:52:40 PDT
Created
attachment 436835
[details]
Patch
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
Created
attachment 436836
[details]
Patch
Kevin Turner
Comment 29
2021-08-30 18:01:38 PDT
Created
attachment 436837
[details]
Patch
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
Created
attachment 436898
[details]
Patch
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
Created
attachment 436968
[details]
Patch
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
<
rdar://problem/82640883
>
Kevin Turner
Comment 36
2021-09-02 11:43:35 PDT
Created
attachment 437175
[details]
Patch
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
Created
attachment 437888
[details]
Patch
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
Created
attachment 437900
[details]
Patch
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
Created
attachment 438004
[details]
Patch
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
Created
attachment 438097
[details]
Patch
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
Created
attachment 438385
[details]
Patch
Kevin Turner
Comment 52
2021-09-24 17:39:40 PDT
Created
attachment 439221
[details]
Patch
Kevin Turner
Comment 53
2021-09-24 17:57:17 PDT
Created
attachment 439223
[details]
Patch
Kevin Turner
Comment 54
2021-09-27 13:41:10 PDT
Created
attachment 439392
[details]
Patch
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
Created
attachment 439434
[details]
Patch
Kevin Turner
Comment 57
2021-09-28 10:29:24 PDT
Created
attachment 439494
[details]
Patch
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
Created
attachment 439522
[details]
Patch
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
Created
attachment 439691
[details]
Patch
Kevin Turner
Comment 64
2021-09-29 20:29:58 PDT
Created
attachment 439694
[details]
Patch
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
Created
attachment 439781
[details]
Patch
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
Created
attachment 439795
[details]
Patch
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
Created
attachment 439825
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug