Summary: | Some color-related cleanup | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||
Component: | Layout and Rendering | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | annulen, bfulgham, darin, dino, ews-watchlist, gyuyoung.kim, kondapallykalyan, ryuan.choi, sam, sergio, simon.fraser, webkit-bug-importer, zalan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2020-05-16 17:21:06 PDT
Created attachment 399576 [details]
Patch
Created attachment 399577 [details]
Patch
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 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 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. (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. (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>? (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? 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. |