[webkit-reviews] review denied: [Bug 200367] AX: don't hard code webprocessloader bundle path : [Attachment 375364] Patch

[bugzilla-daemon at webkit.org]

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

Attachment 375364: Patch

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




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