Bug 234798 – Add resolved/unresolved color type accessors to require users to be explicit about whether they will handle missing/none color components

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Sam Weinig]

As a precursor to 233526 ([CSS Color 4] Add support for "Missing"/"none" color components), which will require allowing color types to have components with NaN values to indicate missing and powerless components, we should use the type system to require users who want to access components to declare whether they will handle those values, or want the existing behavior (and the behavior used for rendering) where those components are converted to 0.

To do this, color types will no longer expose their components publicly, but rather, will only allow access to them via new subclasses, ResolvedColorType<ColorType> and UnresolvedColorType<ColorType> (the former doing the conversion to 0, the later doing nothing). Theses subclasses will expose the components publicly (via using directives). 

This will require call sites to change from:

auto [r, g, b, alpha] = myColorType;

to:

auto [r, g, b, alpha] = myColorType.resolved();

[Sam Weinig]

Created attachment 448182 [details]
Patch

[Sam Weinig]

Created attachment 448184 [details]
Patch

[Sam Weinig]

Created attachment 448186 [details]
Patch

[Sam Weinig]

Created attachment 448191 [details]
Patch

[Sam Weinig]

Created attachment 448194 [details]
Patch

[Sam Weinig]

Created attachment 448197 [details]
Patch

[Sam Weinig]

Created attachment 448199 [details]
Patch

[Sam Weinig]

Created attachment 448201 [details]
Patch

[Sam Weinig]

Created attachment 448202 [details]
Patch

[Antti Koivisto]

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?

[Sam Weinig]

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

[Sam Weinig]

Created attachment 448248 [details]
Patch

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/87069742>