[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

--- Comment #30 from Eric Liang <ericliang at apple.com> ---
(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
> > + at 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.

-- 
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/9b976606/attachment-0001.html>