Bug 57849 – [Chromium] Add WebKit API to query and request unified offline-storage quota

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

Per discussion on public-webapps, we should expose WebKit API to query and request quota for unified offline-storage quota.
http://lists.w3.org/Archives/Public/public-webapps/2011JanMar/0346.html

[Kinuko Yasuda]

Created attachment 88237 [details]
Patch

[WebKit Review Bot]

Attachment 88237 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8334344

[Michael Nordman]

Do we intend to have these api's available from within shared workers too?

[David Levin]

Comment on attachment 88237 [details]
Patch

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

Leaving up for review for Darin.

> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:41
> +    // Callback for WebFrameClient::requestStorageQuota. This may return a smaller amount of quota than the requested, or may return zero when the quota is not enabled (for the requesting origin).

WebKit doesn't have a line length limit but this seems a bit long.

> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:42
> +    zero when the quota system is not enabled for the origin.

This appears to be a mistake.

> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:46
> +    virtual ~WebStorageInfoCallbacks() {}

Space between {}

[Eric U.]

Comment on attachment 88237 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrameClient.h:374
> +        WebFrame*, WebStorageInfoType, WebStorageInfoCallbacks*) { }

Doesn't this need a parameter for how much quota is being requested?

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

Why put these both in the same object?  Will any method ever need both of them?

[WebKit Review Bot]

Attachment 88237 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8336407

[Darin Fisher (:fishd, Google)]

Comment on attachment 88237 [details]
Patch

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.

[Kinuko Yasuda]

Created attachment 88371 [details]
Patch

[WebKit Review Bot]

Attachment 88371 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebKit/chromium/public/WebFrameClient.h:371:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebFrameClient.h:383:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kinuko Yasuda]

Created attachment 88374 [details]
Patch

Fixed style errors.

[Kinuko Yasuda]

Comment on attachment 88237 [details]
Patch

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?

Done.

>> Source/WebKit/chromium/public/WebFrameClient.h:369
>> +    virtual void queryStorageUsage(
> 
> maybe this method should be named queryStorageUsageAndQuota to better reflect
> what it does?

Done.

>> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:36
>> +class WebStorageInfoCallbacks {
> 
> maybe call this WebStorageQuotaCallbacks?

Done.

>> 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?

Done.

>>> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:39
>>> +    virtual void didQueryStorageUsage(unsigned long long usageInBytes, unsigned long long quotaInBytes) = 0;
>> 
>> Why put these both in the same object?  Will any method ever need both of them?
> 
> didQueryStorageUsageAndQuota

Done.  @Eric: it's just to simplify the callback handling implementation.  We've done similar in FileSystem before so it's easier for me to follow the way.

>> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:41
>> +    // Callback for WebFrameClient::requestStorageQuota. This may return a smaller amount of quota than the requested, or may return zero when the quota is not enabled (for the requesting origin).
> 
> WebKit doesn't have a line length limit but this seems a bit long.

Broke the line.

>> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:42
>> +    zero when the quota system is not enabled for the origin.
> 
> This appears to be a mistake.

Added didFail instead.

>> 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

Done.

>> Source/WebKit/chromium/public/WebStorageInfoCallbacks.h:46
>> +    virtual ~WebStorageInfoCallbacks() {}
> 
> Space between {}

Done.

>> 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.

Changed the name to WebStorageQuotaType.

[Kinuko Yasuda]

Created attachment 88580 [details]
Patch

[Kinuko Yasuda]

Created attachment 88582 [details]
Patch

Added WebStorageQuotaError enum.

[Darin Fisher (:fishd, Google)]

Comment on attachment 88582 [details]
Patch

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

R=me because my nit picks are very minor.  Feel free to correct those and then commit.

> Source/WebKit/chromium/public/WebStorageQuotaError.h:38
> +    WebStorageQuotaErrorOk = 0,

do you need the "ok" value?  i can't imagine didFail being called with this value.

> Source/WebKit/chromium/public/WebStorageQuotaError.h:40
> +    WebStorageQuotaErrorSecurity = 18,

it is not very obvious what ErrorSecurity and ErrorAbort mean.  i think ErrorAbort
will be used whenever the operation could not complete because we are shutting down.
what does ErrorSecurity mean?

maybe the name could be improved or maybe a comment could be added or both.

[Darin Fisher (:fishd, Google)]

Comment on attachment 88582 [details]
Patch

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

> Source/WebKit/chromium/public/WebStorageQuotaCallbacks.h:45
> +    virtual void wasGrantedStorageQuota(unsigned long long grantedQuotaInBytes) = 0;

you're going to hate me for suggesting another change here, but maybe
didGrantStorageQuota would be an even better name.  that just occurred
to me.

[Kinuko Yasuda]

Comment on attachment 88582 [details]
Patch

Thanks for reviewing,

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

>> Source/WebKit/chromium/public/WebStorageQuotaCallbacks.h:45
>> +    virtual void wasGrantedStorageQuota(unsigned long long grantedQuotaInBytes) = 0;
> 
> you're going to hate me for suggesting another change here, but maybe
> didGrantStorageQuota would be an even better name.  that just occurred
> to me.

Ok that sounds better... :) I'll just replace the method name and will submit, if the change doesn't affect your r+.

>> Source/WebKit/chromium/public/WebStorageQuotaError.h:38
>> +    WebStorageQuotaErrorOk = 0,
> 
> do you need the "ok" value?  i can't imagine didFail being called with this value.

Sounds true, will drop it.

>> Source/WebKit/chromium/public/WebStorageQuotaError.h:40
>> +    WebStorageQuotaErrorSecurity = 18,
> 
> it is not very obvious what ErrorSecurity and ErrorAbort mean.  i think ErrorAbort
> will be used whenever the operation could not complete because we are shutting down.
> what does ErrorSecurity mean?
> 
> maybe the name could be improved or maybe a comment could be added or both.

For now I will just drop QuotaErrorSecurity until I find a proper usage.

[Kinuko Yasuda]

Committed r83258: <http://trac.webkit.org/changeset/83258>

[Darin Fisher (:fishd, Google)]

(In reply to comment #16)
> Ok that sounds better... :) I'll just replace the method name and will submit, if the change doesn't affect your r+.
...
> Sounds true, will drop it.
...
> For now I will just drop QuotaErrorSecurity until I find a proper usage.

SGTM, thanks!