AX: don't hard code webprocessloader bundle path
<rdar://problem/53838408> AX: don't hard code webprocessloader bundle path
Created attachment 375364 [details] Patch
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.
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
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.
Although maybe I misunderstood. Perhaps _AXSAccessibilityBundlesPath returns the path to the bundle, not just the directory containing bundles, and somehow knows its name?
Thanks so much for tackling this! I think it will be better for the long term.
Thanks Darin for great suggestions! Definitely I forgot to append the webprocessloader.axbundle...
Created attachment 376020 [details] Patch
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.
Created attachment 376021 [details] Patch
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.
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.
(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.
Created attachment 376023 [details] Patch
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.
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.
Created attachment 376027 [details] Patch
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.
(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
Created attachment 376028 [details] Patch
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.
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?
Created attachment 376133 [details] Patch
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.
Created attachment 376460 [details] Patch
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.
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
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.
(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.
Created attachment 376539 [details] Patch
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.
Comment on attachment 376539 [details] Patch Clearing flags on attachment: 376539 Committed r248823: <https://trac.webkit.org/changeset/248823>
All reviewed patches have been landed. Closing bug.