Bug 211991 - Some color-related cleanup
Summary: Some color-related cleanup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-16 17:21 PDT by Simon Fraser (smfr)
Modified: 2020-05-18 10:42 PDT (History)
13 users (show)

See Also:


Attachments
Patch (19.02 KB, patch)
2020-05-16 17:23 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (18.88 KB, patch)
2020-05-16 17:43 PDT, Simon Fraser (smfr)
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2020-05-16 17:21:06 PDT
Some color-related cleanup
Comment 1 Simon Fraser (smfr) 2020-05-16 17:23:30 PDT
Created attachment 399576 [details]
Patch
Comment 2 Simon Fraser (smfr) 2020-05-16 17:43:30 PDT
Created attachment 399577 [details]
Patch
Comment 3 Sam Weinig 2020-05-16 18:07:23 PDT
Comment on attachment 399577 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.cpp:571
> +    float r, g, b, a;
> +    getRGBA(r, g, b, a);

Is there any good reason getRGBA() should take out parameters? Is it just for ADL to distinguish between float and double? Is the double version really necessary or used (I ask without looking)?
Comment 4 Sam Weinig 2020-05-16 18:11:23 PDT
Comment on attachment 399577 [details]
Patch

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

>> Source/WebCore/platform/graphics/Color.cpp:571
>> +    getRGBA(r, g, b, a);
> 
> Is there any good reason getRGBA() should take out parameters? Is it just for ADL to distinguish between float and double? Is the double version really necessary or used (I ask without looking)?

I will try this a follow up cleanup.
Comment 5 Sam Weinig 2020-05-16 18:17:18 PDT
Comment on attachment 399577 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.h:218
> +    // This will convert non-sRGB colorspace colors into sRGB.
> +    FloatComponents toSRGBAComponentsLossy() const;

I think you want to forward declare FloatComponents in this file.
Comment 6 Simon Fraser (smfr) 2020-05-16 18:36:55 PDT
(In reply to Sam Weinig from comment #3)
> Comment on attachment 399577 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399577&action=review
> 
> > Source/WebCore/platform/graphics/Color.cpp:571
> > +    float r, g, b, a;
> > +    getRGBA(r, g, b, a);
> 
> Is there any good reason getRGBA() should take out parameters? Is it just
> for ADL to distinguish between float and double? Is the double version
> really necessary or used (I ask without looking)?

No, getRGBA, getHSL etc can return FloatComponents. I'll do that in a followup.
Comment 7 Simon Fraser (smfr) 2020-05-16 21:56:10 PDT
https://trac.webkit.org/changeset/261791/webkit
Comment 8 Radar WebKit Bug Importer 2020-05-16 21:57:17 PDT
<rdar://problem/63312412>
Comment 9 Sam Weinig 2020-05-17 10:45:11 PDT
(In reply to Simon Fraser (smfr) from comment #6)
> (In reply to Sam Weinig from comment #3)
> > Comment on attachment 399577 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=399577&action=review
> > 
> > > Source/WebCore/platform/graphics/Color.cpp:571
> > > +    float r, g, b, a;
> > > +    getRGBA(r, g, b, a);
> > 
> > Is there any good reason getRGBA() should take out parameters? Is it just
> > for ADL to distinguish between float and double? Is the double version
> > really necessary or used (I ask without looking)?
> 
> No, getRGBA, getHSL etc can return FloatComponents. I'll do that in a
> followup.

Ok. Cool. Can we reconsider the name FloatComponents. I would much prefer something that included the work Color in it. Perhaps just ColorFloatComponents? Or make it a template and combine it with ColorComponents to create ColorComponents<float>/ColorComponents<uint8_t>?
Comment 10 Sam Weinig 2020-05-17 10:46:46 PDT
(In reply to Sam Weinig from comment #9)
> (In reply to Simon Fraser (smfr) from comment #6)
> > (In reply to Sam Weinig from comment #3)
> > > Comment on attachment 399577 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=399577&action=review
> > > 
> > > > Source/WebCore/platform/graphics/Color.cpp:571
> > > > +    float r, g, b, a;
> > > > +    getRGBA(r, g, b, a);
> > > 
> > > Is there any good reason getRGBA() should take out parameters? Is it just
> > > for ADL to distinguish between float and double? Is the double version
> > > really necessary or used (I ask without looking)?
> > 
> > No, getRGBA, getHSL etc can return FloatComponents. I'll do that in a
> > followup.
> 
> Ok. Cool. Can we reconsider the name FloatComponents. I would much prefer
> something that included the work Color in it. Perhaps just
> ColorFloatComponents? Or make it a template and combine it with
> ColorComponents to create ColorComponents<float>/ColorComponents<uint8_t>?

Or add some additional types and have RGBAFloatComponents/HSLAFloatComponents/HSVAFloatComponents?
Comment 11 Darin Adler 2020-05-18 10:42:33 PDT
Excited to see this progress. I have notes about a lot of improvements for Color that I’d like to make, and I hope after you do some of these things there’s almost nothing left for me.