Introducing the activityLogPrivate.deleteActivities() API call.

chrome/browser/extensions/activity_log/counting_policy.cc

Index: chrome/browser/extensions/activity_log/counting_policy.cc
diff --git a/chrome/browser/extensions/activity_log/counting_policy.cc b/chrome/browser/extensions/activity_log/counting_policy.cc
index 43416e3f7f37deccf2aa5321bd0ca730fe9dc4e6..9f438018228750a107507c460bce2a669962efa0 100644
--- a/chrome/browser/extensions/activity_log/counting_policy.cc
+++ b/chrome/browser/extensions/activity_log/counting_policy.cc
@@ -518,6 +518,70 @@ scoped_ptr<Action::ActionVector> CountingPolicy::DoReadFilteredData(
   return actions.Pass();
 }
 
+void CountingPolicy::DoRemoveActions(const std::vector<int64>& action_ids) {

[mvrable, 2014/02/11 17:44:35]

Have you done any comparisons between this version and no batching (just calling
"DELETE FROM %s WHERE rowid = ?" in a loop)?  This version should work, but my
concern is that batching adds a fair bit of complexity and we don't even know if
it is faster.  With the call to GetUniqueStatement SQLite has to re-parse the
SQL statement on each batch.  With no batching and a fixed query string it's
easy to switch to using GetCachedStatement so I'm not even sure it would be
slower.

[pmarch, 2014/02/12 18:19:04]

I removed the complexity and now the function deletes one raw at a time.

+  if (action_ids.empty())
+    return;
+
+  sql::Connection* db = GetDatabaseConnection();
+  if (!db) {
+    LOG(ERROR) << "Unable to connect to database";
+    return;
+  }
+

[mvrable, 2014/02/11 17:44:35]

I would wrap all the deletions in a transaction (see FlushDatabase).  That will
give better performance when issuing multiple statements to the database.

[pmarch, 2014/02/12 18:19:04]

Done.

+  // Flush data first so the activity removal affects queued-up data as well.
+  activity_database()->AdviseFlush(ActivityDatabase::kFlushImmediately);
+
+  int batch_size = 50;
+  int iter = action_ids.size() / batch_size;
+  for (int i = 0; i < iter; i++) {
+    int start = i * batch_size;
+    int end = start + batch_size;
+    std::string rowid_set;
+    for (int j = start; j < end; j++) {
+      rowid_set += "?,";
+    }
+    // Delete the last comma from the set.
+    rowid_set = rowid_set.substr(0, rowid_set.length() - 1);
+    std::string query_str = base::StringPrintf(
+        "DELETE FROM %s WHERE rowid in (%s)", kTableName, rowid_set.c_str());
+    sql::Statement query(db->GetUniqueStatement(query_str.c_str()));
+    for (int j = start; j < end; j++) {
+      query.BindInt64(j - start, action_ids[j]);
+    }
+    if (!query.Run()) {
+      LOG(ERROR) << "Removing activities from database failed: "
+                 << query.GetSQLStatement();
+      CleanStringTables(db);
+      return;
+    }
+  }
+  int start = iter * batch_size;

[mvrable, 2014/02/11 17:44:35]

What happens when action_ids.size() == batch_size?  I think this second query
runs with no row ids and we may want to avoid that.

[pmarch, 2014/02/12 18:19:04]

Correct. I removed this code.

+  int end = action_ids.size();
+  std::string rowid_set;
+  for (int i = start; i < end; i++) {
+    rowid_set += "?,";
+  }
+  // Delete the last comma from the set.
+  rowid_set = rowid_set.substr(0, rowid_set.length() - 1);
+  std::string query_str = base::StringPrintf(
+      "DELETE FROM %s WHERE rowid in (%s)", kTableName, rowid_set.c_str());
+  sql::Statement query(db->GetUniqueStatement(query_str.c_str()));
+  for (int i = start; i < end; i++) {
+    query.BindInt64(i - start, action_ids[i]);
+  }
+  if (!query.Run()) {
+    LOG(ERROR) << "Removing activities from database failed: "
+               << query.GetSQLStatement();
+    CleanStringTables(db);
+    return;
+  }
+
+  // Clean up unused strings from the strings and urls table to really delete
+  // the urls and page titles. Should be called even if an error occured when
+  // removing a URL as there may some things to clean up.
+  CleanStringTables(db);
+}
+
 void CountingPolicy::DoRemoveURLs(const std::vector<GURL>& restrict_urls) {
   sql::Connection* db = GetDatabaseConnection();
   if (!db) {
@@ -692,6 +756,10 @@ void CountingPolicy::ReadFilteredData(
       callback);
 }
 
+void CountingPolicy::RemoveActions(const std::vector<int64>& action_ids) {
+  ScheduleAndForget(this, &CountingPolicy::DoRemoveActions, action_ids);
+}
+
 void CountingPolicy::RemoveURLs(const std::vector<GURL>& restrict_urls) {
   ScheduleAndForget(this, &CountingPolicy::DoRemoveURLs, restrict_urls);
 }