[webkit-reviews] review granted: [Bug 176941] Storage Access API: Web process should ask UI process for grant/deny : [Attachment 322724] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 5 10:37:50 PDT 2017


Chris Dumez <cdumez at apple.com> has granted John Wilander <wilander at apple.com>'s
request for review:
Bug 176941: Storage Access API: Web process should ask UI process for
grant/deny
https://bugs.webkit.org/show_bug.cgi?id=176941

Attachment 322724: Patch

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




--- Comment #9 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 322724
  --> https://bugs.webkit.org/attachment.cgi?id=322724
Patch

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

r=me with comments

> Source/WebCore/dom/Document.cpp:7338
> +	   page->chrome().client().requestStorageAccess(WTFMove(iframeHost),
WTFMove(topHost), [this, promise] (bool granted) {

Should the lambda capture a weakPtr to the Document? What guarantees the
document is still alive when the lambda is called asynchronously?

> Source/WebCore/page/ChromeClient.h:467
> +    virtual void requestStorageAccess(String&& /*subFrameHost*/, String&&
/*topFrameHost*/, WTF::Function<void (bool)>&& /*callback*/) { }

The default implementation should call the callback to avoid hangs.

> Source/WebKit/UIProcess/WebPageProxy.cpp:7045
> +    m_websiteDataStore->requestStorageAccess(WTFMove(subFrameHost),
WTFMove(topFrameHost), [this, webProcessContextId] (bool granted) {

We need a is/was/has prefix for boolean parameters as per coding style. I would
rename granted to wasGranted everywhere.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1373
> +	   return;

You need to call the callback before returning to avoid hangs.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6053
> +    m_storageAccessResponseCallbackMap.set(contextId, WTFMove(callback));

Should probably be a add(), not a set().
Should can also assert that addResult.isNewEntry to be safe.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6060
> +    if (callback)

Shouldn't this be an assertion?


More information about the webkit-reviews mailing list