We have a number of version detection macros which are really platform detection macros. These should be implemented in a way which makes their intention obvious.
Created attachment 388895 [details] Patch
Comment on attachment 388895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388895&action=review All of this should go into PlatformHave.h I think > Source/WebCore/page/SettingsDefaultValues.h:107 > +#if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101400) && !PLATFORM(APPLETV) The condition is perfect here, but this seems to be HAVE(CG_CONTEXT_DRAW_CONIC_GRADIENT). > Source/WebCore/platform/graphics/cg/GradientCG.cpp:185 > +#if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101400) && !PLATFORM(APPLETV) Ditto. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:42 > +#define HAS_CORE_TEXT_WIDTH_ATTRIBUTE (PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(MACCATALYST)) Move to PlatformHave.h? And change the logic to: #if PLATFORM(COCOA) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV) > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1067 > +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV) I think this might be HAVE(CT_FONT_DESCRIPTOR_IS_SYSTEM_UI_FONT) > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1642 > +#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(MACCATALYST) I think this might be HAVE(CT_FONT_DESCRIPTOR_CREATE_LAST_RESORT) Also, it’s arbitrary to write this as the reverse of how we wrote the identical condition above. > Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:85 > +#if PLATFORM(IOS) || PLATFORM(MACCATALYST) The other patch added HAVE(....STYLE_TITLE_0/4) so we should use that here. > Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:127 > +#define WORKAROUND_CORETEXT_VARIATIONS_WITH_FALLBACK_LIST_BUG (ENABLE(VARIATION_FONTS) && (PLATFORM(WATCHOS) || PLATFORM(APPLETV))) I bet the real story is that we don’t need this workaround at all. > Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:159 > +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV) I think this might be HAVE(CT_FONT_DESCRIPTOR_CREATE_LAST_RESORT) > Source/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:60 > auto array = adoptCF(CTFontManagerCreateFontDescriptorsFromData(bufferData.get())); I think this might be HAVE(CT_FONT_MANAGER_CREATE_FONT_DESCRIPTORS_FROM_DATA)
Comment on attachment 388895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388895&action=review >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:127 >> +#define WORKAROUND_CORETEXT_VARIATIONS_WITH_FALLBACK_LIST_BUG (ENABLE(VARIATION_FONTS) && (PLATFORM(WATCHOS) || PLATFORM(APPLETV))) > > I bet the real story is that we don’t need this workaround at all. Probably, I have a hit list of likely incorrect platform differences, and this is on it. If I split this patch further, I wouldn't include this change, but the changes in WebCore were starting to get small, so I thought it would be better to bundle them together. >> Source/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:60 >> auto array = adoptCF(CTFontManagerCreateFontDescriptorsFromData(bufferData.get())); > > I think this might be HAVE(CT_FONT_MANAGER_CREATE_FONT_DESCRIPTORS_FROM_DATA) Differing bit here is actually the fact that watchOS and appleTV won't iterate through itemInCollection, I can't think of a HAVE(...) statement here that wouldn't make things more confusing.
Created attachment 389032 [details] Patch
(In reply to Jonathan Bedard from comment #4) > Created attachment 389032 [details] > Patch I'm not against splitting this patch up further, but the composite patches would start to become one-liners.
Style checker needs to be updated after Platform.h split :(