[webkit-reviews] review granted: [Bug 197636] Move Web Storage to Network Process : [Attachment 370172] 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 197636: Move Web Storage to Network Process
https://bugs.webkit.org/show_bug.cgi?id=197636

Attachment 370172: Patch

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




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

LGTM, a few more comments.


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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1029
> +		   return
RegistrableDomain::uncheckedCreateFromHost(origin.host) == domain;

return domain.matches(origin);

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2692
> +void NetworkProcess::getLocalStorageOriginDetails(PAL::SessionID sessionID,
CompletionHandler<void(Vector<LocalStorageDatabaseTracker::OriginDetails>)>&&
completionHandler)

CompletionHandler should probably take a Vector<>&&

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2702
> +	   completionHandler(details);

WTFMove(details).

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:366
> +    return storageArea;

We could use HashMap.ensure instead.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:384
> +	   if (originAndStorageArea.key == securityOrigin)

Shouldn't we just use m_storageAraeMap.find(securityOrigin)?

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:426
> +		   storageArea->clear();

Ditto, m_storageAreaMap is keyed by SecurityOriginData

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:472
> +    return *m_storageAreaMap.ensure(securityOrigin, [this, securityOrigin]()
mutable {

s/[this, securityOrigin]/[this, &securityOrigin]/
s/*m_storageAreaMap/m_storageAreaMap/

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:727
> +    ASSERT(!RunLoop::isMain());

This is good for now.
In the future, we could do something like:
ASSERT(m_queue.isCurrent())
On GLib, it would check the run loop.
On Cocoa, it would use dispatch_assert_queue.
Can we file a bugzilla for that and add a FIXME.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:731
> +    // FIXME: This should be a message check.

Can we file a bugzilla for these?

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:970
> +	   slot = LocalStorageNamespace::create(*this, storageNamespaceID);

We could use ensure() here and below

> Source/WebKit/UIProcess/API/C/WKKeyValueStorageManager.cpp:67
> +    websiteDataStore.fetchData({ WebKit::WebsiteDataType::LocalStorage,
WebKit::WebsiteDataType::SessionStorage }, { }, [context, callback](auto
dataRecords) {

Is WebKit:: needed, here and below?

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:208
> +    dataStores().removeFirst(this);

This is fragile, dataStores() should probably be a HashSet if we want to be
sure to only include it once and order is not important.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:240
> +    // FIXME: make the handling for isNonPersistentStore be consistent with
data types above.

Bugzilla maybe.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-1812
> -    });

There is a risk that completionHandler is not called.
Let's make sure this is not the case even if this is only testing code.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2082
> +void
WebsiteDataStore::getLocalStorageDetails(Function<void(Vector<LocalStorageDatab
aseTracker::OriginDetails>)>&& completionHandler)

Ideally, this would take a Vector<>&&, if not too much work here or as a
follow-up

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2091
> +	   processPool->networkProcess()->getLocalStorageDetails(m_sessionID,
[completionHandler = WTFMove(completionHandler)](auto details) {

We should only move completionHandler once.
If we need to support multiple pools, we need to consolidate the results
ourselves here.
Please fix this before landing.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1406
> +    PAL::SessionID session(sessionID());

auto sessionID = this->sessionID();

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1434
> +    WebProcess::singleton().removeWebPage(session, m_pageID);

s/session/sessionID/

> Source/WebKit/WebProcess/WebProcess.cpp:1253
> +	   // To recover web storage.

This is not great but is better than nothing.
Let's file a bugzilla and improve that as soon as possible after landing.

> Source/WebKit/WebProcess/WebProcess.cpp:1686
> +	   if (page.value)

Do we need that check?
I would believe enablePrivateBrowsingForTesting to not be called in the middle
of a WebPage creation.