[webkit-reviews] review granted: [Bug 198090] API Test landed in r245540 [Mac WK2] TestWebKitAPI.WKWebView.LocalStorageProcessCrashes is a flaky failure : [Attachment 370439] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 198090: API Test landed in r245540 [Mac WK2]
TestWebKitAPI.WKWebView.LocalStorageProcessCrashes is a flaky failure
https://bugs.webkit.org/show_bug.cgi?id=198090

Attachment 370439: Patch

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




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

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

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:741
> +    m_queue->dispatch([this, protectedThis = makeRef(*this), &connection,
storageMapID, storageNamespaceID, topLevelOriginData =
topLevelOriginData.isolatedCopy(), origin = origin.isolatedCopy()]() mutable {

s/&connection/connectionID = connection->uniqueID()/

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:839
> +void StorageManager::didGetValues(IPC::Connection& connection, uint64_t
storageMapID, const HashMap<String, String>& items, GetValuesCallback&&
completionHandler)

No need for connection.
No need for this as well, this can be a static free function.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:859
> +	  
connection->send(Messages::StorageAreaMap::DidGetValues(storageMapSeed),
storageMapID);

This is a strange design to have both a completion handler and sending in some
cases another IPC message.
It might be interesting to try simplifying this in a follow-up patch.

For instance, why do we not send back such message in the ephemeral case?
Or could we not send back storageMapSeed in the completion handler as an
optional value?

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:925
> +	   connection->send(Messages::StorageAreaMap::DidClear(storageMapSeed),
storageMapID);

It seems strange to not send back StorageAreaMap::DidClear in the ephemeral
case since we are clearing some data apparently.
Even in the case where the page has been closed, it seems good practice to send
back some information that the task is completed.
We could modernize it with Async IPC reply, in which case we would need to call
the completion handler in every case.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.h:46
> +typedef CompletionHandler<void(const HashMap<String, String>&)>
GetValuesCallback;

"using" is the preferred way.