QuotaManager::DeleteOriginData now allows deletion of specific QuotaClients

QuotaManager::DeleteOriginData allows deletion of specific QuotaClients

To enable a more fine-grained extension API that would allow removal of specific data types, this CL changes the QuotaClient::ID enum values to bitmasky flags, and adds a bitmask parameter to QuotaManager::DeleteOriginData and QuotaManager::DeleteHostData to filter the clients that are deleted for each origin.

BUG=94334
TEST=test_shell_tests,unit_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121269

[Mike West, 2011-09-07 14:43:18]

Hi Kinuko and Michael!

In the interests of providing a more fine-grained deletion mechanism to
extensions (see `chrome.experimental.clear` and http://crbug.com/94334), I've
put together this approach to making QuotaManager::DeleteOriginData a bit more
granular as a strawman. This was the first solution I thought of, and it needs
many more tests, but I wanted to run the approach by you two before investing
much more effort in solidifying things.

This isn't high priority, but I'd appreciate you taking a look. If the approach
makes sense to you, brilliant. If not, I'd love suggestions. :)

Jochen: If you could take a look at the BrowsingDataRemover sections, I'd
appreciate it.

Thanks!

-Mike

http://codereview.chromium.org/7839029/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7839029/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:57: int generateQuotaClientMask(int
remove_mask) {
I don't really think this needs to exist anywhere publicly. But maybe there are
use-cases I'm not thinking about?

[jochen (gone - plz use gerrit), 2011-09-12 11:23:00]

http://codereview.chromium.org/7839029/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7839029/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:57: int generateQuotaClientMask(int
remove_mask) {
On 2011/09/07 14:43:19, Mike West wrote:
> I don't really think this needs to exist anywhere publicly. But maybe there
are
> use-cases I'm not thinking about?

tests :)

http://codereview.chromium.org/7839029/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover_unittest.cc (right):

http://codereview.chromium.org/7839029/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover_unittest.cc:40: const quota::StorageType
kTemp = quota::kStorageTypeTemporary;
these abbreviations make it kinda hard to read the tests IMO :/

http://codereview.chromium.org/7839029/diff/1/webkit/quota/mock_quota_manager.cc
File webkit/quota/mock_quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/1/webkit/quota/mock_quota_manager...
webkit/quota/mock_quota_manager.cc:78: int client_mask,
in the header this is quota_client_mask

http://codereview.chromium.org/7839029/diff/1/webkit/quota/mock_quota_manager.h
File webkit/quota/mock_quota_manager.h (right):

http://codereview.chromium.org/7839029/diff/1/webkit/quota/mock_quota_manager...
webkit/quota/mock_quota_manager.h:40: int mask;
quota_client_mask so it's clear what it is?

[Mike West, 2011-09-23 11:27:30]

Friendly ping on this, Kinuko/Michael. :) I'd appreciate some feedback from you
to ensure that this is a direction we're happy with before I dive back into
cleaning this up.

Thanks!

-Mike

[kinuko, 2011-09-26 04:42:25]

Will take a look at it.  What I'm not sure about this is whether we want to
push it as yet another extension-only API or a set of new platform APIs
(like Darin suggested; enumerating and deleting, or adding a set of new
app-facing APIs of the quota API).

Separately, Darin has started filing some bugs about adding the
ability in the web platform for developers to enumerate the data
they've stored in various storage mechanisms (indexed db, websql,
appcache, filesystem) and delete it (for example, see bug 94439
<http://code.google.com/p/chromium/issues/detail?id=94439>)


Kinuko

On Fri, Sep 23, 2011 at 8:27 PM, <mkwst@chromium.org> wrote:

> Friendly ping on this, Kinuko/Michael. :) I'd appreciate some feedback from
> you
> to ensure that this is a direction we're happy with before I dive back into
> cleaning this up.
>
> Thanks!
>
> -Mike
>
>
http://codereview.chromium.**org/7839029/<http://codereview.chromium.org/7839...
>

[Mike West, 2011-09-26 06:49:44]

On 2011/09/26 04:42:25, kinuko wrote:
> Will take a look at it.  What I'm not sure about this is whether we want to
> push it as yet another extension-only API or a set of new platform APIs
> (like Darin suggested; enumerating and deleting, or adding a set of new
> app-facing APIs of the quota API).

I believe the two APIs are complementary, but orthogonal. A web platform API
would allow a developer to enumerate (and remove) data they set for their own
origin. This API gives additional power, clearing data from _all_ (browsing)
origins.

