[webkit-reviews] review granted: [Bug 180728] Storage Access API: Make DocumentLoader::willSendRequest() and WebFrameLoaderClient::detachedFromParent2() tell the network process to get rid of any sub frame access entries : [Attachment 329994] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 180728: Storage Access API: Make DocumentLoader::willSendRequest() and
WebFrameLoaderClient::detachedFromParent2() tell the network process to get rid
of any sub frame access entries
https://bugs.webkit.org/show_bug.cgi?id=180728

Attachment 329994: Patch

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




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

r=me if bots are happy.
It seems http/tests/storageAccess/request-storage-access-top-frame.html has
some issues with Mac-wk2

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

> Source/WebCore/dom/Document.cpp:7560
> +void Document::setHasFrameSpecificStorageAccess(bool value)

If we add this setter, it might be better to add an
hasFrameSpecificStorageAccess getter as well.
I also wonder whether setHasFrameSpecificStorageAccess is called in code that
is not #ifdef by VE(CFNETWORK_STORAGE_PARTITIONING).
In that case, maybe the whole Document::setHasFrameSpecificStorageAccess should
be #ifdef.

> Source/WebCore/loader/DocumentLoader.cpp:538
> +	   frameLoader()->client().dispatchWillChangeDocument();

If there is no specific reason to call dispatchWillChangeDocument after the
blocker checks, I would suggest adding this call this in startMainResourceLoad.

> Source/WebKit/UIProcess/WebPageProxy.h:1248
>      void hasStorageAccess(String&& subFrameHost, String&& topFrameHost,
uint64_t frameID, uint64_t pageID, uint64_t webProcessContextId);

It seems strange to pass r-values for a method called hasXX.
Looking at the implementation, they can probably be changed to const String&.
For instance, Messages::WebPageProxy::HasStorageAccess is taking a const
String&, not a String.
Ditto for requestStorageAccess.
Might be done as a follow-up or in this patch.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1207
> +void WebsiteDataStore::removeStorageAccess(uint64_t frameID, uint64_t
pageID)

I would have expected to have pageID before frameID, since frameID is specific
to its pageID.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1210
> +	   processPool->networkProcess()->removeStorageAccess(m_sessionID,
frameID, pageID);

This seems somehow strange that we go from WebProcess to UIProcess to
NetworkProcess.
Why cannot we go from WebProcess to its NetworkProcess to remove the storage
access there.
If needed, the WebProcess can also update the websitedatastore resource load
statistics with another IPC call from WebProcess to UIProcess.
Is that possible to do that instead?