QuotaManager::GetUsageAndQuota should not hung when the returned usage is negative

QuotaManager::GetUsageAndQuota should not hung even when the returned usage/quota value is negative

BUG=none
TEST=QuotaManager.*

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

[Dai Mikurube (NOT FULLTIME), 2011-05-20 09:16:42]

Basically LGTM with some nits.

[Dai Mikurube (NOT FULLTIME), 2011-05-20 09:19:08]

Sorry, I wrongly clicked the button.

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

http://codereview.chromium.org/7056006/diff/1/webkit/quota/quota_manager.cc#n...
webkit/quota/quota_manager.cc:169: ++waiting_callbacks_;
Counting this up when generating looks a little strange for me.  How about when
calling GetGlobalUsage?  (The same for other functions.)

[kinuko, 2011-05-20 09:27:22]

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

http://codereview.chromium.org/7056006/diff/1/webkit/quota/quota_manager.cc#n...
webkit/quota/quota_manager.cc:169: ++waiting_callbacks_;
On 2011/05/20 09:19:08, Dai Mikurube wrote:
> Counting this up when generating looks a little strange for me.  How about
when
> calling GetGlobalUsage?  (The same for other functions.)

GetGlobalUsage() etc is called in subclasses and I wanted to make the base class
provide some 'automatic' mechanism for them (but I know what you mean).

Alternatively I could add a protected method like AddWaitingCallback() that just
increments waiting_callbacks_ and make subclasses call it each time it makes
additional request that takes callback.  I thought this might look a bit
redundant but wdyt.

[tzik, 2011-05-20 13:41:35]

LGTM

[Dai Mikurube (NOT FULLTIME), 2011-05-23 02:16:11]

LGTM if the following comment is ok.

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

http://codereview.chromium.org/7056006/diff/1/webkit/quota/quota_manager.cc#n...
webkit/quota/quota_manager.cc:169: ++waiting_callbacks_;
On 2011/05/20 09:27:23, kinuko wrote:
> On 2011/05/20 09:19:08, Dai Mikurube wrote:
> > Counting this up when generating looks a little strange for me.  How about
> when
> > calling GetGlobalUsage?  (The same for other functions.)
> 
> GetGlobalUsage() etc is called in subclasses and I wanted to make the base
class
> provide some 'automatic' mechanism for them (but I know what you mean).
> 
> Alternatively I could add a protected method like AddWaitingCallback() that
just
> increments waiting_callbacks_ and make subclasses call it each time it makes
> additional request that takes callback.  I thought this might look a bit
> redundant but wdyt.

I guess you mean adding AddWaitingCallback(), for example, after (or before)
    manager()->temporary_usage_tracker_->GetGlobalUsage(
        NewGlobalUsageCallback());
in RunBody() of sub classes like UsageAndQuotaDispatcherTaskForTemporary.

I agree with it.  It's more reasonable than the current code.

[kinuko, 2011-05-23 03:31:54]

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

http://codereview.chromium.org/7056006/diff/1/webkit/quota/quota_manager.cc#n...
webkit/quota/quota_manager.cc:169: ++waiting_callbacks_;
On 2011/05/23 02:16:11, Dai Mikurube wrote:
> On 2011/05/20 09:27:23, kinuko wrote:
> > On 2011/05/20 09:19:08, Dai Mikurube wrote:
> > > Counting this up when generating looks a little strange for me.  How about
> > when
> > > calling GetGlobalUsage?  (The same for other functions.)
> > 
> > GetGlobalUsage() etc is called in subclasses and I wanted to make the base
> class
> > provide some 'automatic' mechanism for them (but I know what you mean).
> > 
> > Alternatively I could add a protected method like AddWaitingCallback() that
> just
> > increments waiting_callbacks_ and make subclasses call it each time it makes
> > additional request that takes callback.  I thought this might look a bit
> > redundant but wdyt.
> 
> I guess you mean adding AddWaitingCallback(), for example, after (or before)
>     manager()->temporary_usage_tracker_->GetGlobalUsage(
>         NewGlobalUsageCallback());
> in RunBody() of sub classes like UsageAndQuotaDispatcherTaskForTemporary.
> 
> I agree with it.  It's more reasonable than the current code.

That was what I meant when I wrote the previous comment but instead I did
following in patch 3:
- kept the '++waiting_callbacks' code in NewXxxCallback method
- renamed NewXxxCallback to NewWaitableXxxCallback to explicitly indicate that
they are the methods that create a new waiting point (i.e. creates a new
callback which decrements waiting_callbacks when it is called).  I also added a
comment about that.  ptal thanks.

[Dai Mikurube (NOT FULLTIME), 2011-05-23 04:32:22]

On 2011/05/23 03:31:54, kinuko wrote:
> http://codereview.chromium.org/7056006/diff/1/webkit/quota/quota_manager.cc
> File webkit/quota/quota_manager.cc (right):
> 
>
http://codereview.chromium.org/7056006/diff/1/webkit/quota/quota_manager.cc#n...
> webkit/quota/quota_manager.cc:169: ++waiting_callbacks_;
> On 2011/05/23 02:16:11, Dai Mikurube wrote:
> > On 2011/05/20 09:27:23, kinuko wrote:
> > > On 2011/05/20 09:19:08, Dai Mikurube wrote:
> > > > Counting this up when generating looks a little strange for me.  How
about
> > > when
> > > > calling GetGlobalUsage?  (The same for other functions.)
> > > 
> > > GetGlobalUsage() etc is called in subclasses and I wanted to make the base
> > class
> > > provide some 'automatic' mechanism for them (but I know what you mean).
> > > 
> > > Alternatively I could add a protected method like AddWaitingCallback()
that
> > just
> > > increments waiting_callbacks_ and make subclasses call it each time it
makes
> > > additional request that takes callback.  I thought this might look a bit
> > > redundant but wdyt.
> > 
> > I guess you mean adding AddWaitingCallback(), for example, after (or before)
> >     manager()->temporary_usage_tracker_->GetGlobalUsage(
> >         NewGlobalUsageCallback());
> > in RunBody() of sub classes like UsageAndQuotaDispatcherTaskForTemporary.
> > 
> > I agree with it.  It's more reasonable than the current code.
> 
> That was what I meant when I wrote the previous comment but instead I did
> following in patch 3:
> - kept the '++waiting_callbacks' code in NewXxxCallback method
> - renamed NewXxxCallback to NewWaitableXxxCallback to explicitly indicate that
> they are the methods that create a new waiting point (i.e. creates a new
> callback which decrements waiting_callbacks when it is called).  I also added
a
> comment about that.  ptal thanks.

Make sense.  LGTM.