Bug 213706 – Remove remaining makeSimpleColorFrom* variants

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Remove remaining makeSimpleColorFrom* variants

[Sam Weinig]

Created attachment 403017 [details]
Patch

[Sam Weinig]

Created attachment 403020 [details]
Patch

[Darin Adler]

Comment on attachment 403020 [details]
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.

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> Comment on attachment 403020 [details]
> 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.

Looks like this isValid() is totally not needed. isEquivalent() will return false already is the CanvasStyle is in the invalid state. Removed a bunch of these.


> 
> > Source/WebCore/html/canvas/CanvasStyle.h:74
> >          CMYKAColor(const CMYKAColor&) = default;
> 
> Why is this needed?

I am not sure. Will try removing it.

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

Fixed.

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

Fixed.

[Sam Weinig]

Created attachment 403088 [details]
Patch

[EWS]

Committed r263688: <https://trac.webkit.org/changeset/263688>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403088 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64906405>