[Webkit-unassigned] [Bug 230795] Add implementation for CSSNumericValue math functions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=230795

--- Comment #11 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 445099
  --> https://bugs.webkit.org/attachment.cgi?id=445099
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 at 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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211124/b59410b2/attachment-0001.htm>