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

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted 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 339735: Patch

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




--- Comment #28 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 339735
  --> https://bugs.webkit.org/attachment.cgi?id=339735
Patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1297
> +    uint64_t identifier = ++currentHandlerIdentifier;

We now usually try to use typed identifiers using ObjectIdentifier<>.
This allows making sure we are not taking an identifier for another.
Would be good to be done in a follow-up especially as we are handling 3
uint64_t here.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1299
> +    auto result = m_requestStorageAccessCallbacks.ensure(identifier, [this,
subFrameHost, topFrameHost, frameID, pageID, completionHandler =
WTFMove(completionHandler)]() mutable {

No need for ensure since identifier is unique.
m_requestStorageAccessCallbacks.add should be good enough.
You can do ASSERT(!m_requestStorageAccessCallbacks.contains(identifier)) maybe.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1311
> +   
WebProcess::singleton().parentProcessConnection()->send(Messages::WebPageProxy:
:RequestStorageAccessConfirm(frameID, pageID, subFrameHost, topFrameHost,
identifier), m_page.pageID());

Can we have a case where pageID != m_page.pageID?
If not, maybe we should not pass pageID as a method parameter.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1318
> +    auto currentCallbackIter =
m_requestStorageAccessCallbacks.find(callbackIdentifier);

s/Iter/Iterator maybe

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

s/hasActivity/confirmedAccess.

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

We probably need to call all completion handlers of
m_requestStorageAccessCallbacks at destruction of WebChromeClient.
Otherwise, we might hit debug assertions that the completion handlers were not
called.