Bug 200367 – AX: don't hard code accessibility bundle directory path

• Component: Accessibility
• Status: RESOLVED FIXED

[Eric Liang]

AX: don't hard code webprocessloader bundle path

[Eric Liang]

<rdar://problem/53838408> AX: don't hard code webprocessloader bundle path

[Eric Liang]

Created attachment 375364 [details]
Patch

[EWS Watchlist]

Attachment 375364 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:45:  _AXSAccessibilityBundlesPath is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:92:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 375364 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:325
> +    NSString *accessibilityBundlePath = (__bridge NSString *)_AXSAccessibilityBundlesPath();

you'll need to conditonalize this based on availability per platform and release

[Darin Adler]

Comment on attachment 375364 [details]
Patch

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

>> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:325
>> +    NSString *accessibilityBundlePath = (__bridge NSString *)_AXSAccessibilityBundlesPath();
> 
> you'll need to conditonalize this based on availability per platform and release

Exactly.

Also, I think the code above isn’t right yet because it doesn’t add "WebProcessLoader.axbundle" to the end of the path.

One good way to do following the WebKit style is to add lines to Platform.h telling us where it’s available. You can use an example like HAVE_UI_SCROLL_VIEW_INDICATOR_FLASHING_SPI or HAVE_DESIGN_SYSTEM_UI_FONTS to get the general style right. Could name it something like HAVE_ACCESSIBILITY_BUNDLES_PATH

Then I would suggest adding something like this above:

    #if PLATFORM(IOS_FAMILY) && !HAVE(ACCESSIBILITY_BUNDLES_PATH)
    static NSString *accessibilityBundlesDirectory()
    {
        NSString *path = (__bridge NSString *)GSSystemRootDirectory();
    #if PLATFORM(MACCATALYST)
        path = [path stringByAppendingString:@"System/iOSSupport"];
    #endif
        return [path stringByAppendingString:@"System/Library/AccessibilityBundles"];
    }
    #endif

Then below we would write:

    #if HAVE(ACCESSIBILITY_BUNDLES_PATH)
        NSString *directory = (__bridge NSString *)_AXSAccessibilityBundlesPath();
    #else
        NSString *directory = accessibilityBundlesDirectory();
    #endif
        NSString *path = [directory stringByAppendingPathComponent:@"WebProcessLoader.axbundle"];

Later we can delete the !HAVE(ACCESSIBILITY_BUNDLES_PATH) part, once we have the function everywhere.

Also, we need to find a way to test this. I assume the posted patch wasn’t tested yet since it didn’t include the WebProcessLoader bundle name.

[Darin Adler]

Although maybe I misunderstood. Perhaps _AXSAccessibilityBundlesPath returns the path to the bundle, not just the directory containing bundles, and somehow knows its name?

[Darin Adler]

Thanks so much for tackling this! I think it will be better for the long term.

[Eric Liang]

Thanks Darin for great suggestions! Definitely I forgot to append the webprocessloader.axbundle...

[Eric Liang]

Created attachment 376020 [details]
Patch

[EWS Watchlist]

Attachment 376020 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:46:  _AXSAccessibilityBundlesPath is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Liang]

Created attachment 376021 [details]
Patch

[EWS Watchlist]

Attachment 376021 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:46:  _AXSAccessibilityBundlesPath is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 376021 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1501
> +#if PLATFORM(IOS_FAMILY) && (__IPHONE_OS_VERSION_MIN_REQUIRED >= 130000 || __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500)

Doesn’t make sense to check __MAC_OS_X_VERSION_MIN_REQUIRED if we first check PLATFORM(IOS_FAMILY). Do we intend to have this on for Mac? If so then I think it’s more like this:

    #if (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500)

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:339
> +#endif // USE(APPKIT)

When it’s a three line long #if/#endif, I think  the comment on the #endif makes it harder to read, not easier. So I’d not make this change.

[Eric Liang]

