Adding a SQL implementation of the backing store for previews opt outs

components/previews/core/previews_black_list.cc

Index: components/previews/core/previews_black_list.cc
diff --git a/components/previews/core/previews_black_list.cc b/components/previews/core/previews_black_list.cc
index fd1afed3b0ad23b33e14584d4e4fb0188546e89e..392bf24fce4b0611e81ca1cd6ff61953fdd3d298 100644
--- a/components/previews/core/previews_black_list.cc
+++ b/components/previews/core/previews_black_list.cc
@@ -8,20 +8,45 @@
 #include "base/memory/ptr_util.h"
 #include "base/optional.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
 #include "components/previews/core/previews_black_list_item.h"
 #include "components/previews/core/previews_experiments.h"
 #include "url/gurl.h"
 
 namespace previews {
 
+namespace {
+
+void EvictOldestOptOut(BlackListItemMap* black_list_item_map) {

[Scott Hess - ex-Googler, 2016/10/08 00:04:22]

This is a bit involved, but I can't, offhand, think of something better.  About
all I can think of is something where you maintain an ordered vector with an
index into it, which is probably also painful.  std::min_element() would work,
with appropriate predicate, but won't be any more efficient.

[RyanSturm, 2016/10/10 18:50:35]

That is the same conclusion I came to. I don't think the confusion of doubly
indexing this collection to be accessed quick by multiple metrics will help in
the long run.

+  // TODO(ryansturm): Add UMA. crbug.com/647717
+  base::Optional<base::Time> oldest_opt_out;
+  BlackListItemMap::iterator item_to_delete = black_list_item_map->end();
+  for (BlackListItemMap::iterator iter = black_list_item_map->begin();
+       iter != black_list_item_map->end(); ++iter) {
+    if (!iter->second->most_recent_opt_out_time()) {
+      // If there is no opt out time, this is a good choice to evict.
+      item_to_delete = iter;
+      break;
+    }
+    if (!oldest_opt_out ||
+        iter->second->most_recent_opt_out_time().value() <
+            oldest_opt_out.value()) {
+      oldest_opt_out = iter->second->most_recent_opt_out_time().value();
+      item_to_delete = iter;
+    }
+  }
+  black_list_item_map->erase(item_to_delete);
+}
+
+}  // namespace
+
 PreviewsBlackList::PreviewsBlackList(
     std::unique_ptr<PreviewsOptOutStore> opt_out_store,
     std::unique_ptr<base::Clock> clock)
     : loaded_(false),
       opt_out_store_(std::move(opt_out_store)),
       clock_(std::move(clock)),
       weak_factory_(this) {
   if (opt_out_store_) {
     opt_out_store_->LoadBlackList(base::Bind(
         &PreviewsBlackList::LoadBlackListDone, weak_factory_.GetWeakPtr()));
@@ -49,39 +74,38 @@ void PreviewsBlackList::AddPreviewNavigation(const GURL& url,
 }
 
 void PreviewsBlackList::AddPreviewNavigationSync(const GURL& url,
                                                  bool opt_out,
                                                  PreviewsType type) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(url.has_host());
   DCHECK(loaded_);
   std::string host_name = url.host();
   base::Time now = clock_->Now();
-  PreviewsBlackListItem* item = GetBlackListItem(host_name);
-  if (!item) {
-    item = CreateBlackListItem(host_name);
-  }
+  PreviewsBlackListItem* item =
+      GetOrCreateBlackListItem(black_list_item_map_.get(), host_name);
   item->AddPreviewNavigation(opt_out, now);
   DCHECK_LE(black_list_item_map_->size(),
             params::MaxInMemoryHostsInBlackList());
   if (!opt_out_store_)
     return;
   opt_out_store_->AddPreviewNavigation(opt_out, host_name, type, now);
 }
 
 bool PreviewsBlackList::IsLoadedAndAllowed(const GURL& url,
                                            PreviewsType type) const {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(url.has_host());
   if (!loaded_)
     return false;
-  PreviewsBlackListItem* black_list_item = GetBlackListItem(url.host());
+  PreviewsBlackListItem* black_list_item =
+      GetBlackListItem(*black_list_item_map_, url.host());
   return !black_list_item || !black_list_item->IsBlackListed(clock_->Now());
 }
 
 void PreviewsBlackList::ClearBlackList(base::Time begin_time,
                                        base::Time end_time) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_LE(begin_time, end_time);
   // If the |black_list_item_map_| has been loaded from |opt_out_store_|,
   // synchronous operations will be accurate. Otherwise, queue the task to run
   // asynchronously.
@@ -125,59 +149,40 @@ void PreviewsBlackList::LoadBlackListDone(
   loaded_ = true;
   black_list_item_map_ = std::move(black_list_item_map);
 
   // Run all pending tasks. |loaded_| may change if ClearBlackList is queued.
   while (pending_callbacks_.size() > 0 && loaded_) {
     pending_callbacks_.front().Run();
     pending_callbacks_.pop();
   }
 }
 
+// static
 PreviewsBlackListItem* PreviewsBlackList::GetBlackListItem(
-    const std::string& host_name) const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(loaded_);
-  BlackListItemMap::iterator iter = black_list_item_map_->find(host_name);
-  if (iter != black_list_item_map_->end())
+    const BlackListItemMap& black_list_item_map,
+    const std::string& host_name) {
+  BlackListItemMap::const_iterator iter = black_list_item_map.find(host_name);
+  if (iter != black_list_item_map.end())
     return iter->second.get();
   return nullptr;
 }
 
-PreviewsBlackListItem* PreviewsBlackList::CreateBlackListItem(
+// static
+PreviewsBlackListItem* PreviewsBlackList::GetOrCreateBlackListItem(
+    BlackListItemMap* black_list_item_map,
     const std::string& host_name) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(loaded_);
-  DCHECK(!GetBlackListItem(host_name));
-  if (black_list_item_map_->size() >= params::MaxInMemoryHostsInBlackList())
-    EvictOldestOptOut();
-  DCHECK_LT(black_list_item_map_->size(),
-            params::MaxInMemoryHostsInBlackList());
-  PreviewsBlackListItem* black_list_item = new PreviewsBlackListItem(
+  PreviewsBlackListItem* black_list_item =
+      GetBlackListItem(*black_list_item_map, host_name);
+  if (black_list_item)
+    return black_list_item;
+  if (black_list_item_map->size() >= params::MaxInMemoryHostsInBlackList())
+    EvictOldestOptOut(black_list_item_map);
+  DCHECK_LT(black_list_item_map->size(), params::MaxInMemoryHostsInBlackList());
+  black_list_item = new PreviewsBlackListItem(
       params::MaxStoredHistoryLengthForBlackList(),
       params::BlackListOptOutThreshold(), params::BlackListDuration());
-  black_list_item_map_->operator[](host_name) =
+  black_list_item_map->operator[](host_name) =
       base::WrapUnique(black_list_item);
   return black_list_item;
 }
 
-void PreviewsBlackList::EvictOldestOptOut() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(loaded_);
-  // TODO(ryansturm): Add UMA. crbug.com/647717
-  BlackListItemMap::iterator item_to_delete = black_list_item_map_->end();
-  base::Time oldest_opt_out = clock_->Now();
-  for (BlackListItemMap::iterator iter = black_list_item_map_->begin();
-       iter != black_list_item_map_->end(); ++iter) {
-    if (!iter->second->most_recent_opt_out_time()) {
-      // If there is no opt out time, this is a good choice to evict.
-      item_to_delete = iter;
-      break;
-    }
-    if (iter->second->most_recent_opt_out_time().value() < oldest_opt_out) {
-      oldest_opt_out = iter->second->most_recent_opt_out_time().value();
-      item_to_delete = iter;
-    }
-  }
-  black_list_item_map_->erase(item_to_delete);
-}
-
 }  // namespace previews