[webkit-reviews] review granted: [Bug 203971] Cross-thread version StorageQuotaManager : [Attachment 384064] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 203971: Cross-thread version StorageQuotaManager
https://bugs.webkit.org/show_bug.cgi?id=203971

Attachment 384064: Patch

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




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

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

r=me with changes

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:701
> +    auto oldVersionOriginDirectory =
IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(origin.topOrigin,
origin.clientOrigin, rootDirectory, "v0");

"v0"_str

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:702
> +    auto newVersionOriginDirectory =
IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(origin.topOrigin,
origin.clientOrigin, rootDirectory, "v1");

"v1"_str

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:114
> +    WEBCORE_EXPORT static uint64_t diskUsage(const String&, const
ClientOrigin&);

I don't think we should omit the name of the first parameter since it is not
obvious (to me at least).

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1278
> +    

Should probably not add spaces.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:64
> +RefPtr<StorageQuotaManager> InProcessIDBServer::quotaManager(const
ClientOrigin& origin)

Why this change? It does not look like you are transferring ownership to the
caller. The caller can always ref it if it needs to but I don't think there is
any need to return a RefPtr<> here.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:80
> +    m_server = IDBServer::IDBServer::create(sessionID, [this, weakThis =
makeWeakPtr(*this)](const ClientOrigin& origin, uint64_t spaceRequested)
mutable {

I think this should stay in the initializer list to that the member can stay a
Ref<>. You can always add a utility function to construct the IDBServer and
call it in the initializer list so that it looks nice.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:91
> +    m_server = IDBServer::IDBServer::create(sessionID,
databaseDirectoryPath, [this, weakThis = makeWeakPtr(*this)](const
ClientOrigin& origin, uint64_t spaceRequested) mutable {

ditto.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.h:66
> +    IDBServer::IDBServer& server() { return *m_server; }

This does not looks super safe.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.h:67
> +    IDBServer::IDBServer& idbServer() { return *m_server; }

Not quite sure why we have 2 differently named methods returning the same thing
but technically this is not new in your patch. Seems very odd though...

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.h:133
> +    RefPtr<IDBServer::IDBServer> m_server;

I think this should stay a Ref<> since it cannot be null. It would make your
getters look safer above.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:461
> +    auto [iter, isNewEntry] =
m_sessionStorageQuotaManagers.ensure(sessionID, [defaultQuota,
defaultThirdPartyQuota, cacheRootPath] {

&cacheRootPath

> Source/WebKit/NetworkProcess/NetworkProcess.h:477
> +	   RefPtr<WebCore::StorageQuotaManager>
ensureOriginStorageQuotaManager(WebCore::ClientOrigin origin, uint64_t quota,
WebCore::StorageQuotaManager::UsageGetter&& usageGetter,
WebCore::StorageQuotaManager::QuotaIncreaseRequester&& quotaIncreaseRequester)

Seems like this should return a Ref<>, not a RefPtr<>