[webkit-reviews] review granted: [Bug 203312] Implement the CSS exponent functions: pow(), sqrt(), hypot() : [Attachment 436898] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kevin Turner
<kevinturner at utexas.edu>'s request for review:
Bug 203312: Implement the CSS exponent functions: pow(), sqrt(), hypot()
https://bugs.webkit.org/show_bug.cgi?id=203312

Attachment 436898: Patch

https://bugs.webkit.org/attachment.cgi?id=436898&action=review




--- Comment #32 from Darin Adler <darin at apple.com> ---
Comment on attachment 436898
  --> https://bugs.webkit.org/attachment.cgi?id=436898
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.