Convert QuotaManager::GetLRUOrigin into a QuotaEvictionPolicy.

Convert QuotaManager::GetLRUOrigin into a QuotaEvictionPolicy.

This CL refactors the LRU origin fetching in QuotaManager into a
QuotaEvictionPolicy which is used by default. There should be no
functionality changes.

BUG=464234

[calamity, 2015-09-21 05:44:29]

calamity@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2015-09-22 06:58:58]

Overall looks good to me, but like you I don't know the code tons well :)

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
File storage/browser/quota/quota_manager.cc (left):

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
storage/browser/quota/quota_manager.cc:1528: void
QuotaManager::GetLRUOrigin(StorageType type,
nit: This function still exists in the header

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
storage/browser/quota/quota_manager.cc:1512: type, make_scoped_ptr(new
LRUOriginEvictionPolicy(this, type)));
How about setting the LRU policy to the default policy for all StorageTypes in
the constructor (by iterating through them - there isn't many)? Then they could
be overriden.

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
storage/browser/quota/quota_manager.cc:1515:
eviction_policy_map_.find(type)->second->GetEvictionOrigin(
We can avoid the additional map lookup here

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
storage/browser/quota/quota_manager.h:476: eviction_policy_map_;
(I should have picked this up in the other CL) Consider making this a
ScopedVector - since StorageType is just an enum it would make lookups faster.

[raymes, 2015-09-22 07:02:42]

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
storage/browser/quota/quota_manager.h:203: scoped_ptr<QuotaEvictionPolicy>
policy);
(I should've noticed this in the other CL too) I think this isn't called from
anywhere yet. Should we remove the function for now? Would it be reasonable to
configure the policies from inside the constructor of this class?

[calamity, 2015-09-24 06:50:32]

I guess these reviews are on hold while we sort out the first one.

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
File storage/browser/quota/quota_manager.cc (left):

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
storage/browser/quota/quota_manager.cc:1528: void
QuotaManager::GetLRUOrigin(StorageType type,
On 2015/09/22 06:58:58, raymes wrote:
> nit: This function still exists in the header

Done.

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
storage/browser/quota/quota_manager.cc:1512: type, make_scoped_ptr(new
LRUOriginEvictionPolicy(this, type)));
On 2015/09/22 06:58:58, raymes wrote:
> How about setting the LRU policy to the default policy for all StorageTypes in
> the constructor (by iterating through them - there isn't many)? Then they
could
> be overriden.

There are several storage types and I believe only temporary storage is ever
evicted. Perhaps we should DCHECK here that the storage type is temporary
storage.

In fact, I'm not sure why there is a storage type here? Is this system supposed
to be generic for all the storage types? Can any of the storage people shed some
light on this?

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
storage/browser/quota/quota_manager.cc:1515:
eviction_policy_map_.find(type)->second->GetEvictionOrigin(
On 2015/09/22 06:58:58, raymes wrote:
> We can avoid the additional map lookup here

Done.

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
storage/browser/quota/quota_manager.h:203: scoped_ptr<QuotaEvictionPolicy>
policy);
On 2015/09/22 07:02:42, raymes wrote:
> (I should've noticed this in the other CL too) I think this isn't called from
> anywhere yet. Should we remove the function for now? Would it be reasonable to
> configure the policies from inside the constructor of this class?

I just sent out a CL that hooks it all up. Also, it's used for the site
engagement policy test.

https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota...
storage/browser/quota/quota_manager.h:476: eviction_policy_map_;
On 2015/09/22 06:58:58, raymes wrote:
> (I should have picked this up in the other CL) Consider making this a
> ScopedVector - since StorageType is just an enum it would make lookups faster.

Done.