[webkit-reviews] review granted: [Bug 200694] Remove support for macOS < 10.13 : [Attachment 376271] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Keith Rollin
<krollin at apple.com>'s request for review:
Bug 200694: Remove support for macOS < 10.13
https://bugs.webkit.org/show_bug.cgi?id=200694

Attachment 376271: Patch

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




--- Comment #7 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 376271
  --> https://bugs.webkit.org/attachment.cgi?id=376271
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?