Quota: Add quota::StorageType to the GetOriginsCallback definition.

Quota: Add quota::StorageType to the GetOriginsCallback definition.

BUG=90996
TEST=test_shell_tests


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

[Mike West, 2011-07-29 09:52:45]

Hi Kinuko, Michael,

This is a first pass at adding `quota::StorageType` to the `GetOriginsCallback`
definition. I want to add some more tests to the various QuotaClient types, but
this is the core of what needs to be done.

If you're still happy with the idea, I'll rebase this on top of the BDR change
we're discussing in the other CL (but I still do think they should land
separately).

Thanks!

-Mike

[kinuko, 2011-07-29 11:12:36]

lgtm

http://codereview.chromium.org/7533013/diff/1/webkit/appcache/appcache_quota_...
File webkit/appcache/appcache_quota_client_unittest.cc (right):

http://codereview.chromium.org/7533013/diff/1/webkit/appcache/appcache_quota_...
webkit/appcache/appcache_quota_client_unittest.cc:141: quota::StorageType type)
{
nit: indent

either one of
OnGetOriginsComplete(const std::set<GURL>& origins,
                     quota::StorageType type) {
or
void OnGetOriginsComplete(
   const std::set<GURL>& origins, quota::StorageType type) {

http://codereview.chromium.org/7533013/diff/1/webkit/database/database_quota_...
File webkit/database/database_quota_client.h (right):

http://codereview.chromium.org/7533013/diff/1/webkit/database/database_quota_...
webkit/database/database_quota_client.h:75: quota::StorageType type);
nit: indent
maybe we should put all the params in different lines

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

http://codereview.chromium.org/7533013/diff/2001/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:62: GetOriginsCallback* callback) = 0;
For this one it might look a bit strange to have only set<GURL> and type as
callback parameters while we get origins for given {type,host}.  (While
returning type sounds reasonable for other methods that take GetOriginsCallback,
i.e. GetOriginsForType and GetOriginsModifiedSince).

Alternatively you could make this change local to GetOriginsModifiedSince by
defining a new callback type that is only used by the method.

[kinuko, 2011-07-29 12:13:15]

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

http://codereview.chromium.org/7533013/diff/2001/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:62: GetOriginsCallback* callback) = 0;
On 2011/07/29 11:12:37, kinuko wrote:
> For this one it might look a bit strange to have only set<GURL> and type as
> callback parameters while we get origins for given {type,host}.  (While
> returning type sounds reasonable for other methods that take
GetOriginsCallback,
> i.e. GetOriginsForType and GetOriginsModifiedSince).

As we chatted offline, on the second thought it might be ok not to have host for
this method either, since basically the host info is contained in the origin. 
So... I think the patch as is looks good to me.

> Alternatively you could make this change local to GetOriginsModifiedSince by
> defining a new callback type that is only used by the method.

[Mike West, 2011-07-29 12:25:42]

Thanks!

http://codereview.chromium.org/7533013/diff/1/webkit/appcache/appcache_quota_...
File webkit/appcache/appcache_quota_client_unittest.cc (right):

http://codereview.chromium.org/7533013/diff/1/webkit/appcache/appcache_quota_...
webkit/appcache/appcache_quota_client_unittest.cc:141: quota::StorageType type)
{
On 2011/07/29 11:12:36, kinuko wrote:
> nit: indent
> 
> either one of
> OnGetOriginsComplete(const std::set<GURL>& origins,
>                      quota::StorageType type) {
> or
> void OnGetOriginsComplete(
>    const std::set<GURL>& origins, quota::StorageType type) {                  
 
> 

Done.

http://codereview.chromium.org/7533013/diff/1/webkit/database/database_quota_...
File webkit/database/database_quota_client.h (right):

http://codereview.chromium.org/7533013/diff/1/webkit/database/database_quota_...
webkit/database/database_quota_client.h:75: quota::StorageType type);
On 2011/07/29 11:12:36, kinuko wrote:
> nit: indent
> maybe we should put all the params in different lines

Done.

[Mike West, 2011-08-01 09:37:49]

Kinuko, can you take another look at this? I've rebased it on top of the fix for
http://crbug.com/63700 and thrown away some code in BDR. Trivial changes, I
think, but I'll need another LGTM in order to throw it into the commit queue
once the trybots are done. :)

Jochen, can you take a look specifically at the BrowsingDataRemover change?

Thanks!

-Mike

[jochen (gone - plz use gerrit), 2011-08-01 09:44:56]

LGTM with my comment addressed

http://codereview.chromium.org/7533013/diff/7001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/7533013/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover.h:154: void OnGotQuotaManagedOrigins(const
std::set<GURL>&, quota::StorageType);
all function parameters should be named

[Mike West, 2011-08-01 09:47:40]

Thanks Jochen.

http://codereview.chromium.org/7533013/diff/7001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/7533013/diff/7001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover.h:154: void OnGotQuotaManagedOrigins(const
std::set<GURL>&, quota::StorageType);
On 2011/08/01 09:44:56, jochen wrote:
> all function parameters should be named

