Implement GetUsageAndQuotaForEviction in QuotaManager.

Implement GetUsageAndQuotaForEviction in QuotaManager.

This change includes :
* Merge EvictOriginData members into EvictionContext.
* Fix GetAvailableSpaceTest.

BUG=61676
TEST=QuotaManagerTest.GetUsageANdQuotaForEviction.

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

[Dai Mikurube (NOT FULLTIME), 2011-05-18 06:17:49]

I've implemented it and added the basic part.  Some lines conflicts with
http://codereview.chromium.org/7029007/ such as callback_factory_.

[kinuko, 2011-05-18 06:26:55]

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

http://codereview.chromium.org/7039006/diff/3/webkit/quota/quota_manager.cc#n...
webkit/quota/quota_manager.cc:528: quota_for_eviction_(0),
Now I seriously started wondering if we could have all those eviction-related
state/callback variables in a separate inner struct or class.

http://codereview.chromium.org/7039006/diff/3/webkit/quota/quota_manager.cc#n...
webkit/quota/quota_manager.cc:806:
NewCallback(&QuotaManager::OnGetAvailableSpaceForEviction));
We could call all those async methods at once and collect results.

http://codereview.chromium.org/7039006/diff/3/webkit/quota/quota_manager.cc#n...
webkit/quota/quota_manager.cc:820: // TODO(dmikurube): Make
kStorageTypeTemporary an argument?
I think we should either add or drop the type parameter to/from all the
eviction-related methods.

[Dai Mikurube (NOT FULLTIME), 2011-05-18 06:37:20]

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

http://codereview.chromium.org/7039006/diff/3/webkit/quota/quota_manager.cc#n...
webkit/quota/quota_manager.cc:528: quota_for_eviction_(0),
On 2011/05/18 06:26:55, kinuko wrote:
> Now I seriously started wondering if we could have all those eviction-related
> state/callback variables in a separate inner struct or class.

I agree with that, but how about doing that later as a refactoring?

http://codereview.chromium.org/7039006/diff/3/webkit/quota/quota_manager.cc#n...
webkit/quota/quota_manager.cc:806:
NewCallback(&QuotaManager::OnGetAvailableSpaceForEviction));
On 2011/05/18 06:26:55, kinuko wrote:
> We could call all those async methods at once and collect results.

Is that a serious requirement?  Calling them async and collecting the results on
these callbacks looks complicated.

http://codereview.chromium.org/7039006/diff/3/webkit/quota/quota_manager.cc#n...
webkit/quota/quota_manager.cc:820: // TODO(dmikurube): Make
kStorageTypeTemporary an argument?
On 2011/05/18 06:26:55, kinuko wrote:
> I think we should either add or drop the type parameter to/from all the
> eviction-related methods.

I think we might want to do that as a refactoring since the change lists on
eviction are now separated...

[Dai Mikurube (NOT FULLTIME), 2011-05-18 08:29:39]

Packed member variables for GetUsageAndQuotaForEviction.  (The member name is
temporary.)

[commit-bot: I haz the power, 2011-05-18 08:43:00]

No LGTM from valid reviewers yet.

[Dai Mikurube (NOT FULLTIME), 2011-05-18 08:43:56]

Wrongly clicked the checkbox. :(

[kinuko, 2011-05-18 09:08:01]

A few comment about the innner struct... (thx for making the change).

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

http://codereview.chromium.org/7039006/diff/7001/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:791:
get_usage_and_quota_for_eviction_members_.quota_,
this variable name sounds way too long...? (please see the other comment about
struct name)

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

http://codereview.chromium.org/7039006/diff/7001/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:157: struct GetUsageAndQuotaForEvictionMembers {
Can we just move all the eviction-related member variables into one class (once
the other patch is submitted) and call this struct EvictionStatus or something
simpler?

http://codereview.chromium.org/7039006/diff/7001/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:158: GetUsageAndQuotaForEvictionMembers(int64
usage, int64 quota)
Do we want to pass any particular values here?  I imagined we could just set
all-zero here.

http://codereview.chromium.org/7039006/diff/7001/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:165: int64 quota_;
public struct members do not need to have trailing _.

http://codereview.chromium.org/7039006/diff/7001/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:194: void OnGetAvailableSpaceForEviction(
Could we rename OnXxx -> DidXxx on those methods too?

[Dai Mikurube (NOT FULLTIME), 2011-05-18 09:31:30]

Thanks for your comments.

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

http://codereview.chromium.org/7039006/diff/7001/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:791:
get_usage_and_quota_for_eviction_members_.quota_,
On 2011/05/18 09:08:01, kinuko wrote:
> this variable name sounds way too long...? (please see the other comment about
> struct name)

Done.

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