(In reply to Darin Adler from comment #13)
> Comment on attachment 376021 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376021&action=review
> 
> > Source/WTF/wtf/Platform.h:1501
> > +#if PLATFORM(IOS_FAMILY) && (__IPHONE_OS_VERSION_MIN_REQUIRED >= 130000 || __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500)
> 
> Doesn’t make sense to check __MAC_OS_X_VERSION_MIN_REQUIRED if we first
> check PLATFORM(IOS_FAMILY). Do we intend to have this on for Mac? If so then
> I think it’s more like this:
> 
>     #if (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)
> || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500)
> 

This will only exist on ios13 and catalyst 10.15. I changed to:
#if PLATFORM(IOS_FAMILY) && ((defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500))
Is this correct?

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:339
> > +#endif // USE(APPKIT)
> 
> When it’s a three line long #if/#endif, I think  the comment on the #endif
> makes it harder to read, not easier. So I’d not make this change.

[Eric Liang]

Created attachment 376023 [details]
Patch

[Darin Adler]

Comment on attachment 376023 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1501
> +#if PLATFORM(IOS_FAMILY) && ((defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500))

I’m not sure how to do this right for Catalyst. I don’t think it involved __MAC_OS_X_VERSION_MIN_REQUIRED at all, but it might.

[EWS Watchlist]

Attachment 376023 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:46:  _AXSAccessibilityBundlesPath is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Liang]

Created attachment 376027 [details]
Patch

[EWS Watchlist]

Attachment 376027 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:46:  _AXSAccessibilityBundlesPath is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Liang]

(In reply to Darin Adler from comment #16)
> Comment on attachment 376023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376023&action=review
> 
> > Source/WTF/wtf/Platform.h:1501
> > +#if PLATFORM(IOS_FAMILY) && ((defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500))
> 
> I’m not sure how to do this right for Catalyst. I don’t think it involved
> __MAC_OS_X_VERSION_MIN_REQUIRED at all, but it might.

Thanks. I will add it just in case because I think catalyst is using macos sdk. I would be surprised if it is using __IPHONE_OS_VERSION_MIN_REQUIRED

[Eric Liang]

Created attachment 376028 [details]
Patch

[EWS Watchlist]

Attachment 376028 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:46:  _AXSAccessibilityBundlesPath is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 376028 [details]
Patch

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

Failing to compile on all our iOS build bots, so not ready to land yet.

> Source/WTF/wtf/Platform.h:1501
> +#if PLATFORM(IOS_FAMILY) && ((defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500))

This is either correct or incorrect. There’s no space for "just in case" here. We need to understand how the version checking works for Catalyst rather than guessing.

As I said, I would be very surprised if __MAC_OS_X_VERSION_MIN_REQUIRED is a factor, but it’s possible. Can we find someone who can tell us how it works and what is correct?

[Eric Liang]

Created attachment 376133 [details]
Patch

[EWS Watchlist]

Attachment 376133 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:46:  _AXSAccessibilityBundlesPath is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Liang]

Created attachment 376460 [details]
Patch

[EWS Watchlist]

Attachment 376460 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:46:  _AXSAccessibilityBundlesPath is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 376460 [details]
Patch

Attachment 376460 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12922002

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline

[Darin Adler]

Comment on attachment 376460 [details]
Patch

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

Looks great to me.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:320
> +static NSString *accessibilityWebProcessLoaderBundlePath()

I think the name of this function should be "web process loader accessibility bundle path", not "accessibility web process loader bundle path".

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:343
> +    NSString *webProcessLoaderPath = accessibilityWebProcessLoaderBundlePath();

I don’t think naming this "web process loader path" is good, since it’s the path to the accessibility bundle for the web process loader, not the path to the web process loader. Given the context is just this short function I would call the local variable something like bundlePath or path, or leave it named accessibilityBundlePath like it was before.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/AccessibilityTestPlugin.mm:37
> +@interface AccessibilityTestPlugin : NSObject<WKWebProcessPlugIn, WKWebProcessPlugInLoadDelegate>

Seems like we should also declare ourselves as supporting AccessibilityTestSupportProtocol so that gets checked at compile time.

