Use base::Callback in Quota related code.

Use base::Callback in Quota related code.

BUG=None
TEST='Compilation should finished successfull and all quota related tests should be passed.'

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

[tzik, 2011-09-29 02:32:33]

Hi.

I'm trying to replace callback_old.h with new callback.h in webkit/quota and
related code.
Could you check it (or redirect if needed)?

kinuko: webkit/quota, webkit/fileapi
micheln: webkit/appcache, webkit/database
jam: content/browser
arv: chrome/browser

[jam, 2011-09-29 16:36:27]

content/browser/renderer_host lgtm, I defer to michaeln on the other files in
content since he's in the OWNERS file for them

[awong, 2011-09-29 18:05:15]

Wow...big CL!

Thanks for doing such a large chunk!

I added a number of in-line comments, but it basically breaks down into 2 style
issues:
  1) base::Callback<> should be passed by const-reference not copy.
  2) There are some places where we missed changing over NewRunnableMehtod to
base::Bind.

and a couple of suggested refactoring.  For instance, I think you can remove
some/all uses of QuotaTask with PostTaskAndReply.  See comment here:
 
http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...

And then for some custom delayed execution structs, I think you can also replace
them completely by storing base::Closure objects.  See comment here:
 
http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_quo...

For these last two points, feel free to address them in a separate CL since this
one is so large.

-Albert

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
File content/browser/in_process_webkit/indexed_db_context.cc (right):

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_context.cc:297:
NewRunnableMethod(this,
Use Base::Bind here as well?

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
File content/browser/in_process_webkit/indexed_db_quota_client.cc (right):

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.cc:34: class
IndexedDBQuotaClient::DeleteOriginTask : public HelperTask {
Could this be simplified with PostTaskAndReply?

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.cc:235:
scoped_refptr<DeleteOriginTask> task(
I'm thinking something like

webkit_thread_message_loop_proxy_->PostTaskAndReply(
  FROM_HERE,
  base::Bind(&IndexDBContext::DeleteIndexedDBForOrigin, indexed_db_context_,
origin_url),
  base::Bind(callback_, quota::kQuotaStatusOk));

For that last bind, you'd need this CL to be checked in first: 
http://codereview.chromium.org/8073012/  but I think that'll go in today, and if
PostTaskAndReply is good enough, it'd get rid of the whole HelperTask thing.

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
File content/browser/in_process_webkit/indexed_db_quota_client.h (right):

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.h:35: GetUsageCallback
callback) OVERRIDE;
It's better to pass the Callback by const &.

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.h:37:
GetOriginsCallback callback) OVERRIDE;
pass by const &

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.h:40:
GetOriginsCallback callback) OVERRIDE;
pass by const &

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.h:43: DeletionCallback
callback) OVERRIDE;
pass by const &

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_quo...
File webkit/appcache/appcache_quota_client.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_quo...
webkit/appcache/appcache_quota_client.cc:204: UsageRequest& request =
pending_usage_requests_.front();
Hmm...I wonder if you could replace all these request structs a Callback.

So, up in GetOriginHelper(), youd have:
pending_origins_request_.push_back(
  base::Bind(&AppCacheQuotaClient::GetOriginUsage,
  base::Unretained(this),  // Safe since this class outlives the callback.
  opt_host, type, callback);

Then here, you'd do:
  base::Closure request = pending_usage_requests_.front();
  request.Run();
  pending_usage_requests_.pop_front();

Then get rid of Abort, since all these GetOriginUsage, GetOriginsHelper, an
DeleteOriginData all check service_ before running and invoke the Callback with
the correct behavior anyways.
  
And then you can actually throw all of the requests into one queue, assuming you
don't need to preserve the batching behavior where all the
pending_usage_requests run before the pending_origins_requests, etc.

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_sto...
File webkit/appcache/appcache_storage_impl_unittest.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl_unittest.cc:134: GetUsageAndQuotaCallback
callback) OVERRIDE {
const ref.

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl_unittest.cc:139: NewRunnableMethod(this,
&MockQuotaManager::CallCallback,
base::Bind?

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl_unittest.cc:143: CallCallback(callback);
Maybe just do callback.Run(quota::kQuotaStatusOk, 0, kMockQuota); here?

http://codereview.chromium.org/8070001/diff/1052/webkit/database/database_quo...
File webkit/database/database_quota_client.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/database/database_quo...
webkit/database/database_quota_client.h:37: GetUsageCallback callback) OVERRIDE;
const &

http://codereview.chromium.org/8070001/diff/1052/webkit/database/database_quo...
webkit/database/database_quota_client.h:39: GetOriginsCallback callback)
OVERRIDE;
const &