http://codereview.chromium.org/7039006/diff/7001/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:157: struct GetUsageAndQuotaForEvictionMembers {
On 2011/05/18 09:08:01, kinuko wrote:
> Can we just move all the eviction-related member variables into one class
(once
> the other patch is submitted) and call this struct EvictionStatus or something
> simpler?

Renamed it to EvictionContext.  (EvictOriginData-related members are not
included yet.  They're not landed.)

http://codereview.chromium.org/7039006/diff/7001/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:158: GetUsageAndQuotaForEvictionMembers(int64
usage, int64 quota)
On 2011/05/18 09:08:01, kinuko wrote:
> Do we want to pass any particular values here?  I imagined we could just set
> all-zero here.

Done.

http://codereview.chromium.org/7039006/diff/7001/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:165: int64 quota_;
On 2011/05/18 09:08:01, kinuko wrote:
> public struct members do not need to have trailing _.

Done.

http://codereview.chromium.org/7039006/diff/7001/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:194: void OnGetAvailableSpaceForEviction(
On 2011/05/18 09:08:01, kinuko wrote:
> Could we rename OnXxx -> DidXxx on those methods too?

Done.

[kinuko, 2011-05-18 10:02:45]

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

http://codereview.chromium.org/7039006/diff/6002/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:816: 
DCHECK(!get_usage_and_quota_callback.get()) here?

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

http://codereview.chromium.org/7039006/diff/6002/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:38: class QuotaEvictionHandler {
could you add virtual ~QuotaEvictionHandler (protected) to this interface while
you're there?

http://codereview.chromium.org/7039006/diff/6002/webkit/quota/quota_manager_u...
File webkit/quota/quota_manager_unittest.cc (right):

http://codereview.chromium.org/7039006/diff/6002/webkit/quota/quota_manager_u...
webkit/quota/quota_manager_unittest.cc:779: // TODO(dmikurube): It might be
flaky?  (Also in GetAvailableSpaceTest.)
Oh that is true.  Sorry could you change this test (and the one in
GetAvailableSpaceTest) to EXPECT_GE(0, available_space()) or something?

[Dai Mikurube (NOT FULLTIME), 2011-05-18 11:35:25]

Thanks.

In addition:
* Merged EvictOriginData members into EvictionContext.
* Fixed GetAvailableSpaceTest.

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

http://codereview.chromium.org/7039006/diff/6002/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:816: 
On 2011/05/18 10:02:45, kinuko wrote:
> DCHECK(!get_usage_and_quota_callback.get()) here?

Done.

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

http://codereview.chromium.org/7039006/diff/6002/webkit/quota/quota_manager.h...
webkit/quota/quota_manager.h:38: class QuotaEvictionHandler {
On 2011/05/18 10:02:45, kinuko wrote:
> could you add virtual ~QuotaEvictionHandler (protected) to this interface
while
> you're there?

Done.

http://codereview.chromium.org/7039006/diff/6002/webkit/quota/quota_manager_u...
File webkit/quota/quota_manager_unittest.cc (right):

http://codereview.chromium.org/7039006/diff/6002/webkit/quota/quota_manager_u...
webkit/quota/quota_manager_unittest.cc:779: // TODO(dmikurube): It might be
flaky?  (Also in GetAvailableSpaceTest.)
On 2011/05/18 10:02:45, kinuko wrote:
> Oh that is true.  Sorry could you change this test (and the one in
> GetAvailableSpaceTest) to EXPECT_GE(0, available_space()) or something?

Done for GetUsageAndQuotaForEviction with EXPECT_LE.
Mofified GetAvailableSpaceTest to update "available_space", not "quota".  Then
removed direct_called.

[kinuko, 2011-05-18 14:10:51]

looking good.

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

http://codereview.chromium.org/7039006/diff/6007/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:782: // TODO(dmikurube): Handle error.
might want to record the # of errors in the eviction context?  (At least we
could show the values via quota-internals)

http://codereview.chromium.org/7039006/diff/6007/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:789:
eviction_context_.num_eviction_requested_clients) {
Nit: Looking at this code again (with longer variable names) I wonder (as
michael did) if we could do this with single variable.  clients_.size() is
quasi-constant and won't change throughout the almost entire life of the quota
manager.  Keeping these two variables and 
re-assigning requested_clients = clients_.size() every time we call EvictOrigin
looks a bit verbose.

[Dai Mikurube (NOT FULLTIME), 2011-05-19 02:28:03]

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

http://codereview.chromium.org/7039006/diff/6007/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:782: // TODO(dmikurube): Handle error.
On 2011/05/18 14:10:51, kinuko wrote:
> might want to record the # of errors in the eviction context?  (At least we
> could show the values via quota-internals)

Done.  Added a TODO: "The origin with some error should have lower priority in
the next eviction?"

