Bug 212704

Summary:

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

Product:

WebKit

Reporter:

Andy Estes <aestes>

Component:

Accessibility

Assignee:

chris fleizach <cfleizach>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aestes, benjamin, cdumez, cfleizach, cmarcelo, darin, ericliang, ews-watchlist, thorton, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=207828

Attachments:

Description	Flags
patch	darin: review+
patch	none

Description Andy Estes 2020-06-03 09:44:43 PDT

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)

Comment 1 Radar WebKit Bug Importer 2020-06-03 09:48:06 PDT

<rdar://problem/63931340>

Comment 2 chris fleizach 2020-06-08 15:18:19 PDT

Created attachment 401380 [details]
patch

Comment 3 Darin Adler 2020-06-08 15:49:08 PDT

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.

Comment 4 chris fleizach 2020-06-08 15:50:56 PDT

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

Comment 5 chris fleizach 2020-06-08 15:54:27 PDT

Created attachment 401388 [details]
patch

Comment 6 Darin Adler 2020-06-08 16:01:54 PDT

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.

Comment 7 chris fleizach 2020-06-08 16:04:42 PDT

(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

Comment 8 Darin Adler 2020-06-08 16:26:01 PDT

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

Comment 9 Tim Horton 2020-06-08 18:25:31 PDT

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

Comment 10 EWS 2020-06-08 21:34:39 PDT

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

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