Bug 231085 - Drop support for macOS < 10.15
Summary: Drop support for macOS < 10.15
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 233671
  Show dependency treegraph
 
Reported: 2021-10-01 09:51 PDT by Chris Dumez
Modified: 2021-12-15 10:05 PST (History)
24 users (show)

See Also:


Attachments
Patch (48.18 KB, patch)
2021-10-01 10:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (48.18 KB, patch)
2021-10-01 10:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (113.57 KB, patch)
2021-10-01 12:17 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (112.54 KB, patch)
2021-10-01 12:22 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (98.99 KB, patch)
2021-10-01 12:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (100.53 KB, patch)
2021-10-01 16:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (98.74 KB, patch)
2021-10-01 17:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (98.65 KB, patch)
2021-10-01 17:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-10-01 09:51:35 PDT
Drop support for macOS < 10.15.
Comment 1 Chris Dumez 2021-10-01 10:05:49 PDT
Created attachment 439871 [details]
Patch
Comment 2 Chris Dumez 2021-10-01 10:09:43 PDT
Created attachment 439872 [details]
Patch
Comment 3 Darin Adler 2021-10-01 10:57:56 PDT
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)
Comment 4 Brent Fulgham 2021-10-01 11:29:20 PDT
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?
Comment 5 Chris Dumez 2021-10-01 11:30:13 PDT
(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.
Comment 6 Brent Fulgham 2021-10-01 11:33:48 PDT
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.
Comment 7 Devin Rousso 2021-10-01 11:34:06 PDT
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 :)
Comment 8 Chris Dumez 2021-10-01 12:17:08 PDT
Created attachment 439896 [details]
Patch
Comment 9 Chris Dumez 2021-10-01 12:22:06 PDT
Created attachment 439897 [details]
Patch
Comment 10 Chris Dumez 2021-10-01 12:30:09 PDT
(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.
Comment 11 Chris Dumez 2021-10-01 12:42:30 PDT
Created attachment 439899 [details]
Patch
Comment 12 Chris Dumez 2021-10-01 15:17:59 PDT
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).
Comment 13 Darin Adler 2021-10-01 15:55:52 PDT
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.
Comment 14 Chris Dumez 2021-10-01 16:05:56 PDT
(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.
Comment 15 Chris Dumez 2021-10-01 16:10:56 PDT
Created attachment 439925 [details]
Patch
Comment 16 Chris Dumez 2021-10-01 16:11:42 PDT
(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.
Comment 17 Chris Dumez 2021-10-01 17:03:54 PDT
Created attachment 439935 [details]
Patch
Comment 18 Chris Dumez 2021-10-01 17:33:54 PDT
Created attachment 439941 [details]
Patch
Comment 19 EWS 2021-10-01 19:48:12 PDT
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].
Comment 20 Radar WebKit Bug Importer 2021-10-01 19:49:15 PDT
<rdar://problem/83791576>
Comment 21 Chris Dumez 2021-10-04 15:43:07 PDT
Follow-up build fix in <https://commits.webkit.org/r283518>.
Comment 22 Chris Dumez 2021-10-04 15:45:35 PDT
(In reply to Chris Dumez from comment #21)
> Follow-up build fix in <https://commits.webkit.org/r283518>.

and <https://commits.webkit.org/r283519>.
Comment 23 Devin Rousso 2021-12-15 10:05:07 PST
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 :)