Summary: | Drop support for macOS < 10.15 | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | achristensen, benjamin, bfulgham, cmarcelo, darin, eric.carlson, ews-watchlist, glenn, hi, jer.noble, jiewen_tan, joepeck, keith_miller, mark.lam, mmaxfield, msaboff, pangle, philipj, pvollan, saam, sergio, thorton, tzagallo, webkit-bug-importer | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=231081 https://bugs.webkit.org/show_bug.cgi?id=234339 |
||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 233671, 238056 | ||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2021-10-01 09:51:35 PDT
Created attachment 439871 [details]
Patch
Created attachment 439872 [details]
Patch
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 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? (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 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 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 :) Created attachment 439896 [details]
Patch
Created attachment 439897 [details]
Patch
(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. Created attachment 439899 [details]
Patch
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 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. (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. Created attachment 439925 [details]
Patch
(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. Created attachment 439935 [details]
Patch
Created attachment 439941 [details]
Patch
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]. Follow-up build fix in <https://commits.webkit.org/r283518>. (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 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 :) |