Bug 57927 - Add mock implementation and plumbing code for unified Quota API
Summary: Add mock implementation and plumbing code for unified Quota API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 57948 57849 57918
Blocks: 60355
  Show dependency treegraph
 
Reported: 2011-04-06 01:14 PDT by Kinuko Yasuda
Modified: 2011-05-06 02:02 PDT (History)
10 users (show)

See Also:


Attachments
Patch (56.75 KB, patch)
2011-04-06 09:11 PDT, Kinuko Yasuda
levin: review-
Details | Formatted Diff | Diff
Patch (36.18 KB, patch)
2011-04-06 21:46 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (36.20 KB, patch)
2011-04-06 21:50 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (36.22 KB, patch)
2011-04-06 22:36 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (36.74 KB, patch)
2011-04-08 02:12 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (37.15 KB, patch)
2011-04-10 23:55 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (37.15 KB, patch)
2011-04-10 23:59 PDT, Kinuko Yasuda
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2011-04-06 01:14:52 PDT
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
Comment 1 Kinuko Yasuda 2011-04-06 09:11:46 PDT
Created attachment 88442 [details]
Patch
Comment 2 David Levin 2011-04-06 11:06:10 PDT
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.
Comment 3 Kinuko Yasuda 2011-04-06 21:46:01 PDT
Created attachment 88568 [details]
Patch

Got rid of the idl part.
Comment 4 Kinuko Yasuda 2011-04-06 21:50:30 PDT
Created attachment 88569 [details]
Patch

Minor fix
Comment 5 Kinuko Yasuda 2011-04-06 21:53:42 PDT
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.
Comment 6 Kinuko Yasuda 2011-04-06 22:36:41 PDT
Created attachment 88575 [details]
Patch
Comment 7 WebKit Review Bot 2011-04-06 22:42:36 PDT
Attachment 88575 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8346487
Comment 8 Kinuko Yasuda 2011-04-06 22:46:47 PDT
(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.
Comment 9 Kinuko Yasuda 2011-04-08 02:12:12 PDT
Created attachment 88783 [details]
Patch

Rebased as now the corresponding WebKit API change has been submitted.
Comment 10 David Levin 2011-04-08 07:12:06 PDT
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.
Comment 11 Kinuko Yasuda 2011-04-10 23:55:57 PDT
Created attachment 88972 [details]
Patch
Comment 12 Kinuko Yasuda 2011-04-10 23:59:13 PDT
Created attachment 88973 [details]
Patch
Comment 13 Kinuko Yasuda 2011-04-11 00:08:17 PDT
(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.
Comment 14 Kinuko Yasuda 2011-04-12 23:21:53 PDT
(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,
Comment 15 Kinuko Yasuda 2011-04-13 07:20:39 PDT
Committed r83729: <http://trac.webkit.org/changeset/83729>
Comment 16 Joseph Pecoraro 2011-05-04 20:25:41 PDT
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!
Comment 17 Kinuko Yasuda 2011-05-06 02:02:03 PDT
(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,