http://codereview.chromium.org/8070001/diff/1052/webkit/database/database_quo...
webkit/database/database_quota_client.h:42: GetOriginsCallback callback) 
OVERRIDE;
const &

http://codereview.chromium.org/8070001/diff/1052/webkit/database/database_quo...
webkit/database/database_quota_client.h:45: DeletionCallback callback) OVERRIDE;
const &

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation.h:114:
quota::QuotaManager::GetUsageAndQuotaCallback callback);
const &

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_unittest.cc:54: GetUsageAndQuotaCallback
callback) OVERRIDE {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_write_unittest.cc:52:
GetUsageAndQuotaCallback callback) OVERRIDE {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
File webkit/fileapi/file_system_quota_client.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.cc:141: DeletionCallback callback)
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.cc:217: GetOriginsCallback callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.cc:240: GetOriginsCallback callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
File webkit/fileapi/file_system_quota_client.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.h:46: GetUsageCallback callback)
OVERRIDE;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.h:48: GetOriginsCallback callback)
OVERRIDE;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.h:51: GetOriginsCallback callback)
OVERRIDE;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
File webkit/quota/mock_quota_manager.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.cc:26: GetOriginsCallback callback)
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.cc:54: StatusCallback callback)
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.cc:114: base::Time modified_since,
GetOriginsCallback callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.cc:127: StatusCallback callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
File webkit/quota/mock_quota_manager.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.h:64: GetOriginsCallback callback) OVERRIDE;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.h:70: StatusCallback callback) OVERRIDE;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
File webkit/quota/mock_storage_client.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:110: GetUsageCallback callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:112: FROM_HERE,
runnable_factory_.NewRunnableMethod(
weak_ptr + base::Bind?  Or is that for another pass?

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:118: StorageType type, GetOriginsCallback
callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:127: GetOriginsCallback callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:144: const GURL& origin_url, StorageType
type, GetUsageCallback callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:154: StorageType type, GetOriginsCallback
callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:166: GetOriginsCallback callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:180: DeletionCallback callback) {
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
File webkit/quota/mock_storage_client.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:52: GetUsageCallback callback) OVERRIDE;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:54: GetOriginsCallback callback) OVERRIDE;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:56: GetOriginsCallback callback) OVERRIDE;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:59: DeletionCallback callback) OVERRIDE;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:64: GetUsageCallback callback);
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:66: GetOriginsCallback callback);
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:69: GetOriginsCallback callback);
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:72: DeletionCallback callback);
const &.

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

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:25: typedef base::Callback<void (int64)>
GetUsageCallback;  // NOLINT
prevailing style seems to not insert a space between the return value and the
arg list.  So

base::Callback<void(int64)>

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:51: GetUsageCallback callback) = 0;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:56: GetOriginsCallback callback) = 0;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:62: GetOriginsCallback callback) = 0;
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:67: DeletionCallback callback) = 0;
const &.

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

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:54: typedef base::Callback<void (const GURL&)>
GetLRUOriginCallback;
Remove space after void.  Here and elsewhere in file.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:107: GetUsageAndQuotaCallback callback);
const &.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:136: StatusCallback callback);
const &.

okay...I give up.  Const& whenever you're passing the callback.  Only take a
copy if you intend to capture it and store it somewhere.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_types.h
File webkit/quota/quota_types.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_types.h#n...
webkit/quota/quota_types.h:39: typedef base::Callback<void (StorageType, int64)>
UsageCallback;
remove space after void.

