[webkit-reviews] review denied: [Bug 186903] Resource Load Statistics: Make WebResourceLoadStatisticsStore::updateCookiePartitioningForDomains() wait for the network process before calling its callback : [Attachment 343389] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied John Wilander <wilander at apple.com>'s
request for review:
Bug 186903: Resource Load Statistics: Make
WebResourceLoadStatisticsStore::updateCookiePartitioningForDomains() wait for
the network process before calling its callback
https://bugs.webkit.org/show_bug.cgi?id=186903

Attachment 343389: Patch

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




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

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:406
> +void
NetworkProcess::updatePrevalentDomainsToPartitionOrBlockCookies(PAL::SessionID
sessionID, const Vector<String>& domainsToPartition, const Vector<String>&
domainsToBlock, const Vector<String>& domainsToNeitherPartitionNorBlock, bool
shouldClearFirst, uint64_t contextId)

We usually call these callbackID, not contextId.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:410
> +   
parentProcessConnection()->send(Messages::NetworkProcessProxy::UpdatePartitionO
rBlockCookiesDone(contextId), 0);

We would normally call this DidUpdatePartitionOrBlockCookies

> Source/WebKit/NetworkProcess/NetworkProcess.h:137
> +    void updatePrevalentDomainsToPartitionOrBlockCookies(PAL::SessionID,
const Vector<String>& domainsToPartition, const Vector<String>& domainsToBlock,
const Vector<String>& domainsToNeitherPartitionNorBlock, bool shouldClearFirst,
uint64_t contextId);

ditto.

> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:84
> +    UpdatePrevalentDomainsToPartitionOrBlockCookies(PAL::SessionID
sessionID, Vector<String> domainsToPartition, Vector<String> domainsToBlock,
Vector<String> domainsToNeitherPartitionNorBlock, bool shouldClearFirst,
uint64_t contextId)

We usually call these callbackID, not contextId.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:434
> +static uint64_t nextUpdatePartitionOrBlockCookiesContextId()

Not needed, please rely on the existing generateCallbackID() in this file.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:447
> +    auto contextId = nextUpdatePartitionOrBlockCookiesContextId();

Please use generateCallbackID() and call this variable callbackID everywhere.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:453
> +void NetworkProcessProxy::updatePartitionOrBlockCookiesDone(uint64_t
contextId)

callbackID

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:455
> +    auto callback =
m_updatePartitionOrBlockCookiesCallbackMap.take(contextId);

In NetworkProcessProxy::clearCallbackStates(), please make sure you call your
completion handlers (This is called when the network process crashes during a
request).