Bug 200694 – Remove support for macOS < 10.13

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Keith Rollin]

Update conditionals that reference __MAC_OS_X_VERSION_MIN_REQUIRED and __MAC_OS_X_VERSION_MAX_ALLOWED, assuming that they both have values >= 101300. This means that expressions like "__MAC_OS_X_VERSION_MIN_REQUIRED < 101300" are always False and "__MAC_OS_X_VERSION_MIN_REQUIRED >= 101300" are always True.

[Radar WebKit Bug Importer]

<rdar://problem/54278851>

[Keith Rollin]

Other platforms will be handled with subsequent patches.

[Keith Rollin]

Bug 200706 will take care of one of these conditionals, so my patch is leaving it alone.

[Keith Rollin]

Created attachment 376271 [details]
Patch

[EWS Watchlist]

Attachment 376271 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:76:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:86:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:99:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/cocoa/AVFoundationSoftLinkTest.mm:141:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:217:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:222:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/IOSurfaceSPI.h:95:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:42:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:974:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1073:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1651:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/CommonCryptoSPI.h:28:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/cocoa/NetworkExtensionContentFilter.mm:78:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:32:  Incorrect OS version check. VERSION_MIN_REQUIRED values never include a minor version. You may be looking for a combination of VERSION_MIN_REQUIRED for target OS version check and VERSION_MAX_ALLOWED for SDK check.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:32:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:34:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:56:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: PerformanceTests/StitchMarker/wtf/Platform.h:1259:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:105:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:422:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:44:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/NEFilterSourceSPI.h:72:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:59:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:685:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:701:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:128:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1316:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1325:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 28 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

All of those style errors are pre-existing. Some will be cleaned up in subsequent patches that remove support for older OSes on other platforms.

[youenn fablet]

Comment on attachment 376271 [details]
Patch

Nice to see these simplifications.
Is the plan to continue with iOS as well?

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

> Source/WebCore/PAL/pal/spi/cocoa/CommonCryptoSPI.h:29
>  #define HAVE_CCRSAGetCRTComponents 1

It seems that HAVE_CCRSAGetCRTComponents will always be 1 in all our builds.
Should we do a follow-up to remove it?
Might apply to other defined values.

> Source/WebCore/PAL/pal/spi/mac/AVFoundationSPI.h:57
> +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)

AVFoundationSPI.h is probably COCOA specific so we could remove this #if, here or as a follow-up.

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:44
> +#if (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)

Can we remove that code and below as well?

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

Possibility to simplify the code here

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:127
>  // FIXME: Remove this when <rdar://problem/28449441> is fixed.
> -#define WORKAROUND_CORETEXT_VARIATIONS_WITH_FALLBACK_LIST_BUG (ENABLE(VARIATION_FONTS) && ((PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)))
> +// NOTE: That radar was fixed 11/16/16, so this can be removed. TBD.

I would update the FIXME to state the radar is fixed.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:56
> +    return WebCore::MacApplication::isSafari() || dyld_get_program_sdk_version() > DYLD_MACOSX_VERSION_10_13;

Do we have a case where dyld_get_program_sdk_version() can return below DYLD_MACOSX_VERSION_10_13?

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:-493
> -#endif

Shouldn't it be @NO for iOS?

> Tools/TestWebKitAPI/Tests/WebCore/FontCache.cpp:26
>  #include "config.h"

Should we remove that file entirely?

> PerformanceTests/StitchMarker/wtf/Platform.h:669
>  #define HAVE_CFNETWORK_STORAGE_PARTITIONING 1

Is it used somewhere?

[Keith Rollin]

(In reply to youenn fablet from comment #7)
> Comment on attachment 376271 [details]
> Patch
> 
> Nice to see these simplifications.
> Is the plan to continue with iOS as well?

Yes. That's what I was trying to say with my comment "Other platforms will be handled with subsequent patches."

I'll file bugs for all the places you suggest follow-ups.



> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376271&action=review
> 
> > Source/WebCore/PAL/pal/spi/cocoa/CommonCryptoSPI.h:29
> >  #define HAVE_CCRSAGetCRTComponents 1
> 
> It seems that HAVE_CCRSAGetCRTComponents will always be 1 in all our builds.
> Should we do a follow-up to remove it?
> Might apply to other defined values.

Yes, these can and should be examined in follow-ups.

> > Source/WebCore/PAL/pal/spi/mac/AVFoundationSPI.h:57
> > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
> 
> AVFoundationSPI.h is probably COCOA specific so we could remove this #if,
> here or as a follow-up.

Yes, one of the follow-ups will be to examine this pattern and see if it can be converted it #if COCOA.

> > Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:44
> > +#if (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)
> 
> Can we remove that code and below as well?
> 
> > 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))
> 
> Possibility to simplify the code here