[awong, 2011-09-30 07:37:55]

FYI, http://codereview.chromium.org/8073012/ is in the commit queue.  After
that's in, it will be possible to take a Callback and Bind it all the way down
into a Closure like one of my comment suggested last time.

-Albert

[michaeln, 2011-10-03 18:51:27]

lgtm... wow... big cl is right!

can you also run this thru a valgrind trybot?

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

http://codereview.chromium.org/8070001/diff/12001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:552: AddRef();  // balanced in the
OnQuotaCallback
Is the manual AddRef() and balancing Release() needed if base::Unretained is
removed from the 'this' parameter below?

[michaeln, 2011-10-03 22:44:41]

Also, given the size of the CL already, i think it may be preferable to not make
additional changes in here, and instead do further work in separate CLs.

[awong, 2011-10-03 23:40:16]

LGTM

Agreed with Michael. Looks good to check in as is.

For the other comments I made in the previous pass, feel free to address them in
another CL, or ignore them if you think it is not worth the effort.

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

http://codereview.chromium.org/8070001/diff/12001/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:9: #include <utility>
alphabetical order.

[awong, 2011-10-07 01:34:54]

gentle ping :)

Anything I can do to help get this checked in?

On 2011/10/03 23:40:16, awong wrote:
> LGTM
> 
> Agreed with Michael. Looks good to check in as is.
> 
> For the other comments I made in the previous pass, feel free to address them
in
> another CL, or ignore them if you think it is not worth the effort.
> 
>
http://codereview.chromium.org/8070001/diff/12001/webkit/quota/mock_storage_c...
> File webkit/quota/mock_storage_client.h (right):
> 
>
http://codereview.chromium.org/8070001/diff/12001/webkit/quota/mock_storage_c...
> webkit/quota/mock_storage_client.h:9: #include <utility>
> alphabetical order.

[kinuko, 2011-10-10 15:58:06]

LGTM for the changes under quota/.
+1 to run on this valgrind and +1 to make them another CL for task- cleanup
changes.

Great work, thanks for doing this!

On 2011/10/07 01:34:54, awong wrote:
> gentle ping :)

Last week half of tokyo team was ooo for hackathon so I believe he'll be
returning to this change this week :)

> Anything I can do to help get this checked in?
> On 2011/10/03 23:40:16, awong wrote:
> > LGTM
> > 
> > Agreed with Michael. Looks good to check in as is.
> > 
> > For the other comments I made in the previous pass, feel free to address
them
> in
> > another CL, or ignore them if you think it is not worth the effort.
> > 
> >
>
http://codereview.chromium.org/8070001/diff/12001/webkit/quota/mock_storage_c...
> > File webkit/quota/mock_storage_client.h (right):
> > 
> >
>
http://codereview.chromium.org/8070001/diff/12001/webkit/quota/mock_storage_c...
> > webkit/quota/mock_storage_client.h:9: #include <utility>
> > alphabetical order.

[tzik, 2011-10-11 04:53:57]

Sorry for late submission, and thanks for your patience.
Changes from previous my comment are:
 - style fix,
 - using const ref instead of value.

