Bug 231085 – Drop support for macOS < 10.15

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Drop support for macOS < 10.15.

[Chris Dumez]

Created attachment 439871 [details]
Patch

[Chris Dumez]

Created attachment 439872 [details]
Patch

[Darin Adler]

Comment on attachment 439871 [details]
Patch

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

Many of these HAVE() macros might be in code that is already Cocoa-specific, so could drop them entirely. Others are close to that.

> Source/JavaScriptCore/API/JSValue.h:-556
> -#if (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130000)
> -typedef NSString *JSValueProperty;
> -#else
>  typedef id JSValueProperty;
> -#endif

Are there different considerations for this public header that is in the SDK? Do we need this header to stay compatible with targeting older versions even if the WebKit code doesn’t?

> Source/WTF/wtf/PlatformEnableCocoa.h:177
>  // FIXME: Seems likely this can be enabled for tvOS.
> -#if !defined(ENABLE_CSS_CONIC_GRADIENTS) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101400) && !PLATFORM(APPLETV)
> +#if !defined(ENABLE_CSS_CONIC_GRADIENTS) && !PLATFORM(APPLETV)
>  #define ENABLE_CSS_CONIC_GRADIENTS 1
>  #endif

Pretty sure we don’t need this at all, and the tvOS thing here was just a mistake. Doesn’t need to be fixed in this patch, but may want to check this out and remove it entirely.

> Source/WTF/wtf/PlatformEnableCocoa.h:231
> +#if !defined(ENABLE_ENCRYPTED_MEDIA) && && !PLATFORM(MACCATALYST)

Oops: "&& &&"

> Source/WTF/wtf/PlatformEnableCocoa.h:407
>  // FIXME: We almost certainly do not need this monospace font exception for watchOS and tvOS.
> -#if !defined(ENABLE_MONOSPACE_FONT_EXCEPTION) && ((PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) || PLATFORM(WATCHOS) || PLATFORM(APPLETV))
> +#if !defined(ENABLE_MONOSPACE_FONT_EXCEPTION) && (PLATFORM(WATCHOS) || PLATFORM(APPLETV))
>  #define ENABLE_MONOSPACE_FONT_EXCEPTION 1
>  #endif

Pretty sure we don’t need this at all, and the watchOS/tvOS thing here was just a mistake. Doesn’t need to be fixed in this patch, but may want to check with Myles.

> Source/WTF/wtf/PlatformHave.h:253
> +#if PLATFORM(IOS) || PLATFORM(MAC)

I think items like this might be clearer as:

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

Not sure, though. I think the IOS || MAC form is not as clear and explicit.

> Source/WTF/wtf/PlatformHave.h:321
> +#if PLATFORM(MAC) || (PLATFORM(IOS_FAMILY) && !PLATFORM(IOS_FAMILY_SIMULATOR))

#if PLATFORM(COCOA) && !PLATFORM(IOS_FAMILY_SIMULATOR)

> Source/WTF/wtf/PlatformHave.h:360
> -#if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> +#if PLATFORM(COCOA) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
>  #define HAVE_HSTS_STORAGE_PATH 1
>  #endif

I wonder if this is really different for watchOS and tvOS or if it’s just a mistake.

> Source/WTF/wtf/PlatformHave.h:433
> -#if PLATFORM(COCOA) && !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
> +#if PLATFORM(COCOA) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
>  #define HAVE_CFNETWORK_NSURLSESSION_STRICTRUSTEVALUATE 1
>  #endif

I wonder if this is really different for watchOS and tvOS or if it’s just a mistake.

> Source/WTF/wtf/PlatformHave.h:444
> +#if PLATFORM(COCOA)
>  #define HAVE_CFNETWORK_NEGOTIATED_SSL_PROTOCOL_CIPHER 1
>  #endif

This is only used in Cocoa-specific code, so it can be removed entirely.

> Source/WTF/wtf/PlatformHave.h:448
> +#if PLATFORM(COCOA)
>  #define HAVE_CFNETWORK_SAMESITE_COOKIE_API 1
>  #endif

This is only used in a Cocoa-specific source file, so it can be removed entirely.

