Add functions to clean URLs from the activity log

chrome/browser/extensions/activity_log/activity_database.h

 // Copyright (c) 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.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_DATABASE_H_
 #define CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_DATABASE_H_
 
 #include <string>
 #include <vector>
 #include "base/basictypes.h"
@@ skipping 43 matching lines @@
 //      ActivityLogPolicy object.
 //   5. ActivityDatabase::Close() finishes running by deleting the
 //      ActivityDatabase object.
 //
 // (This assumes the common case that the ActivityLogPolicy uses an
 // ActivityDatabase and implements the ActivityDatabase::Delegate interface.
 // It is also possible for an ActivityLogPolicy to not use a database at all,
 // 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
   // ActivityDatabase should not call these methods.
   class Delegate {
    protected:
     friend class ActivityDatabase;
 
     // A Delegate is never directly deleted; it should instead delete itself
     // after any final cleanup when OnDatabaseClose() is invoked.
@@ skipping 40 matching lines @@
   // 1 = yesterday, etc. Only returns 1 day at a time. Actions are sorted from
   // newest to oldest.
   scoped_ptr<std::vector<scoped_refptr<Action> > > GetActions(
       const std::string& extension_id, const int days_ago);
 
   // Handle errors in database writes.
   void DatabaseErrorCallback(int error, sql::Statement* stmt);
 
   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);
   void SetTimerForTesting(int milliseconds);
 
  private:
   // This should never be invoked by another class. Use Close() to order a
   // suicide.
   virtual ~ActivityDatabase();
 
   sql::InitStatus InitializeTable(const char* table_name,
                                   const char* table_structure);
 
   // When we're in batched mode (which is on by default), we write to the db
   // every X minutes instead of on every API call. This prevents the annoyance
   // of writing to disk multiple times a second.
   void StartTimer();
   void RecordBatchedActions();
 
   // If an error is unrecoverable or occurred while we were trying to close
   // the database properly, we take "emergency" actions: break any outstanding
   // transactions, raze the database, and close. When next opened, the
   // database will be empty.
   void HardFailureClose();
 
   // Doesn't actually close the DB, but changes bools to prevent further writes
   // 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();
 
   // A reference a Delegate for policy-specific database behavior.  See the
   // top-level comment for ActivityDatabase for comments on cleanup.
   Delegate* delegate_;
 
   base::Clock* testing_clock_;
   sql::Connection db_;
   bool valid_db_;
   bool batch_mode_;
   std::vector<scoped_refptr<Action> > batched_actions_;
   base::RepeatingTimer<ActivityDatabase> timer_;
   bool already_closed_;
   bool did_init_;
 
   DISALLOW_COPY_AND_ASSIGN(ActivityDatabase);
 };
 
 }  // namespace extensions
 #endif  // CHROME_BROWSER_EXTENSIONS_ACTIVITY_LOG_ACTIVITY_DATABASE_H_