Task to Bind replacement will be done in other CL.

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
File content/browser/in_process_webkit/indexed_db_context.cc (right):

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_context.cc:297:
NewRunnableMethod(this,
On 2011/09/29 18:05:15, awong wrote:
> Use Base::Bind here as well?

Will be done later.

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
File content/browser/in_process_webkit/indexed_db_quota_client.cc (right):

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.cc:34: class
IndexedDBQuotaClient::DeleteOriginTask : public HelperTask {
On 2011/09/29 18:05:15, awong wrote:
> Could this be simplified with PostTaskAndReply?

Later.

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.cc:235:
scoped_refptr<DeleteOriginTask> task(
On 2011/09/29 18:05:15, awong wrote:
> I'm thinking something like
> 
> webkit_thread_message_loop_proxy_->PostTaskAndReply(
>   FROM_HERE,
>   base::Bind(&IndexDBContext::DeleteIndexedDBForOrigin, indexed_db_context_,
> origin_url),
>   base::Bind(callback_, quota::kQuotaStatusOk));
> 
> For that last bind, you'd need this CL to be checked in first: 
> http://codereview.chromium.org/8073012/  but I think that'll go in today, and
if
> PostTaskAndReply is good enough, it'd get rid of the whole HelperTask thing.

Sounds good. But this CL is already too big, let me do it in other CL.

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
File content/browser/in_process_webkit/indexed_db_quota_client.h (right):

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.h:35: GetUsageCallback
callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> It's better to pass the Callback by const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.h:37:
GetOriginsCallback callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> pass by const &

Done.

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.h:40:
GetOriginsCallback callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> pass by const &

Done.

http://codereview.chromium.org/8070001/diff/1052/content/browser/in_process_w...
content/browser/in_process_webkit/indexed_db_quota_client.h:43: DeletionCallback
callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> pass by const &

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_quo...
File webkit/appcache/appcache_quota_client.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_quo...
webkit/appcache/appcache_quota_client.cc:204: UsageRequest& request =
pending_usage_requests_.front();
On 2011/09/29 18:05:15, awong wrote:
> Hmm...I wonder if you could replace all these request structs a Callback.
> 
> So, up in GetOriginHelper(), youd have:
> pending_origins_request_.push_back(
>   base::Bind(&AppCacheQuotaClient::GetOriginUsage,
>   base::Unretained(this),  // Safe since this class outlives the callback.
>   opt_host, type, callback);
> 
> Then here, you'd do:
>   base::Closure request = pending_usage_requests_.front();
>   request.Run();
>   pending_usage_requests_.pop_front();
> 
> Then get rid of Abort, since all these GetOriginUsage, GetOriginsHelper, an
> DeleteOriginData all check service_ before running and invoke the Callback
with
> the correct behavior anyways.
>   
> And then you can actually throw all of the requests into one queue, assuming
you
> don't need to preserve the batching behavior where all the
> pending_usage_requests run before the pending_origins_requests, etc.

Sounds great. But let me do in another CL.

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_sto...
File webkit/appcache/appcache_storage_impl_unittest.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl_unittest.cc:134: GetUsageAndQuotaCallback
callback) OVERRIDE {
On 2011/09/29 18:05:15, awong wrote:
> const ref.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/appcache/appcache_sto...
webkit/appcache/appcache_storage_impl_unittest.cc:139: NewRunnableMethod(this,
&MockQuotaManager::CallCallback,
On 2011/09/29 18:05:15, awong wrote:
> base::Bind?

Later.

http://codereview.chromium.org/8070001/diff/1052/webkit/database/database_quo...
File webkit/database/database_quota_client.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/database/database_quo...
webkit/database/database_quota_client.h:37: GetUsageCallback callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/database/database_quo...
webkit/database/database_quota_client.h:39: GetOriginsCallback callback)
OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/database/database_quo...
webkit/database/database_quota_client.h:42: GetOriginsCallback callback) 
OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/database/database_quo...
webkit/database/database_quota_client.h:45: DeletionCallback callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation.h:114:
quota::QuotaManager::GetUsageAndQuotaCallback callback);
On 2011/09/29 18:05:15, awong wrote:
> const &

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_unittest.cc:54: GetUsageAndQuotaCallback
callback) OVERRIDE {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation_write_unittest.cc:52:
GetUsageAndQuotaCallback callback) OVERRIDE {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
File webkit/fileapi/file_system_quota_client.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.cc:141: DeletionCallback callback)
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.cc:217: GetOriginsCallback callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.cc:240: GetOriginsCallback callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
File webkit/fileapi/file_system_quota_client.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.h:46: GetUsageCallback callback)
OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.h:48: GetOriginsCallback callback)
OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_client.h:51: GetOriginsCallback callback)
OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
File webkit/quota/mock_quota_manager.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.cc:26: GetOriginsCallback callback)
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.cc:54: StatusCallback callback)
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.cc:114: base::Time modified_since,
GetOriginsCallback callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.cc:127: StatusCallback callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
File webkit/quota/mock_quota_manager.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.h:64: GetOriginsCallback callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_quota_mana...
webkit/quota/mock_quota_manager.h:70: StatusCallback callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
File webkit/quota/mock_storage_client.cc (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:110: GetUsageCallback callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:112: FROM_HERE,
runnable_factory_.NewRunnableMethod(
On 2011/09/29 18:05:15, awong wrote:
> weak_ptr + base::Bind?  Or is that for another pass?

Yes, I'll do it later.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:118: StorageType type, GetOriginsCallback
callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:127: GetOriginsCallback callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:144: const GURL& origin_url, StorageType
type, GetUsageCallback callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:154: StorageType type, GetOriginsCallback
callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:166: GetOriginsCallback callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.cc:180: DeletionCallback callback) {
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
File webkit/quota/mock_storage_client.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:52: GetUsageCallback callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:54: GetOriginsCallback callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:56: GetOriginsCallback callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:59: DeletionCallback callback) OVERRIDE;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:64: GetUsageCallback callback);
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:66: GetOriginsCallback callback);
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:69: GetOriginsCallback callback);
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/mock_storage_cl...
webkit/quota/mock_storage_client.h:72: DeletionCallback callback);
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

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

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:25: typedef base::Callback<void (int64)>
GetUsageCallback;  // NOLINT
On 2011/09/29 18:05:15, awong wrote:
> prevailing style seems to not insert a space between the return value and the
> arg list.  So
> 
> base::Callback<void(int64)>

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:51: GetUsageCallback callback) = 0;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:56: GetOriginsCallback callback) = 0;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:62: GetOriginsCallback callback) = 0;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_client.h#...
webkit/quota/quota_client.h:67: DeletionCallback callback) = 0;
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

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

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:54: typedef base::Callback<void (const GURL&)>
GetLRUOriginCallback;
On 2011/09/29 18:05:15, awong wrote:
> Remove space after void.  Here and elsewhere in file.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:107: GetUsageAndQuotaCallback callback);
On 2011/09/29 18:05:15, awong wrote:
> const &.

