In https://bugs.webkit.org/show_bug.cgi?id=181073, WebKit added the following WKNavigationDelegate SPI: - (_WKPluginModuleLoadPolicy)_webView:(WKWebView *)webView decidePolicyForPluginLoadWithCurrentPolicy:(_WKPluginModuleLoadPolicy)policy pluginInfo:(NSDictionary *)info unavailabilityDescription:(NSString *)unavailabilityDescription API_AVAILABLE(macosx(10.14)); One intent of this SPI was to allow clients to return a custom unavailabilityDescription, but they can't do that currently because this parameter is an NSString * instead of an NSString **.
<rdar://problem/43534382>
The original bug was actually https://bugs.webkit.org/show_bug.cgi?id=181413.
Created attachment 347586 [details] Patch
Comment on attachment 347586 [details] Patch Give it a CompletionHandler that has two parameters.
That makes much more sense than blindly following the old SPI. Will do.
I'm not sure how to implement your suggestion easily. WebPage::createPlugin() in the web process sends a synchronous IPC message that's handled by WebPageProxy::findPlugin() in the UI process, which ends up calling the navigation delegate synchronously. If we wanted to make the navigation delegate method asynchronous with a completion handler, I think this would require rearchitecting how plug-ins are created in the web process.
I followed the chain back from WebPage::createPlugin() a bit more in the WebProcess, and the fact that plugin creation is synchronous seems pretty integral. I don't think I should attempt to change this. In theory, I guess we could still use a CompletionHandler with the WKNavigationDelegate method, with an implied contract that the client has to call the completion handler before returning from the method so WebPageProxy::findPlugin() can return synchronously, but that seems like a clear antipattern to me. I'm going to reupload my original patch for review.
Created attachment 347651 [details] Patch
Comment on attachment 347651 [details] Patch If you add "Delayed" after FindPlugin(...)->(...) in WebPageProxy.messages.in you should be able to accomplish this the right way. This way causes problems with arc.
That's the bit I was missing, thanks. I have a working patch.
Created attachment 347713 [details] Patch
Comment on attachment 347713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347713&action=review Looks much better! > Source/WebKit/UIProcess/API/APINavigationClient.h:120 > + virtual void decidePolicyForPluginLoad(WebKit::WebPageProxy&, WebKit::PluginModuleLoadPolicy currentPluginLoadPolicy, Dictionary&, WTF::Function<void(WebKit::PluginModuleLoadPolicy, const WTF::String&)>&& completionHandler) WTF::Function -> CompletionHandler here and everywhere.
Created attachment 347726 [details] Patch
Comment on attachment 347726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347726&action=review > Source/WebKit/UIProcess/API/APINavigationClient.h:122 > + completionHandler(currentPluginLoadPolicy, WTF::String()); { } here and elsewhere where we create an empty String. The type isn't important. The fact that we have nothing is all we need to represent in the code. > Source/WebKit/UIProcess/Cocoa/NavigationState.mm:374 > + completionHandler(pluginModuleLoadPolicy(policy), String(unavailabilityDescription)); String has a non-explicit NSString* constructor, so you shouldn't need the "String(" and ")" > Source/WebKit/UIProcess/WebPageProxy.cpp:2168 > + auto findPluginCompletion = [processType, &reply, &newMimeType, &plugin] (uint32_t pluginLoadPolicy, const String& unavailabilityDescription) { reply = WTFMove(reply), newMimeType = WTFMove(newMimeType), plugin = WTFMove(plugin) It is unsafe to capture pointers to stack variables in a lambda that might be called after this stack frame returns. > Source/WebKit/UIProcess/WebPageProxy.cpp:2190 > + m_navigationClient->decidePolicyForPluginLoad(*this, static_cast<PluginModuleLoadPolicy>(pluginLoadPolicy), pluginInformation.get(), [findPluginCompletion = WTFMove(findPluginCompletion)] (PluginModuleLoadPolicy pluginLoadPolicy, const String& unavailabilityDescription) mutable { You should just be able to WTFMove(findPluginCompletion) into the function call as a parameter instead of creating a lambda does nothing but call it. > Source/WebKit/UIProcess/WebPageProxy.cpp:2195 > pluginLoadPolicy = m_loaderClient->pluginLoadPolicy(*this, static_cast<PluginModuleLoadPolicy>(pluginLoadPolicy), pluginInformation.get(), unavailabilityDescription); It would probably be cleaner if you passed the CompletionHandler to the loader client, too.
Comment on attachment 347726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347726&action=review >> Source/WebKit/UIProcess/API/APINavigationClient.h:122 >> + completionHandler(currentPluginLoadPolicy, WTF::String()); > > { } here and elsewhere where we create an empty String. The type isn't important. The fact that we have nothing is all we need to represent in the code. Done. >> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:374 >> + completionHandler(pluginModuleLoadPolicy(policy), String(unavailabilityDescription)); > > String has a non-explicit NSString* constructor, so you shouldn't need the "String(" and ")" I confirmed that works, done. >> Source/WebKit/UIProcess/WebPageProxy.cpp:2168 >> + auto findPluginCompletion = [processType, &reply, &newMimeType, &plugin] (uint32_t pluginLoadPolicy, const String& unavailabilityDescription) { > > reply = WTFMove(reply), newMimeType = WTFMove(newMimeType), plugin = WTFMove(plugin) > It is unsafe to capture pointers to stack variables in a lambda that might be called after this stack frame returns. Fixed. I had to move the initialization of pluginInformation before the lambda, since it depends on plugin. >> Source/WebKit/UIProcess/WebPageProxy.cpp:2190 >> + m_navigationClient->decidePolicyForPluginLoad(*this, static_cast<PluginModuleLoadPolicy>(pluginLoadPolicy), pluginInformation.get(), [findPluginCompletion = WTFMove(findPluginCompletion)] (PluginModuleLoadPolicy pluginLoadPolicy, const String& unavailabilityDescription) mutable { > > You should just be able to WTFMove(findPluginCompletion) into the function call as a parameter instead of creating a lambda does nothing but call it. Nice optimization, fixed. >> Source/WebKit/UIProcess/WebPageProxy.cpp:2195 >> pluginLoadPolicy = m_loaderClient->pluginLoadPolicy(*this, static_cast<PluginModuleLoadPolicy>(pluginLoadPolicy), pluginInformation.get(), unavailabilityDescription); > > It would probably be cleaner if you passed the CompletionHandler to the loader client, too. Fixed.
Created attachment 347816 [details] Patch
Comment on attachment 347816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347816&action=review > Source/WebKit/UIProcess/Cocoa/NavigationState.mm:366 > + completionHandler(currentPluginLoadPolicy, String()); { } > Source/WebKit/UIProcess/Cocoa/NavigationState.mm:373 > + [(id <WKNavigationDelegatePrivate>)navigationDelegate _webView:m_navigationState.m_webView decidePolicyForPluginLoadWithCurrentPolicy:wkPluginModuleLoadPolicy(currentPluginLoadPolicy) pluginInfo:wrapper(pluginInformation) completionHandler:^(_WKPluginModuleLoadPolicy policy, NSString *unavailabilityDescription) { I'm surprised this compiles without using BlockPtr because CompletionHandler is not copyable and blocks are.
Comment on attachment 347816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347816&action=review >> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:366 >> + completionHandler(currentPluginLoadPolicy, String()); > > { } Oops, missed this. Fixed. >> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:373 >> + [(id <WKNavigationDelegatePrivate>)navigationDelegate _webView:m_navigationState.m_webView decidePolicyForPluginLoadWithCurrentPolicy:wkPluginModuleLoadPolicy(currentPluginLoadPolicy) pluginInfo:wrapper(pluginInformation) completionHandler:^(_WKPluginModuleLoadPolicy policy, NSString *unavailabilityDescription) { > > I'm surprised this compiles without using BlockPtr because CompletionHandler is not copyable and blocks are. Interesting. I'll add this. It also occurred to me that I should use a CompletionHandlerCallChecker here as well.
https://trac.webkit.org/changeset/235200/webkit
Comment on attachment 347816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347816&action=review >>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:366 >>> + completionHandler(currentPluginLoadPolicy, String()); >> >> { } > > Oops, missed this. Fixed. This is missing a return statement.. > Source/WebKit/UIProcess/Cocoa/NavigationState.mm:370 > + completionHandler(currentPluginLoadPolicy, { }); ditto.. -> https://bugs.webkit.org/show_bug.cgi?id=191753