[webkit-reviews] review granted: [Bug 234925] Manage IndexedDB storage by origin : [Attachment 451054] 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 234925: Manage IndexedDB storage by origin
https://bugs.webkit.org/show_bug.cgi?id=234925

Attachment 451054: Patch

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




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

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

r=me with fixes.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:51
> +	   m_manager = WeakPtr { *manager };

WeakPtr { } is not needed, you can just assign `*manager`.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2212
> +    if (auto* storageManager = session->storageManager())

You are failing to call the completion handler in the else case.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:248
> +    if (auto* storageManager = session->storageManager())

Callback is not getting called in the else case.

> Source/WebKit/NetworkProcess/storage/IDBStorageManager.cpp:45
> +    for (auto name : fileNames) {

auto&, assuming this is a String.

> Source/WebKit/NetworkProcess/storage/IDBStorageManager.cpp:244
> +    if (identifier.isTransient())

Probably can merge with the previous if condition given that the return is the
same.

> Source/WebKit/NetworkProcess/storage/IDBStorageManager.h:53
> +    ~IDBStorageManager() final { }

Seem we shouldn't need this?

> Source/WebKit/NetworkProcess/storage/IDBStorageRegistry.h:47
> +    WebCore::IDBServer::IDBConnectionToClient&
getOrCreateConnectionToClient(IPC::Connection::UniqueID,
WebCore::IDBConnectionIdentifier);

Maybe ensureConnectionToClient() would look better?

> Source/WebKit/NetworkProcess/storage/IDBStorageRegistry.h:51
> +    WebCore::IDBServer::UniqueIDBDatabaseConnection* getConnection(uint64_t
identifier);

No "get" prefixes in getters.

> Source/WebKit/NetworkProcess/storage/IDBStorageRegistry.h:54
> +    WebCore::IDBServer::UniqueIDBDatabaseTransaction*
getTransaction(WebCore::IDBResourceIdentifier);

ditto.

> Source/WebKit/NetworkProcess/storage/QuotaManager.cpp:59
> +    auto clearIsHandlingRequests = makeScopeExit([&] {

I think this is a clear use case for SetForScope?

SetForScope<bool> isHandlingRequests(m_isHandlingRequests, true);

> Source/WebKit/NetworkProcess/storage/QuotaManager.cpp:83
> +    bool shouldUpdateQuotaBasedonUsage = !m_usage;

OnUsage (case issue)

> Source/WebKit/NetworkProcess/storage/QuotaManager.cpp:125
> +    m_quotaCountdown = 0;

Maybe this could call resetQuotaForTesting()?

> Source/WebKit/NetworkProcess/storage/QuotaManager.h:41
> +    enum class Decision { Deny, Grant };

enum class Decision : bool { Deny, Grant };

> Tools/ChangeLog:17
> +2022-02-03  Sihui Liu  <sihui_liu at apple.com>

Double changelog issue.