Summary: | Add resolved/unresolved color type accessors to require users to be explicit about whether they will handle missing/none color components | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, changseok, cmarcelo, darin, dino, dmazzoni, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, jcraig, jdiggs, koivisto, kondapallykalyan, luiz, macpherson, menard, mifenton, pdr, sabouhallawa, samuel_white, schenney, sergio, simon.fraser, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | Other | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 233526 | ||||||||||||||||||||||||
Attachments: |
|
Description
Sam Weinig
2022-01-02 11:11:13 PST
Created attachment 448182 [details]
Patch
Created attachment 448184 [details]
Patch
Created attachment 448186 [details]
Patch
Created attachment 448191 [details]
Patch
Created attachment 448194 [details]
Patch
Created attachment 448197 [details]
Patch
Created attachment 448199 [details]
Patch
Created attachment 448201 [details]
Patch
Created attachment 448202 [details]
Patch
Comment on attachment 448202 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448202&action=review > Source/WebCore/ChangeLog:33 > + opertunity to replace all its uses with the generic toColorTypeLossy<SRGBA<>>. spelling "opertunity" > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:95 > + auto [r, g, b, a] = bgColor.toColorTypeLossy<SRGBA<uint8_t>>().resolved(); There is a lot of this. Could we have a template-free shortcut for this apparently common case with a sensible name that hints the proper use? One should be able to write WebKit code without becoming a color space expert or mindlessly copy-pasting. > Source/WebCore/accessibility/atspi/AccessibilityObjectTextAtspi.cpp:827 > - auto [r, g, b, a] = bgColor.toSRGBALossy<uint8_t>(); > + auto [r, g, b, a] = bgColor..toColorTypeLossy<SRGBA<uint8_t>>().resolved(); what's up with those double ..? > Source/WebCore/accessibility/atspi/AccessibilityObjectTextAtspi.cpp:833 > + auto [r, g, b, a] = fgColor..toColorTypeLossy<SRGBA<uint8_t>>().resolved(); here too > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1534 > + auto originColorAsSRGB = originColor.toColorTypeLossy<SRGBA<float>>().resolved(); Similarly maybe this could have a shortcut? (In reply to Antti Koivisto from comment #10) > Comment on attachment 448202 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448202&action=review > > > Source/WebCore/ChangeLog:33 > > + opertunity to replace all its uses with the generic toColorTypeLossy<SRGBA<>>. > > spelling "opertunity" Always taking "opertunities" to innovate in the spelling game. > > > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:95 > > + auto [r, g, b, a] = bgColor.toColorTypeLossy<SRGBA<uint8_t>>().resolved(); > > There is a lot of this. Could we have a template-free shortcut for this > apparently common case with a sensible name that hints the proper use? > > One should be able to write WebKit code without becoming a color space > expert or mindlessly copy-pasting. Heh, I removed the shortcut I had because I didn't think it was useful, but I don't think the shortcut really resolved this problem. The old one was: 'color.toSRGBALossy<>()' What I have found so far is that making these very explicit has made it much less likely for people to make mistakes with the color code (or at the very least, made it easy to notice when things are being done incorrectly after the fact). For the most part, you almost never need to access color channels directly like this, and when you do, you really need to know which color space you want it in. I would love to figure out ways to simplify these things, but so far I haven't found any silver bullets. The main way I have been trying to make it easier for people is by removing the needs for accessing the color channels directly by having shared utilities for the common uses (interpolation, contrasting, etc.). Its certainly something we should continue to improve. > > > Source/WebCore/accessibility/atspi/AccessibilityObjectTextAtspi.cpp:827 > > - auto [r, g, b, a] = bgColor.toSRGBALossy<uint8_t>(); > > + auto [r, g, b, a] = bgColor..toColorTypeLossy<SRGBA<uint8_t>>().resolved(); > > what's up with those double ..? > > > Source/WebCore/accessibility/atspi/AccessibilityObjectTextAtspi.cpp:833 > > + auto [r, g, b, a] = fgColor..toColorTypeLossy<SRGBA<uint8_t>>().resolved(); > > here too They are what we call in the industry...bugs :). I guess those files don't get compiled in any of the EWS bots. Will fix. > > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1534 > > + auto originColorAsSRGB = originColor.toColorTypeLossy<SRGBA<float>>().resolved(); > > Similarly maybe this could have a shortcut? One tiny shortcut would be to have resolved and unresolved versions of toColorTypeLossy (e.g. toResolvedColorTypeLossy and toUnresolvedColorTypeLossy) which do the call after for you, but I am not sure that really helps. Thanks for the review. I will continue looking into ways to make this simpler. I would also love to try and do a little more education in this area, so that more people can become more comfortable with it. The concepts aren't too tricky once they are explained, but finding good resources to learn them was harder than it should have been. Maybe one thing I could do is make one of those pages like we have for RefPtr on it? Created attachment 448248 [details]
Patch
Committed r287552 (245687@main): <https://commits.webkit.org/245687@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448248 [details]. |