[webkit-reviews] review denied: [Bug 206096] Remove old iOS version macros : [Attachment 388042] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jonathan Bedard <jbedard at apple.com>'s
request for review:
Bug 206096: Remove old iOS version macros
https://bugs.webkit.org/show_bug.cgi?id=206096

Attachment 388042: Patch

https://bugs.webkit.org/attachment.cgi?id=388042&action=review




--- Comment #17 from Darin Adler <darin at apple.com> ---
Comment on attachment 388042
  --> https://bugs.webkit.org/attachment.cgi?id=388042
Patch

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

There are multiple places here where this patch got things backwards. Must fix
those or we are changing behavior

Way too much to review all at once. This has to be done in smaller batches,
because it’s so easy to get it wrong and so hard to spot mistakes. And it’s
subtle, because getting this wrong often might just turn off some workaround or
run a less efficient code path. It won’t always lead to a compilation failure
or test failure, and even when it does it might be for a platform not on EWS.

Most of this refactoring ends up being about tvOS and watchOS, or about
limitations of Catalyst. I think we should write these expressions so they
specifically mention these platforms rather than having lines of code that
mention the others in the iOS family. This makes it easier to ask the question
"is it right to have these be exceptions".

The counterargument is that there might be another iOS family member in the
future that is more like watchOS and tvOS than it is like iOS and Catalyst. But
I think a more important trend is all these platform converging to be more
alike as the past differences go away, and so I think it’s best to list "Cocoa
minus the unusual cases" in more places. I said "Cocoa" and not "iOS family"
because I think this applies with Mac too, so we should write things like this:

#if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101400) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

This kind of "listing exceptions to the norm/future" is best for things that
will eventually converge for all the Cocoa platforms, which is the case for a
of these USE/ENABLE/HAVE things.

I ran out of steam after reviewing the first 3/4 of the patch, so didn’t make
the similar comments on the last 1/4.

> Source/WTF/wtf/PlatformEnable.h:211
>  #if !defined(ENABLE_WKPDFVIEW)
> -#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV) && !PLATFORM(MACCATALYST) &&
__IPHONE_OS_VERSION_MIN_REQUIRED >= 120000
> +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV) && !PLATFORM(MACCATALYST)
>  #define ENABLE_WKPDFVIEW 1
>  #endif
>  #endif

How about just:

    #if PLATFORM(IOS)

instead? Like for ENABLE_PREVIEW_CONVERTER below.

As an aside, we have a style problem with this file. I don’t understand the use
of nested #if, if we are using these blocks sometimes to set things to 0, other
times leave them alone if the value should be 0. Either it’s this file’s job to
set the value always to 0 or 1, or it’s this file’s job to only set things that
need to be 1. If we’re supposed to set things to 0 or 1, then we need an #else
on any nested #if. For example, for watchOS on this. But if we don’t then we
should just write a single #if like this:

#if !defined(ENABLE_WKPDFVIEW) && PLATFORM(IOS)
#define ENABLE_WKPDFVIEW 1
#endif

And we should delete all the ones that define a value of "0" or replace them
with comments.

> Source/WTF/wtf/PlatformEnable.h:1037
> -#if !defined(ENABLE_MONOSPACE_FONT_EXCEPTION) && ((PLATFORM(MAC) &&
__MAC_OS_X_VERSION_MIN_REQUIRED < 101500) || (PLATFORM(IOS_FAMILY) &&
__IPHONE_OS_VERSION_MIN_REQUIRED < 130000))
> +#if !defined(ENABLE_MONOSPACE_FONT_EXCEPTION) && ((PLATFORM(MAC) &&
__MAC_OS_X_VERSION_MIN_REQUIRED < 101500) || PLATFORM(IOS) ||
PLATFORM(MACCATALYST))

This change is backwards. The old code was false for iOS 13. The new code is
true for iOS 13. A correct behavior-preserving refactoring would be:

#if !defined(ENABLE_MONOSPACE_FONT_EXCEPTION) && ((PLATFORM(MAC) &&
__MAC_OS_X_VERSION_MIN_REQUIRED < 101500) || PLATFORM(WATCHOS) ||
PLATFORM(APPLETV))

