[webkit-reviews] review granted: [Bug 234087] Merge StorageManager with NetworkStorageManager to manage WebStorage by origin : [Attachment 446838] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted  review:
Bug 234087: Merge StorageManager with NetworkStorageManager to manage
WebStorage by origin
https://bugs.webkit.org/show_bug.cgi?id=234087

Attachment 446838: Patch

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




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

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

r=me assuming the bots are happy.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:321
> +    for (auto& manager : m_storageManagers.values())

I think eventually we'll want to store the NetworkStorageManager on the
NetworkSession object and get rid of the NetworkProcess::m_storageManagers
HashMap.
Probably shouldn't be done in this already very large patch though.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1933
> +	      
iterator->value->deleteDataForRegistrableDomains(websiteDataTypes,
domainsToDeleteAllNonCookieWebsiteDataFor, [callbackAggregator](auto
deletedDomains) mutable {

auto&&

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1934
> +		   for (auto domain : deletedDomains)

auto&

> Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:144
> +	   m_storageArea =
makeUnique<SQLiteStorageArea>(localStorageQuotaInBytes, origin, path, Ref {
*workQueue });

Unnecessary ref counting churn, you want `workQueue.releaseNonNull()`.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:426
> +	       if (openingOrigin.startsWith("."))

openingOrigin.startsWith('.') is more efficient here.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:457
> +    for (auto origin : getAllOrigins()) {

auto&

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:491
> +    for (auto origin : getAllOrigins()) {

auto&

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:148
> +	       return file != ".DS_Store"_s && file != originFileName;

Probably want a #if PLATFORM(COCOA) for the .DS_Store check.

> Source/WebKit/NetworkProcess/storage/SessionStorageManager.h:59
> +

extra lines