Yes, the IOS_FAMILY check will be simplified in an IOS-specific update, and then the whole expression could be simplified to just #if COCOA in the follow-up described above.

> > Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:56
> > +    return WebCore::MacApplication::isSafari() || dyld_get_program_sdk_version() > DYLD_MACOSX_VERSION_10_13;
> 
> Do we have a case where dyld_get_program_sdk_version() can return below
> DYLD_MACOSX_VERSION_10_13?
> 
> > Source/WebKitLegacy/mac/WebView/WebPreferences.mm:-493
> > -#endif
> 
> Shouldn't it be @NO for iOS?

My intent with this patch is to maintain the status quo. We can examine the above two cases in a follow-up.

> > Tools/TestWebKitAPI/Tests/WebCore/FontCache.cpp:26
> >  #include "config.h"
> 
> Should we remove that file entirely?

Follow-up.

> > PerformanceTests/StitchMarker/wtf/Platform.h:669
> >  #define HAVE_CFNETWORK_STORAGE_PARTITIONING 1
> 
> Is it used somewhere?

Follow-up.

[Keith Rollin]

> > Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:56
> > +    return WebCore::MacApplication::isSafari() || dyld_get_program_sdk_version() > DYLD_MACOSX_VERSION_10_13;
> 
> Do we have a case where dyld_get_program_sdk_version() can return below
> DYLD_MACOSX_VERSION_10_13?
> 
> > Source/WebKitLegacy/mac/WebView/WebPreferences.mm:-493
> > -#endif
> 
> Shouldn't it be @NO for iOS?

Ah, you're right. I got over aggressive here. I'll fix.

[Keith Rollin]

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:56
>> +    return WebCore::MacApplication::isSafari() || dyld_get_program_sdk_version() > DYLD_MACOSX_VERSION_10_13;
>
>
> Do we have a case where dyld_get_program_sdk_version() can return below DYLD_MACOSX_VERSION_10_13?

I think you're right. I'll remove that clause.

[Keith Rollin]