On the other hand, I think it’s likely we don’t need this on the latest watchOS
and tvOS. It would be good for someone to look into that, and remove those.

> Source/WTF/wtf/PlatformHave.h:359
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
PLATFORM(IOS) || PLATFORM(MACCATALYST)
>  #define HAVE_HSTS_STORAGE_PATH 1

This seems to be a behavior-preserving refactoring. But I would prefer it say
this:

    #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
(PLATFORM(IOS_FAMILY) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV))

or this:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101500) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

I suspect this is incorrect for recent watchOS and tvOS; mentioning them
explicitly could help us spot the mistake. But also I would like to come up
with a good way to keep track of this as an outstanding item: checking whether
this needs to be turned on for watchOS and tvOS.

> Source/WTF/wtf/PlatformHave.h:446
> +#if (PLATFORM(MAC) && (__MAC_OS_X_VERSION_MIN_REQUIRED >= 101400 &&
__MAC_OS_X_VERSION_MAX_ALLOWED >= 101404)) || PLATFORM(WATCHOS) ||
PLATFORM(APPLETV)
>  #define HAVE_CFNETWORK_OVERRIDE_SESSION_COOKIE_ACCEPT_POLICY 1

This change is backwards. The old code was true for iOS 13. The new code is
false for iOS 13. A correct behavior-preserving refactoring would be:

#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400 &&
__MAC_OS_X_VERSION_MAX_ALLOWED >= 101404) || PLATFORM(IOS_FAMILY)

or:

#if PLATFORM(COCOA) && !(PLATFORM(MAC) && !(__MAC_OS_X_VERSION_MIN_REQUIRED >=
101400 && __MAC_OS_X_VERSION_MAX_ALLOWED >= 101404))

> Source/WTF/wtf/PlatformHave.h:450
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
PLATFORM(IOS) || PLATFORM(MACCATALYST)
>  #define HAVE_CFNETWORK_NSURLSESSION_STRICTRUSTEVALUATE 1

This seems to be a behavior-preserving refactoring. But I would prefer it say:

    #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
(PLATFORM(IOS_FAMILY) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV))

or:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101500) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

I suspect this is incorrect for recent watchOS and tvOS, and mentioning them
explicitly could help us spot the mistake. But also I would like to come up
with a good way to keep track of this as an outstanding item: checking whether
this needs to be turned on for watchOS and tvOS.

> Source/WTF/wtf/PlatformHave.h:474
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
PLATFORM(IOS) || PLATFORM(MACCATALYST)
>  #define HAVE_MDNS_FAST_REGISTRATION 1

This seems to be a behavior-preserving refactoring. But I would prefer it say:

    #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
(PLATFORM(IOS_FAMILY) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV))

or:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101500) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

I suspect this is incorrect for recent watchOS and tvOS, and mentioning them
explicitly could help us spot the mistake. But also I would like to come up
with a good way to keep track of this as an outstanding item: checking whether
this needs to be turned on for watchOS and tvOS.

> Source/WTF/wtf/PlatformHave.h:482
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
PLATFORM(IOS) || PLATFORM(MACCATALYST)
>  #define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION 1

This seems to be a behavior-preserving refactoring. But I would prefer it say:

    #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
(PLATFORM(IOS_FAMILY) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV))

or:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101500) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

This will likely eventually be incorrect for recent watchOS and tvOS, and
mentioning them explicitly could help us spot the mistake. But also I would
like to come up with a good way to keep track of this as an outstanding item:
checking whether this needs to be turned on for watchOS and tvOS.

> Source/WTF/wtf/PlatformHave.h:486
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
PLATFORM(IOS) || PLATFORM(MACCATALYST)
>  #define HAVE_CTFONTTRANSFORMGLYPHSWITHLANGUAGE 1

This seems to be a behavior-preserving refactoring. But I would prefer it say:

    #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
(PLATFORM(IOS_FAMILY) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV))

or:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101500) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

This will likely eventually be incorrect for recent watchOS and tvOS, and
mentioning them explicitly could help us spot the mistake. But also I would
like to come up with a good way to keep track of this as an outstanding item:
checking whether this needs to be turned on for watchOS and tvOS.

