[webkit-reviews] review granted: [Bug 213706] Remove remaining makeSimpleColorFrom* variants : [Attachment 403020] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 213706: Remove remaining makeSimpleColorFrom* variants
https://bugs.webkit.org/show_bug.cgi?id=213706

Attachment 403020: Patch

https://bugs.webkit.org/attachment.cgi?id=403020&action=review




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

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:918
> +    if (state().strokeStyle.isValid() &&
state().strokeStyle.isEquivalent(color))

Seems awkward that we need to explicitly check isValid. Why does isEquivalent
ever return true when isValid is false? Wish invalid color was a little more
like Optional's nullopt.

> Source/WebCore/html/canvas/CanvasStyle.h:74
>	   CMYKAColor(const CMYKAColor&) = default;

Why is this needed?

> Source/WebCore/platform/graphics/ColorUtilities.h:38
> +template<typename> struct CMYKA;

Alphabetical order, please

> Source/WebCore/platform/graphics/ColorUtilities.h:67
> -    return std::clamp(static_cast<int>(std::lroundf(f)), 0, 255);
> +    return std::clamp(static_cast<int>(std::round(f)), 0, 255);

I think maybe std::lround would be better than std::round. Since the library
can round and convert to int in a single operation, why not do it?

Or if we are trying to make this work for values larger or smaller than an int,
then we should not cast to int, and need a clamp that takes a floating point
value as an argument.

> Source/WebCore/platform/graphics/ColorUtilities.h:72
> -    return std::clamp(static_cast<int>(std::lroundf(f * 255.0f)), 0, 255);
> +    return std::clamp(static_cast<int>(std::round(f * 255.0f)), 0, 255);

Ditto.