Refactors DatabaseQuotaClient using base::Callback.

Refactors DatabaseQuotaClient using base::Callback.

BUG=139270
TEST=content_unittests


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

[kinuko, 2012-08-14 13:42:15]

lgtm, but please wait michaeln@'s comment.

http://codereview.chromium.org/10831305/diff/1/webkit/database/database_quota...
File webkit/database/database_quota_client.cc (right):

http://codereview.chromium.org/10831305/diff/1/webkit/database/database_quota...
webkit/database/database_quota_client.cc:26: int64 GetOriginUsageOnTargetThread(
nit: maybe TargetThread -> DBThread?

http://codereview.chromium.org/10831305/diff/1/webkit/database/database_quota...
webkit/database/database_quota_client.cc:31:
DatabaseUtil::GetOriginIdentifier(origin_url), &info))
nit: indent is off

http://codereview.chromium.org/10831305/diff/1/webkit/database/database_quota...
webkit/database/database_quota_client.cc:68: quota::StorageType)>& callback,
nit: if Callback type spans multiple lines having typedef might be better for
readability.

[nhiroki, 2012-08-15 02:12:53]

Thank you for reviewing.
I see. I wait for his comment.

http://codereview.chromium.org/10831305/diff/1/webkit/database/database_quota...
File webkit/database/database_quota_client.cc (right):

http://codereview.chromium.org/10831305/diff/1/webkit/database/database_quota...
webkit/database/database_quota_client.cc:26: int64 GetOriginUsageOnTargetThread(
On 2012/08/14 13:42:15, kinuko wrote:
> nit: maybe TargetThread -> DBThread?

Done.

http://codereview.chromium.org/10831305/diff/1/webkit/database/database_quota...
webkit/database/database_quota_client.cc:31:
DatabaseUtil::GetOriginIdentifier(origin_url), &info))
On 2012/08/14 13:42:15, kinuko wrote:
> nit: indent is off

Done.

http://codereview.chromium.org/10831305/diff/1/webkit/database/database_quota...
webkit/database/database_quota_client.cc:68: quota::StorageType)>& callback,
On 2012/08/14 13:42:15, kinuko wrote:
> nit: if Callback type spans multiple lines having typedef might be better for
> readability.

Done.

[michaeln, 2012-08-16 21:39:48]

https://chromiumcodereview.appspot.com/10831305/diff/1005/webkit/database/dat...
File webkit/database/database_quota_client.cc (right):

https://chromiumcodereview.appspot.com/10831305/diff/1005/webkit/database/dat...
webkit/database/database_quota_client.cc:143:
base::Bind(&GetOriginsForTypeOnDBThread,
naming nit: Maybe call the helper method GetAllOriginsOnDBThread since we've
filtered for 'type' already and there is no 'type' parameter at this point.

https://chromiumcodereview.appspot.com/10831305/diff/1005/webkit/database/dat...
webkit/database/database_quota_client.cc:198: base::Bind(&DidDeleteOriginData,
callback));
Not sure this PostTaskAndReplyWithResult usage is correct from a threading point
of view. The 'callback' should be invoked in the current thread (the IO thread)
in all cases.

* in the async return case, 'callback' will be run on the current thread by
virtue of the Reply handling... so this much looks good.

* but.. in the immediate return case, 'callback' will be run directly on the
db_tracker_thread w/o going thru the reply task posting stuff... which looks
like a bug

[michaeln, 2012-08-16 22:16:34]

https://chromiumcodereview.appspot.com/10831305/diff/1005/webkit/database/dat...
File webkit/database/database_quota_client.cc (right):

https://chromiumcodereview.appspot.com/10831305/diff/1005/webkit/database/dat...
webkit/database/database_quota_client.cc:198: base::Bind(&DidDeleteOriginData,
callback));
On 2012/08/16 21:39:48, michaeln wrote:
> Not sure this PostTaskAndReplyWithResult usage is correct from a threading
point
> of view. The 'callback' should be invoked in the current thread (the IO
thread)
> in all cases.
> 
> * in the async return case, 'callback' will be run on the current thread by
> virtue of the Reply handling... so this much looks good.
> 
> * but.. in the immediate return case, 'callback' will be run directly on the
> db_tracker_thread w/o going thru the reply task posting stuff... which looks
> like a bug

I mixed up the async vs immediate cases in the prev comment. Its the async case
(where the dbtracker doesn't delete things immediately) that looks like
'callback' will be run on the wrong thread. In that case the bind closure
created on line 197 will be used on the background thread.

[kinuko, 2012-08-20 06:41:38]

https://chromiumcodereview.appspot.com/10831305/diff/1005/webkit/database/dat...
File webkit/database/database_quota_client.cc (right):

https://chromiumcodereview.appspot.com/10831305/diff/1005/webkit/database/dat...
webkit/database/database_quota_client.cc:198: base::Bind(&DidDeleteOriginData,
callback));
On 2012/08/16 22:16:34, michaeln wrote:
> On 2012/08/16 21:39:48, michaeln wrote:
> > Not sure this PostTaskAndReplyWithResult usage is correct from a threading
> point
> > of view. The 'callback' should be invoked in the current thread (the IO
> thread)
> > in all cases.
> > 
> > * in the async return case, 'callback' will be run on the current thread by
> > virtue of the Reply handling... so this much looks good.
> > 
> > * but.. in the immediate return case, 'callback' will be run directly on the
> > db_tracker_thread w/o going thru the reply task posting stuff... which looks
> > like a bug
> 
> I mixed up the async vs immediate cases in the prev comment. Its the async
case
> (where the dbtracker doesn't delete things immediately) that looks like
> 'callback' will be run on the wrong thread. In that case the bind closure
> created on line 197 will be used on the background thread.

Thanks for catching this!  In the original code we used to post on the original
thread to call completion task in DeleteOriginTask::OnCompletionCallback but in
the new code we directly pass the callback, which will result in what michaeln
said.  Maybe we can pass/bind the origintal task runner to the closure to make
sure we always run the callback on the correct runner.

[nhiroki, 2012-08-20 09:32:23]

Thank you for reviewing.
Please take another look, thanks.

http://codereview.chromium.org/10831305/diff/1005/webkit/database/database_qu...
File webkit/database/database_quota_client.cc (right):

http://codereview.chromium.org/10831305/diff/1005/webkit/database/database_qu...
webkit/database/database_quota_client.cc:143:
base::Bind(&GetOriginsForTypeOnDBThread,
On 2012/08/16 21:39:48, michaeln wrote:
> naming nit: Maybe call the helper method GetAllOriginsOnDBThread since we've
> filtered for 'type' already and there is no 'type' parameter at this point.

Done.

http://codereview.chromium.org/10831305/diff/1005/webkit/database/database_qu...
webkit/database/database_quota_client.cc:198: base::Bind(&DidDeleteOriginData,
callback));
As kinuko@ mentioned, I modified to pass the original task runner and always run
the callback on the original thread. Done.

[michaeln, 2012-08-20 20:03:29]

LGTM

[commit-bot: I haz the power, 2012-08-21 01:00:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/10831305/11003

[kinuko, 2012-08-21 03:24:25]

lgtm2

[commit-bot: I haz the power, 2012-08-21 03:26:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/10831305/11003

[commit-bot: I haz the power, 2012-08-21 05:40:45]

Change committed as 152514