[webkit-reviews] review granted: [Bug 203312] Implement the CSS exponent functions: pow(), sqrt(), hypot() : [Attachment 437888] 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 437888: Patch

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




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