<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add implementation for CSSNumericValue math functions"
   href="https://bugs.webkit.org/show_bug.cgi?id=230795#c16">Comment # 16</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add implementation for CSSNumericValue math functions"
   href="https://bugs.webkit.org/show_bug.cgi?id=230795">bug 230795</a>
              from <span class="vcard"><a class="email" href="mailto:cdumez@apple.com" title="Chris Dumez <cdumez@apple.com>"> <span class="fn">Chris Dumez</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=445117&action=diff" name="attach_445117" title="Patch">attachment 445117</a> <a href="attachment.cgi?id=445117&action=edit" title="Patch">[details]</a></span>
Patch

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

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:54
> +inline const String* CSSNumericValue::commonUnit(const Vector<Ref<CSSNumericValue>>& numericVector)</span >

Should probably return a String instead of a String*. A String is already a pointer (to a StringImpl) and already has a "null" state.

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:57
> +        return nullptr;</span >

return { };

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:61
> +    for (auto it = numericVector.begin(); it != numericVector.end(); ++it) {</span >

It looks like this loop is unnecessarily checking the first item in numericVector again.

I think I would write this like so:
```
if (numericVector.isEmpty())
    return { };

String unit;
for (auto& numericValue : numericVector) {
    if (!is<CSSUnitValue>(numericValue.get()))
        return { };
    auto& unitValue = downcast<CSSUnitValue>(numericValue.get());
    if (unit.isNull())
        unit = unitValue.unit();
    else if (unitValue.unit() != unit)
        return { };
}

return unit;
```

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:63
> +            return nullptr;</span >

return { };

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:69
> +inline const String* CSSNumericValue::unitForProduct(const Vector<Ref<CSSNumericValue>>& numericVector)</span >

Same comment, seems this should return a `String`.

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:178
> +        numericVector.append(Ref<CSSNumericValue> { *this });</span >

`Ref<CSSNumericValue> { *this }` -> `Ref { *this }`

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:190
> +        return Ref<CSSNumericValue> { *this };</span >

return Ref { *this };

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:198
> +        numericVector.append(Ref<CSSNumericValue> { *this });</span >

Ref { *this }

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:222
> +        numericVector.append(Ref<CSSNumericValue> { *this });</span >

Ref { *this }

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:251
> +        numericVector.append(Ref<CSSNumericValue> { *this });</span >

Ref { *this }

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:327
> +    return CSSMathNegate::create(RefPtr<CSSNumericValue> { this });</span >

RefPtr { this }

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.cpp:344
> +    return Ref<CSSNumericValue> { CSSMathInvert::create(RefPtr<CSSNumericValue> { this }) };</span >

RefPtr { this }

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.h:47
> +    const String* commonUnit(const Vector<Ref<CSSNumericValue>>&);</span >

Why is this public? Does it even need to be in the header? Can be be a static function in the cpp only?

<span class="quote">> Source/WebCore/css/typedom/CSSNumericValue.h:48
> +    const String* unitForProduct(const Vector<Ref<CSSNumericValue>>&);</span >

ditto.

<span class="quote">> Source/WebCore/css/typedom/numeric/CSSMathInvert.h:38
> +    Ref<CSSNumericValue> value() const { return m_value.copyRef(); }</span >

Not sure this change is actually useful. The caller can construct a Ref<> if they need one. Otherwise, it may cause ref counting churn unnecessarily.

<span class="quote">> Source/WebCore/css/typedom/numeric/CSSMathMax.h:45
>      CSSMathMax(Vector<CSSNumberish>&&);</span >

explicit.

<span class="quote">> Source/WebCore/css/typedom/numeric/CSSMathMax.h:46
> +    CSSMathMax(Vector<Ref<CSSNumericValue>>&&);</span >

explicit.

<span class="quote">> Source/WebCore/css/typedom/numeric/CSSMathMin.h:45
>      CSSMathMin(Vector<CSSNumberish>&&);</span >

explicit.

<span class="quote">> Source/WebCore/css/typedom/numeric/CSSMathMin.h:46
> +    CSSMathMin(Vector<Ref<CSSNumericValue>>&&);</span >

explicit.

<span class="quote">> Source/WebCore/css/typedom/numeric/CSSMathNegate.h:41
> +    Ref<CSSNumericValue> value() const { return m_value.copyRef(); }</span >

Not convinced this change is a good idea.

<span class="quote">> Source/WebCore/css/typedom/numeric/CSSMathProduct.h:47
>      CSSMathProduct(Vector<CSSNumberish>&&);</span >

explicit.

<span class="quote">> Source/WebCore/css/typedom/numeric/CSSMathProduct.h:48
> +    CSSMathProduct(Ref<CSSNumericArray>&&);</span >

explicit.

<span class="quote">> Source/WebCore/css/typedom/numeric/CSSMathSum.h:47
> +    CSSMathSum(Ref<CSSNumericArray>&&);</span >

explicit.</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>