[Webkit-unassigned] [Bug 234798] Add resolved/unresolved color type accessors to require users to be explicit about whether they will handle missing/none color components

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=234798

--- Comment #11 from Sam Weinig <sam at webkit.org> ---
(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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220103/b00c19b0/attachment.htm>