> Source/WTF/wtf/PlatformHave.h:452
> +#if PLATFORM(COCOA)
>  #define HAVE_CFNETWORK_METRICS_CONNECTION_PROPERTIES 1
>  #endif

This is only used in a Cocoa-specific source file, so it can be removed entirely.

> Source/WTF/wtf/PlatformHave.h:476
> +#if PLATFORM(COCOA) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
>  #define HAVE_MDNS_FAST_REGISTRATION 1
>  #endif

I wonder if this is really different for watchOS and tvOS or if it’s just a mistake.

> Source/WTF/wtf/PlatformHave.h:480
> +#if PLATFORM(COCOA) && !PLATFORM(WATCHOS) && !PLATFORM(APPLETV)
>  #define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION 1
>  #endif

I wonder if this is really different for watchOS and tvOS or if it’s just a mistake.

> Source/WTF/wtf/PlatformHave.h:561
> +#if PLATFORM(COCOA)
>  #define HAVE_TLS_PROTOCOL_VERSION_T 1
>  #endif

This is used only in a Cocoa-specific source file, so we can remove it.

> Source/WTF/wtf/PlatformHave.h:565
> +#if PLATFORM(MAC)
>  #define HAVE_SUBVIEWS_IVAR_SPI 1
>  #endif

This is used only in Mac-specific parts of source files, so we can remove it.

> Source/WTF/wtf/PlatformHave.h:569
> +#if PLATFORM(MAC)
>  #define HAVE_AX_CLIENT_TYPE 1
>  #endif

This is used only in an SPI header, so we can remove it.

> Source/WTF/wtf/PlatformHave.h:581
> +#if PLATFORM(COCOA)
>  #define HAVE_DATA_PROTECTION_KEYCHAIN 1
>  #endif

This is used only in Cocoa-specific source files, so we can remove it.

> Source/WTF/wtf/PlatformHave.h:583
> +#if PLATFORM(MAC) || (PLATFORM(IOS_FAMILY) && !PLATFORM(IOS_FAMILY_SIMULATOR))

#if PLATFORM(COCOA) && !PLATFORM(IOS_FAMILY_SIMULATOR)

> Source/WTF/wtf/PlatformHave.h:593
> -#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)
> +#if PLATFORM(COCOA)
>  #define HAVE_AVASSETWRITERDELEGATE 1
>  #endif

I think this change is turning this on for watchOS and tvOS for the first time, since I believe they have __IPHONE_OS_VERSION_MIN_REQUIRED values lower than 130000.

Not saying that’s incorrect.

> Source/WTF/wtf/PlatformHave.h:613
> +#if PLATFORM(COCOA)
>  #define HAVE_GCEXTENDEDGAMEPAD_BUTTONS_OPTIONS_MENU 1
>  #endif

This is used only in a Cocoa-specific source file, so we can remove it.

> Source/WTF/wtf/PlatformHave.h:625
> +#if PLATFORM(MAC)
>  #define HAVE_HID_FRAMEWORK 1
>  #endif

This is unused so it can be removed.

> Source/WTF/wtf/PlatformHave.h:689
> +#if PLATFORM(IOS) || PLATFORM(MAC)
>  #define HAVE_PASSKIT_BOUND_INTERFACE_IDENTIFIER 1
>  #endif

Pretty sure we can remove this; it’s used only in code that’s already PassKit-specific.

> Source/WTF/wtf/PlatformHave.h:693
> +#if PLATFORM(IOS) || PLATFORM(MAC)
>  #define HAVE_PASSKIT_INSTALLMENTS 1
>  #endif

Pretty sure we can remove this; it’s used only in code that’s already PassKit-specific.

> Source/WTF/wtf/PlatformHave.h:697
> +#if PLATFORM(IOS) || PLATFORM(MAC)
>  #define HAVE_PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS 1
>  #endif

Pretty sure we can remove this; it’s used only in code that’s already PassKit-specific.

> Source/WTF/wtf/PlatformHave.h:710
> +#if PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(MACCATALYST)
>  #define HAVE_QUICKLOOK_THUMBNAILING 1
>  #endif

