Bug 202371 – Remove some support for < iOS 13

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Keith Rollin]

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.

[Radar WebKit Bug Importer]

<rdar://problem/55853960>

[Keith Rollin]

Created attachment 379841 [details]
Patch

[Keith Rollin]

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.

[Keith Rollin]

Should these be enabled for macCatalyst?

#define HAVE_CA_WHERE_ADDITIVE_TRANSFORMS_ARE_REVERSED 1
#define USE_UICONTEXTMENU 1

[youenn fablet]

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).

[Brent Fulgham]

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)" ?

[Tim Horton]

(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.

[Keith Rollin]

> #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.

[Keith Rollin]

Created attachment 380599 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 380599 [details]
Patch

Clearing flags on attachment: 380599

Committed r250953: <https://trac.webkit.org/changeset/250953>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Tim Horton]

(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).