Exclude in-use origins from storage evictions for all QuotaEvictionPolicies.

storage/browser/quota/quota_manager.cc

Index: storage/browser/quota/quota_manager.cc
diff --git a/storage/browser/quota/quota_manager.cc b/storage/browser/quota/quota_manager.cc
index 0e7ccc0d15b75d4a11f3aef21aaa2bfca271b3a3..072f77acfffc49e4f36b36a23e756bc6853f8435 100644
--- a/storage/browser/quota/quota_manager.cc
+++ b/storage/browser/quota/quota_manager.cc
@@ -141,12 +141,12 @@ bool InitializeOnDBThread(int64* temporary_quota_override,
 }
 
 bool GetLRUOriginOnDBThread(StorageType type,
-                            std::set<GURL>* exceptions,
+                            const std::set<GURL>& exceptions,
                             SpecialStoragePolicy* policy,
                             GURL* url,
                             QuotaDatabase* database) {
   DCHECK(database);
-  database->GetLRUOrigin(type, *exceptions, policy, url);
+  database->GetLRUOrigin(type, exceptions, policy, url);
   return true;
 }
 
@@ -1458,6 +1458,7 @@ void QuotaManager::GetEvictionOrigin(StorageType type,
   auto eviction_policy = eviction_policy_map_.find(type);
   if (eviction_policy != eviction_policy_map_.end()) {
     eviction_policy->second->GetEvictionOrigin(special_storage_policy_,
+                                               GetEvictionOriginExceptions(),
                                                did_get_origin_callback);
   }
 
@@ -1465,9 +1466,32 @@ void QuotaManager::GetEvictionOrigin(StorageType type,
   GetLRUOrigin(type, did_get_origin_callback);
 }
 
+std::set<GURL> QuotaManager::GetEvictionOriginExceptions() {
+  std::set<GURL> exceptions;
+  for (const auto& p : origins_in_use_) {
+    if (p.second > 0)
+      exceptions.insert(p.first);
+  }
+
+  for (const auto& p : origins_in_error_) {
+    if (p.second > QuotaManager::kThresholdOfErrorsToBeBlacklisted)
+      exceptions.insert(p.first);
+  }

[raymes, 2015/09/22 07:16:10]

I'm confused a bit by this stuff - but I see you've just copied it from below!

e.g. why do we consider access_notified_origins_ below but not here. Also why
don't we consider origins_in_error_ below? =\

[calamity, 2015/09/23 01:55:21]

Good question. Okay, so, access_notified_origins_ is populated during the
eviction origin task and accounts for origins accessed during the task. ie These
lists represent the exceptions before the task is launched and
access_notified_origins_ are exceptions that arise during the task. (I'm not
sure why origins_in_use_ isn't just checked again below. Maybe it was deemed not
performant?)

I'm not sure why something similar isn't done for the origins in error, or in
fact why origins in error need to excepted from eviction.

+
+  return exceptions;
+}
+
 void QuotaManager::DidGetEvictionOrigin(const GetOriginCallback& callback,
                                         const GURL& origin) {
-  callback.Run(origin);
+  // Make sure the returned origin is (still) not in the origin_in_use_ set
+  // and has not been accessed since we posted the task.
+  if (origins_in_use_.find(origin) != origins_in_use_.end() ||
+      access_notified_origins_.find(origin) != access_notified_origins_.end()) {
+    callback.Run(GURL());
+  } else {
+    callback.Run(origin);
+  }
+  access_notified_origins_.clear();
 
   is_getting_eviction_origin_ = false;
 }
@@ -1513,31 +1537,12 @@ void QuotaManager::GetLRUOrigin(StorageType type,
     return;
   }
 
-  // TODO(calamity): make all QuotaEvictionPolicies aware of these exceptions.
-  std::set<GURL>* exceptions = new std::set<GURL>;
-  for (std::map<GURL, int>::const_iterator p = origins_in_use_.begin();
-       p != origins_in_use_.end();
-       ++p) {
-    if (p->second > 0)
-      exceptions->insert(p->first);
-  }
-  for (std::map<GURL, int>::const_iterator p = origins_in_error_.begin();
-       p != origins_in_error_.end();
-       ++p) {
-    if (p->second > QuotaManager::kThresholdOfErrorsToBeBlacklisted)
-      exceptions->insert(p->first);
-  }
-
   GURL* url = new GURL;
   PostTaskAndReplyWithResultForDBThread(
       FROM_HERE,
-      base::Bind(&GetLRUOriginOnDBThread,
-                 type,
-                 base::Owned(exceptions),
-                 special_storage_policy_,
-                 base::Unretained(url)),
-      base::Bind(&QuotaManager::DidGetLRUOrigin,
-                 weak_factory_.GetWeakPtr(),
+      base::Bind(&GetLRUOriginOnDBThread, type, GetEvictionOriginExceptions(),
+                 special_storage_policy_, base::Unretained(url)),
+      base::Bind(&QuotaManager::DidGetLRUOrigin, weak_factory_.GetWeakPtr(),
                  base::Owned(url)));
 }
 
@@ -1595,14 +1600,8 @@ void QuotaManager::DidInitialize(int64* temporary_quota_override,
 void QuotaManager::DidGetLRUOrigin(const GURL* origin,
                                    bool success) {
   DidDatabaseWork(success);
-  // Make sure the returned origin is (still) not in the origin_in_use_ set
-  // and has not been accessed since we posted the task.
-  if (origins_in_use_.find(*origin) != origins_in_use_.end() ||
-      access_notified_origins_.find(*origin) != access_notified_origins_.end())
-    lru_origin_callback_.Run(GURL());
-  else
-    lru_origin_callback_.Run(*origin);
-  access_notified_origins_.clear();
+
+  lru_origin_callback_.Run(*origin);
   lru_origin_callback_.Reset();
 }