Bug 207828 – AX: Adopt _AXSCopyPathForAccessibilityBundle for WebKit

• Component: Accessibility
• Status: RESOLVED FIXED

[Eric Liang]

AX: Adopt _AXSCopyPathForAccessibilityBundle for WebKit

[Radar WebKit Bug Importer]

<rdar://problem/59496899>

[Eric Liang]

Created attachment 390892 [details]
Patch

[Darin Adler]

Comment on attachment 390892 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:-412
> -    accessibilityBundlesPath = (__bridge NSString *)_AXSAccessibilityBundlesPath();

Wouldn’t we want to continue using this on iOS 13?

[Darin Adler]

Comment on attachment 390892 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:410
>  #if HAVE(ACCESSIBILITY_BUNDLES_PATH)

We should use a new HAVE for the new function, not redefine the existing HAVE that is named after the existing function. Like HAVE(COPY_PATH_FOR_ACCESSIBILITY_BUNDLE).

[Eric Liang]

In theory yes we should define a new HAVE, but I was trying to simplify the code, reasons:
1) HAVE_ACCESSIBILITY_BUNDLES_PATH is only used in this one place
2) _AXSAccessibilityBundlesPath() is extremely simple: it just returns "/System/Library/AccessibilitBundles" for iOS and "/System/iOSSupport/System/Library/AccessibilitBundles" for catalyst. 
3) The new _AXSCopyPathForAccessibilityBundle is also very simple: it just returns "/System/Library/AccessibilitBundles/WebProcessLoader.axbundle" for iOS and "/System/iOSSupport/System/Library/AccessibilitBundles/WebProcessLoader.axbundle" for catalyst.

Based on above three reason, my personal pref (that makes code a little bit easier to read) would be to change the old HAVE and bump to iOS 14. This way, we don't need to have three code paths (1) for less than ios13 (2) for between iOS 13 and iOS 14 (3) for iOS14 and above. Under my preferences, the code that runs in less than iOS 14 is still perfectly valid.

But if you think adding the new HAVE still makes more sense, I could for sure switch to that.

[Darin Adler]

Comment on attachment 390892 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:411
> +    return (__bridge_transfer NSString *)_AXSCopyPathForAccessibilityBundle(CFSTR("WebProcessLoader"));

In my obsession with encouraging you to keep using the old function, I missed something more important. This file is not compiled with ARC, and in this case I suspect __bridge_transfer is not going to do the right thing. We need something that will compile into a call to autorelease; I think a call to CFAutorelease.

I’m going to say review+ but please don’t land this if it’s leaking the string. I don’t think __bridge_transfer actually correctly causes the autorelease needed in non-ARC code.

[Eric Liang]

Created attachment 391264 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391264 [details]
Patch

Clearing flags on attachment: 391264

Committed r257083: <https://trac.webkit.org/changeset/257083>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

Comment on attachment 391264 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:423
> -#if PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000 && __IPHONE_OS_VERSION_MAX_ALLOWED >= 130100
> +#if PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000

This means that HAVE_ACCESSIBILITY_BUNDLES_PATH is undefined on tvOS and watchOS (because __IPHONE_OS_VERSION_MIN_REQUIRED will never be >= 140000 on those platforms). Was that your intention?

Filed <https://webkit.org/b/212704> HAVE(ACCESSIBILITY_BUNDLES_PATH) is defined in terms of PLATFORM(IOS_FAMILY) but only checks the version of __IPHONE_OS_VERSION_MIN_REQUIRED