> Source/WTF/wtf/PlatformHave.h:514
> +#if PLATFORM(IOS) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
>  #define HAVE_UI_SCROLL_VIEW_INDICATOR_FLASHING_SPI 1

This seems to be a behavior-preserving refactoring. But is it correct that we
don’t have this in Catalyst? To emphasize this I think we should instead write:

    #if PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST)

And figure out a way to get someone to follow up and figure out if this is
correct.

> Source/WTF/wtf/PlatformHave.h:518
> +#if PLATFORM(IOS) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
>  #define HAVE_APP_LINKS_WITH_ISENABLED 1

This seems to be a behavior-preserving refactoring. But is it correct that we
don’t have this in Catalyst? To emphasize this I think we should instead write:

    #if PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST)

And figure out a way to get someone to follow up and figure out if this is
correct.

> Source/WTF/wtf/PlatformHave.h:522
> +#if PLATFORM(IOS)
>  #define HAVE_ROUTE_SHARING_POLICY_LONG_FORM_VIDEO 1

This seems to be a behavior-preserving refactoring.

But it’s factually incorrect. We have this on recent versions of watchOS and
tvOS. Not sure about Catalyst. I think we probably have this on all versions of
all the iOS family that we currently support. Most likely, the correct change
after a bit of research is to remove this entirely and make the code guarded by
it unconditional.

> Source/WTF/wtf/PlatformHave.h:529
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
PLATFORM(IOS_FAMILY)

A better way to write this might be:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101500)

> Source/WTF/wtf/PlatformHave.h:534
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
PLATFORM(IOS) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
>  #define HAVE_AVPLAYER_RESOURCE_CONSERVATION_LEVEL 1

This seems to be a behavior-preserving refactoring. But is it correct that we
don’t have this in Catalyst? To emphasize this I think we should instead write:

    #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
(PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST))

or:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101500) && !PLATFORM(MACCATALYST)

And figure out a way to get someone to follow up and figure out if this is
correct.

> Source/WTF/wtf/PlatformHave.h:538
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) ||
PLATFORM(IOS) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
>  #define HAVE_CORETEXT_AUTO_OPTICAL_SIZING 1

This seems to be a behavior-preserving refactoring. But is it correct that we
don’t have this in Catalyst? To emphasize this I think we should instead write:

    #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
(PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST))

or:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101500) && !PLATFORM(MACCATALYST))

And figure out a way to get someone to follow up and figure out if this is
correct.

> Source/WTF/wtf/PlatformHave.h:541
> -#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) ||
(PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130000)
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) ||
PLATFORM(IOS)

This change is backwards. The old code was false for iOS 13. The new code is
true for iOS 13. A correct behavior-preserving refactoring would be to not
mention anything in the iOS family:

#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500

> Source/WTF/wtf/PlatformHave.h:549
> +#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >=
101500) || PLATFORM(WATCHOS) || PLATFORM(APPLETV) || PLATFORM(MACCATALYST)

This seems to be a behavior-preserving refactoring. But I suggest we use
IOS_FAMILY:

    #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
PLATFORM(IOS_FAMILY)

or:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101500)

> Source/WTF/wtf/PlatformHave.h:570
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
PLATFORM(IOS) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
>  #define HAVE_DESIGN_SYSTEM_UI_FONTS 1

This seems to be a behavior-preserving refactoring. But is it correct that we
don’t have this in Catalyst? To emphasize this I think we should instead write
something more like this:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101500) && !PLATFORM(MACCATALYST)

And also figure out a way to get someone to follow up and figure out if this is
correct.

> Source/WTF/wtf/PlatformUse.h:323
> +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
>  #define USE_PLATFORM_SYSTEM_FALLBACK_LIST 1

Could say PLATFORM(COCOA) instead of MAC || IOS_FAMILY.

Note also that many (most) of the uses of this are in Cocoa-specific source
files, so we can/should get rid of any uses of it from those places.

> Source/WebCore/PAL/pal/cocoa/AVFoundationSoftLink.mm:34
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
PLATFORM(IOS) || PLATFORM(MACCATALYST)
>  SOFT_LINK_FRAMEWORK_FOR_SOURCE_WITH_EXPORT(PAL, AVFoundation, PAL_EXPORT)

