[webkit-reviews] review granted: [Bug 200367] AX: don't hard code accessibility bundle directory path : [Attachment 376460] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Liang <ericliang at apple.com>'s
request for review:
Bug 200367: AX: don't hard code accessibility bundle directory path
https://bugs.webkit.org/show_bug.cgi?id=200367

Attachment 376460: Patch

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




--- Comment #29 from Darin Adler <darin at apple.com> ---
Comment on attachment 376460
  --> https://bugs.webkit.org/attachment.cgi?id=376460
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
> + at 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/WebProcessLo
ader.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/WebProcessLo
ader.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.