Bug 206096 – Remove old iOS version macros

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

We have a handful of old iOS 11 and 12 macros which should be removed.

[Radar WebKit Bug Importer]

<rdar://problem/58491675>

[Jonathan Bedard]

Created attachment 387391 [details]
Patch

[Tim Horton]

Comment on attachment 387391 [details]
Patch

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

> Source/WTF/wtf/FeatureDefines.h:181
> +#if !PLATFORM(WATCHOS) && !PLATFORM(APPLETV) && !PLATFORM(MACCATALYST)

This can simplify to just PLATFORM(IOS)

> Source/WTF/wtf/Platform.h:1620
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || PLATFORM(IOS) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)

I *think* this is wrong and should be IOS_FAMILY (right now it seemingly-wrongly excludes ONLY macCatalyst)

[Alexey Proskuryakov]

> I *think* this is wrong and should be IOS_FAMILY (right now it seemingly-wrongly excludes ONLY macCatalyst)

That should be a separate patch of course.

[Jonathan Bedard]

Created attachment 387405 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 387405 [details]
Patch

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

> 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_FAMILY)

I don't think that this is a no-op. Isn't __IPHONE_OS_VERSION_MIN_REQUIRED frozen at a lower number on watchOS and tvOS? I think that this is actually "Mac or iOS or macCatalyst".

This change may or may not be desirable, but this patch shouldn't change behavior of course.

Really, all these things should be in Platform.h, not inline. It seems OK to leave those that will go away soon, but those that encode decisions like excluding watchOS and tvOS should be centralized. It is a lot of work, so I don't want to block this patch on it, but it would be a substantial improvement.

> Source/WebCore/PAL/pal/spi/cg/CoreGraphicsSPI.h:285
> -#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000)
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || PLATFORM(IOS_FAMILY)

Ditto.

> Source/WebCore/PAL/pal/spi/ios/MediaPlayerSPI.h:-37
> -#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000

Ditto. I stopped looking at this point, please check the rest of the patch.

[Jonathan Bedard]

Created attachment 387601 [details]
Patch

[Jonathan Bedard]

Created attachment 387659 [details]
Patch

[Aakash Jain]

Comment on attachment 387659 [details]
Patch

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

> Tools/Scripts/webkitpy/port/image_diff.py:61
> +            print('    Comparing {} vs {}'.formt(len(actual_contents), len(expected_contents)))

typo: formt

This breaks layout-tests.

[Jonathan Bedard]

Created attachment 387670 [details]
Patch

[Jonathan Bedard]

(In reply to Aakash Jain from comment #9)
> Comment on attachment 387659 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387659&action=review
> 
> > Tools/Scripts/webkitpy/port/image_diff.py:61
> > +            print('    Comparing {} vs {}'.formt(len(actual_contents), len(expected_contents)))
> 
> typo: formt
> 
> This breaks layout-tests.

Among other things, yes.

That's why I hadn't marked it for review. Far more worried about the building, actually.

[Jonathan Bedard]

Created attachment 387676 [details]
Patch

[Jonathan Bedard]

Created attachment 387965 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #13)
> Created attachment 387965 [details]
> Patch

Needed to rebase on some recent changes.

[Alexey Proskuryakov]

The latest patch fails to build.

[Jonathan Bedard]

Created attachment 388042 [details]
Patch

[Darin Adler]

Comment on attachment 388042 [details]
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.

[Jonathan Bedard]

Comment on attachment 388042 [details]
Patch

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

I'm going to upload a patch with Darin's edits, but I'm not going to mark it for review. I think I can reasonably split this change up so it's a bit easier to review.

>> Source/WTF/wtf/PlatformHave.h:359
>>  #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.

This probably isn't the only place in this patch where we're disabling something on watchOS and tvOS that should be enabled.

I was going to split the changes that modify behavior into a different patch and keep this as a behavior-preserving refactor, but it's becoming increasingly clear that I should split this patch up too.

>> Source/WTF/wtf/PlatformHave.h:522
>>  #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.

I'm putting together a list of this sort of thing to work through once this refactor (or probably more accurately, it's pieces) land so that we can have changes that are simple to roll-out if we get them wrong.

>> Source/WTF/wtf/PlatformHave.h:534
>>  #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.

There is a good chance it's not correct, but yes, this is trying to preserve behavior.

>> Source/WebCore/PAL/pal/spi/cocoa/IOSurfaceSPI.h:108
>>  #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

Adding this to my list of changes after the refactor.

>> Source/WebCore/PAL/pal/spi/cocoa/NSProgressSPI.h:30
>> +#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.

I think the !PLATFORM(WATCHOS) && !PLATFORM(APPLETV) is what we want here.

The fact that like 35 contains a different macro for this same #define, despite watchOS and tvOS not being built in OpenSource, supports this being moved to PlatformUse.h, but that might need to be a stand-alone change.

[Jonathan Bedard]

Created attachment 388451 [details]
Patch

[Jonathan Bedard]

All of the sub-tasks are finished, closing this bug.