| Summary: | Add helpers to access CoreGraphics color spaces more easily in generic contexts | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||
| Component: | New Bugs | Assignee: | 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
Sam Weinig
2022-01-08 10:21:52 PST
Created attachment 448674 [details]
Patch
Created attachment 448694 [details]
Patch
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. (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. Created attachment 448706 [details]
Patch
> I don’t understand what leads to this more complex thing.
That magical intangible technical constraint of my feeble memory :).
Created attachment 448707 [details]
Patch
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]. Patch 448706 does not build |