<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
          <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Implement the CSS exponent functions: pow(), sqrt(), hypot()"
   href="https://bugs.webkit.org/show_bug.cgi?id=203312">bug 203312</a>
          <br>
             <table border="1" cellspacing="0" cellpadding="8">
          <tr>
            <th>What</th>
            <th>Removed</th>
            <th>Added</th>
          </tr>

         <tr>
           <td style="text-align:right;">Attachment #436898 Flags</td>
           <td>review?
           </td>
           <td>review+
           </td>
         </tr></table>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Implement the CSS exponent functions: pow(), sqrt(), hypot()"
   href="https://bugs.webkit.org/show_bug.cgi?id=203312#c32">Comment # 32</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Implement the CSS exponent functions: pow(), sqrt(), hypot()"
   href="https://bugs.webkit.org/show_bug.cgi?id=203312">bug 203312</a>
              from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=436898&action=diff" name="attach_436898" title="Patch">attachment 436898</a> <a href="attachment.cgi?id=436898&action=edit" title="Patch">[details]</a></span>
Patch

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=436898&action=review">https://bugs.webkit.org/attachment.cgi?id=436898&action=review</a>

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.

<span class="quote">> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:131
> +    // TODO: sin, cos, tan, asin, acos, atan, atan2.</span >

While modifying this, should change from the non-WebKit-style TODO to a WebKit FIXME

<span class="quote">> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:181
> +    // TODO: sin, cos, tan, asin, acos, atan, atan2.</span >

Ditto.

<span class="quote">> 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</span >

Not new wording, but what does "adding the types" mean here?

<span class="quote">> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:332
> +    for (auto& child : values) {
> +        if (child->category() != CalculationCategory::Number)
> +            return nullptr;
> +    }</span >

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)
    {
        // ...
    }

<span class="quote">> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:346
> +    for (unsigned i = 1; i < values.size(); ++i) {</span >

Strange to use "unsigned" here when Vector indices are size_t.

<span class="quote">> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:643
> +        // Don't simplify out sqrt() or hypot() before they are evaluated.</span >

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?"

<span class="quote">> 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));
> +        }</span >

Please merge this into a single case like the case above for Min/Max/Clamp.

<span class="quote">> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:89
> +    case CalcOperator::Pow: {</span >

Really confused about how CSSCalcValue uses double, and CalcExpressionOperation uses float.</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>