[webkit-reviews] review denied: [Bug 186166] Make NetworkProcess get cache storage parameters at creation of the CacheStorage engine : [Attachment 341762] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied youenn fablet
<youennf at gmail.com>'s request for review:
Bug 186166: Make NetworkProcess get cache storage parameters at creation of the
CacheStorage engine
https://bugs.webkit.org/show_bug.cgi?id=186166

Attachment 341762: Patch

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




--- Comment #4 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 341762
  --> https://bugs.webkit.org/attachment.cgi?id=341762
Patch

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

Is it common to load the first page without initializing a cache storage engine
at all?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:855
> +void NetworkProcess::cacheStorageParameters(PAL::SessionID sessionID,
CacheStorageParametersCallback&& callback)

This seems to ask the UIProcess every time this is called unless it is called
more than once before we get a response.  Do we store the result anywhere?  A
cacheStorageParameters cache if you will...

if (we already have parameters)
    call completion handler immediately
else
    retrieve cache storage parameters

Then store them on NetworkProcess in NetworkProcess::setCacheStorageParameters

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:582
> +	   m_websiteDataStores.set(store->sessionID(), WTFMove(store));

Parameter evaluation order is undefined.  We need to get the sessionID before
moving from store.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:590
> +    if (!sessionID.isEphemeral())
> +	   m_websiteDataStores.remove(sessionID);

Don't we want to remove ephemeral sessions, too?