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

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has asked  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 #24 from youenn fablet <youennf at gmail.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

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1313
> +    auto callbackTuple =
m_requestStorageAccessCallbacks.take(callbackIdentifier);

We often exit early if there is no callbakIdentifier in the map.
Might use find/remove(iterator)

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:366
> +    Lock m_requestStorageAccessCallbackLock;

Do we need this lock, maybe all calls are main thread only?

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

Since this is a map of CompletionHandler, you might need to call all callbacks
in WebChromeClient destructor.
Probably my preference only, but defining a struct instead of a tuple gives
usually more readable code to me.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1056
> +    void requestStorageAccess(String&& subFrameHost, String&& topFrameHost,
uint64_t frameID, uint64_t pageID, WTF::CompletionHandler<void(bool)>&&
callback);

Might be able to remove WTF::

> Source/WebKit/WebProcess/WebProcess.cpp:1693
> +void WebProcess::didRequestStorageAccessConfirm(uint64_t frameID, uint64_t
pageID, uint64_t callbackIdentifier, bool confirmed)

s/confirmed/didConfirm?

> Source/WebKit/WebProcess/WebProcess.h:322
> +    void didRequestStorageAccessConfirm(uint64_t frameID, uint64_t pageID,
uint64_t callbackIdentifier, bool hasActivity);

s/hasActivity/didConfirm?