Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1118)

Issue 1355793002: Convert QuotaManager::GetLRUOrigin into a QuotaEvictionPolicy. (Closed)

Created:
5 years, 3 months ago by calamity
Modified:
4 years, 11 months ago
Reviewers:
raymes
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@respect_exceptions
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 10

Patch Set 2 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -52 lines) Patch
M storage/browser/quota/quota_manager.h View 1 6 chunks +4 lines, -8 lines 0 comments Download
M storage/browser/quota/quota_manager.cc View 1 7 chunks +55 lines, -44 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 5 (1 generated)
calamity
5 years, 3 months ago (2015-09-21 05:44:29 UTC) #2
raymes
Overall looks good to me, but like you I don't know the code tons well ...
5 years, 3 months ago (2015-09-22 06:58:58 UTC) #3
raymes
https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota_manager.h File storage/browser/quota/quota_manager.h (right): https://codereview.chromium.org/1355793002/diff/1/storage/browser/quota/quota_manager.h#newcode203 storage/browser/quota/quota_manager.h:203: scoped_ptr<QuotaEvictionPolicy> policy); (I should've noticed this in the other ...
5 years, 3 months ago (2015-09-22 07:02:42 UTC) #4
calamity
5 years, 3 months ago (2015-09-24 06:50:32 UTC) #5
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.

Powered by Google App Engine
This is Rietveld 408576698