> Tools/TestWebKitAPI/Tests/ios/AccessibilityTestsIOS.mm:142
> +    [remoteObjectProxy checkAccessibilityWebProcessLoaderBundleIsLoaded:^(BOOL bundleLoaded, NSString *loadedPath) {
> +#if PLATFORM(IOS_FAMILY)
> +        EXPECT_TRUE(static_cast<bool>(bundleLoaded));
> +#if PLATFORM(MACCATALYST)
> +        EXPECT_TRUE(static_cast<bool>([loadedPath hasSuffix:@"/System/iOSSupport/System/Library/AccessibilityBundles/WebProcessLoader.axbundle"]));
> +#else
> +        EXPECT_TRUE(static_cast<bool>([loadedPath hasSuffix:@"/System/Library/AccessibilityBundles/WebProcessLoader.axbundle"]));
> +        EXPECT_FALSE(static_cast<bool>([loadedPath hasSuffix:@"/System/iOSSupport/System/Library/AccessibilityBundles/WebProcessLoader.axbundle"]));
> +#endif
> +#elif PLATFORM(MAC)
> +        EXPECT_FALSE(static_cast<bool>(bundleLoaded));
> +#endif // PLATFORM(IOS_FAMILY)

I’m surprised all of these static_cast are needed.

[Eric Liang]

(In reply to Darin Adler from comment #29)
> Comment on attachment 376460 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376460&action=review
> 
> Looks great to me.
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:320
> > +static NSString *accessibilityWebProcessLoaderBundlePath()
> 
> I think the name of this function should be "web process loader
> accessibility bundle path", not "accessibility web process loader bundle
> path".
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:343
> > +    NSString *webProcessLoaderPath = accessibilityWebProcessLoaderBundlePath();
> 
> I don’t think naming this "web process loader path" is good, since it’s the
> path to the accessibility bundle for the web process loader, not the path to
> the web process loader. Given the context is just this short function I
> would call the local variable something like bundlePath or path, or leave it
> named accessibilityBundlePath like it was before.
> 
I agree above are better. changed.
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/AccessibilityTestPlugin.mm:37
> > +@interface AccessibilityTestPlugin : NSObject<WKWebProcessPlugIn, WKWebProcessPlugInLoadDelegate>
> 
> Seems like we should also declare ourselves as supporting
> AccessibilityTestSupportProtocol so that gets checked at compile time.
> 
Added.
> > Tools/TestWebKitAPI/Tests/ios/AccessibilityTestsIOS.mm:142
> > +    [remoteObjectProxy checkAccessibilityWebProcessLoaderBundleIsLoaded:^(BOOL bundleLoaded, NSString *loadedPath) {
> > +#if PLATFORM(IOS_FAMILY)
> > +        EXPECT_TRUE(static_cast<bool>(bundleLoaded));
> > +#if PLATFORM(MACCATALYST)
> > +        EXPECT_TRUE(static_cast<bool>([loadedPath hasSuffix:@"/System/iOSSupport/System/Library/AccessibilityBundles/WebProcessLoader.axbundle"]));
> > +#else
> > +        EXPECT_TRUE(static_cast<bool>([loadedPath hasSuffix:@"/System/Library/AccessibilityBundles/WebProcessLoader.axbundle"]));
> > +        EXPECT_FALSE(static_cast<bool>([loadedPath hasSuffix:@"/System/iOSSupport/System/Library/AccessibilityBundles/WebProcessLoader.axbundle"]));
> > +#endif
> > +#elif PLATFORM(MAC)
> > +        EXPECT_FALSE(static_cast<bool>(bundleLoaded));
> > +#endif // PLATFORM(IOS_FAMILY)
> 
> I’m surprised all of these static_cast are needed.
removed.

Thanks for the review.

[Eric Liang]

Created attachment 376539 [details]
Patch

[EWS Watchlist]

Attachment 376539 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:46:  _AXSAccessibilityBundlesPath is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 376539 [details]
Patch

Clearing flags on attachment: 376539

Committed r248823: <https://trac.webkit.org/changeset/248823>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.