[webkit-reviews] review denied: [Bug 229786] Implement abs, sign calc functions : [Attachment 438540] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 229786: Implement abs,sign calc functions
https://bugs.webkit.org/show_bug.cgi?id=229786

Attachment 438540: Patch

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




--- Comment #7 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 438540
  --> https://bugs.webkit.org/attachment.cgi?id=438540
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438540&action=review

> Source/WebCore/ChangeLog:14
> +	   Added support for calc functions sign and abs. Involved adding new
css keywords and handling
> +	   for parsing calc expression and computing the resulting value. Spec
for these functions:
> +	   https://drafts.csswg.org/css-values-4/#sign-funcs.

This paragraph should go above the Tests line.

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:198
>      // TODO: clamp, sin, cos, tan, asin, acos, atan, atan2, pow, sqrt, hypot

This list is no longer correct.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:466
> +	       if (calcOperator() == CalcOperator::Sign)
> +		   combinedUnitType = CSSUnitType::CSS_NUMBER;

The spec says "The abs(A) function contains one calculation A, and returns the
absolute value of A, as the same type as the input" so I'm not sure this is
right.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:1108
> +	   if (children.size() != 1)
> +	       return std::numeric_limits<double>::quiet_NaN();
> +	   if (children[0] > 0)
> +	       return 1;
> +	   if (children[0] < 0)
> +	       return -1;
> +	   return children[0];

Does this work for -0?

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:117
> +	   if (m_children.size() != 1)
> +	       return std::numeric_limits<double>::quiet_NaN();
> +	   if (m_children[0]->evaluate(maxValue) > 0)
> +	       return 1;
> +	   if (m_children[0]->evaluate(maxValue) < 0)
> +	       return -1;
> +	   return m_children[0]->evaluate(maxValue);

Same question about -0

>
LayoutTests/imported/w3c/web-platform-tests/css/css-values/signs-abs-computed.h
tml:15
> +test_math_used('sign(-1)', '-1', {type:'number', approx:0.1});

Test with -0

>
LayoutTests/imported/w3c/web-platform-tests/css/css-values/signs-abs-computed.h
tml:31
> +test_math_used('calc(sign(0))', '0', {type:'number', approx:0.1});

Need to also test with inputs that are lengths, angles etc to test the "as the
same type as the input" part.