Bug 200367 - AX: don't hard code accessibility bundle directory path
Summary: AX: don't hard code accessibility bundle directory path
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-01 16:26 PDT by Eric Liang
Modified: 2019-08-17 12:14 PDT (History)
11 users (show)

See Also:


Attachments
Patch (2.79 KB, patch)
2019-08-01 16:37 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (17.73 KB, patch)
2019-08-10 15:47 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (17.71 KB, patch)
2019-08-10 15:51 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (17.76 KB, patch)
2019-08-10 16:26 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (17.84 KB, patch)
2019-08-10 20:02 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (17.85 KB, patch)
2019-08-10 20:26 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (17.98 KB, patch)
2019-08-12 20:46 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (18.61 KB, patch)
2019-08-15 18:21 PDT, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (18.52 KB, patch)
2019-08-16 14:17 PDT, Eric Liang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Liang 2019-08-01 16:26:56 PDT
AX: don't hard code webprocessloader bundle path
Comment 1 Eric Liang 2019-08-01 16:27:31 PDT
<rdar://problem/53838408> AX: don't hard code webprocessloader bundle path
Comment 2 Eric Liang 2019-08-01 16:37:43 PDT
Created attachment 375364 [details]
Patch
Comment 3 Build Bot 2019-08-01 16:39:58 PDT
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 4 chris fleizach 2019-08-02 15:49:34 PDT
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 5 Darin Adler 2019-08-04 10:44:16 PDT
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.
Comment 6 Darin Adler 2019-08-04 10:46:49 PDT
Although maybe I misunderstood. Perhaps _AXSAccessibilityBundlesPath returns the path to the bundle, not just the directory containing bundles, and somehow knows its name?
Comment 7 Darin Adler 2019-08-04 10:47:54 PDT
Thanks so much for tackling this! I think it will be better for the long term.
Comment 8 Eric Liang 2019-08-04 11:25:43 PDT
Thanks Darin for great suggestions! Definitely I forgot to append the webprocessloader.axbundle...
Comment 9 Eric Liang 2019-08-10 15:47:39 PDT
Created attachment 376020 [details]
Patch
Comment 10 Build Bot 2019-08-10 15:49:57 PDT
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.
Comment 11 Eric Liang 2019-08-10 15:51:57 PDT
Created attachment 376021 [details]
Patch
Comment 12 Build Bot 2019-08-10 15:54:05 PDT
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 13 Darin Adler 2019-08-10 16:20:57 PDT
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.
Comment 14 Eric Liang 2019-08-10 16:24:09 PDT
(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.
Comment 15 Eric Liang 2019-08-10 16:26:32 PDT
Created attachment 376023 [details]
Patch
Comment 16 Darin Adler 2019-08-10 16:27:58 PDT
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.
Comment 17 Build Bot 2019-08-10 16:28:59 PDT
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.
Comment 18 Eric Liang 2019-08-10 20:02:16 PDT
Created attachment 376027 [details]
Patch
Comment 19 Build Bot 2019-08-10 20:03:25 PDT
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.
Comment 20 Eric Liang 2019-08-10 20:05:49 PDT
(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
Comment 21 Eric Liang 2019-08-10 20:26:23 PDT
Created attachment 376028 [details]
Patch
Comment 22 Build Bot 2019-08-10 20:29:24 PDT
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 23 Darin Adler 2019-08-12 09:42:00 PDT
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?
Comment 24 Eric Liang 2019-08-12 20:46:47 PDT
Created attachment 376133 [details]
Patch
Comment 25 Build Bot 2019-08-12 20:48:28 PDT
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.
Comment 26 Eric Liang 2019-08-15 18:21:11 PDT
Created attachment 376460 [details]
Patch
Comment 27 Build Bot 2019-08-15 18:25:13 PDT
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 28 Build Bot 2019-08-15 21:52:42 PDT
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 29 Darin Adler 2019-08-16 09:59:32 PDT
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.
Comment 30 Eric Liang 2019-08-16 14:14:37 PDT
(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.
Comment 31 Eric Liang 2019-08-16 14:17:52 PDT
Created attachment 376539 [details]
Patch
Comment 32 Build Bot 2019-08-16 14:19:10 PDT
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 33 WebKit Commit Bot 2019-08-17 12:14:05 PDT
Comment on attachment 376539 [details]
Patch

Clearing flags on attachment: 376539

Committed r248823: <https://trac.webkit.org/changeset/248823>
Comment 34 WebKit Commit Bot 2019-08-17 12:14:07 PDT
All reviewed patches have been landed.  Closing bug.