Bug 213706 - Remove remaining makeSimpleColorFrom* variants
Summary: Remove remaining makeSimpleColorFrom* variants
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-28 16:44 PDT by Sam Weinig
Modified: 2020-06-29 13:57 PDT (History)
14 users (show)

See Also:


Attachments
Patch (44.52 KB, patch)
2020-06-28 17:10 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (44.53 KB, patch)
2020-06-28 18:04 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (44.88 KB, patch)
2020-06-29 11:07 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-06-28 16:44:18 PDT
Remove remaining makeSimpleColorFrom* variants
Comment 1 Sam Weinig 2020-06-28 17:10:53 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2020-06-28 18:04:24 PDT
Created attachment 403020 [details]
Patch
Comment 3 Darin Adler 2020-06-29 09:35:28 PDT
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.
Comment 4 Sam Weinig 2020-06-29 10:29:53 PDT
(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.
Comment 5 Sam Weinig 2020-06-29 11:07:04 PDT
Created attachment 403088 [details]
Patch
Comment 6 EWS 2020-06-29 13:31:50 PDT
Committed r263688: <https://trac.webkit.org/changeset/263688>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403088 [details].
Comment 7 Radar WebKit Bug Importer 2020-06-29 13:57:07 PDT
<rdar://problem/64906405>