I think this might be clearer as:

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

> Source/WTF/wtf/PlatformHave.h:774
> +#if PLATFORM(COCOA)
>  #define HAVE_CTFONTGETPHYSICALSYMBOLICTRAITS 1
>  #endif

This is used only in a Cocoa-specific source file, so it can be removed entirely.

> Source/WTF/wtf/PlatformHave.h:865
> +#if PLATFORM(IOS) || PLATFORM(MACCATALYST) || PLATFORM(MAC)
>  #define HAVE_SPEECHRECOGNIZER 1
>  #endif

I think this might be clearer as:

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

[Brent Fulgham]

Comment on attachment 439872 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebCore/cocoa/AVFoundationSoftLinkTest.mm:164
> +#if PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)

I wonder if this could be USE(COCOA) or something like that? Maybe we just don't want CATALYST?

[Chris Dumez]

(In reply to Brent Fulgham from comment #4)
> Comment on attachment 439872 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439872&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebCore/cocoa/AVFoundationSoftLinkTest.mm:164
> > +#if PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
> 
> I wonder if this could be USE(COCOA) or something like that? Maybe we just
> don't want CATALYST?

Right, that would be a behavior change in Catalyst, I don't think I want to take any chances in this patch.

[Brent Fulgham]

Comment on attachment 439872 [details]
Patch

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

> Source/WebKit/GPUProcess/mac/com.apple.WebKit.GPUProcess.sb.in:121
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

I think we can just remove this guard entirely. This file is only used by macOS or catalyst, so now all uses of this file need this rule.

> Source/WebKit/GPUProcess/mac/com.apple.WebKit.GPUProcess.sb.in:691
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

Ditto. This file is not used on iOS or elsewhere -- only Mac and MacCatalyst.

> Source/WebKit/GPUProcess/mac/com.apple.WebKit.GPUProcess.sb.in:718
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

Ditto.

> Source/WebKit/GPUProcess/mac/com.apple.WebKit.GPUProcess.sb.in:723
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

Ditto.

> Source/WebKit/WebAuthnProcess/mac/com.apple.WebKit.WebAuthnProcess.sb.in:357
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

We can remove the guard and the '#else' clause here. Only Mac/Maccatlyst use this file.

> Source/WebKit/WebAuthnProcess/mac/com.apple.WebKit.WebAuthnProcess.sb.in:364
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

Remove

> Source/WebKit/WebAuthnProcess/mac/com.apple.WebKit.WebAuthnProcess.sb.in:369
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

Ditto.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:309
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

We can remove this guard. Only Mac/MacCatalyst use this file.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1351
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

Ditto.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1387
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

Ditto.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1404
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

Ditto.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1709
> +#if PLATFORM(MAC) || PLATFORM(MACCATALYST)

Ditto.

[Devin Rousso]

Comment on attachment 439872 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/PassKitSPI.h:445
> +#if PLATFORM(COCOA)

i think you can drop this entirely since PassKit only exists on cocoa

> Source/WebCore/inspector/InspectorFrontendHost.cpp:-405
> -#elif PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
> -    return "mojave"_s;

Can you file a Web Inspector bug about removing support for Mojave?  I don't think there's much work to do but it'd be nice to remove dead code :)

[Chris Dumez]

Created attachment 439896 [details]
Patch

[Chris Dumez]

Created attachment 439897 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> Created attachment 439897 [details]
> Patch

Looks like enabling PASSKIT_INSTALLMENTS on TVOS was not OK after all.

[Chris Dumez]

Created attachment 439899 [details]
Patch

[Chris Dumez]

Patch is green on EWS. I have cleared the HAVE_AVASSETWRITERDELEGATE change with Eric (who says it is fine as long as it builds, we don't actually support capture in WebKit on TV or Watch).

[Darin Adler]

Comment on attachment 439899 [details]
Patch

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

> Source/JavaScriptCore/API/JSValue.h:-556
> -#if (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130000)
> -typedef NSString *JSValueProperty;
> -#else
>  typedef id JSValueProperty;
> -#endif

I asked you this before and you didn’t respond:

Is it OK to make this change in a public SDK header? Maybe we still support targeting these older versions with this header even if WebKit *code* no longer supports them?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:805
>          if (shouldComputePhysicalTraits == ShouldComputePhysicalTraits::Yes) {

Remove braces because this is now one line?

> Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:41
> +#if PLATFORM(MAC)

Is this file used on iOS family at all?

> Source/WebKit/PluginProcess/mac/com.apple.WebKit.plugin-common.sb.in:171
> +#if PLATFORM(MAC)

Same question.

> Tools/TestRunnerShared/spi/AppKitTestSPI.h:-33
> -#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500
>  #define HAVE_NSDRAGGINGITEM_INITWITHITEM 1
> -#endif

This is only used in 3 places; we should simply remove the #if in all 3.

[Chris Dumez]

(In reply to Darin Adler from comment #13)
> Comment on attachment 439899 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439899&action=review
> 
> > Source/JavaScriptCore/API/JSValue.h:-556
> > -#if (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130000)
> > -typedef NSString *JSValueProperty;
> > -#else
> >  typedef id JSValueProperty;
> > -#endif
> 
> I asked you this before and you didn’t respond:
> 
> Is it OK to make this change in a public SDK header? Maybe we still support
> targeting these older versions with this header even if WebKit *code* no
> longer supports them?

I asked on the JSC channel. I haven't gotten a response yet. If I don't get the OK from JSC people, I'll land without this header change.

> 
> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:805
> >          if (shouldComputePhysicalTraits == ShouldComputePhysicalTraits::Yes) {
> 
> Remove braces because this is now one line?

OK.

> 
> > Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:41
> > +#if PLATFORM(MAC)
> 
> Is this file used on iOS family at all?
> 
> > Source/WebKit/PluginProcess/mac/com.apple.WebKit.plugin-common.sb.in:171
> > +#if PLATFORM(MAC)
> 
> Same question.

Per Brent's previous comment, it seems those sandbox files are used on MAC and MACCATALYST. If so, dropping this #if would cause a behavior change on Catalyst I think.

> 
> > Tools/TestRunnerShared/spi/AppKitTestSPI.h:-33
> > -#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500
> >  #define HAVE_NSDRAGGINGITEM_INITWITHITEM 1
> > -#endif
> 
> This is only used in 3 places; we should simply remove the #if in all 3.

OK.

[Chris Dumez]

Created attachment 439925 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #14)
> (In reply to Darin Adler from comment #13)
> > Comment on attachment 439899 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=439899&action=review
> > 
> > > Source/JavaScriptCore/API/JSValue.h:-556
> > > -#if (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130000)
> > > -typedef NSString *JSValueProperty;
> > > -#else
> > >  typedef id JSValueProperty;
> > > -#endif
> > 
> > I asked you this before and you didn’t respond:
> > 
> > Is it OK to make this change in a public SDK header? Maybe we still support
> > targeting these older versions with this header even if WebKit *code* no
> > longer supports them?
> 
> I asked on the JSC channel. I haven't gotten a response yet. If I don't get
> the OK from JSC people, I'll land without this header change.

I didn't get a response yet so I'll just land without the JSValue.h header change for now and follow-up if needed.

[Chris Dumez]

Created attachment 439935 [details]
Patch

[Chris Dumez]

Created attachment 439941 [details]
Patch

[EWS]

Committed r283431 (242418@main): <https://commits.webkit.org/242418@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439941 [details].

[Radar WebKit Bug Importer]

<rdar://problem/83791576>

[Chris Dumez]

Follow-up build fix in <https://commits.webkit.org/r283518>.

[Chris Dumez]

(In reply to Chris Dumez from comment #21)
> Follow-up build fix in <https://commits.webkit.org/r283518>.

and <https://commits.webkit.org/r283519>.

[Devin Rousso]

Comment on attachment 439941 [details]
Patch

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

> Source/WebCore/inspector/InspectorFrontendHost.cpp:-405
> -    return "mojave"_s;

this probably should be removed from `Source/WebInspectorUI` as well :)