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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 7 11:53:50 PDT 2018

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

Attachment 339735: Patch


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

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
> +   
: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 =

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;


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

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

More information about the webkit-reviews mailing list