[kinuko, 2011-09-26 09:36:55]

On Mon, Sep 26, 2011 at 3:49 PM, <mkwst@chromium.org> wrote:

> On 2011/09/26 04:42:25, kinuko wrote:
>
>> Will take a look at it.  What I'm not sure about this is whether we want
>> to
>> push it as yet another extension-only API or a set of new platform APIs
>> (like Darin suggested; enumerating and deleting, or adding a set of new
>> app-facing APIs of the quota API).
>>
>
> I believe the two APIs are complementary, but orthogonal. A web platform
> API
> would allow a developer to enumerate (and remove) data they set for their
> own
> origin. This API gives additional power, clearing data from _all_
> (browsing)
> origins.


Right, and my another concern is if we want such powerful deletion API as a
Web platform API..


>
http://codereview.chromium.**org/7839029/<http://codereview.chromium.org/7839...
>

[Mike West, 2011-09-26 09:41:26]

On 2011/09/26 09:36:55, kinuko wrote:

> Right, and my another concern is if we want such powerful deletion API as a
> Web platform API..

I don't think we do. At least, I don't. :) I think it makes sense to offer this
functionality to extension authors, behind a specific permission warning. I
don't think it makes sense to offer it to every website behind an infobar. This
CL is specifically about making the `chrome.clear.*` APIs available. It
shouldn't have any effect on the platform discussion.

[kinuko, 2011-09-26 09:43:48]

cosmetic issues only; will take another look.

http://codereview.chromium.org/7839029/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7839029/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:57: int generateQuotaClientMask(int
remove_mask) {
The first letter should be capital?

http://codereview.chromium.org/7839029/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover_unittest.cc (right):

http://codereview.chromium.org/7839029/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover_unittest.cc:40: const quota::StorageType
kTemp = quota::kStorageTypeTemporary;
On 2011/09/12 11:23:00, jochen wrote:
> these abbreviations make it kinda hard to read the tests IMO :/

We use such abbrevs in other tests as well (since kStorageTypeWhatever is kinda
long) so I think it'd be ok (IMHO) :)

http://codereview.chromium.org/7839029/diff/1/webkit/quota/quota_client.h
File webkit/quota/quota_client.h (right):

http://codereview.chromium.org/7839029/diff/1/webkit/quota/quota_client.h#new...
webkit/quota/quota_client.h:38: kMockStart,  // This needs to be the end of the
enum.
what should we do if we want to test the feature with the mock storage?

http://codereview.chromium.org/7839029/diff/1/webkit/quota/quota_client.h#new...
webkit/quota/quota_client.h:42: kIndexedDatabase;
-1?

http://codereview.chromium.org/7839029/diff/1/webkit/quota/quota_client.h#new...
webkit/quota/quota_client.h:75: QuotaClient::kIndexedDatabase;
is this a dupe of line 41?

[Mike West, 2012-01-18 05:17:31]

On 2011/09/26 09:43:48, kinuko wrote:
> cosmetic issues only; will take another look.

So, after ~4 months of doing _everything else ever_, I've rewritten/rebased this
patch onto trunk and addressed these comments.

Would you mind evaluating the code's core a bit more deeply, while I work on
writing a few more tests? :)

[michaeln, 2012-01-18 23:07:48]

this looks good to me, but kinuko really should take a look

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:483: quota_client_mask_ ==
QuotaClient::kAllClientsMask) {
is the second condition needed or will the first eval to true in all cases where
the second would?

[Mike West, 2012-01-19 13:09:11]

Cool. Then I'll continue adding tests, and I look forward to hearing from Kinuko
and Jochen. :)

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:483: quota_client_mask_ ==
QuotaClient::kAllClientsMask) {
On 2012/01/18 23:07:48, michaeln wrote:
> is the second condition needed or will the first eval to true in all cases
where
> the second would?

In an earlier review, Kinuko suggested using -1 for the `kAllClientsMask` to
reduce the potential that the "all clients" mask falls out of sync with the
enum. Doing that requires the second check.

[jochen (gone - plz use gerrit), 2012-01-19 13:11:19]

browser data remover stuff lgtm

[kinuko, 2012-01-19 13:53:11]

Looking good (if the tests to be added pass :)).

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:111: current->quota_client_mask ==
QuotaClient::kAllClientsMask))
the second check is not necessary?

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:483: quota_client_mask_ ==
QuotaClient::kAllClientsMask) {
On 2012/01/19 13:09:11, Mike West (chromium) wrote:
> On 2012/01/18 23:07:48, michaeln wrote:
> > is the second condition needed or will the first eval to true in all cases
> where
> > the second would?
> 
> In an earlier review, Kinuko suggested using -1 for the `kAllClientsMask` to
> reduce the potential that the "all clients" mask falls out of sync with the
> enum. Doing that requires the second check.

If quota_client_mask_ is kAllClientsMask (-1) the first check always pass,
doesn't it?

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager.h
File webkit/quota/quota_manager.h (right):

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager....
webkit/quota/quota_manager.h:151: // Called by UI.
Can we add a brief comment about the quota_client_mask?

[Mike West, 2012-01-19 16:50:59]

Thanks Kinuko! I've added some tests for the QuotaManager. I also made some
small changes to MockStorageType so that I can set a non-arbitrary ID, and to
the unit tests for the same reason. Do those changes look alright to you?

Thanks!

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:111: current->quota_client_mask ==
QuotaClient::kAllClientsMask))
On 2012/01/19 13:53:11, kinuko wrote:
> the second check is not necessary?

