[webkit-reviews] review denied: [Bug 185335] Add experimental feature to prompt for Storage Access API use : [Attachment 339697] 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 339697: Patch

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




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

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

Better, but still some substantial changes needed.

> 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); }

This Function could be a CompletionHandler

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

auto listener.	Really.  This would make the code cleaner, and make listener a
ref, and you would need to call listener.ptr() below.

> Source/WebKit/UIProcess/Cocoa/UIDelegate.h:91
> +	   void requestStorageAccessConfirm(WebPageProxy&, WebFrameProxy&,
const WTF::String& requestingDomain, const WTF::String& currentDomain,
Function<void(bool)>&& completionHandler) final;

CompletionHandler

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1294
> +    static std::atomic<uint64_t> currentHandlerIdentifier;

Can this be accessed by more than one thread?  If not (and I think the answer
is no) then this doesn't need to be atomic and we don't need
m_requestStorageAccessCallbackLock.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:367
> +    HashMap<uint64_t, std::tuple<String, String,
CompletionHandler<void(bool)>>> m_requestStorageAccessCallbacks;

You can just make the value of this HashMap a CompletionHandler and capture
subFrameHost and topFrameHost and the given CompletionHandler in a lambda.