[webkit-reviews] review granted: [Bug 229775] Implement atan, acos, asin, atan2 calc functions : [Attachment 438299] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 229775: Implement atan, acos, asin, atan2 calc functions
https://bugs.webkit.org/show_bug.cgi?id=229775

Attachment 438299: Patch

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




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

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

> Source/WebCore/ChangeLog:8
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Tests:
imported/w3c/web-platform-tests/css/css-values/acos-asin-atan-atan2-computed.ht
ml

Words here please (like a spec link etc).

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:490
> +
>  

Too many blank lines.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:-441
> -	   return;

Is this removal significant?

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:630
> +	   auto newChild =
CSSCalcPrimitiveValueNode::create(CSSPrimitiveValue::create(resolvedValue,
CSSUnitType::CSS_DEG));

Is it CSS_DEG because the spec says that degrees are the canonical angle unit?

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:1133
> +	   return rad2deg(std::asin(children[0]));

Maybe link to the part of the spec that explains why returning angles in
degrees is correct.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:1138
> +	   return rad2deg(std::asin(children[0]));

acts, not asin? Why did tests not catch this.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:1143
> +	   return rad2deg(std::asin(children[0]));

asin -> atan? Again, where is the test coverage.