This seems to be a behavior-preserving refactoring. But is it what we want for
tvOS and watchOS? We should consider writing it this way:

    #if !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) &&
!PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:216
> -#if PLATFORM(MAC) || (PLATFORM(IOS_FAMILY) &&
__IPHONE_OS_VERSION_MIN_REQUIRED >= 110000)
> +#if PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(MACCATALYST)

This seems to be a behavior-preserving refactoring. But I think it is wrong for
recent tvOS and watchOS. Not sure. And I would write it in a way that
emphasizes these exceptions instead of listing everything normal:

    #if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

> Source/WebCore/PAL/pal/spi/cocoa/IOSurfaceSPI.h:108
> -#if __IPHONE_OS_VERSION_MIN_REQUIRED < 110000
> +#if PLATFORM(WATCHOS) || PLATFORM(APPLETV)
>  typedef uint32_t IOSurfaceID;
>  #endif

This is a behavior-preserving refactoring.

But code is definitely unneeded on recent tvOS and watchOS. It’s just harmless
on any platform since it’s a typedef that exactly matches the header, and
that’s always allowed and harmless. So it would be OK for this #if to say
anything, 0, 1, any random set of platforms.

Better to remove this entirely

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:32
> +#define USE_SECURE_ARCHIVER_API (PLATFORM(MAC) || PLATFORM(IOS_FAMILY))

This is a behavior-preserving refactoring.

But this should be PLATFORM(COCOA) and maybe can be removed entirely since this
is a Cocoa-specific header. If we were keeping it, we should move it to
PlatformUse.h.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:34
> +#define USE_SECURE_ARCHIVER_FOR_ATTRIBUTED_STRING (PLATFORM(MAC) ||
PLATFORM(IOS_FAMILY))

This is a behavior-preserving refactoring.

But this should be PLATFORM(COCOA) and maybe can be removed entirely since this
is a Cocoa-specific header. If we were keeping it, we should move it to
PlatformUse.h.

> Source/WebCore/PAL/pal/spi/cocoa/NSProgressSPI.h:30
> -#define USE_NSPROGRESS_PUBLISHING_SPI PLATFORM(IOS_FAMILY) &&
__IPHONE_OS_VERSION_MIN_REQUIRED < 130000
> +#define USE_NSPROGRESS_PUBLISHING_SPI PLATFORM(IOS_FAMILY) &&
(PLATFORM(WATCHOS) || PLATFORM(APPLETV))

This is incorrect. The old version was false for watchOS and tvOS and the new
version is true.

The way this was written elsewhere in this patch was PLATFORM(IOS) ||
PLATFORM(MACCATALYST)

Another way to write it is PLATFORM(IOS_FAMILY) && !PLATFORM(WATCHOS) &&
!PLATFORM(APPLETV)

But not sure why we'd choose something different here. Also, this belongs in
PlatformUse.h, not in NSProgressSPI.h.

> Source/WebCore/PAL/pal/spi/ios/MediaPlayerSPI.h:38
> -#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000
> +#if PLATFORM(IOS) || PLATFORM(MACCATALYST)
>  #import <MediaPlayer/MPMediaControlsConfiguration.h>

This is a behavior-preserving refactoring. But it should be written this way
like the ones below:

#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

But it’s not clear that this is correct for watchOS and tvOS.

> Source/WebCore/PAL/pal/spi/ios/MediaPlayerSPI.h:41
> -#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000 && !PLATFORM(WATCHOS) &&
!PLATFORM(APPLETV)
> +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

This is a behavior-preserving refactoring.

But it’s not clear that this is correct for watchOS and tvOS. And it’s also
strange that we express the same thing two lines up with a different
expression.

> Source/WebCore/PAL/pal/spi/ios/MediaPlayerSPI.h:67
> -#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000 && !PLATFORM(WATCHOS) &&
!PLATFORM(APPLETV)
> +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

This is a behavior-preserving refactoring.

