Bug 230795 - Add implementation for CSSNumericValue math functions
Summary: Add implementation for CSSNumericValue math functions
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, WPTImpact
: 233400 (view as bug list)
Depends on:
Blocks: 175733
  Show dependency treegraph
 
Reported: 2021-09-25 04:10 PDT by Qiaosong Zhou
Modified: 2022-03-01 09:53 PST (History)
13 users (show)

See Also:


Attachments
Patch (3.64 KB, patch)
2021-09-25 04:11 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (3.64 KB, patch)
2021-09-25 04:33 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (5.52 KB, patch)
2021-09-25 12:32 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (24.66 KB, patch)
2021-11-23 18:54 PST, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (35.31 KB, patch)
2021-11-23 19:01 PST, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (35.42 KB, patch)
2021-11-24 10:18 PST, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (36.01 KB, patch)
2021-11-24 21:50 PST, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (36.13 KB, patch)
2022-01-04 11:59 PST, Qiaosong Zhou
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Qiaosong Zhou 2021-09-25 04:10:29 PDT
Add implementation for CSSNumericValue math functions
Comment 1 Qiaosong Zhou 2021-09-25 04:11:20 PDT
Created attachment 439259 [details]
Patch
Comment 2 Qiaosong Zhou 2021-09-25 04:33:26 PDT
Created attachment 439260 [details]
Patch
Comment 3 Qiaosong Zhou 2021-09-25 12:32:29 PDT
Created attachment 439268 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2021-10-02 04:11:15 PDT
<rdar://problem/83796319>
Comment 5 Qiaosong Zhou 2021-11-20 15:28:35 PST
*** Bug 233400 has been marked as a duplicate of this bug. ***
Comment 6 Qiaosong Zhou 2021-11-23 18:54:40 PST
Created attachment 445056 [details]
Patch
Comment 7 Qiaosong Zhou 2021-11-23 18:58:37 PST
Passing all tests in http://wpt.live/css/css-typed-om/stylevalue-subclasses/numeric-objects/arithmetic.tentative.html except the ones concerning CSSNumericType, which will be added in a separate patch.
Comment 8 Qiaosong Zhou 2021-11-23 19:01:52 PST
Created attachment 445057 [details]
Patch
Comment 9 Simon Fraser (smfr) 2021-11-24 09:48:32 PST
Comment on attachment 445057 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

You need a description here of what this change is. If you're implementing a spec, provide a link to the spec.
Comment 10 Qiaosong Zhou 2021-11-24 10:18:31 PST
Created attachment 445099 [details]
Patch
Comment 11 Simon Fraser (smfr) 2021-11-24 10:43:17 PST
Comment on attachment 445099 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> +2021-11-23  Johnson Zhou  <johnson.zhou.sh@icloud.com>

This should be your Apple email address.

> Source/WebCore/ChangeLog:6
> +        Add implementation for CSSNumericValue math functions according to CSS Typed OM Spec. 
> +        https://drafts.css-houdini.org/css-typed-om-1/#numeric-value
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=230795

The correct format here is prepared for you by "webkit-patch upload":

    Add implementation for CSSNumericValue math functions
    https://bugs.webkit.org/show_bug.cgi?id=230795

    Add implementations of the CSS Types OM numeric operations: https://drafts.css-houdini.org/css-typed-om-1/#numeric-value

and add additional notes if there is anything interesting or confusing in the implementation that needs to be explained.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:60
> +        if (!is<CSSUnitValue>(*it) || downcast<CSSUnitValue>(numericVector.begin()->get()).unit() != downcast<CSSUnitValue>(it->get()).unit())

Might be worth copying downcast<CSSUnitValue>(numericVector.begin()->get()).unit() into a local variable.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:102
> +    // FIXME: Lacks impl. of "Let type be the result of adding the types of every item in values. If type is failure, throw a TypeError."