http://codereview.chromium.org/7039006/diff/6007/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:789:
eviction_context_.num_eviction_requested_clients) {
On 2011/05/18 14:10:51, kinuko wrote:
> Nit: Looking at this code again (with longer variable names) I wonder (as
> michael did) if we could do this with single variable.  clients_.size() is
> quasi-constant and won't change throughout the almost entire life of the quota
> manager.  Keeping these two variables and 
> re-assigning requested_clients = clients_.size() every time we call
EvictOrigin
> looks a bit verbose.

Is that so impressive?  As I replied to Michael, I know it's a little redundant,
but it is useful for debugging (and testing with some additional interface).

If we think that clients_.size() every time looks a bit verbose, I believe we
should replace QuotaClientList with std::vector, not std::list, unless the
number of clients is a solid constant.  std::vector::push_back() doesn't cost so
much if clients_.size() is quasi-constant.

I addition, even if we do it with a single variable, I guess we have to
initialize the variable with clients_.size().

[kinuko, 2011-05-19 02:48:42]

LGTM

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

http://codereview.chromium.org/7039006/diff/6007/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:789:
eviction_context_.num_eviction_requested_clients) {
On 2011/05/19 02:28:03, Dai Mikurube wrote:
> On 2011/05/18 14:10:51, kinuko wrote:
> > Nit: Looking at this code again (with longer variable names) I wonder (as
> > michael did) if we could do this with single variable.  clients_.size() is
> > quasi-constant and won't change throughout the almost entire life of the
quota
> > manager.  Keeping these two variables and 
> > re-assigning requested_clients = clients_.size() every time we call
> EvictOrigin
> > looks a bit verbose.
> 
> Is that so impressive?

Mikurube-san, as you can see this comment is prefixed by 'nit', which wouldn't
mean to express something impressive.
I wouldn't oppose if you have a strong reason.

>  As I replied to Michael, I know it's a little redundant,
> but it is useful for debugging (and testing with some additional interface).

This part I didn't get very well, but since this is really a tiny bit I'd just
trust you.

> If we think that clients_.size() every time looks a bit verbose, I believe we
> should replace QuotaClientList with std::vector, not std::list, unless the
> number of clients is a solid constant.  std::vector::push_back() doesn't cost
so
> much if clients_.size() is quasi-constant.

vector::push_back is in general very efficient, often more efficient than list
even if it changes its size if the efficiency is the point.  You can put TODO at
QuotaClientList if you want or can just go ahead changing it.

> I addition, even if we do it with a single variable, I guess we have to
> initialize the variable with clients_.size().

That's true, I should have said initializing two vars looked a bit verbose to
me.

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

Thank you. :)

I'll be committing it after try-bots look like succeeding.  (It may be before
observing all try-bots succeed since the commit-bot will run try, too.)

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

http://codereview.chromium.org/7039006/diff/6007/webkit/quota/quota_manager.c...
webkit/quota/quota_manager.cc:789:
eviction_context_.num_eviction_requested_clients) {
On 2011/05/19 02:48:42, kinuko wrote:
> On 2011/05/19 02:28:03, Dai Mikurube wrote:
> > On 2011/05/18 14:10:51, kinuko wrote:
> > > Nit: Looking at this code again (with longer variable names) I wonder (as
> > > michael did) if we could do this with single variable.  clients_.size() is
> > > quasi-constant and won't change throughout the almost entire life of the
> quota
> > > manager.  Keeping these two variables and 
> > > re-assigning requested_clients = clients_.size() every time we call
> > EvictOrigin
> > > looks a bit verbose.
> > 
> > Is that so impressive?
> 
> Mikurube-san, as you can see this comment is prefixed by 'nit', which wouldn't
> mean to express something impressive.
> I wouldn't oppose if you have a strong reason.
> 
> >  As I replied to Michael, I know it's a little redundant,
> > but it is useful for debugging (and testing with some additional interface).
> 
> This part I didn't get very well, but since this is really a tiny bit I'd just
> trust you.

FYI, what I thought was tracking both of the total number and the evicted number
helps us debugging, for example, when some of clients don't callback.  We could
see the total number immediately when it happens.

> > If we think that clients_.size() every time looks a bit verbose, I believe
we
> > should replace QuotaClientList with std::vector, not std::list, unless the
> > number of clients is a solid constant.  std::vector::push_back() doesn't
cost
> so
> > much if clients_.size() is quasi-constant.
> 
> vector::push_back is in general very efficient, often more efficient than list
> even if it changes its size if the efficiency is the point.  You can put TODO
at
> QuotaClientList if you want or can just go ahead changing it.

Added TODO.

> > I addition, even if we do it with a single variable, I guess we have to
> > initialize the variable with clients_.size().
> 
> That's true, I should have said initializing two vars looked a bit verbose to
> me.

[commit-bot: I haz the power, 2011-05-19 05:26:07]

Change committed as 85874