[webkit-reviews] review denied: [Bug 190598] Add a storage limit for IndexedDB : [Attachment 352417] 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 352417: Patch

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




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

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

This needs a test.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:842
> +    uint64_t diskFreeSpaceSize = 0;

Can you please ASSERT(!isMainThread()); since you're doing file system
operations?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:844
> +    return std::min(diskFreeSpaceSize * 0.5, MB * 500.0);

I think 500 * MB would look less backward. Also, I think I would use a global
constant for this magic value:
const uint64_t maximumQuota = 500 * MB;
return std::min(diskFreeSpaceSize / 2, maximumQuota);

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:849
> +    // The maximum size for one database file is the quota for its origin,
minus size of all databases within that origin,

Can you please ASSERT(!isMainThread()); since you're doing file system
operations?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:851
> +    uint64_t diskUsage = 0;

I think it would look better if we moved this one right before the for loop.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:859
> +

ASSERT(diskUsage >= databaseFileSize);

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:860
> +    if (quota < diskUsage || quota < databaseFileSize)

If my assertion above is correct, then checking `quota < diskUsage` seems
unnecessary, maybe I am missing something?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:863
> +    return quota - diskUsage + databaseFileSize;

uint64_t maximumSize = quota - diskUsage + databaseFileSize;
ASSERT(maximumSize >= 0);
return maximumSize;

BTW, I am not sure this assertion holds, what guarantees that this returns a
positive value? Quota is a factor of free disk space and therefore may become
really small.

I think the logic in this method needs to be clarified and be made more robust.