Bug 202371 - Remove some support for < iOS 13
Summary: Remove some support for < iOS 13
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-30 14:01 PDT by Keith Rollin
Modified: 2019-10-10 21:51 PDT (History)
14 users (show)

See Also:


Attachments
Patch (2.70 KB, patch)
2019-09-30 14:05 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (2.74 KB, patch)
2019-10-09 18:28 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2019-09-30 14:01:57 PDT
Remove some support for iOS versions less than 13.0. 

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

This removal is part of a series of patches effecting the removal of dead code for old versions of iOS. This particular pass involves changes in which Dean Jackson was involved. These changes are isolated from other similar changes in order to facilitate the reviewing process.
Comment 1 Radar WebKit Bug Importer 2019-09-30 14:02:52 PDT
<rdar://problem/55853960>
Comment 2 Keith Rollin 2019-09-30 14:05:57 PDT
Created attachment 379841 [details]
Patch
Comment 3 Keith Rollin 2019-09-30 14:06:44 PDT
Reviewers note: Please check that the conditional around HAVE_CA_WHERE_ADDITIVE_TRANSFORMS_ARE_REVERSED is correct. It's not apparent to me if this should be enabled for all of the iOS family or just iOS per se.
Comment 4 Keith Rollin 2019-09-30 14:29:46 PDT
Should these be enabled for macCatalyst?

#define HAVE_CA_WHERE_ADDITIVE_TRANSFORMS_ARE_REVERSED 1
#define USE_UICONTEXTMENU 1
Comment 5 youenn fablet 2019-10-07 11:59:52 PDT
Comment on attachment 379841 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1446
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || PLATFORM(IOS)

Seems ok to move to PLATFORM(IOS) but just want to double check we do not want PLATFORM(IOS_FAMILY).
Comment 6 Brent Fulgham 2019-10-07 12:06:27 PDT
Comment on attachment 379841 [details]
Patch

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

>> Source/WTF/wtf/Platform.h:1446
>> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || PLATFORM(IOS)
> 
> Seems ok to move to PLATFORM(IOS) but just want to double check we do not want PLATFORM(IOS_FAMILY).

It does seem like this would apply to IOS_FAMILY. Or perhaps it should be "... PLATFORM(IOS) || PLATFORM(MACCATALYST)" ?
Comment 7 Tim Horton 2019-10-07 12:43:52 PDT
(In reply to Brent Fulgham from comment #6)
> Comment on attachment 379841 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379841&action=review
> 
> >> Source/WTF/wtf/Platform.h:1446
> >> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || PLATFORM(IOS)
> > 
> > Seems ok to move to PLATFORM(IOS) but just want to double check we do not want PLATFORM(IOS_FAMILY).
> 
> It does seem like this would apply to IOS_FAMILY. Or perhaps it should be
> "... PLATFORM(IOS) || PLATFORM(MACCATALYST)" ?

It seems incredibly likely that this should be IOS_FAMILY.
Comment 8 Keith Rollin 2019-10-09 16:42:41 PDT
> #define HAVE_CA_WHERE_ADDITIVE_TRANSFORMS_ARE_REVERSED 1

This is the one at Platform.h:1446 that everyone commented on. I'll include this for IOS_FAMILY.

> #define USE_UICONTEXTMENU 1

I'll include this for PLATFORM(IOS) || PLATFORM(MACCATALYST). It looks like the facilities it enables are defined on watch, but not tv, so this can't be for IOS_FAMILY. If someone thinks this should be enabled for watch, they can add that, but for now I'll keep the previous behavior.
Comment 9 Keith Rollin 2019-10-09 18:28:11 PDT
Created attachment 380599 [details]
Patch
Comment 10 WebKit Commit Bot 2019-10-09 19:23:13 PDT
Comment on attachment 380599 [details]
Patch

Clearing flags on attachment: 380599

Committed r250953: <https://trac.webkit.org/changeset/250953>
Comment 11 WebKit Commit Bot 2019-10-09 19:23:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2019-10-10 17:42:58 PDT
Comment on attachment 380599 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1449
> -#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || PLATFORM(IOS_FAMILY)
>  #define HAVE_CA_WHERE_ADDITIVE_TRANSFORMS_ARE_REVERSED 1
>  #endif

This seems like a change for watchOS, tvOS, and perhaps for Catalyst as well. Is it an intentional change that’s either valuable or harmless? I ask because it’s not mentioned in the change log, which only mentions removing support for older versions of iOS. And because if it’s valuable, there’s no test that validates the improvement.

> Source/WTF/wtf/Platform.h:1620
> -#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000
> +#if PLATFORM(IOS) || PLATFORM(MACCATALYST)
>  #define USE_UICONTEXTMENU 1
>  #endif

This seems like a change in behavior for Catalyst. Is it an intentional change that’s valuable? I ask because there’s no mention in the change log and no additional testing to validate the improvement.
Comment 13 Tim Horton 2019-10-10 21:51:40 PDT
(In reply to Darin Adler from comment #12)
> Comment on attachment 380599 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380599&action=review
> 
> > Source/WTF/wtf/Platform.h:1449
> > -#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || PLATFORM(IOS_FAMILY)
> >  #define HAVE_CA_WHERE_ADDITIVE_TRANSFORMS_ARE_REVERSED 1
> >  #endif
> 
> This seems like a change for watchOS, tvOS, and perhaps for Catalyst as
> well. Is it an intentional change that’s either valuable or harmless? I ask
> because it’s not mentioned in the change log, which only mentions removing
> support for older versions of iOS. And because if it’s valuable, there’s no
> test that validates the improvement.

This is a valuable change. See https://trac.webkit.org/changeset/241852/webkit.
The test from that commit can probably be made to work on watchOS/Catalyst, but test coverage in those contexts is... not ideal at the moment.

> > Source/WTF/wtf/Platform.h:1620
> > -#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000
> > +#if PLATFORM(IOS) || PLATFORM(MACCATALYST)
> >  #define USE_UICONTEXTMENU 1
> >  #endif
> 
> This seems like a change in behavior for Catalyst. Is it an intentional
> change that’s valuable? I ask because there’s no mention in the change log
> and no additional testing to validate the improvement.

This one surprises me more, but could very easily be right (and we have no Catalyst testing so that part is not a surprise).