[webkit-reviews] review denied: [Bug 57849] [Chromium] Add WebKit API to query and request unified offline-storage quota : [Attachment 88237] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 57849: [Chromium] Add WebKit API to query and request unified
offline-storage quota
https://bugs.webkit.org/show_bug.cgi?id=57849

Attachment 88237: Patch
https://bugs.webkit.org/attachment.cgi?id=88237&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88237&action=review

> Source/WebKit/chromium/public/WebFrameClient.h:368
> +    // Queries the origin's offline storage usage and quota information.

this is not just for "offline" use, right?  might be better to leave out that
word.

it seems like you should document what callback method will be called, and you
should also document how the lifetime of the callbacks object is managed.  who
is responsible for deleting it?

> Source/WebKit/chromium/public/WebFrameClient.h:369
> +    virtual void queryStorageUsage(

maybe this method should be named queryStorageUsageAndQuota to better reflect
what it does?

> Source/WebKit/chromium/public/WebFrameClient.h:373
> +    virtual void requestStorageQuota(

so there is no need for this to take a "size hint" parameter?  i would have
expected there to be one much as there is one for openFileSystem.

> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:36
> +class WebStorageInfoCallbacks {

maybe call this WebStorageQuotaCallbacks?

> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:37
> +public:

should this interface have a didFail method so that we have a way to abort
early in case the web page is going away?

> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:39
> +    virtual void didQueryStorageUsage(unsigned long long usageInBytes,
unsigned long long quotaInBytes) = 0;

didQueryStorageUsageAndQuota

> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:43
> +    virtual void didRequestStorageQuota(unsigned long long
grantedQuotaInBytes) = 0;

It seems like the name of this function should better correspond to the idea of

being granted something:

wasGrantedStorageQuota

> Source/WebKit/chromium/public/WebStorageInfoType.h:36
> +enum WebStorageInfoType {

It seems like this is meant to describe quota / life-time policy.  Maybe the
name
could be improved by using those terms?

enum WebStorageQuotaType {
    WebStorageQuotaTemporary,
    WebStorageQuotaPersistent
};

I'm not sure that "quota" is the best term here either.