[webkit-reviews] review denied: [Bug 195283] Introduce a quota manager for Cache API/Service Worker/IDB storage : [Attachment 363600] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied youenn fablet <youennf at gmail.com>'s
request for review:
Bug 195283: Introduce a quota manager for Cache API/Service Worker/IDB storage
https://bugs.webkit.org/show_bug.cgi?id=195283

Attachment 363600: Patch

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




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

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

> Source/WebKit/ChangeLog:13
> +

Extra blank line.

> Source/WebCore/storage/QuotaManager.cpp:61
> +void QuotaManager::requestSpaceIncreaseCompleted(Optional<uint64_t> quota)

quota -> newQuota ?

> Source/WebCore/storage/QuotaManager.cpp:66
> +    auto pendingRequests = WTFMove(m_pendingRequests);

Seems weird to take all pending requests here...

> Source/WebCore/storage/QuotaManager.cpp:77
> +	   requestSpace(request.spaceIncrease, WTFMove(request.callback));

... just to add them all back here.

This also means that if the request.callback() call above keeps making quota
requests, we'll never process following requests. I think this needs to be
refactored a bit to be more reliable. We really want to process the requests in
the order they came in and it looks like you are not guaranteeing this here.

> Source/WebCore/storage/QuotaManager.h:38
> +class QuotaManager : public CanMakeWeakPtr<QuotaManager> {

The name seem too generic given that it is not in a "Storage" namespace. I
would suggest StorageQuotaManager. A quota could be for many things otherwise.

Should be fast allocated.
We should also likely prevent copying.

> Source/WebCore/storage/QuotaManager.h:48
> +    void addQuotaUser(QuotaUser& user)

QuotaUser should probably be const.

> Source/WebCore/storage/QuotaManager.h:54
> +    void removeQuotaUser(QuotaUser& user)

QuotaUser should probably be const.

> Source/WebCore/storage/QuotaManager.h:71
> +    HashSet<QuotaUser*> m_quotaUsers;

QuotaUser should probably be const. Given the number of quota users we'll have,
we can probably use a Vector instead of a HashSet.

> Source/WebCore/storage/QuotaUser.h:30
> +class QuotaUser {

I think this would be nicer as a "User" class inside the "StorageQuotaManager"
class scope. I.e. StorageQuotaManager::User.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2307
> +    return *m_quotaManagers.ensure(origin, [this, sessionID, &origin] {

I think something needs to destroy quota managers when the session is
destroyed, otherwise this is ever growing and really bad for private browsing
where we have a private session per tab.

> Source/WebKit/NetworkProcess/NetworkProcess.h:326
> +    WebCore::QuotaManager& quotaManager(PAL::SessionID, const
WebCore::ClientOrigin&);

storageQuotaManager ?

> Source/WebKit/NetworkProcess/NetworkProcess.h:530
> +    HashMap<WebCore::ClientOrigin, std::unique_ptr<WebCore::QuotaManager>>
m_quotaManagers;

m_storageQuotaManager?

> Source/WebKit/NetworkProcess/NetworkProcess.h:531
> +    HashMap<PAL::SessionID, uint64_t> m_quotas;

m_perSessionStorageQuotas ?