Bug 57927 – Add mock implementation and plumbing code for unified Quota API

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

[Kinuko Yasuda]

Per public discussion on public-webapps, we want expose the unified quota API that allows webapps to request/query the current storage usage and quota.
http://lists.w3.org/Archives/Public/public-webapps/2011JanMar/0346.html

[Kinuko Yasuda]

Created attachment 88442 [details]
Patch

[David Levin]

Comment on attachment 88442 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        No new tests: tests will be added when we add the implementation.

Usually the idl is the last thing added (after the implementation) and tests are required when the idl is added.


Maybe you should do the changes without the idl and add it in with tests when the impl is there.

> Source/WebCore/page/DOMWindow.cpp:111
> +#if ENABLE(QUOTA)

No if needed here. (The header has it.) Just put the include with the others.

> Source/WebCore/storage/StorageInfo.cpp:38
> +#include "ScriptExecutionContext.h"

Not needed a fwd decl will do.

> Source/WebCore/storage/StorageInfo.h:49
> +        TEMPORARY,

Enum members should user InterCaps with an initial capital letter. (I think you're using a chromium style.)

> Source/WebCore/storage/StorageInfo.h:58
> +    ~StorageInfo();

Consider making this private and making RefCounted<StorageInfo> a friend.

> Source/WebCore/storage/StorageInfo.idl:2
> + * Copyright (C) 2008, 2009 Apple Inc. All rights reserved.

This is an odd copyright header.

> Source/WebKit/chromium/src/StorageInfoChromium.cpp:57
> +    }

What if !isDocument?

I'm bothered by the raw pointer (new...) but I understand. I wonder if there is another way to do this.

> Source/WebKit/chromium/src/StorageInfoChromium.cpp:67
> +    }

Ditto.

[Kinuko Yasuda]

Created attachment 88568 [details]
Patch

Got rid of the idl part.

[Kinuko Yasuda]

Created attachment 88569 [details]
Patch

Minor fix

[Kinuko Yasuda]

Comment on attachment 88442 [details]
Patch

Thanks for reviewing,

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

>> Source/WebCore/ChangeLog:15
>> +        No new tests: tests will be added when we add the implementation.
> 
> Usually the idl is the last thing added (after the implementation) and tests are required when the idl is added.
> 
> 
> Maybe you should do the changes without the idl and add it in with tests when the impl is there.

Discarded the idl changes.

>> Source/WebCore/page/DOMWindow.cpp:111
>> +#if ENABLE(QUOTA)
> 
> No if needed here. (The header has it.) Just put the include with the others.

Discarded this change for now.

>> Source/WebCore/storage/StorageInfo.cpp:38
>> +#include "ScriptExecutionContext.h"
> 
> Not needed a fwd decl will do.

Done.

>> Source/WebCore/storage/StorageInfo.h:49
>> +        TEMPORARY,
> 
> Enum members should user InterCaps with an initial capital letter. (I think you're using a chromium style.)

They're named after the IDL's constant names to enable enum checks.  (I removed the IDL changes from the patch so I removed this enum for now too.)

>> Source/WebCore/storage/StorageInfo.h:58
>> +    ~StorageInfo();
> 
> Consider making this private and making RefCounted<StorageInfo> a friend.

Done.

>> Source/WebCore/storage/StorageInfo.idl:2
>> + * Copyright (C) 2008, 2009 Apple Inc. All rights reserved.
> 
> This is an odd copyright header.

Will fix. (The IDL changes have been discarded from this change)

>> Source/WebKit/chromium/src/StorageInfoChromium.cpp:57
>> +    }
> 
> What if !isDocument?
> 
> I'm bothered by the raw pointer (new...) but I understand. I wonder if there is another way to do this.

Added error handling code for !document cases.  As for the raw pointer I made a wrapper create method to make it less naked...

>> Source/WebKit/chromium/src/StorageInfoChromium.cpp:67
>> +    }
> 
> Ditto.

Did the same.

[Kinuko Yasuda]

Created attachment 88575 [details]
Patch

[WebKit Review Bot]

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

[Kinuko Yasuda]

