[webkit-reviews] review granted: [Bug 234179] Move SWServers from NetworkProcess to NetworkSession : [Attachment 446909] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 234179: Move SWServers from NetworkProcess to NetworkSession
https://bugs.webkit.org/show_bug.cgi?id=234179

Attachment 446909: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 446909
  --> https://bugs.webkit.org/attachment.cgi?id=446909
Patch

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:504
> +	   auto& server = session->ensureSWServer();
> +	   if (!server.isImportCompleted()) {

Scope it using the semicolon style?

    if (auto& server = session->ensureSWServer(); !server.isImportCompleted())
{

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:509
> +		   ASSERT(networkSession() && networkSession()->swServer() &&
networkSession()->swServer()->isImportCompleted());

I would write three separate assertions. Nicer to know which one failed.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1596
> +	       // If we are deleting all of the data types that the resource
load statistics store monitors
> +	       // we do not need to re-grandfather old data.
> +	       auto shouldGrandfather = ((monitoredTypesRaw & deletedTypesRaw)
== monitoredTypesRaw) ? ShouldGrandfatherStatistics::No :
ShouldGrandfatherStatistics::Yes;

The enum makes this harder to read. I hope there’s other code that makes it
easier to read.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1664
> +	   Vector<String> hosts;
> +	   hosts.reserveInitialCapacity(originDatas.size());
> +	   for (auto& origin : originDatas)
> +	       hosts.uncheckedAppend(origin.host);

Should do this more directly with Vector::map?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1849
> +	   Vector<String> registrableDomainsToDelete;
> +	  
registrableDomainsToDelete.reserveInitialCapacity(domainsToDeleteAllNonCookieWe
bsiteDataFor.size());
> +	   for (auto& domain : domainsToDeleteAllNonCookieWebsiteDataFor)
> +	       registrableDomainsToDelete.uncheckedAppend(domain.string());

Same question.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:553
> +    UNUSED_PARAM(critical);

Could we omit the argument name instead of using UNUSED_PARAM?