(In reply to Keith Rollin from comment #10)
> >> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:56
> >> +    return WebCore::MacApplication::isSafari() || dyld_get_program_sdk_version() > DYLD_MACOSX_VERSION_10_13;
> >
> >
> > Do we have a case where dyld_get_program_sdk_version() can return below DYLD_MACOSX_VERSION_10_13?
> 
> I think you're right. I'll remove that clause.

Actually, while it can't return below DYLD_MACOSX_VERSION_10_13, it can return exactly DYLD_MACOSX_VERSION_10_13. Since the test is for > DYLD_MACOSX_VERSION_10_13 and not >= DYLD_MACOSX_VERSION_10_13, then it can be false sometimes and true sometimes, so I think we still need it.

[Keith Rollin]

(In reply to Keith Rollin from comment #9)

> > > Source/WebKitLegacy/mac/WebView/WebPreferences.mm:-493
> > > -#endif
> > 
> > Shouldn't it be @NO for iOS?
> 
> Ah, you're right. I got over aggressive here. I'll fix.


Actually, this code is in a file in a "mac" directory", so I don't know if iOS is involved. But this file is probably included in macCatalyst builds. Regardless, the safe thing is to just remove the OS version test and keep the rest is it was, so I'll revert the relevant parts.

[Keith Rollin]

Created attachment 376293 [details]
Patch

[Keith Rollin]

Created attachment 376294 [details]
Patch

[EWS Watchlist]

Attachment 376294 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:76:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:86:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:99:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/cocoa/AVFoundationSoftLinkTest.mm:141:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:217:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:222:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/IOSurfaceSPI.h:95:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:42:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:974:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1073:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1651:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/CommonCryptoSPI.h:28:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/cocoa/NetworkExtensionContentFilter.mm:78:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:32:  Incorrect OS version check. VERSION_MIN_REQUIRED values never include a minor version. You may be looking for a combination of VERSION_MIN_REQUIRED for target OS version check and VERSION_MAX_ALLOWED for SDK check.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:32:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:34:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:56:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: PerformanceTests/StitchMarker/wtf/Platform.h:1259:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:105:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:422:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:44:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/PAL/pal/spi/cocoa/NEFilterSourceSPI.h:72:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:59:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:685:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:701:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:127:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1316:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1325:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 28 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 376294 [details]
Patch

Clearing flags on attachment: 376294

Committed r248697: <https://trac.webkit.org/changeset/248697>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

(In reply to Keith Rollin from comment #11)
> (In reply to Keith Rollin from comment #10)
> > >> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:56
> > >> +    return WebCore::MacApplication::isSafari() || dyld_get_program_sdk_version() > DYLD_MACOSX_VERSION_10_13;
> > >
> > >
> > > Do we have a case where dyld_get_program_sdk_version() can return below DYLD_MACOSX_VERSION_10_13?
> > 
> > I think you're right. I'll remove that clause.
> 
> Actually, while it can't return below DYLD_MACOSX_VERSION_10_13, it can
> return exactly DYLD_MACOSX_VERSION_10_13. Since the test is for >
> DYLD_MACOSX_VERSION_10_13 and not >= DYLD_MACOSX_VERSION_10_13, then it can
> be false sometimes and true sometimes, so I think we still need it.

I must not understand. Surely an app built targeting < 10.13 can still run on current software? And that's the whole point of a SDK check -- to make sure old software gets the old behavior. So I'm glad you left it, but your reason is insufficient, and you should be sure not to remove it in the future. (unless I'm missing something)

[Tim Horton]

(In reply to Keith Rollin from comment #12)
> (In reply to Keith Rollin from comment #9)
> 
> > > > Source/WebKitLegacy/mac/WebView/WebPreferences.mm:-493
> > > > -#endif
> > > 
> > > Shouldn't it be @NO for iOS?
> > 
> > Ah, you're right. I got over aggressive here. I'll fix.
> 
> 
> Actually, this code is in a file in a "mac" directory", so I don't know if
> iOS is involved. But this file is probably included in macCatalyst builds.
> Regardless, the safe thing is to just remove the OS version test and keep
> the rest is it was, so I'll revert the relevant parts.

FWIW you can NOT trust directory names. WebPreferences is absolutely a part of iOS WebKit.

[Darin Adler]

Comment on attachment 376294 [details]
Patch

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

Good to do this housekeeping. I see opportunities to clean up a bit more so I commented on the ones I spotted.

> Source/WTF/wtf/FeatureDefines.h:-302
>  #if !defined(HAVE_PASSKIT_GRANULAR_ERRORS)
> -#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
>  #define HAVE_PASSKIT_GRANULAR_ERRORS 1
>  #endif
> -#endif

Additional cleanup we can do to follow this up: Remove this entirely. HAVE(PASSKIT_GRANULAR_ERRORS) is now always true.

> Source/WTF/wtf/FeatureDefines.h:-308
>  #if !defined(HAVE_PASSKIT_API_TYPE)
> -#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300 && __MAC_OS_X_VERSION_MAX_ALLOWED >= 101304
>  #define HAVE_PASSKIT_API_TYPE 1
>  #endif
> -#endif

Additional cleanup we can do to follow this up: Remove this entirely. HAVE(PASSKIT_API_TYPE) is now always true.

> Source/WTF/wtf/Platform.h:1428
> +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)

PLATFORM(MAC) || PLATFORM(IOS_FAMILY) can also be written as PLATFORM(COCOA)

> Source/WTF/wtf/Platform.h:1439
> -#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MAX_ALLOWED >= 101302 && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || PLATFORM(IOS_FAMILY)
> +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
>  #define HAVE_CFNETWORK_WITH_IGNORE_HSTS 1
>  #endif

Additional cleanup we can do to follow this up: Remove this entirely. HAVE(CFNETWORK_WITH_IGNORE_HSTS) is now always true everywhere it is checked (the source files are Objective-C so only used on these platforms).

> Source/WTF/wtf/Platform.h:1442
>  #define HAVE_CFNETWORK_WITH_AUTO_ADDED_HTTP_HEADER_SUPPRESSION_SUPPORT 1

Additional cleanup we can do to follow this up: Remove this entirely. HAVE(CFNETWORK_WITH_AUTO_ADDED_HTTP_HEADER_SUPPRESSION_SUPPORT) is only checked in one place and it’s in an Objective-C part of a header.

> Source/WTF/wtf/Platform.h:1483
> +#if (PLATFORM(MAC) && !defined(__i386__)) || PLATFORM(IOS) || PLATFORM(WATCHOS)

I’m pretty sure WebKit doesn’t support 32-bit Mac any more, so this can be just PLATFORM(MAC) rather than (PLATFORM(MAC) && !defined(__i386__)). Might want to clean up more cases of that if we can find them.

> Source/WTF/wtf/mac/AppKitCompatibilityDeclarations.h:-46
> -#if __MAC_OS_X_VERSION_MAX_ALLOWED < 101300
> -#import <AppKit/AppKit.h>
> -#endif
> -
> -#if __MAC_OS_X_VERSION_MAX_ALLOWED < 101300
> -
> -typedef NSInteger NSControlStateValue;
> -static const NSControlStateValue NSControlStateValueMixed = NSMixedState;
> -static const NSControlStateValue NSControlStateValueOff = NSOffState;
> -static const NSControlStateValue NSControlStateValueOn = NSOnState;
> -
> -static const NSLevelIndicatorStyle NSLevelIndicatorStyleRelevancy = NSRelevancyLevelIndicatorStyle;
> -static const NSLevelIndicatorStyle NSLevelIndicatorStyleContinuousCapacity = NSContinuousCapacityLevelIndicatorStyle;
> -static const NSLevelIndicatorStyle NSLevelIndicatorStyleDiscreteCapacity = NSDiscreteCapacityLevelIndicatorStyle;
> -static const NSLevelIndicatorStyle NSLevelIndicatorStyleRating = NSRatingLevelIndicatorStyle;
> -
> -#endif // __MAC_OS_X_VERSION_MAX_ALLOWED < 101300

Additional cleanup we can do to follow this up: Remove this header entirely.

> Source/WebCore/Modules/applepay/cocoa/PaymentContactCocoa.mm:45
> +#if PLATFORM(IOS_FAMILY) || PLATFORM(MAC)

We can remove this #if entirely. That conditional covers all the Cocoa platforms and thus all the platforms that compile this source file. And we can also remove this function entirely.

> Source/WebCore/Modules/applepay/cocoa/PaymentContactCocoa.mm:55
> +#if PLATFORM(IOS_FAMILY) || PLATFORM(MAC)

Ditto.

> Source/WebCore/Modules/applepay/cocoa/PaymentContactCocoa.mm:65
> +#if PLATFORM(IOS_FAMILY) || PLATFORM(MAC)

Ditto.

> Source/WebCore/Modules/applepay/cocoa/PaymentContactCocoa.mm:75
> +#if PLATFORM(IOS_FAMILY) || PLATFORM(MAC)

Ditto.

> Source/WebCore/PAL/pal/spi/mac/AVFoundationSPI.h:57
> +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)

We can remove this conditional entirely, since Mac plus IOS_FAMILY cover the entire set of Cocoa platforms.

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:127
> +// FIXME: Remove this when <rdar://problem/28449441> is fixed. NOTE: That radar was fixed 11/16/16, so this can be removed. TBD.
> +#define WORKAROUND_CORETEXT_VARIATIONS_WITH_FALLBACK_LIST_BUG (ENABLE(VARIATION_FONTS) && (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000))

We don’t need the FIXME. Like all other version conditionals, this should be removed when it’s no longer needed based on the iOS version check; doesn’t seem to require a comment.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:488
> +#if PLATFORM(MAC)

As you discussed with Tim, this file in the "mac" directory is not Mac-specific, but Cocoa-specific. It’s OK to some day move it to a directory named "cocoa" instead.

> Source/WebKitLegacy/mac/WebView/WebView.mm:9195
>  #endif // PLATFORM(MAC)

Commend on the "#endif" should probably be removed when the body is just one line.

> Source/WebKitLegacy/mac/WebView/WebView.mm:9859
>      if (!_private->mediaTouchBarProvider) {
> -#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
>          _private->mediaTouchBarProvider = adoptNS([allocAVTouchBarPlaybackControlsProviderInstance() init]);
> -#else
> -        _private->mediaTouchBarProvider = adoptNS([allocAVFunctionBarPlaybackControlsProviderInstance() init]);
> -#endif // __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
>      }

WebKit coding style says we should remove the braces now.

> Tools/MiniBrowser/AppKitCompatibilityDeclarations.h:-36
> -#if __MAC_OS_X_VERSION_MAX_ALLOWED < 101300
> -
> -#import <AppKit/AppKit.h>
> -
> -typedef NSInteger NSControlStateValue;
> -static const NSControlStateValue NSControlStateValueOff = NSOffState;
> -static const NSControlStateValue NSControlStateValueOn = NSOnState;
> -
> -#endif

Additional cleanup we can do to follow this up: Remove this header entirely.

> Tools/TestWebKitAPI/Tests/WebCore/FontCache.cpp:-43
> -class FontCacheTest : public testing::Test {

I don’t understand what was going on here, and there is no change log comment that makes it clear. If this test is needed but was never updated for newer versions of macOS, maybe it should be rather than deleting it. If the test is not needed, then we can follow up by removing the file entirely.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ExitFullscreenOnEnterPiP.mm:28
> +#if PLATFORM(MAC) || (PLATFORM(IOS_FAMILY) && !PLATFORM(IOS_FAMILY_SIMULATOR))

This can just be #if !PLATFORM(IOS_FAMILY_SIMULATOR)

> PerformanceTests/ChangeLog:15
> +        * StitchMarker/wtf/Platform.h:

I don’t understand if or why we should "maintain" the code in the StitchMarker directory. Should find someone who knows. I normally just ignore this code, even though I am irritated when I see it on all my global searches.