Bug 235004

Summary: Add helpers to access CoreGraphics color spaces more easily in generic contexts
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, darin, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
darin: review+
Patch
ews-feeder: commit-queue-
Patch ews-feeder: commit-queue-

Description Sam Weinig 2022-01-08 10:21:52 PST
Add helpers to access CoreGraphics color spaces more easily in generic contexts
Comment 1 Sam Weinig 2022-01-08 11:09:56 PST Comment hidden (obsolete)
Comment 2 Sam Weinig 2022-01-08 19:59:01 PST
Created attachment 448694 [details]
Patch
Comment 3 Darin Adler 2022-01-09 08:49:44 PST
Comment on attachment 448694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448694&action=review

> Source/WebCore/platform/graphics/cg/ColorSpaceCG.h:141
> +    template<ColorSpace C> static auto test(int) -> TrueIfDefined<decltype(CGColorSpaceMapping<C>::colorSpace())>;

Consider "space" or "colorSpace" instead of C? Here and just below.

> Source/WebCore/platform/graphics/cg/ColorSpaceCG.h:142
> +    template<ColorSpace C> static auto test(long) -> std::false_type;

Omit "C" here.

> Source/WebCore/platform/graphics/cg/ColorSpaceCG.h:145
> +template<ColorSpace C> inline constexpr bool HasCGColorSpaceMapping = decltype(HasCGColorSpaceMappingHelper::test<C>(0))();

This looks more complicated than what ColorTypes.h uses for HasDescriptorMember, etc. And HasBeginFunctionMember in Hasher.h is what I came up with when I needed to solve a similar problem. I am not sure if the problems are close enough to the same, but I am surprised we need more code here.

HasModernDecoder in ArgumentCoder.h is more complex, closer to what we have here.

Would like it if the idioms were similar for cases like these, and I don’t understand what leads to this more complex thing.
Comment 4 Sam Weinig 2022-01-09 09:24:29 PST
(In reply to Darin Adler from comment #3)
> Comment on attachment 448694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448694&action=review
> 
> > Source/WebCore/platform/graphics/cg/ColorSpaceCG.h:141
> > +    template<ColorSpace C> static auto test(int) -> TrueIfDefined<decltype(CGColorSpaceMapping<C>::colorSpace())>;
> 
> Consider "space" or "colorSpace" instead of C? Here and just below.
> 
> > Source/WebCore/platform/graphics/cg/ColorSpaceCG.h:142
> > +    template<ColorSpace C> static auto test(long) -> std::false_type;
> 
> Omit "C" here.
> 
> > Source/WebCore/platform/graphics/cg/ColorSpaceCG.h:145
> > +template<ColorSpace C> inline constexpr bool HasCGColorSpaceMapping = decltype(HasCGColorSpaceMappingHelper::test<C>(0))();
> 
> This looks more complicated than what ColorTypes.h uses for
> HasDescriptorMember, etc. And HasBeginFunctionMember in Hasher.h is what I
> came up with when I needed to solve a similar problem. I am not sure if the
> problems are close enough to the same, but I am surprised we need more code
> here.
> 
> HasModernDecoder in ArgumentCoder.h is more complex, closer to what we have
> here.
> 
> Would like it if the idioms were similar for cases like these, and I don’t
> understand what leads to this more complex thing.

Dang, I was trying to remember where I (or someone else) had done this before and was coming up blank. These options are much more clear. Thanks.
Comment 5 Sam Weinig 2022-01-09 09:27:56 PST
Created attachment 448706 [details]
Patch
Comment 6 Sam Weinig 2022-01-09 09:29:25 PST
> I don’t understand what leads to this more complex thing.

That magical intangible technical constraint of my feeble memory :).
Comment 7 Sam Weinig 2022-01-09 09:33:10 PST
Created attachment 448707 [details]
Patch
Comment 8 EWS 2022-01-09 10:07:11 PST
Committed r287821 (245873@main): <https://commits.webkit.org/245873@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448707 [details].
Comment 9 Radar WebKit Bug Importer 2022-01-09 10:08:22 PST
<rdar://problem/87312687>
Comment 10 EWS 2022-01-09 10:19:36 PST
Patch 448706 does not build