You should file a bugzilla bug for this work and reference it here.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:114
> +    if (is<CSSMathSum>(*this)) {

It might be worth adding links to the spec for these; for example, at https://drafts.css-houdini.org/css-typed-om-1/#dom-cssnumericvalue-add, it's useful to know that the values from |this| are prepended.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:147
> +    

Blank line.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:156
> +    // FIXME: Lacks impl. of "Let type be the result of adding the types of every item in values. If type is failure, throw a TypeError."

File and reference a bug.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:225
> +            if (value < minValue)
> +                minValue = value;

Can we just use std::min()?

> Source/WebCore/css/typedom/CSSNumericValue.cpp:256
> +            if (value > maxValue)
> +                maxValue = value;

Can we just use std::max()?

> Source/WebCore/css/typedom/CSSNumericValue.cpp:320
> +    if (is<CSSUnitValue>(*this))
> +        return CSSUnitValue::create(-downcast<CSSUnitValue>(*this).value(), downcast<CSSUnitValue>(*this).unit());

Put downcast<CSSUnitValue>(*this) into a local variable.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:334
> +    if (is<CSSUnitValue>(*this) && downcast<CSSUnitValue>(*this).unit() == "number") {
> +        if (downcast<CSSUnitValue>(*this).value() == 0.0)
> +            return Exception { RangeError, "Cannot invert CSSUnitValue with value 0.0 or -0.0"_s };
> +    
> +        return Ref<CSSNumericValue> { CSSUnitValue::create(1.0 / downcast<CSSUnitValue>(*this).value(), downcast<CSSUnitValue>(*this).unit()) };

Put downcast<CSSUnitValue>(*this) into a local variable.

> Source/WebCore/css/typedom/CSSNumericValue.h:48
> +    const String* getSameUnit(const Vector<Ref<CSSNumericValue>>&);
> +    const String* getProductUnit(const Vector<Ref<CSSNumericValue>>&);

We don't generally use "get" in function names. I suggest:

commonUnit()
unitForProduct()

> Source/WebCore/css/typedom/CSSNumericValue.h:55
>      Ref<CSSNumericValue> add(Vector<CSSNumberish>&&);
>      Ref<CSSNumericValue> sub(Vector<CSSNumberish>&&);
>      Ref<CSSNumericValue> mul(Vector<CSSNumberish>&&);
> -    Ref<CSSNumericValue> div(Vector<CSSNumberish>&&);
> +    ExceptionOr<Ref<CSSNumericValue>> div(Vector<CSSNumberish>&&);
>      Ref<CSSNumericValue> min(Vector<CSSNumberish>&&);
>      Ref<CSSNumericValue> max(Vector<CSSNumberish>&&);

I think these should all be const functions, right? They don't change the value of |this|. Ideally we'd name them something different to make this more clear, but this naming matches the spec.

> Source/WebCore/css/typedom/CSSNumericValue.h:69
> +    Ref<CSSNumericValue> addImpl(Vector<Ref<CSSNumericValue>>&&);
> +    Ref<CSSNumericValue> mulImpl(Vector<Ref<CSSNumericValue>>&&);

Can these be const?
Comment 12 Qiaosong Zhou 2021-11-24 10:50:56 PST
(In reply to Simon Fraser (smfr) from comment #11)

> > Source/WebCore/ChangeLog:1
> > +2021-11-23  Johnson Zhou  <johnson.zhou.sh@icloud.com>
> 
> This should be your Apple email address.
> 

I'm no longer working at Apple, should I still use my Apple email that I no longer have access to?
Comment 13 Simon Fraser (smfr) 2021-11-24 12:50:22 PST
(In reply to Qiaosong Zhou from comment #12)
> (In reply to Simon Fraser (smfr) from comment #11)
> 
> > > Source/WebCore/ChangeLog:1
> > > +2021-11-23  Johnson Zhou  <johnson.zhou.sh@icloud.com>
> > 
> > This should be your Apple email address.
> > 
> 
> I'm no longer working at Apple, should I still use my Apple email that I no
> longer have access to?

I see! Probably not.
Comment 14 Qiaosong Zhou 2021-11-24 20:45:50 PST
(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 445099 [details]
> 
> > Source/WebCore/css/typedom/CSSNumericValue.h:55
> >      Ref<CSSNumericValue> add(Vector<CSSNumberish>&&);
> >      Ref<CSSNumericValue> sub(Vector<CSSNumberish>&&);
> >      Ref<CSSNumericValue> mul(Vector<CSSNumberish>&&);
> > -    Ref<CSSNumericValue> div(Vector<CSSNumberish>&&);
> > +    ExceptionOr<Ref<CSSNumericValue>> div(Vector<CSSNumberish>&&);
> >      Ref<CSSNumericValue> min(Vector<CSSNumberish>&&);
> >      Ref<CSSNumericValue> max(Vector<CSSNumberish>&&);
> 
> I think these should all be const functions, right? They don't change the
> value of |this|. Ideally we'd name them something different to make this
> more clear, but this naming matches the spec.
> 
> > Source/WebCore/css/typedom/CSSNumericValue.h:69
> > +    Ref<CSSNumericValue> addImpl(Vector<Ref<CSSNumericValue>>&&);
> > +    Ref<CSSNumericValue> mulImpl(Vector<Ref<CSSNumericValue>>&&);
> 
> Can these be const?

I can't seem to make them const because I'm converting *this into a Ref that is then prepended into the Vector.
Comment 15 Qiaosong Zhou 2021-11-24 21:50:51 PST
Created attachment 445117 [details]
Patch
Comment 16 Chris Dumez 2021-11-29 12:21:17 PST
Comment on attachment 445117 [details]
Patch

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

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

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

> Source/WebCore/css/typedom/CSSNumericValue.cpp:57
> +        return nullptr;

return { };

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

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;
```

> Source/WebCore/css/typedom/CSSNumericValue.cpp:63
> +            return nullptr;

return { };

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

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

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

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

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

return Ref { *this };

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

Ref { *this }

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

Ref { *this }

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

Ref { *this }

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

RefPtr { this }

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

RefPtr { this }

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

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

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

ditto.

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

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.

> Source/WebCore/css/typedom/numeric/CSSMathMax.h:45
>      CSSMathMax(Vector<CSSNumberish>&&);

explicit.

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

explicit.

> Source/WebCore/css/typedom/numeric/CSSMathMin.h:45
>      CSSMathMin(Vector<CSSNumberish>&&);

explicit.

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

explicit.

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

Not convinced this change is a good idea.

> Source/WebCore/css/typedom/numeric/CSSMathProduct.h:47
>      CSSMathProduct(Vector<CSSNumberish>&&);

explicit.

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

explicit.

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

explicit.
Comment 17 Qiaosong Zhou 2022-01-04 11:59:15 PST
Created attachment 448316 [details]
Patch
Comment 19 Antoine Quint 2022-03-01 09:51:17 PST
Scratch that, those tests rely on CSSNumericValue.parse().