Bug 211991 – Some color-related cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Some color-related cleanup

[Simon Fraser (smfr)]

Created attachment 399576 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 399577 [details]
Patch

[Sam Weinig]

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)?

[Sam Weinig]

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.

[Sam Weinig]

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.

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/261791/webkit

[Radar WebKit Bug Importer]

<rdar://problem/63312412>

[Sam Weinig]

(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>?

[Sam Weinig]

(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?

[Darin Adler]

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.