[webkit-reviews] review denied: [Bug 188764] WKNavigationDelegate needs to allow clients to specify a custom blocked plug-in message : [Attachment 347726] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Jeff Miller
<jeffm at apple.com>'s request for review:
Bug 188764: WKNavigationDelegate needs to allow clients to specify a custom
blocked plug-in message
https://bugs.webkit.org/show_bug.cgi?id=188764

Attachment 347726: Patch

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




--- Comment #14 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 347726
  --> https://bugs.webkit.org/attachment.cgi?id=347726
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.