[webkit-reviews] review denied: [Bug 193323] Add a new SPI to request for cache storage quota increase : [Attachment 359394] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied youenn fablet
<youennf at gmail.com>'s request for review:
Bug 193323: Add a new SPI to request for cache storage quota increase
https://bugs.webkit.org/show_bug.cgi?id=193323

Attachment 359394: Patch

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




--- Comment #9 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 359394
  --> https://bugs.webkit.org/attachment.cgi?id=359394
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKStorageQuotaDelegatePrivate.h:32
> + at protocol WKStorageQuotaDelegatePrivate <NSObject>

_WKWebsiteDataStoreDelegate since this is not public API, then the callback
selectors don't need underscores.
Also, needs availability macros.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStoreInternal.h:44
> +    RetainPtr<id <WKStorageQuotaDelegatePrivate> > _quotaDelegate;

Other delegates use a WeakObjCPtr.  I see no compelling reason to not do the
same here, especially since the ObjC property is labeled weak.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreQuotaManager.h:36
> +class WebsiteDataStoreQuotaManager {

Why not make this more general so we can add other callbacks here?
WebsiteDataStoreDelegate

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreQuotaManager.h:40
> +    virtual void requestCacheStorageSpace(const WebCore::SecurityOriginData&
topOrigin, const WebCore::SecurityOriginData& frameOrigin, uint64_t quota,
uint64_t currentSize, uint64_t spaceRequired,
WTF::CompletionHandler<void(Optional<uint64_t>)>&& completionHandler)

WTF:: is unnecessary here.

> LayoutTests/http/wpt/cache-storage/cache-quota.any.js:157
>     }).then(() => {

You could also add a space here.