(In reply to comment #7)
> Attachment 88575 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/8346487

This does not compile because the patch depends on issue 57849.

[Kinuko Yasuda]

Created attachment 88783 [details]
Patch

Rebased as now the corresponding WebKit API change has been submitted.

[David Levin]

Comment on attachment 88783 [details]
Patch

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

Seems fine. Only one question before I r+.

> Source/WebKit/chromium/src/StorageInfoChromium.cpp:57
> +        webFrame->client()->queryStorageUsageAndQuota(webFrame, static_cast<WebStorageQuotaType>(storageType), WebStorageQuotaCallbacksImpl::createForUsageCallback(successCallback, errorCallback));

How do we know that storageType has a valid value?

What will happen if it doesn't?

Do we need to check for validity here?

> Source/WebKit/chromium/src/StorageInfoChromium.cpp:72
> +        webFrame->client()->requestStorageQuota(webFrame, static_cast<WebStorageQuotaType>(storageType), newQuotaInBytes, WebStorageQuotaCallbacksImpl::createForQuotaCallback(successCallback, errorCallback));

Ditto.

[Kinuko Yasuda]

Created attachment 88972 [details]
Patch

[Kinuko Yasuda]

Created attachment 88973 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #10)
> (From update of attachment 88783 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88783&action=review
> 
> Seems fine. Only one question before I r+.
> 
> > Source/WebKit/chromium/src/StorageInfoChromium.cpp:57
> > +        webFrame->client()->queryStorageUsageAndQuota(webFrame, static_cast<WebStorageQuotaType>(storageType), WebStorageQuotaCallbacksImpl::createForUsageCallback(successCallback, errorCallback));
> 
> How do we know that storageType has a valid value?
> 
> What will happen if it doesn't?
> 
> Do we need to check for validity here?

I was planning to check for validity in the embedder side (i.e. chromium code) but I added the validity check here too, as we're crossing modules where the API defines valid storage types.

> > Source/WebKit/chromium/src/StorageInfoChromium.cpp:72
> > +        webFrame->client()->requestStorageQuota(webFrame, static_cast<WebStorageQuotaType>(storageType), newQuotaInBytes, WebStorageQuotaCallbacksImpl::createForQuotaCallback(successCallback, errorCallback));
> 
> Ditto.

[Kinuko Yasuda]

(In reply to comment #13)
> (In reply to comment #10)
> > (From update of attachment 88783 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=88783&action=review
> > 
> > Seems fine. Only one question before I r+.

David, could you or someone take another look when you have time?  Thanks and sorry for pushing,

[Kinuko Yasuda]

Committed r83729: <http://trac.webkit.org/changeset/83729>

[Joseph Pecoraro]

This looks great! The last I heard on this was this nice summary
posted later in the public-webapps thread:
http://lists.w3.org/Archives/Public/public-webapps/2011JanMar/0542.html

What is the next step for this? Is a WebApps draft spec in the works,
or is that on hold until an implementation is fleshed out? Considering
the feedback from public-webapps that wouldn't be a bad idea.

You've done a good job with Building the Bugzilla Bug dependency
tree / graph. Is there an overall tracking bug I can follow to make sure
I don't miss new bugs for this feature?

Thanks and Cheers!

[Kinuko Yasuda]

(In reply to comment #16)
> This looks great! The last I heard on this was this nice summary
> posted later in the public-webapps thread:
> http://lists.w3.org/Archives/Public/public-webapps/2011JanMar/0542.html
> 
> What is the next step for this? Is a WebApps draft spec in the works,
> or is that on hold until an implementation is fleshed out? Considering
> the feedback from public-webapps that wouldn't be a bad idea.

Yes for now I'm planning to add an experimental implementation first to flesh out how the implementation/API should be like.  After that we will bring the topic again in the public-webapps (if no one disagrees).

> You've done a good job with Building the Bugzilla Bug dependency
> tree / graph. Is there an overall tracking bug I can follow to make sure
> I don't miss new bugs for this feature?
> 
> Thanks and Cheers!

As for the bug dependency tree/graph, I just filed an umbrella bug for the feature/API: bug 60355
Probably you'll want track the bug to follow the feature.  I also sent out an email to webkit-dev to get more feedbacks/comments in the webkit developers community.  Please feel free to write to the thread if you have any comments!  Thanks,