[Webkit-unassigned] [Bug 203312] Implement the CSS exponent functions: pow(), sqrt(), hypot()

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #436898|review?                     |review+
              Flags|                            |

--- Comment #32 from Darin Adler <darin at apple.com> ---
Comment on attachment 436898
  --> https://bugs.webkit.org/attachment.cgi?id=436898
Patch

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

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.

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:131
> +    // TODO: sin, cos, tan, asin, acos, atan, atan2.

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

> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:181
> +    // TODO: sin, cos, tan, asin, acos, atan, atan2.

Ditto.

> 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

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

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

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

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

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

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

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

> 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));
> +        }

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

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:89
> +    case CalcOperator::Pow: {

Really confused about how CSSCalcValue uses double, and CalcExpressionOperation uses float.

-- 
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/20210831/8688e3ea/attachment.htm>