[Webkit-unassigned] [Bug 200367] AX: don't hard code accessibility bundle directory path

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #376460|review?                     |review+
              Flags|                            |

--- 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/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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190816/34688119/attachment.html>