Bug 214514 - Additional Color related cleanups
Summary: Additional Color related cleanups
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-07-17 20:53 PDT by Sam Weinig
Modified: 2020-07-19 11:00 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.01 KB, patch)
2020-07-17 20:58 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (8.96 KB, patch)
2020-07-19 10:14 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-07-17 20:53:16 PDT
Additional Color related cleanups
Comment 1 Sam Weinig 2020-07-17 20:58:22 PDT
Created attachment 404634 [details]
Patch
Comment 2 Darin Adler 2020-07-19 08:38:53 PDT
Comment on attachment 404634 [details]
Patch

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

> Source/WebCore/platform/graphics/ColorUtilities.h:97
> +    return std::clamp(component, static_cast<ComponentType>(0), static_cast<ComponentType>(255));

Could this be written this way instead?

    return std::clamp<ComponentType>(component, 0, 255);

Are there disadvantages to writing it this way?

> Source/WebCore/platform/graphics/ColorUtilities.h:102
> +    return std::clamp(component, static_cast<ComponentType>(0.0), static_cast<ComponentType>(1.0));

Could this be written this way instead?

    return std::clamp<ComponentType>(component, 0, 1);

Are there disadvantages to writing it this way?

> Source/WebCore/platform/graphics/ColorUtilities.h:153
> +    copy.red = std::invoke(functor, color.red);
> +    copy.green = std::invoke(functor, color.green);
> +    copy.blue = std::invoke(functor, color.blue);

No forwarding, not even on the third call? I wish I understood universal references better.
Comment 3 Sam Weinig 2020-07-19 10:12:49 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 404634 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404634&action=review
> 
> > Source/WebCore/platform/graphics/ColorUtilities.h:97
> > +    return std::clamp(component, static_cast<ComponentType>(0), static_cast<ComponentType>(255));
> 
> Could this be written this way instead?
> 
>     return std::clamp<ComponentType>(component, 0, 255);
> 
> Are there disadvantages to writing it this way?
> 
> > Source/WebCore/platform/graphics/ColorUtilities.h:102
> > +    return std::clamp(component, static_cast<ComponentType>(0.0), static_cast<ComponentType>(1.0));
> 
> Could this be written this way instead?
> 
>     return std::clamp<ComponentType>(component, 0, 1);
> 
> Are there disadvantages to writing it this way?

You know, I am not sure. It just didn't occur to me. Seems like your proposal is better, and the literals should convert cleanly.

> 
> > Source/WebCore/platform/graphics/ColorUtilities.h:153
> > +    copy.red = std::invoke(functor, color.red);
> > +    copy.green = std::invoke(functor, color.green);
> > +    copy.blue = std::invoke(functor, color.blue);
> 
> No forwarding, not even on the third call? I wish I understood universal
> references better.

Seems like I could forward it to the last one. Will change.

Thanks for all the reviews.
Comment 4 Sam Weinig 2020-07-19 10:14:53 PDT
Created attachment 404674 [details]
Patch
Comment 5 EWS 2020-07-19 10:59:09 PDT
Committed r264577: <https://trac.webkit.org/changeset/264577>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404674 [details].
Comment 6 Radar WebKit Bug Importer 2020-07-19 11:00:23 PDT
<rdar://problem/65792738>