Implement DatabaseQuotaClient's DeteleteOriginData method.

Implement DatabaseQuotaClient's DeteleteOriginData method.

TEST=database_quota_client_unittest.cc
BUG=61676
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86561

[michaeln, 2011-05-20 00:40:30]

i still have to write some tests

[kinuko, 2011-05-20 02:13:24]

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

http://codereview.chromium.org/7046019/diff/1/webkit/database/database_quota_...
webkit/database/database_quota_client.cc:150: caller_callback_.reset();  //
should i call it with the ABORT status code?
If we could assume this task wouldn't be aborted except for when client gets
deleted just resetting it should be fine (as we don't do that in fs quota client
either).

[tzik, 2011-05-20 03:19:54]

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

http://codereview.chromium.org/7046019/diff/1/webkit/database/database_quota_...
webkit/database/database_quota_client.cc:276:
callback->Run(quota::kQuotaStatusOk);
maybe callback->Run(quota::kQuotaErrorNotSupported); ?

[michaeln, 2011-05-20 05:14:23]

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

http://codereview.chromium.org/7046019/diff/1/webkit/database/database_quota_...
webkit/database/database_quota_client.cc:150: caller_callback_.reset();  //
should i call it with the ABORT status code?
On 2011/05/20 02:13:24, kinuko wrote:
> If we could assume this task wouldn't be aborted except for when client gets
> deleted just resetting it should be fine (as we don't do that in fs quota
client
> either).

keeping parity with the fsclient is good, so long as this is ok with the caller,
which i think is since clients stay around until the end.

http://codereview.chromium.org/7046019/diff/1/webkit/database/database_quota_...
webkit/database/database_quota_client.cc:276:
callback->Run(quota::kQuotaStatusOk);
On 2011/05/20 03:19:54, tzik wrote:
> maybe callback->Run(quota::kQuotaErrorNotSupported); ?

There's no way for the caller to know not to call this method or the others for
the 'perm' type. My thinking was that OK is less likely to be interpreted as
"something went wrong" by the caller. There is no error, there's just nothing to
delete for the <origin,type>.

So a little less programming error prone to return OK was my thinking.

http://codereview.chromium.org/7046019/diff/4001/webkit/database/database_quo...
webkit/database/database_quota_client.cc:10: #include "base/memory/scoped_ptr.h"
i'll fix the ordering here

[tzik, 2011-05-20 13:26:49]

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

http://codereview.chromium.org/7046019/diff/1/webkit/database/database_quota_...
webkit/database/database_quota_client.cc:276:
callback->Run(quota::kQuotaStatusOk);
On 2011/05/20 05:14:23, michaeln wrote:
> On 2011/05/20 03:19:54, tzik wrote:
> > maybe callback->Run(quota::kQuotaErrorNotSupported); ?
> 
> There's no way for the caller to know not to call this method or the others
for
> the 'perm' type. My thinking was that OK is less likely to be interpreted as
> "something went wrong" by the caller. There is no error, there's just nothing
to
> delete for the <origin,type>.
> 
> So a little less programming error prone to return OK was my thinking.
> 

I see. I totally agree with you.

[michaeln, 2011-05-21 03:23:31]

added a test, ptal

[michaeln, 2011-05-21 03:27:25]

http://codereview.chromium.org/7046019/diff/7002/webkit/database/database_quo...
File webkit/database/database_quota_client_unittest.cc (right):

http://codereview.chromium.org/7046019/diff/7002/webkit/database/database_quo...
webkit/database/database_quota_client_unittest.cc:69: virtual int
DeleteDataForOrigin(
oh... i need to include a change in database_tracker.h that makes this method
virtual to allow testing

[michaeln, 2011-05-21 03:34:46]

> webkit/database/database_quota_client_unittest.cc:69: virtual int
> DeleteDataForOrigin(
> oh... i need to include a change in database_tracker.h that makes this method
> virtual to allow testing

Done

[michaeln, 2011-05-22 21:22:33]

kinuko, i found and fixed some bugs in the DeleteTask

* it was using the is_complete flag in a non-thread safe fashion
* adde support for a quota task not really being complete after
RunOnTargetThread has returned... derived classes can call ReRegisterTask in
this case

[kinuko, 2011-05-23 04:55:53]

lgtm

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

http://codereview.chromium.org/7046019/diff/13007/webkit/database/database_qu...
webkit/database/database_quota_client.cc:167: Release();  // balanced in
RunOnTargetThread
nit: RunOnTargetThread -> ...Async (in the comment)

http://codereview.chromium.org/7046019/diff/13007/webkit/database/database_qu...
webkit/database/database_quota_client.cc:264: // All databases are in the temp
namespace for now.
Can we also say something like '... and no need to delete anything for other
types.'?  I was about to make the same comment as tzik-san did.

http://codereview.chromium.org/7046019/diff/13007/webkit/quota/quota_task.cc
File webkit/quota/quota_task.cc (right):

http://codereview.chromium.org/7046019/diff/13007/webkit/quota/quota_task.cc#...
webkit/quota/quota_task.cc:70: if (RunOnTargetThreadAsync())
when I saw the previous code I thought you would want to do something like
this...

[tzik, 2011-05-23 05:30:59]

LGTM

http://codereview.chromium.org/7046019/diff/13007/webkit/quota/quota_task.h
File webkit/quota/quota_task.h (right):

http://codereview.chromium.org/7046019/diff/13007/webkit/quota/quota_task.h#n...
webkit/quota/quota_task.h:69: // One of the following should be overriden.
Could you add some more comment for RunOnTargetThreadAsync?

[michaeln, 2011-05-23 21:56:52]

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

http://codereview.chromium.org/7046019/diff/13007/webkit/database/database_qu...
webkit/database/database_quota_client.cc:167: Release();  // balanced in
RunOnTargetThread
On 2011/05/23 04:55:53, kinuko wrote:
> nit: RunOnTargetThread -> ...Async (in the comment)

Done.

http://codereview.chromium.org/7046019/diff/13007/webkit/database/database_qu...
webkit/database/database_quota_client.cc:264: // All databases are in the temp
namespace for now.
On 2011/05/23 04:55:53, kinuko wrote:
> Can we also say something like '... and no need to delete anything for other
> types.'?  I was about to make the same comment as tzik-san did.

Done.

http://codereview.chromium.org/7046019/diff/13007/webkit/quota/quota_task.h
File webkit/quota/quota_task.h (right):

http://codereview.chromium.org/7046019/diff/13007/webkit/quota/quota_task.h#n...
webkit/quota/quota_task.h:69: // One of the following should be overriden.
On 2011/05/23 05:30:59, tzik wrote:
> Could you add some more comment for RunOnTargetThreadAsync?

Done.

  // One of the following Run methods should be overriden for execution
  // on the target thread.

  // A task to invoke the CallCompleted() method on the original thread will
  // be scheduled immediately upon return from RunOnTargetThread().
  virtual void RunOnTargetThread();

  // A task to invoke the CallCompleted() method on the original thread will
  // only be scheduled if RunOnTargetThreadAsync returns true. If false is
  // returned, the derived class should schedule a task to do so upon actual
  // completion.
  virtual bool RunOnTargetThreadAsync();