[webkit-reviews] review granted: [Bug 180722] REGRESSION (r225789): API tests WKProcessPool.InitialWarmedProcessUsed and WebKit.WebsiteDataStoreCustomPaths are failing : [Attachment 329225] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 180722: REGRESSION (r225789): API tests
WKProcessPool.InitialWarmedProcessUsed and WebKit.WebsiteDataStoreCustomPaths
are failing
https://bugs.webkit.org/show_bug.cgi?id=180722

Attachment 329225: Patch

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




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

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

r=me with comments.

> Source/WebKit/StorageProcess/StorageProcess.cpp:437
> +	  
parentProcessConnection()->send(Messages::StorageProcessProxy::EstablishWorkerC
ontextConnectionToStorageProcessForExplicitSession(*sessionID), 0);

There is no reason we cannot have a single IPC message that takes in a
std::optional<PAL::SessionID>, is there?

> Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm:447
> +    bool serviceWorkerProcessWasFound = false;

I think this whole block should be protected by #if !ASSERT_DISABLED.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:60
> +    static NeverDestroyed<HashMap<PAL::SessionID, WebsiteDataStore*>> map;

We may want to add a RELEASE_ASSERT to make sure this is always called on the
main thread. I know how clients like to use our API from various threads and
such HashMap could get corrupted.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:99
> +    ASSERT(allDataStores().get(m_sessionID) == this);

remove() returns a boolean, I would have been more efficient to
ASSERT_UNUSED(wasRemoved, wasRemoved); after the remove() call.