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
Created attachment 88237 [details] Patch
Attachment 88237 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8334344
Do we intend to have these api's available from within shared workers too?
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 {}
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?
Attachment 88237 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8336407
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.
Created attachment 88371 [details] Patch
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.
Created attachment 88374 [details] Patch Fixed style errors.
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.
Created attachment 88580 [details] Patch
Created attachment 88582 [details] Patch Added WebStorageQuotaError enum.
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.
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.
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.
Committed r83258: <http://trac.webkit.org/changeset/83258>
(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!