Done.

[kinuko, 2011-08-01 11:16:19]

lgtm except for nits.  Thanks!

http://codereview.chromium.org/7533013/diff/10002/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager_unittest.cc (right):

http://codereview.chromium.org/7533013/diff/10002/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager_unittest.cc:75: int GetDeletionCallbackCount() {
nit: can all of these getter methods const?

naming-nit: I might name these simple inline getters like:

int deletion_callback_count() const;
MockQuotaManager manager() const;
const std::set<GURL>& origins() const;

http://www.chromium.org/developers/coding-style#TOC-Inline-functions

http://codereview.chromium.org/7533013/diff/10002/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager_unittest.cc:83: const std::set<GURL>
GetOrigins() {
could this return const std::set<GURL>& (a reference)?

[Mike West, 2011-08-01 12:55:22]

Thanks Kinuko!

http://codereview.chromium.org/7533013/diff/10002/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager_unittest.cc (right):

http://codereview.chromium.org/7533013/diff/10002/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager_unittest.cc:75: int GetDeletionCallbackCount() {
On 2011/08/01 11:16:19, kinuko wrote:
> nit: can all of these getter methods const?
> 
> naming-nit: I might name these simple inline getters like:
> 
> int deletion_callback_count() const;
> MockQuotaManager manager() const;
> const std::set<GURL>& origins() const;
> 
> http://www.chromium.org/developers/coding-style#TOC-Inline-functions

Done.

http://codereview.chromium.org/7533013/diff/10002/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager_unittest.cc:83: const std::set<GURL>
GetOrigins() {
On 2011/08/01 11:16:19, kinuko wrote:
> could this return const std::set<GURL>& (a reference)?

Done.

[commit-bot: I haz the power, 2011-08-02 07:34:45]

Presubmit check for 7533013-7025 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/in_process_webkit/indexed_db_quota_client_unittest.cc,content/browser/in_process_webkit/indexed_db_quota_client.h,content/browser/in_process_webkit/indexed_db_quota_client.cc

Presubmit checks took 1.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Mike West, 2011-08-02 09:02:44]

On 2011/08/02 07:34:45, I haz the power (commit-bot) wrote:

> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
>
content/browser/in_process_webkit/indexed_db_quota_client_unittest.cc,content/browser/in_process_webkit/indexed_db_quota_client.h,content/browser/in_process_webkit/indexed_db_quota_client.cc

Good morning michaeln!  Can you take a look at this?

-Mike

[michaeln, 2011-08-02 18:01:01]

LGTM

> Missing LGTM from an OWNER for:
> Good morning michaeln!  Can you take a look at this?

Oooops... I didn't realize the owners thing was in the way.

[commit-bot: I haz the power, 2011-08-05 08:39:56]

Change committed as 95591