Add functions to clean URLs from the activity log

chrome/browser/extensions/activity_log/activity_database.cc

 // Copyright (c) 2012 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 <string>
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread.h"
@@ skipping 16 matching lines @@
 
 bool SortActionsByTime(const scoped_refptr<extensions::Action> a,
                        const scoped_refptr<extensions::Action> b) {
   return a->time() > b->time();
 }
 
 }  // namespace
 
 namespace extensions {
 
+const char* ActivityDatabase::kURLFields[] =
+      {"url_tld", "url_title", "url_path"};
+
 ActivityDatabase::ActivityDatabase(ActivityDatabase::Delegate* delegate)
     : delegate_(delegate),
       testing_clock_(NULL),
       valid_db_(false),
       already_closed_(false),
       did_init_(false) {
   // We don't batch commits when in testing mode.
   batch_mode_ = !(CommandLine::ForCurrentProcess()->
       HasSwitch(switches::kEnableExtensionActivityLogTesting));
 }
@@ skipping 207 matching lines @@
 }
 
 void ActivityDatabase::SetTimerForTesting(int ms) {
   timer_.Stop();
   timer_.Start(FROM_HERE,
                base::TimeDelta::FromMilliseconds(ms),
                this,
                &ActivityDatabase::RecordBatchedActionsWhileTesting);
 }
 
+std::string ActivityDatabase::ExtractTLD(const GURL& gurl) {

[mvrable, 2013/07/17 16:41:23]

I know you just took the name from the DOMAction code, but the term "TLD" here
doesn't seem right to me.  In a URL like http://www.google.com/, the top-level
domain (TLD) would be .com; a term like "origin" or "host" should be used to
refer to "www.google.com".

I was thinking of using just a single url column (instead of separate host and
path columns) in the database after refactoring, in which case this method might
not be needed?  So the name might not matter much.

[karenlees, 2013/08/08 23:36:37]

No longer need this method.

[karenlees, 2013/08/08 23:36:37]

Method no longer needed, removed.

+  std::string url_tld = gurl.GetOrigin().spec();
+  if ((url_tld.size() > 0) && (url_tld[url_tld.size()-1] == '/')) {
+    url_tld.erase(url_tld.size()-1);
+  }
+  return url_tld;
+}
+
+void ActivityDatabase::ConstructRemoveURLQuery(const std::string& url,
+                                               std::string* query) {
+  if (arraysize(kURLFields) < 1) {
+    DLOG(INFO) << "No fields to clean";
+    return;
+  }
+
+  std::string set_fields_str =  base::StringPrintf("%s=''", kURLFields[0]);
+  for (uint32_t i = 1; i < arraysize(kURLFields); i++) {
+    set_fields_str = base::StringPrintf("%s,%s=''", set_fields_str.c_str(),

[mvrable, 2013/07/17 16:41:23]

Personal preference: I would use NULL rather than '' to represent a cleared URL,
but the two are nearly equivalent so I'll leave it up to you (unless other
reviewers want to comment on NULL vs. empty values in Chrome databases, if there
are conventions to follow).

[karenlees, 2013/08/08 23:36:37]

I like NULL, updated.

+                                        kURLFields[i]);
+  }
+
+  if (url.empty()) {
+    // Set fields to empty string for all rows.
+    *query = base::StringPrintf("UPDATE %s SET %s",
+                                DOMAction::kTableName,
+                                set_fields_str.c_str());
+  } else {
+    // Set fields to empty string if the row has the url.
+    // TODO(karenlees): for the moment this matches anything with the same top
+    // level domain. After the  database refactoring it should match the extact
+    // URLs in the column. For now it's better to clean too much than too
+    // little.
+    *query = base::StringPrintf("UPDATE %s SET %s "
+                                "WHERE url_tld='%s'",

[mvrable, 2013/07/17 16:41:23]

You shouldn't insert the URL directly into the query like this, as it can lead
to SQL injection vulnerabilties; you should use a placeholder ("?") and then use
BindString to set the value.  Inserting column names or text you know is safe is
fine.

(It might be safe depending on whether special characters like ' are always
escaped in the URL...but much better to be obviously safe.)

[karenlees, 2013/08/08 23:36:37]

I like safe, updated.

+                                DOMAction::kTableName,
+                                set_fields_str.c_str(),
+                                ExtractTLD(GURL(url)).c_str());
+  }
+}
+
+void ActivityDatabase::RemoveURLs(const std::vector<GURL>& gurls) {
+  DLOG(INFO) << "Removing URLs from the activity log";
+  if (!gurls.empty()) {
+    for (uint32_t i = 0; i < gurls.size(); ++i) {
+      RemoveURL(gurls[i]);
+    }
+  } else {
+    RemoveAllURLs();
+  }
+}
+
+void ActivityDatabase::RemoveURL(const GURL& gurl) {
+  std::string tld = ExtractTLD(gurl);
+  if (tld.empty()) {
+    LOG(ERROR) << "Top level domain was empty, cannot delete.";
+    return;
+  }
+
+  std::string remove_urls_str;
+  ConstructRemoveURLQuery(tld, &remove_urls_str);
+  sql::Statement remove_statement(
+      db_.GetCachedStatement(SQL_FROM_HERE, remove_urls_str.c_str()));
+  if (!remove_statement.is_valid()) {
+    DLOG(ERROR) << "Invalid sql statement from: " << remove_urls_str.c_str();
+    return;
+  }
+  DLOG(INFO) << "Running: " << remove_urls_str.c_str();
+  remove_statement.Run();
+}
+
+void ActivityDatabase::RemoveAllURLs() {
+  DLOG(INFO) << "Removing all URLs from the activity log";
+  std::string remove_urls_str;
+  ConstructRemoveURLQuery("", &remove_urls_str);
+  sql::Statement remove_statement(
+      db_.GetCachedStatement(SQL_FROM_HERE, remove_urls_str.c_str()));
+  if (!remove_statement.is_valid()) {
+    DLOG(ERROR) << "Invalid sql statement from: " << remove_urls_str.c_str();
+    return;
+  }
+  DLOG(INFO) << "Running: " << remove_urls_str.c_str();
+  remove_statement.Run();
+}
+
 }  // namespace extensions