AppCache + Quota integration

AppCache + Quota integration

* Notify the QuotaManager of accesses and modifications to the amount of storage utlized.
* Implement the QuotaClient interface so the manager can query the appcache for usage and delete data.
* When storing appcaches, use QuotaManager GetUsageAndQuota and respect the limit.
* Remove the old and unsed support for storing per-origin quota values in the appcache DB.

BUG=61676
TEST=unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88746

[michaeln, 2011-06-01 05:25:44]

hi kinuko, this isn't quite done yet, but it's getting close.

what's done is

* implemented the Client interface
* calling NotifyStorage[Accessed|Modified]

what's not done is

* getting the limit from the QM when needed
  (there's a TODO for that)
* unittests
* odds and ends around creating/registering the Client, pointing the
client/service at each other and snipping ptrs when needed, notifying the Client
when the appcache is 'ready'

[kinuko, 2011-06-02 08:38:06]

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.cc (right):

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.cc:42: DeletePendingRequests();
do we need this?  we seem to be deleting callbacks before deleting 'this'.

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.cc:62: // Note: Requests may not be
processed in order.
Does the QM's usage tracker need to be able to handle out-of-order GetUsage
callbacks?

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.h (right):

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.h:29: // RefCountedThreadSafe
?

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_se...
File webkit/appcache/appcache_service.cc (right):

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_se...
webkit/appcache/appcache_service.cc:231: CallCallback(!failures_ ? net::OK :
net::ERR_FAILED);
for readability can we write like (failures_ == 0 ? net::OK...)?  (unless !xx
for x==0 is recommended style in chrome)

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1052: usage_map_.clear();  //
TODO(michaeln): What to tell the QuotaManager?
hmm

do we actually delete data when we disable the storage?

[michaeln, 2011-06-02 22:46:55]

i've still got a bit todo, notably tests and getting the limit from the QM

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.cc (right):

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.cc:42: DeletePendingRequests();
On 2011/06/02 08:38:06, kinuko wrote:
> do we need this?  we seem to be deleting callbacks before deleting 'this'.

I don't think we do... thnx.

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.cc:62: // Note: Requests may not be
processed in order.
On 2011/06/02 08:38:06, kinuko wrote:
> Does the QM's usage tracker need to be able to handle out-of-order GetUsage
> callbacks?

I'll rearrange things here to respond in order instead.

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.h (right):

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.h:29: // RefCountedThreadSafe
On 2011/06/02 08:38:06, kinuko wrote:
> ?

ooops, pardon my dust

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_se...
File webkit/appcache/appcache_service.cc (right):

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_se...
webkit/appcache/appcache_service.cc:231: CallCallback(!failures_ ? net::OK :
net::ERR_FAILED);
On 2011/06/02 08:38:06, kinuko wrote:
> for readability can we write like (failures_ == 0 ? net::OK...)?  (unless !xx
> for x==0 is recommended style in chrome)

i thought it was a style thing, but not entirely sure?

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7031065/diff/25001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1052: usage_map_.clear();  //
TODO(michaeln): What to tell the QuotaManager?
On 2011/06/02 08:38:06, kinuko wrote:
> hmm
> 
> do we actually delete data when we disable the storage?

Being disabled is being in an error condition.

By delete data not sure what you mean? If you mean delete appcaches.. no can
do... the db|diskcache is borked, that's why we're disabling.

If you mean delete the usage_map... maybe we don't need to, i'll look into that.
When disabled the QM won't be able to delete things, but it still could see how
much is (or was) there.

[michaeln, 2011-06-08 00:47:48]

On 2011/06/02 22:46:55, michaeln wrote:
> i've still got a bit todo, notably tests and getting the limit from the QM

I've added code to get and respect the limit according to the QM, and added
unittests. I still need to add more tests but the production code is pretty much
done (i think, minus a couple of TODOs about how to handle an error condition).

[kinuko, 2011-06-08 13:49:25]

I want to take another look, but I think it looks good.

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client_unittest.cc (right):

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client_unittest.cc:205: 
nit: extra empty line

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client_unittest.cc:287: EXPECT_EQ(2,
mock_service_.delete_called_count());
don't we need to call Call_NotifyAppCacheDestroyed /
Call_OnQuotaManagerDestroyed?

[michaeln, 2011-06-08 18:09:54]

Thnx for looking...

Please ignore the comments for webkit/appcache/appcache_storage_impl_unittest.cc
(the trailing comments). Those are just notes for myself about how i need to
update those tests. When i update them, i'll ping you for a final review.

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.cc (right):

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.cc:215:
pending_delete_requests_.pop_front();
Fixed a bug here. Needed to pop prior to calling DeleteOriginData since it may
try to start the next request prior to return.

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client_unittest.cc (right):

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client_unittest.cc:205: 
On 2011/06/08 13:49:25, kinuko wrote:
> nit: extra empty line

Done.

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client_unittest.cc:287: EXPECT_EQ(2,
mock_service_.delete_called_count());
On 2011/06/08 13:49:25, kinuko wrote:
> don't we need to call Call_NotifyAppCacheDestroyed /
> Call_OnQuotaManagerDestroyed?

Yes... done... thnx

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:468: // TODO(michaeln): what do we do
if status is not OK, fail the store op?
Took care of this TODO by setting space_available_ to 0 when status != OK.

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1112: usage_map_.clear();  //
TODO(michaeln): What to tell the QuotaManager?
Took are of this TODO with ClearUsageMapAndNotify(), we'll send notifcations
that look like all appcache data has been deleted.

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl_unittest.cc (right):

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:333: EXPECT_EQ(cache.get(),
delegate()->loaded_cache_.get());
verify access notification

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:363: void Verify_CreateGroup()
{
verify access notification

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:409: 
verify access notification

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:422:
EXPECT_TRUE(delegate()->loaded_group_->HasOneRef());
verify access notification

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:437: cache_ = new
AppCache(service(), storage()->NewCacheId());
add an Entry to have a non-zero cache size

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:456:
EXPECT_TRUE(database()->FindCache(cache_->cache_id(), &cache_record));
also verify notification of the modification

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:472: cache2_ = new
AppCache(service(), 2);
ditto add an entry

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:492:
EXPECT_FALSE(database()->FindCache(1, &cache_record));
ditto verify modification notification

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:542: EXPECT_EQ(100,
entry_records[0].response_size);
verify modification notification

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:582: 
add a test for querying the quotaMgr for the space available, perhaps two, one
where the QM returns immediately and another where it's async

http://codereview.chromium.org/7031065/diff/41022/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:642:
EXPECT_TRUE(whitelist_records.empty());
verify modification notification

[michaeln, 2011-06-09 01:28:17]

I've modified the storage_impl unittests to test for the usage_map being update
properly. These tests are tedious (that class is a large unit). So... I still
have to add tests to observe that the quota related notifications are being
invoked properly by the storage_impl class. There's a start at in in commented
out code.

[michaeln, 2011-06-10 01:01:24]

Added tests and gypi file additions. I think this is ready now. I'll start some
try jobs.

[kinuko, 2011-06-10 10:04:48]

lgtm

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.cc (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.cc:136: void
AppCacheQuotaClient::DidDeleteAppCachesForOrigin(int rv) {
To make sure... after it's cancelled this won't be called?  Do we need to check
if service_ is not null like we do for quota_manager?

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.h (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.h:28: // with the quota  management
system. The QuotaClient interface is
nit: two spaces between quota and management

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client_unittest.cc (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client_unittest.cc:423:
EXPECT_EQ(quota::kQuotaErrorAbort, delete_status_);
need Call_OnQuotaManagerDestroyed(client) here?

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_se...
File webkit/appcache/appcache_service_unittest.cc (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_se...
webkit/appcache/appcache_service_unittest.cc:30: 
protected: around here?
(we seem to have public member vars finishing with '_')

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:597: // also exceeds the quota?
http://crbug.com/83968
fyi for filesystem we reject any further append-writes but allow overwrites if
offset+new_write_size is within the original file size (no truncation).

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl_unittest.cc (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:214: 
nit: protected: ?

[michaeln, 2011-06-10 18:49:15]

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.cc (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.cc:136: void
AppCacheQuotaClient::DidDeleteAppCachesForOrigin(int rv) {
On 2011/06/10 10:04:48, kinuko wrote:
> To make sure... after it's cancelled this won't be called?  Do we need to
check
> if service_ is not null like we do for quota_manager?

Added a DCHECK(service_). After a cancelable callback is canceled, there's no
chance of getting here.

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.h (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.h:28: // with the quota  management
system. The QuotaClient interface is
On 2011/06/10 10:04:48, kinuko wrote:
> nit: two spaces between quota and management

Done.

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client_unittest.cc (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client_unittest.cc:423:
EXPECT_EQ(quota::kQuotaErrorAbort, delete_status_);
On 2011/06/10 10:04:48, kinuko wrote:
> need Call_OnQuotaManagerDestroyed(client) here?

Done.

gah! thank you kinuko-valgrind-san!

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_se...
File webkit/appcache/appcache_service_unittest.cc (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_se...
webkit/appcache/appcache_service_unittest.cc:30: 
On 2011/06/10 10:04:48, kinuko wrote:
> protected: around here?
> (we seem to have public member vars finishing with '_')

Done.

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:597: // also exceeds the quota?
http://crbug.com/83968
On 2011/06/10 10:04:48, kinuko wrote:
> fyi for filesystem we reject any further append-writes but allow overwrites if
> offset+new_write_size is within the original file size (no truncation).

i'm doing that for appcaches too, but the consequence is that a stale old
version of the appcache can't be updated to the latest version... so you're
stuck with the old one.

i think i may need to delete the old one in this case to un-wedge things.

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl_unittest.cc (right):

http://codereview.chromium.org/7031065/diff/40055/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:214: 
On 2011/06/10 10:04:48, kinuko wrote:
> nit: protected: ?

I'd have to add getters and setter methods to poke at these members (here and in
the class below) from within test fixtures. I think would just clutter things up
more than help really.