https://drafts.csswg.org/css-values-4/#exponent-funcs
Giving this a look!
Created attachment 417716 [details] Patch
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?
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
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()".
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.
Created attachment 417728 [details] Patch
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.
Created attachment 417738 [details] Patch
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.
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.
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.
(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.
Created attachment 417920 [details] Patch
Created attachment 418371 [details] Patch
Cleaned up spacing in switch statements, added check for hypot() with no children, and added additional non-parsing test cases.
Created attachment 436678 [details] Patch
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)?
Created attachment 436786 [details] Patch
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.
Created attachment 436797 [details] Patch
(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?
(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?
(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.
Created attachment 436830 [details] Patch
Created attachment 436835 [details] Patch
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
Created attachment 436836 [details] Patch
Created attachment 436837 [details] Patch
An update of css-values WPT is being tracked here https://bugs.webkit.org/show_bug.cgi?id=229696
Created attachment 436898 [details] Patch
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.
Created attachment 436968 [details] Patch
(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.
<rdar://problem/82640883>
Created attachment 437175 [details] Patch
(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.
We should probably use double everywhere.
Created attachment 437888 [details] Patch
Rebased to include trig implementations landed with https://bugs.webkit.org/show_bug.cgi?id=229507.
I've filed a separate bug to track standardizing on using double everywhere: https://bugs.webkit.org/show_bug.cgi?id=230161
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.
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.
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.
Created attachment 437900 [details] Patch
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.
Created attachment 438004 [details] Patch
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.
Created attachment 438097 [details] Patch
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.
Created attachment 438385 [details] Patch
Created attachment 439221 [details] Patch
Created attachment 439223 [details] Patch
Created attachment 439392 [details] Patch
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.
Created attachment 439434 [details] Patch
Created attachment 439494 [details] Patch
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.
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.
(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.
Created attachment 439522 [details] Patch
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.
Created attachment 439691 [details] Patch
Created attachment 439694 [details] Patch
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.
(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?
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?
(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.
Created attachment 439781 [details] Patch
(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.
Created attachment 439795 [details] Patch
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Created attachment 439825 [details] Patch
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].