Introducing the activityLogPrivate.deleteActivities() API call.

chrome/browser/extensions/activity_log/counting_policy.cc

 // Copyright 2013 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.
 
 // A policy for storing activity log data to a database that performs
 // aggregation to reduce the size of the database.  The database layout is
 // nearly the same as FullStreamUIPolicy, which stores a complete log, with a
 // few changes:
 //   - a "count" column is added to track how many log records were merged
 //     together into this row
@@ skipping 500 matching lines @@
             static_cast<base::DictionaryValue*>(parsed_value.release())));
       }
     }
     action->set_count(query.ColumnInt(9));
     actions->push_back(action);
   }
 
   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) {
     LOG(ERROR) << "Unable to connect to database";
     return;
   }
 
   // Flush data first so the URL clearing affects queued-up data as well.
   activity_database()->AdviseFlush(ActivityDatabase::kFlushImmediately);
 
@@ skipping 154 matching lines @@
                  base::Unretained(this),
                  extension_id,
                  type,
                  api_name,
                  page_url,
                  arg_url,
                  days_ago),
       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);
 }
 
 void CountingPolicy::RemoveExtensionData(const std::string& extension_id) {
   ScheduleAndForget(this, &CountingPolicy::DoRemoveExtensionData, extension_id);
 }
 
 void CountingPolicy::DeleteDatabase() {
   ScheduleAndForget(this, &CountingPolicy::DoDeleteDatabase);
@@ skipping 40 matching lines @@
   return true;
 }
 
 void CountingPolicy::Close() {
   // The policy object should have never been created if there's no DB thread.
   DCHECK(BrowserThread::IsMessageLoopValid(BrowserThread::DB));
   ScheduleAndForget(activity_database(), &ActivityDatabase::Close);
 }
 
 }  // namespace extensions