[webkit-reviews] review denied: [Bug 185335] Add experimental feature to prompt for Storage Access API use : [Attachment 339659] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 185335: Add experimental feature to prompt for Storage Access API use
https://bugs.webkit.org/show_bug.cgi?id=185335

Attachment 339659: Patch

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




--- Comment #14 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 339659
  --> https://bugs.webkit.org/attachment.cgi?id=339659
Patch

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

> Source/WebKit/UIProcess/API/APIUIClient.h:131
> +    virtual void requestStorageAccessConfirm(WebKit::WebPageProxy*,
WebKit::WebFrameProxy*, const WTF::String& requestingDomain, const WTF::String&
currentDomain, Function<void (bool)>&& completionHandler) {
completionHandler(true); }

I think we should make the parameters a WebPageProxy& and a WebFrameProxy&

> Source/WebKit/UIProcess/API/C/WKPage.cpp:1535
> +    Function<void (bool)> m_completionHandler;

This and all uses of Function in this patch could be a CompletionHandler.
I also think we don't have a space between void and (bool) in our style.

> Source/WebKit/UIProcess/API/C/WKPage.cpp:2028
> +	       RefPtr<RequestStorageAccessConfirmResultListener> listener =
RequestStorageAccessConfirmResultListener::create(WTFMove(completionHandler));

auto listener

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:119
> +- (void)webView:(WKWebView *)webView
requestStorageAccessPanelForDomain:(NSString *)requestingDomain
underCurrentDomain:(NSString *)currentDomain completionHandler:(void (^)(BOOL
result))completionHandler;

webView should be _webView because this is SPI

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:345
> +    RefPtr<CompletionHandlerCallChecker> checker =
CompletionHandlerCallChecker::create(delegate.get(),
@selector(webView:requestStorageAccessPanelForDomain:underCurrentDomain:complet
ionHandler:));

auto checker

> Source/WebKit/UIProcess/WebPageProxy.cpp:4292
> +    WTFLogAlways("ResourceLoadLog |
WebPageProxy::requestStorageAccessConfirm().");

Please remove

> Source/WebKit/UIProcess/WebPreferences.cpp:137
> +    if (key == WebPreferencesKey::storageAccessPromptsEnabledKey())

This feels out of place because it's the only such thing here.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1292
>  void WebChromeClient::requestStorageAccess(String&& subFrameHost, String&&
topFrameHost, uint64_t frameID, uint64_t pageID, WTF::CompletionHandler<void
(bool)>&& callback)

We can remove WTF:: here

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1295
> +    if
(!WebProcess::singleton().parentProcessConnection()->sendSync(Messages::WebPage
Proxy::RequestStorageAccessConfirm(frameID, subFrameHost, topFrameHost),
Messages::WebPageProxy::RequestStorageAccessConfirm::Reply(result), pageID,
Seconds::infinity(), IPC::SendSyncOption::InformPlatformProcessWillSuspend) ||
!result) {

I don't think we should add synchronous IPC for this.  I think we should have a
different message type for the reply, and store the CompletionHandler in a
HashMap with an identifier so we don't hang the WebProcess during this.