Bug 212704 – HAVE(ACCESSIBILITY_BUNDLES_PATH) is defined in terms of PLATFORM(IOS_FAMILY) but only checks the version of __IPHONE_OS_VERSION_MIN_REQUIRED

• Component: Accessibility
• Status: RESOLVED FIXED

[Andy Estes]

HAVE_ACCESSIBILITY_BUNDLES_PATH is defined like so:

>#if PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000
>#define HAVE_ACCESSIBILITY_BUNDLES_PATH 1
>#endif

The use of PLATFORM(IOS_FAMILY) suggests we should enable this HAVE() on some tvOS and watchOS platforms, but __IPHONE_OS_VERSION_MIN_REQUIRED will never be >= 140000 on these platforms.

If we only intend to support iOS, we should change PLATFORM(IOS_FAMILY) to PLATFORM(IOS). Otherwise, we need to add a version check for each platform we intend to support, e.g.:

>(PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) \
>    || (PLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 60000) \
>    || (PLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 130000)

[Radar WebKit Bug Importer]

<rdar://problem/63931340>

[chris fleizach]

Created attachment 401380 [details]
patch

[Darin Adler]

Comment on attachment 401380 [details]
patch

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

> Source/WTF/wtf/PlatformHave.h:388
> -#if PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000) \
> +    || (PLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 70000) \
> +    || (PLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 140000)

Seems like an improvement. Also note that in IOS_FAMILY, besides iOS, watchOS, and tvOS, there is Mac Catalyst.

[chris fleizach]

Comment on attachment 401380 [details]
patch

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

>> Source/WTF/wtf/PlatformHave.h:388
>> +    || (PLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 140000)
> 
> Seems like an improvement. Also note that in IOS_FAMILY, besides iOS, watchOS, and tvOS, there is Mac Catalyst.

yes good point. we should also add that

[chris fleizach]

Created attachment 401388 [details]
patch

[Darin Adler]

Comment on attachment 401388 [details]
patch

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

> Source/WTF/wtf/PlatformHave.h:389
> +    || (PLATFORM(MACCATALYST) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)

Tim Horton asked earlier if __IPHONE_OS_VERSION_MIN_REQUIRED was the correct macro to check for MACCATALYST. He was pretty sure it was not.

[chris fleizach]

(In reply to Darin Adler from comment #6)
> Comment on attachment 401388 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401388&action=review
> 
> > Source/WTF/wtf/PlatformHave.h:389
> > +    || (PLATFORM(MACCATALYST) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)
> 
> Tim Horton asked earlier if __IPHONE_OS_VERSION_MIN_REQUIRED was the correct
> macro to check for MACCATALYST. He was pretty sure it was not.

I think it works I don't know if it's the best one. I saw this was already being used

#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) || (PLATFORM(MACCATALYST) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)
#define HAVE_MEDIA_USAGE_FRAMEWORK 1
#endif

[Darin Adler]

OK. maybe HAVE(MEDIA_USAGE_FRAMEWORK) is right and Tim is wrong. Or maybe ... the other way around? Sure would be nice to confirm.

[Tim Horton]

(In reply to Darin Adler from comment #8)
> OK. maybe HAVE(MEDIA_USAGE_FRAMEWORK) is right and Tim is wrong. Or maybe
> ... the other way around? Sure would be nice to confirm.

It looks like I was wrong! (or am wrong now, anyway!)

[EWS]

Committed r262765: <https://trac.webkit.org/changeset/262765>

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