Add functions to clean URLs from the activity log

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 8f9b1a177585b8d0a0a96c142f82cf5bdacdd9b5..9f64c1454c97513c42e85a0152dfca8c5a153257 100644
--- a/chrome/browser/extensions/activity_log/counting_policy.cc
+++ b/chrome/browser/extensions/activity_log/counting_policy.cc
@@ -538,4 +538,71 @@ void CountingPolicy::Close() {
   ScheduleAndForget(activity_database(), &ActivityDatabase::Close);
 }
 
+void CountingPolicy::DoRemoveURLs(const std::vector<GURL>& restrict_urls) {
+  sql::Connection* db = GetDatabaseConnection();
+  if (!db) {
+    LOG(ERROR) << "Unable to connect to database";
+    return;
+  }
+
+  // Ensure data is flushed to the database first so that we query over all

[mvrable, 2013/08/26 20:38:50]

I'd just update this comment as "query" does not describe what this function is
doing; "flush data first so the URL clearing affects queued-up data as well" or
something similar.

[karenlees, 2013/08/26 22:58:36]

Done. Sorry for abusing the term for query for this sql statement.

+  // data.
+  activity_database()->AdviseFlush(ActivityDatabase::kFlushImmediately);
+
+  // If no restrictions then then all URLs need to be removed.
+  if (restrict_urls.empty()) {
+    std::string sql_str = base::StringPrintf(
+      "UPDATE %s SET page_url_x=NULL,page_title_x=NULL,arg_url_x=NULL",
+      kTableName);
+
+    sql::Statement statement;
+    statement.Assign(db->GetCachedStatement(
+        sql::StatementID(SQL_FROM_HERE), sql_str.c_str()));
+
+    if (!statement.Run()) {
+      LOG(ERROR) << "Removing all URLs from database failed: "
+                 << statement.GetSQLStatement();
+    }
+  }
+
+  // If URLs are specified then restrict to only those URLs.
+  for (uint32_t i = 0; i < restrict_urls.size(); ++i) {

[mvrable, 2013/08/26 20:38:50]

size_t instead of uint32_t?

[karenlees, 2013/08/26 22:58:36]

Done.

[karenlees, 2013/08/26 22:58:36]

Done.

+    int64 url_id;
+    if (!restrict_urls[i].is_valid() ||
+        !url_table_.StringToInt(db, restrict_urls[i].spec(), &url_id)) {
+      continue;
+    }
+
+    DLOG(INFO) << "Removing URL " << restrict_urls[i].spec();
+    std::string sql_str = base::StringPrintf(
+      "UPDATE %s SET page_url_x=NULL,page_title_x=NULL,arg_url_x=NULL "
+      "WHERE page_url_x=? OR arg_url_x=?",

[mvrable, 2013/08/26 18:14:57]

Use "WHERE page_url_x IS ? OR arg_url_x IS ?" to handle null values in the
database properly.

[mvrable, 2013/08/26 20:38:50]

However, I might still prefer using two separate update statements: one to clear
page_url & page_title, and one for arg_url.  (It doesn't seem that a match on
page_url should cause arg_url to be cleared, and vice versa.)  In that case
using "=" or "IS" should be equivalent.

[karenlees, 2013/08/26 22:58:36]

Done. Sorry I did try the statements on friday, not sure why I couldn't get it
to work without the OR statement (just on page_url). That works now, but I still
can't get the full thing to work with the OR statement, even if the IS is used. 
I'll do in two statements as you suggested as alternative.

[karenlees, 2013/08/26 22:58:36]

Yep, doing, see above comment.

+      kTableName);
+
+    sql::Statement statement;
+    statement.Assign(db->GetCachedStatement(
+        sql::StatementID(SQL_FROM_HERE), sql_str.c_str()));
+    statement.BindInt64(0, url_id);
+    statement.BindInt64(0, url_id);
+
+    if (!statement.Run()) {
+      LOG(ERROR) << "Removing URL from database failed: "
+                 << statement.GetSQLStatement();
+    }
+  }
+
+  // Clean up unused strings from the strings and urls table to really delete
+  // the urls and page titles.
+  CleanStringTables(db);
+}
+
+void CountingPolicy::RemoveURLs(const std::vector<GURL>& restrict_urls) {
+  BrowserThread::PostTask(

[mvrable, 2013/08/26 20:38:50]

I think you can use ScheduleAndForget instead of PostTask.

+      BrowserThread::DB,
+      FROM_HERE,
+      base::Bind(&CountingPolicy::DoRemoveURLs,
+                 base::Unretained(this),
+                 restrict_urls));
+}
+
 }  // namespace extensions