Add remaining implementations to PreviewsOptOutStoreSQL

components/previews/core/previews_opt_out_store_sql.cc

Index: components/previews/core/previews_opt_out_store_sql.cc
diff --git a/components/previews/core/previews_opt_out_store_sql.cc b/components/previews/core/previews_opt_out_store_sql.cc
index 1673b78b5e29842db96fd553f242668c2a2a4982..d4e6a59eac658be2f87160b297f2300a116749e2 100644
--- a/components/previews/core/previews_opt_out_store_sql.cc
+++ b/components/previews/core/previews_opt_out_store_sql.cc
@@ -1,39 +1,48 @@
 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/previews/core/previews_opt_out_store_sql.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
+#include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/sequenced_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/previews/core/previews_black_list.h"
 #include "components/previews/core/previews_black_list_item.h"
 #include "components/previews/core/previews_experiments.h"
 #include "sql/connection.h"
 #include "sql/recovery.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
 
 namespace previews {
 
 namespace {
 
 // This is a macro instead of a const, so it can be used inline in other SQL
 // statements below.
 #define PREVIEWS_TABLE_NAME "previews_v1"
 
+// The maximum number of entries allowed per host. Entries are evicted based on
+// entry time.
+const int kMaxRowsPerHost = 32;
+
+// The maximum number of entries allowed in the data base. Entries are evicted
+// based on entry time.
+const int kMaxRowsInDB = 3200;
+
 void CreateSchema(sql::Connection* db) {
   const char kSql[] = "CREATE TABLE IF NOT EXISTS " PREVIEWS_TABLE_NAME
                       " (host_name VARCHAR NOT NULL,"
                       " time INTEGER NOT NULL,"
                       " opt_out INTEGER NOT NULL,"
                       " type INTEGER NOT NULL,"
                       " PRIMARY KEY(host_name, time DESC, opt_out, type))";
   if (!db->Execute(kSql))
     return;
 }
@@ -87,90 +96,189 @@ void InitDatabase(sql::Connection* db, base::FilePath path) {
   if (!base::CreateDirectoryAndGetError(path.DirName(), &err)) {
     return;
   }
   if (!db->Open(path)) {
     return;
   }
 
   CreateSchema(db);
 }
 
+// Adds the new entry to the data base.
+void AddPreviewNavigationToDataBase(sql::Connection* db,
+                                    bool opt_out,
+                                    const std::string& host_name,
+                                    PreviewsType type,
+                                    base::Time now) {
+  // Adds the new entry.
+  const char kSqlInsert[] = "INSERT INTO " PREVIEWS_TABLE_NAME
+                            " (host_name, time, opt_out, type)"
+                            " VALUES "
+                            " (?, ?, ?, ?)";
+
+  sql::Statement statement_insert(
+      db->GetCachedStatement(SQL_FROM_HERE, kSqlInsert));
+  statement_insert.BindString(0, host_name);
+  statement_insert.BindInt64(1, now.ToInternalValue());
+  statement_insert.BindBool(2, opt_out);
+  statement_insert.BindInt(3, static_cast<int>(type));
+  statement_insert.Run();
+}
+
+// Removes entries for |host_name| if the per-host row limit is exceeded.
+// Removes entries if per data base row limit is exceeded.
+void MaybeEvictHostEntryFromDataBase(sql::Connection* db,
+                                     const std::string& host_name) {
+  // Delete the oldest entries if there are more than |kMaxRowsPerHost| for
+  // |host_name|.
+  // DELETE ... LIMIT -1 OFFSET x means delete all but the first x entries.
+  const char kSqlDeleteByHost[] = "DELETE FROM " PREVIEWS_TABLE_NAME
+                                  " WHERE ROWID IN"
+                                  " (SELECT ROWID from " PREVIEWS_TABLE_NAME
+                                  " WHERE host_name == ?"
+                                  " ORDER BY time DESC"
+                                  " LIMIT -1 OFFSET ?)";
+
+  sql::Statement statement_delete_by_host(
+      db->GetCachedStatement(SQL_FROM_HERE, kSqlDeleteByHost));
+  statement_delete_by_host.BindString(0, host_name);
+  statement_delete_by_host.BindInt(1, kMaxRowsPerHost);
+  statement_delete_by_host.Run();
+}
+
 void LoadBlackListFromDataBase(
     sql::Connection* db,
     scoped_refptr<base::SingleThreadTaskRunner> runner,
     LoadBlackListCallback callback) {
   // Gets the table sorted by host and time. Limits the number of hosts using
   // most recent opt_out time as the limiting function.
   const char kSql[] =
       "SELECT host_name, time, opt_out"
       " FROM " PREVIEWS_TABLE_NAME;
 
   sql::Statement statement(db->GetUniqueStatement(kSql));
 
   std::unique_ptr<BlackListItemMap> black_list_item_map(new BlackListItemMap());
+  int count = 0;
   // Add the host name, the visit time, and opt out history to
   // |black_list_item_map|.
   while (statement.Step()) {
+    ++count;
     std::string host_name = statement.ColumnString(0);
     PreviewsBlackListItem* black_list_item =
         PreviewsBlackList::GetOrCreateBlackListItem(black_list_item_map.get(),
                                                     host_name);
     DCHECK_LE(black_list_item_map->size(),
               params::MaxInMemoryHostsInBlackList());
     // Allows the internal logic of PreviewsBlackListItem to determine how to
     // evict entries when there are more than
     // |StoredHistoryLengthForBlackList()| for the host.
     black_list_item->AddPreviewNavigation(
         statement.ColumnBool(2),
         base::Time::FromInternalValue(statement.ColumnInt64(1)));
   }
 
+  // TODO(ryansturm): Add UMA to log |count|. crbug.com/656739

[Scott Hess - ex-Googler, 2016/10/17 23:18:09]

Also please be sure to test this.

In thinking about this, I'm finding myself mixed.  On the one hand, I'd prefer
not to have multiple full-table scans like this, so counting and only running
when necessary seems like a good idea.  On the other hand, code which almost
never runs sometimes is found to be broken.

So I'd support either run-as-needed like this, or just running the query every
time.  If I understand things right, even when someone gets to this situation
their data still most likely fit in the cache, so there shouldn't be a
cache-thrashing issue.

[RyanSturm, 2016/10/17 23:26:38]

Acknowledged.

+  if (count > kMaxRowsInDB) {
+    // Delete the oldest entries if there are more than |kMaxEntriesInDB|.
+    // DELETE ... LIMIT -1 OFFSET x means delete all but the first x entries.
+    const char kSqlDeleteByDBSize[] = "DELETE FROM " PREVIEWS_TABLE_NAME
+                                      " WHERE ROWID IN"
+                                      " (SELECT ROWID from " PREVIEWS_TABLE_NAME
+                                      " ORDER BY time DESC"
+                                      " LIMIT -1 OFFSET ?)";
+
+    sql::Statement statement_delete(
+        db->GetCachedStatement(SQL_FROM_HERE, kSqlDeleteByDBSize));
+    statement_delete.BindInt(0, kMaxRowsInDB);
+    statement_delete.Run();
+  }
+
   runner->PostTask(FROM_HERE,
                    base::Bind(callback, base::Passed(&black_list_item_map)));
 }
 
-// Synchronous implementation, this is run on the background thread
-// and actually does the work to access SQL.
+// Synchronous implementations, these are run on the background thread
+// and actually do the work to access the SQL data base.
 void LoadBlackListSync(sql::Connection* db,
                        const base::FilePath& path,
                        scoped_refptr<base::SingleThreadTaskRunner> runner,
                        LoadBlackListCallback callback) {
   if (!db->is_open())
     InitDatabase(db, path);
 
   LoadBlackListFromDataBase(db, runner, callback);
 }
 
+// Deletes every row in the table that has entry time between |begin_time| and
+// |end_time|.
+void ClearBlackListSync(sql::Connection* db,
+                        base::Time begin_time,
+                        base::Time end_time) {
+  const char kSql[] =
+      "DELETE FROM " PREVIEWS_TABLE_NAME " WHERE time >= ? and time <= ?";
+
+  sql::Statement statement(db->GetUniqueStatement(kSql));
+  statement.BindInt64(0, begin_time.ToInternalValue());
+  statement.BindInt64(1, end_time.ToInternalValue());
+  statement.Run();
+}
+
+void AddPreviewNavigationSync(bool opt_out,
+                              const std::string& host_name,
+                              PreviewsType type,
+                              base::Time now,
+                              sql::Connection* db) {
+  sql::Transaction transaction(db);
+  if (!transaction.Begin())
+    return;
+  AddPreviewNavigationToDataBase(db, opt_out, host_name, type, now);
+  MaybeEvictHostEntryFromDataBase(db, host_name);
+  transaction.Commit();
+}
+
 }  // namespace
 
 PreviewsOptOutStoreSQL::PreviewsOptOutStoreSQL(
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     scoped_refptr<base::SequencedTaskRunner> background_task_runner,
     const base::FilePath& path)
     : io_task_runner_(io_task_runner),
       background_task_runner_(background_task_runner),
       db_file_path_(path) {}
 
 PreviewsOptOutStoreSQL::~PreviewsOptOutStoreSQL() {
   DCHECK(io_task_runner_->BelongsToCurrentThread());
   if (db_.get()) {
     background_task_runner_->DeleteSoon(FROM_HERE, db_.release());
   }
 }
 
 void PreviewsOptOutStoreSQL::AddPreviewNavigation(bool opt_out,
                                                   const std::string& host_name,
                                                   PreviewsType type,
-                                                  base::Time now) {}
+                                                  base::Time now) {
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  DCHECK(db_.get());
+  background_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&AddPreviewNavigationSync, opt_out, host_name, type,
+                            now, db_.get()));
+}
 
 void PreviewsOptOutStoreSQL::ClearBlackList(base::Time begin_time,
-                                            base::Time end_time) {}
+                                            base::Time end_time) {
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  DCHECK(db_.get());
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&ClearBlackListSync, db_.get(), begin_time, end_time));
+}
 
 void PreviewsOptOutStoreSQL::LoadBlackList(LoadBlackListCallback callback) {
   DCHECK(io_task_runner_->BelongsToCurrentThread());
   if (!db_)
     db_ = base::MakeUnique<sql::Connection>();
   background_task_runner_->PostTask(
       FROM_HERE, base::Bind(&LoadBlackListSync, db_.get(), db_file_path_,
                             base::ThreadTaskRunnerHandle::Get(), callback));
 }