Add functions to clean URLs from the activity log

chrome/browser/extensions/activity_log/activity_database.h

Index: chrome/browser/extensions/activity_log/activity_database.h
diff --git a/chrome/browser/extensions/activity_log/activity_database.h b/chrome/browser/extensions/activity_log/activity_database.h
index f7472e905a7fb2afc217e9156584be95d546a156..e02ff7ce96cc284b891e3f3cc59b315c81a634d2 100644
--- a/chrome/browser/extensions/activity_log/activity_database.h
+++ b/chrome/browser/extensions/activity_log/activity_database.h
@@ -61,6 +61,9 @@ namespace extensions {
 // in which case ActivityLogPolicy::Close() should directly delete itself.)
 class ActivityDatabase {
  public:
+  // All the fields that need to be cleaned up when URLs are cleaned.
+  static const char* kURLFields[];

[felt, 2013/07/17 01:55:40]

This constant should probably be moved into the ActivityPolicy classes, since
each one might have a slightly different database layout. Check out the new
Delegate class here inside the ActivityDatabase. (This has changed since we
originally talked about this -- the Delegate class didn't exist at that point in
time.)

Perhaps the removal methods could take more parameters: the database name and
the columns to clear, rather having constants in the ActivityDatabase.

Another option this could be a method on the Delegate class, actually
implemented inside the policy classes. That might be more confusing than adding
more parameters, since it would be like... ActivityPolicy->RemoveURLs calls
ActivityDatabase->RemoveURLs calls ActivityPolicy->RemoveURLsInternal or
something.

Michael, do you have thoughts/a preference?

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

If the history clearing code is in the ActivityDatabase, even if it takes
various parameters, its flexibility is still limited (imagine a policy that is
performing URL clustering or something similar).  Long-term I think it would
make sense to have the code in the ActivityPolicy (your second option).

One other approach would be to have a general method in the ActivityDatabase to
run (on the database thread) a callback that is passed the database handle.  The
end result would still be a call chain like ActivityPolicy::RemoveURLs ->
ActivityDatabase::RemoveURLs -> ActivityPolicy::RemoveURLsInternal, except that
the code in ActivityDatabase wouldn't be tied to clearing history at all and
could be re-used for other purposes (if needed).

I'm fine with this code as-is, however, and can move the database code into the
policy as needed when I'm migrating the other policy-specific code in
ActivityDatabase.

+
   // Interface defining calls that the ActivityDatabase can make into a
   // ActivityLogPolicy instance to implement policy-specific behavior.  Methods
   // are always invoked on the database thread.  Classes other than
@@ -121,6 +124,23 @@ class ActivityDatabase {
 
   bool is_db_valid() const { return valid_db_; }
 
+  // Cleans all instances of particular URLs from the database. Used when user
+  // cleans particular URLs from history. If the vector is empty then all URLS
+  // are cleaned from the db.
+  void RemoveURLs(const std::vector<GURL>& gurls);
+
+  // Removes a single URL from the db. If the top level domain of the gurl is
+  // empty then it does not attempt to remove anything from the db.
+  void RemoveURL(const GURL& gurl);
+
+  // Cleans all URL columns from the database.
+  void RemoveAllURLs();
+
+  // Constructs an SQL query to clear urls from the database. If url is the
+  // empty string then constructs a query to remove all urls.
+  static void ConstructRemoveURLQuery(const std::string& url,
+                                      std::string* query);
+
   // For unit testing only.
   void SetBatchModeForTesting(bool batch_mode);
   void SetClockForTesting(base::Clock* clock);
@@ -150,6 +170,13 @@ class ActivityDatabase {
   // or changes to it.
   void SoftFailureClose();
 
+  // Gets the top level domain for the URL and removes a '/' if one is present
+  // at the end of the URL.
+  // TODO(karenlees): this is the same code used in the DOMAction::Record
+  // method. Is there anywhere to put this method so it can be used by parts of
+  // the code so they are kept in sync?
+  static std::string ExtractTLD(const GURL& gurl);

[felt, 2013/07/17 01:55:40]

Maybe it could be a static method on the activity_actions.h class?

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

That would be a good location for it.

+
   // For unit testing only.
   void RecordBatchedActionsWhileTesting();