[webkit-reviews] review granted: [Bug 209192] Add WKUIDelegatePrivate SPI _webView:printFrame:completionHandler: : [Attachment 393777] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 209192: Add WKUIDelegatePrivate SPI _webView:printFrame:completionHandler:
https://bugs.webkit.org/show_bug.cgi?id=209192

Attachment 393777: Patch

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




--- Comment #3 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 393777
  --> https://bugs.webkit.org/attachment.cgi?id=393777
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393777&action=review

r=me

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:1099
> +    auto handle = API::FrameHandle::create(webFrameProxy.frameID());
> +    if (m_uiDelegate.m_delegateMethods.webViewPrintFrame) {
> +	   [(id <WKUIDelegatePrivate>)delegate _webView:m_uiDelegate.m_webView
printFrame:wrapper(handle)];
> +	   completionHandler();
> +    } else if
(m_uiDelegate.m_delegateMethods.webViewPrintFrameCompletionHandler) {
> +	   auto checker = CompletionHandlerCallChecker::create(delegate.get(),
@selector(_webView:printFrame:completionHandler:));
> +	   [(id <WKUIDelegatePrivate>)delegate _webView:m_uiDelegate.m_webView
printFrame:wrapper(handle) completionHandler:makeBlockPtr([checker =
WTFMove(checker), completionHandler = WTFMove(completionHandler)] () mutable {
> +	       if (checker->completionHandlerHasBeenCalled())
> +		   return;
> +	       checker->didCallCompletionHandler();
> +	       completionHandler();
> +	   }).get()];
> +    } else
> +	   completionHandler();

I think you want to do the delegate method check in the opposite order, so the
new API wins if implemented. That way, a client can implement both if they need
to support older WebKits, without doing #ifdef stuff.