Done.

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:136: StatusCallback callback);
On 2011/09/29 18:05:15, awong wrote:
> const &.
> 
> okay...I give up.  Const& whenever you're passing the callback.  Only take a
> copy if you intend to capture it and store it somewhere.

Done for all (I hope).
It's too boring...

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_types.h
File webkit/quota/quota_types.h (right):

http://codereview.chromium.org/8070001/diff/1052/webkit/quota/quota_types.h#n...
webkit/quota/quota_types.h:39: typedef base::Callback<void (StorageType, int64)>
UsageCallback;
On 2011/09/29 18:05:15, awong wrote:
> remove space after void.

Done.

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

http://codereview.chromium.org/8070001/diff/12001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:552: AddRef();  // balanced in the
OnQuotaCallback
On 2011/10/03 18:51:27, michaeln wrote:
> Is the manual AddRef() and balancing Release() needed if base::Unretained is
> removed from the 'this' parameter below?

TODO in another CL.

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

http://codereview.chromium.org/8070001/diff/12001/webkit/quota/mock_storage_c...
webkit/quota/mock_storage_client.h:9: #include <utility>
On 2011/10/03 23:40:16, awong wrote:
> alphabetical order.

Done.

[tzik, 2011-10-11 08:24:19]

Valgrind blames a leak, but there is a leak without my change.
I'll retry later.

[tzik, 2011-10-12 05:20:47]

passed valgrind try.

+estade
Could you review chrome/browser/* of this CL?

[tzik, 2011-10-18 06:42:46]

This CL has some dependents and not lgtm'ed part of CL is trivial.
So, I'll commit it after tests passed.

If you mind, prevent me.