Done.

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:483: quota_client_mask_ ==
QuotaClient::kAllClientsMask) {
On 2012/01/19 13:53:11, kinuko wrote:

> If quota_client_mask_ is kAllClientsMask (-1) the first check always pass,
> doesn't it?

... I need to go back to bitwise-school. You're right. I'm dropping the second
test. :)

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager.h
File webkit/quota/quota_manager.h (right):

http://codereview.chromium.org/7839029/diff/16001/webkit/quota/quota_manager....
webkit/quota/quota_manager.h:151: // Called by UI.
On 2012/01/19 13:53:11, kinuko wrote:
> Can we add a brief comment about the quota_client_mask?

Done.

[kinuko, 2012-01-22 18:35:27]

I haven't read all the new code yet but sending some comments earlier (before I
forget!)

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:110: current->quota_client_mask &
quota_client)
style-nit: to improve readability can we put each condition in different line?

current->origin == origin &&
current->type == type &&
current->quota_client_mask & quota_client

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:131: void
MockQuotaManager::DeleteOriginData(const GURL& origin, StorageType type,
style-nit: please break the line after '(' and align all the args with 4 spaces
for the sake of style consistency (here and above)

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager.h (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:53: // GetOriginsModifiedSince.
nit: can we also add a brief comment for |quota_client_mask| here and below? 
(That'll help others to modify the code)

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_storage_c...
File webkit/quota/mock_storage_client.cc (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.cc:50:
id_(MockStorageClientIDSequencer::GetInstance()->NextMockID()),
I suspect this NextModkID() routine no longer works?

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_storage_c...
File webkit/quota/mock_storage_client.h (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:79: void Populate_(const MockOriginData*
mock_data, size_t mock_data_size);
Why does this method name end with '_'?  (It doesn't look very common in the
chrome codebase?)

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:1337: quota_client_mask == QuotaClient::kUnknown)
{
Not sure this is really meaningful for a bitmask?

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:1549: 
nit: extra empty line

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/quota_manager.h
File webkit/quota/quota_manager.h (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/quota_manager....
webkit/quota/quota_manager.h:155: // specified as a bitmask built from
QuotaClient::IDs.
It might be helpful to note that the caller specify the wildcard mask
(kAllClientsMask) to delete all.

[Mike West, 2012-01-22 20:54:37]

Thanks Kinuko!

The only item I didn't address is your comment regarding `NextMockID()`. What
would you like me to do there?

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:110: current->quota_client_mask &
quota_client)
On 2012/01/22 18:35:31, kinuko wrote:
> style-nit: to improve readability can we put each condition in different line?
> 
> current->origin == origin &&
> current->type == type &&
> current->quota_client_mask & quota_client

Done.

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:131: void
MockQuotaManager::DeleteOriginData(const GURL& origin, StorageType type,
On 2012/01/22 18:35:31, kinuko wrote:
> style-nit: please break the line after '(' and align all the args with 4
spaces
> for the sake of style consistency (here and above)

Done.

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager.h (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:53: // GetOriginsModifiedSince.
On 2012/01/22 18:35:31, kinuko wrote:
> nit: can we also add a brief comment for |quota_client_mask| here and below? 
> (That'll help others to modify the code)

Done.

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_storage_c...
File webkit/quota/mock_storage_client.cc (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.cc:50:
id_(MockStorageClientIDSequencer::GetInstance()->NextMockID()),
On 2012/01/22 18:35:31, kinuko wrote:
> I suspect this NextModkID() routine no longer works?

Why not?

Honestly, I'm not sure it was used in the first place, so I wouldn't mind
ripping it out if you like. Specifying the ID manually makes more sense to me,
so if there's not a good reason not to...

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_storage_c...
File webkit/quota/mock_storage_client.h (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:79: void Populate_(const MockOriginData*
mock_data, size_t mock_data_size);
On 2012/01/22 18:35:31, kinuko wrote:
> Why does this method name end with '_'?  (It doesn't look very common in the
> chrome codebase?)

Probably because I've been writing too much JavaScript recently, where "private"
doesn't exist, but is signified with an underscore. *cough* Fixing that here. :)

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:1337: quota_client_mask == QuotaClient::kUnknown)
{
On 2012/01/22 18:35:31, kinuko wrote:
> Not sure this is really meaningful for a bitmask?

Actually, this doesn't make any sense at all. I added it in, thinking that
deleting _only_ kUnknown would be a no-op, but there's no reason that should be
the case. Dropping the check entirely. :)

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:1549: 
On 2012/01/22 18:35:31, kinuko wrote:
> nit: extra empty line

Done.

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/quota_manager.h
File webkit/quota/quota_manager.h (right):

http://codereview.chromium.org/7839029/diff/25002/webkit/quota/quota_manager....
webkit/quota/quota_manager.h:155: // specified as a bitmask built from
QuotaClient::IDs.
On 2012/01/22 18:35:31, kinuko wrote:
> It might be helpful to note that the caller specify the wildcard mask
> (kAllClientsMask) to delete all.

Good call. Done, both here and in MockQuotaManager.

[Mike West, 2012-01-25 13:57:18]

By the way, don't stress about this CL. It doesn't need to be in for M18, so I'm
perfectly happy if you don't look at it again until after the branch point. :)

[Mike West, 2012-02-03 14:56:03]

On 2012/01/25 13:57:18, Mike West (chromium) wrote:
> By the way, don't stress about this CL. It doesn't need to be in for M18, so
I'm
> perfectly happy if you don't look at it again until after the branch point. :)

Ok Kinuko, now you should stress this CL again! Kinda. :)

It's not terribly high priority, but I'd like to land it early enough in the
M19-cycle so I can move the `clear`/`browsingData` API out of experimental.
Thanks!

[kinuko, 2012-02-09 14:43:33]

Sorry for my super late review, lgtm except for some nits.

http://codereview.chromium.org/7839029/diff/54001/webkit/quota/mock_storage_c...
File webkit/quota/mock_storage_client.cc (right):

http://codereview.chromium.org/7839029/diff/54001/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.cc:66: void MockStorageClient::Populate(const
MockOriginData* mock_data,
style-nit: please break the line after '(' (or align the line 67 at the same
position with 'const MockOriginData...' on line 66

http://codereview.chromium.org/7839029/diff/54001/webkit/quota/mock_storage_c...
File webkit/quota/mock_storage_client.h (right):

http://codereview.chromium.org/7839029/diff/54001/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:34: size_t mock_data_size);
Do we still need this constructor?

http://codereview.chromium.org/7839029/diff/54001/webkit/quota/quota_client.h
File webkit/quota/quota_client.h (right):

http://codereview.chromium.org/7839029/diff/54001/webkit/quota/quota_client.h...
webkit/quota/quota_client.h:38: kMockStart = 1 << 5,  // This needs to be the
end of the enum.
If we remove the MockQuotaClient's 1st constructor can we remove this one as
well?

http://codereview.chromium.org/7839029/diff/54001/webkit/quota/quota_manager_...
File webkit/quota/quota_manager_unittest.cc (right):

http://codereview.chromium.org/7839029/diff/54001/webkit/quota/quota_manager_...
webkit/quota/quota_manager_unittest.cc:75: const MockOriginData* mock_data,
size_t mock_data_size,
style-nit: break the line after 'mock_data,' if all args don't fit into a single
line

[Mike West, 2012-02-09 14:55:00]

On 2012/02/09 14:43:33, kinuko wrote:
> Sorry for my super late review, lgtm except for some nits.

Much appreciated.

I'll fix the style nits, but I'd like to deal with removing `kMockStart` in a
separate CL. I've filed http://crbug.com/113380 to deal with it, and I'll have a
CL for you by the end of the day tomorrow.

Thanks!

[commit-bot: I haz the power, 2012-02-09 18:15:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/7839029/59002

[commit-bot: I haz the power, 2012-02-09 19:40:28]

Change committed as 121269