[webkit-reviews] review denied: [Bug 190598] Add a storage limit for IndexedDB : [Attachment 352925] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 190598: Add a storage limit for IndexedDB
https://bugs.webkit.org/show_bug.cgi?id=190598

Attachment 352925: Patch

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




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

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

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:657
> +	   if (database)

I am pretty sure this null check is not needed, would seem weird to store null
in the HashMap.

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:107
> +    uint64_t perOriginQuota() { return m_perOriginQuota; }

should be const.

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:134
> +    uint64_t m_perOriginQuota;

Needs a default initializer, e.g. uint64_t m_perOriginQuota { 0 };

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:855
> +    uint64_t quota = quotaForOrigin();

What if quota is 0 (i.e. uninitialized)?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:203
> +    uint64_t m_quota;

Needs a default initializer: uint64_t m_quota { 0 }; and then handling for the
default value if the quota never gets set.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1207
> +    for (auto& sessionID : m_idbServers.keys())

Iterating over the keys and then looking up each key is a terrible pattern for
performance. Why aren't you iterating over values directly?

> Source/WebKitLegacy/Storage/WebDatabaseProvider.h:38
> +

Why the blank line?

> Source/WebKitLegacy/Storage/WebDatabaseProvider.h:59
> +    uint64_t m_idbPerOriginQuota { 500 * MB };

This value is currently hardcoded in 2 places. Would it be possible to move it
to a shared place? maybe IDBServer ?

> Source/WebKitLegacy/win/Interfaces/IWebDatabaseManager.idl:64
> +    HRESULT setIDBPerOriginQuota([in] unsigned long long quota);

You're not allowed to modify existing interfaces like this or it will break
iTunes. Either you create a new IWebDatabaseManager3 with a new uuid and your
new method, or my personal favorite, do not add support on Windows and skip the
test there.

> Tools/DumpRenderTree/TestRunner.cpp:931
> +    TestRunner* controller =
static_cast<TestRunner*>(JSObjectGetPrivate(thisObject));

auto*

> LayoutTests/storage/indexeddb/resources/storage-limit.js:9
> +if (window.testRunner) {

Unnecessary curly brackets

> LayoutTests/storage/indexeddb/resources/storage-limit.js:27
> +    evalAndLog("request = store.add(new Uint8Array(" + quota  + "), 0)");

Question: Why does it fail with quota (seems like it would fit)? I would have
expected quota + 1 here.

> LayoutTests/storage/indexeddb/resources/storage-limit.js:36
> +	   debug("Add operation should fail because storage limit is reached,
but succeeded.");

testFailed("");