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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 10 11:18:08 PDT 2021

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()

Attachment 437888: Patch


--- Comment #42 from Darin Adler <darin at apple.com> ---
Comment on attachment 437888
  --> https://bugs.webkit.org/attachment.cgi?id=437888

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

> 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
> +	   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.

More information about the webkit-reviews mailing list