But it’s not clear that this is correct for watchOS and tvOS. And it’s also
strange that we express the same thing two lines up with a different
expression.

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:76
> -#if (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000) ||
PLATFORM(MAC)
> +#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(MACCATALYST)

This is a behavior-preserving refactoring. But given where it is, another
better way to write it is:

    #if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:99
> -#elif (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000)
|| PLATFORM(MAC)
> +#elif PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(MACCATALYST)

This is a behavior-preserving refactoring, but seems inelegant. The #if above
handles PLATFORM(MACCATALYST) so we definitely don’t want or need to mention it
again. Given this is in a Cocoa-specific file, I suggest we write it like this:

    #elif !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

> Source/WebCore/page/SettingsDefaultValues.h:107
> +#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >=
101400) || PLATFORM(WATCHOS) || PLATFORM(MACCATALYST)

This is a behavior-preserving refactoring. But I think it would be better as:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101400) && !PLATFORM(APPLETV)

And then someone should check why we have this tvOS exception. And maybe this
should be a USE instead of something written out here. Not really clear on why
this defaults false on non-Cocoa platforms either.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:185
> -#if (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000) ||
(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) ||
PLATFORM(WATCHOS)
> +#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >=
101400) || PLATFORM(WATCHOS)

This is a behavior-preserving refactoring. Seems clearly related to the one
above and should also be:

    #if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <
101400) && !PLATFORM(APPLETV)

But then I think this needs to be a USE or HAVE instead. The fact that it shows
up separately in SettingsDefaultValues.h and GradientCG.cpp seems especially
problematic. One is a runtime thing and the other a compile-time thing.
Peculiar.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:317
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) ||
PLATFORM(IOS) || PLATFORM(MACCATALYST)

This is a behavior-preserving refactoring and it’s already inside an #if
PLATFORM(COCOA). I think we should write it more like this:

    #if !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101400) &&
!PLATFORM(APPLETV)

And check if the tvOS one is correct. But it should ideally be
HAVE(CGPATHADDUNEVENCORNERSROUNDEDRECT) rather than being written out here.
Although the "all caps" version of that reads horribly!

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:42
> -#define HAS_CORE_TEXT_WIDTH_ATTRIBUTE (PLATFORM(MAC) ||
(PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000))
> +#define HAS_CORE_TEXT_WIDTH_ATTRIBUTE (PLATFORM(MAC) || PLATFORM(IOS) ||
PLATFORM(MACCATALYST))

This is a behavior-preserving refactoring. It’s in a Cocoa-specific file so it
should be:

    #define HAS_CORE_TEXT_WIDTH_ATTRIBUTE (!PLATFORM(WATCHOS) &&
!PLATFORM(APPLETV))

And as with all the others, I suspect this is incorrect. And it should be a
HAVE in PlatformHave.h, not here.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1067
> -#if (PLATFORM(MAC) || (PLATFORM(IOS_FAMILY) &&
__IPHONE_OS_VERSION_MIN_REQUIRED >= 110000))
> +#if (PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(MACCATALYST))

Same thing. Should be a HAVE or a USE macro and until then should be:

    #if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1642
> -#if (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000) ||
PLATFORM(MAC)
> +#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(MACCATALYST)

Ditto.

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:127
> -#define WORKAROUND_CORETEXT_VARIATIONS_WITH_FALLBACK_LIST_BUG
(ENABLE(VARIATION_FONTS) && (PLATFORM(IOS_FAMILY) &&
__IPHONE_OS_VERSION_MIN_REQUIRED < 110000))
> +#define WORKAROUND_CORETEXT_VARIATIONS_WITH_FALLBACK_LIST_BUG
(ENABLE(VARIATION_FONTS) && (PLATFORM(IOS) || PLATFORM(MACCATALYST)))

This is backwards. This was false for iOS 13 and the new code is true for iOS
13. I am pretty sure this should just be unconditionally true. But to correctly
refactor it for now it could be:

    #define WORKAROUND_CORETEXT_VARIATIONS_WITH_FALLBACK_LIST_BUG
(ENABLE(VARIATION_FONTS) && (PLATFORM(WATCHOS) || PLATFORM(APPLETV)))

I suspect this isn’t needed at all any more.