Relax strict quota limit checks when reading pre-existing DomStorage database files. The other chec…

Relax strict quota limit checks when reading pre-existing DomStorage database files. The other checks will disallow increases that exceed the limit, but when it comes to existing files its not clear that the checks on open will pass. So we'll grandfather in the existing files.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=125524

[michaeln, 2012-03-05 21:53:44]

ptal

[benm (inactive), 2012-03-06 11:31:15]

http://codereview.chromium.org/9594038/diff/2002/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_map.cc (right):

http://codereview.chromium.org/9594038/diff/2002/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_map.cc:78: // Only check quota if the size is
increasing since a descrese is
s/descrease/decrease/

http://codereview.chromium.org/9594038/diff/2002/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_map.cc:80: if ((new_item_size > old_item_size) &&
(new_bytes_used > quota_))
should this be new_item_size >= old_item_size? That would cover the case that
when we're over quota (due to grandfathering in a legacy DB) we'll only allow
the DB to shrink?

nit: can remove some parantheses

http://codereview.chromium.org/9594038/diff/2002/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_map_unittest.cc (right):

http://codereview.chromium.org/9594038/diff/2002/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_map_unittest.cc:109: map->SwapValues(&swap);
What is this bit of the test verifying now? I think we can remove it, or if you
add the code back and switch the expectation below with EXPECT_EQ(2u,
map->Length()) we could verify that swapping (i.e. grandfathering) will bypass
the quota check.

wdyt?

[michaeln, 2012-03-06 20:37:51]

http://codereview.chromium.org/9594038/diff/2002/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_map.cc (right):

http://codereview.chromium.org/9594038/diff/2002/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_map.cc:78: // Only check quota if the size is
increasing since a descrese is
On 2012/03/06 11:31:15, benm wrote:
> s/descrease/decrease/

Done.

http://codereview.chromium.org/9594038/diff/2002/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_map.cc:80: if ((new_item_size > old_item_size) &&
(new_bytes_used > quota_))
On 2012/03/06 11:31:15, benm wrote:
> should this be new_item_size >= old_item_size? That would cover the case that
> when we're over quota (due to grandfathering in a legacy DB) we'll only allow
> the DB to shrink?

Seems cruel to reject a change where the size is not increasing.

> nit: can remove some parantheses

Done.

http://codereview.chromium.org/9594038/diff/2002/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_map_unittest.cc (right):

http://codereview.chromium.org/9594038/diff/2002/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_map_unittest.cc:109: map->SwapValues(&swap);
On 2012/03/06 11:31:15, benm wrote:
> What is this bit of the test verifying now? I think we can remove it, or if
you
> add the code back and switch the expectation below with EXPECT_EQ(2u,
> map->Length()) we could verify that swapping (i.e. grandfathering) will bypass
> the quota check.
> 
> wdyt?

Done, added tests to verify the quota checking behavior.

[benm (inactive), 2012-03-06 23:55:24]

http://codereview.chromium.org/9594038/diff/7001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_map_unittest.cc (right):

http://codereview.chromium.org/9594038/diff/7001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_map_unittest.cc:117:
EXPECT_TRUE(map->SetItem(kKey, string16(), &old_nullable_value));
I think that this SetItem() will take us under quota again, so the next
expectation will just verify that another call to Set will enforce the quota
(which is tested above).

We could reorder the expectations slightly so the one below comes in the middle,
i.e. 

verify equal size is true with kValue
verify greater size is false with (kValue + kValue)
verify lesser size is true with string16()

I think that should ensure the new if condition is covered?

[michaeln, 2012-03-07 00:43:19]

On 2012/03/06 23:55:24, benm wrote:
>
http://codereview.chromium.org/9594038/diff/7001/webkit/dom_storage/dom_stora...
> File webkit/dom_storage/dom_storage_map_unittest.cc (right):
> 
>
http://codereview.chromium.org/9594038/diff/7001/webkit/dom_storage/dom_stora...
> webkit/dom_storage/dom_storage_map_unittest.cc:117:
> EXPECT_TRUE(map->SetItem(kKey, string16(), &old_nullable_value));
> I think that this SetItem() will take us under quota again, so the next
> expectation will just verify that another call to Set will enforce the quota
> (which is tested above).
> 
> We could reorder the expectations slightly so the one below comes in the
middle,
> i.e. 
> 
> verify equal size is true with kValue
> verify greater size is false with (kValue + kValue)
> verify lesser size is true with string16()
> 
> I think that should ensure the new if condition is covered?

bah... thnx for spotting this... trying again :)

[michaeln, 2012-03-07 19:45:45]

new snapshot (i had forgotten to upload yesterday)

[benm (inactive), 2012-03-07 23:25:07]

lgtm!

http://codereview.chromium.org/9594038/diff/12001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_map_unittest.cc (right):

http://codereview.chromium.org/9594038/diff/12001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_map_unittest.cc:115: // When overbudget, a new
value of greater size the the existing value can
s/the/than/ ?

[michaeln, 2012-03-08 01:17:59]

> s/the/than/ ?

Done