Extension activity log database refactoring (step 1)

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 8c3c42fae1bd993390c0f22c5accb03dbc11afb3..a299122695bafc190385bc7b4f1a0ad23b92e0c5 100644
--- a/chrome/browser/extensions/activity_log/activity_database.h
+++ b/chrome/browser/extensions/activity_log/activity_database.h
@@ -27,6 +27,29 @@ class FilePath;
 
 namespace extensions {
 
+// Interface defining callbacks that the ActivityDatabase can make into a
+// higher-level ActivityLogPolicy instance.  Callbacks are always invoked on
+// the database thread.  Classes other than ActivityDatabase should not call
+// these methods.
+class ActivityPolicyCallbacks {
+ protected:

[mvrable, 2013/07/09 21:41:02]

Interfaces are normally public instead of protected, but we only want these
methods to be called by ActivityDatabase.  I'm unsure whether this is the right
visibility to use.

[felt, 2013/07/09 22:14:51]

This seems right to me.

+  friend class ActivityDatabase;
+
+  // Initializes the database schema; this gives a policy a change to create or
+  // update database tables as needed.  Should return true on success.
+  virtual bool DatabaseInitCallback(sql::Connection* db) = 0;
+
+  // Called by ActivityDatabase just before the ActivityDatabase object is
+  // deleted.  The database will make no further callbacks after invoking this
+  // method, so it is an appropriate time for the policy to delete itself.
+  virtual void DatabaseCloseCallback() = 0;

[mvrable, 2013/07/09 21:41:02]

Alternately, it could make sense to drop the use of a CloseCallback() and just
have ActivityDatabase call the destructor (which it can do since it is a friend
class).

[felt, 2013/07/10 00:14:36]

I personally like the current way better because you can tell from looking at
the ActivityLogPolicy where the destruction happens, but not a strong preference
if Matt feels differently.

+
+  // An ActivityDatabaseCallbacks instance should not be deleted directly; it
+  // is instead deleted by the ActivityDatabase which calls the Cleanup()
+  // method.
+  virtual ~ActivityPolicyCallbacks() {}
+};
+
 // Encapsulates the SQL connection for the activity log database.
 // This class holds the database connection and has methods for writing.
 // All of the methods except constructor need to be
@@ -35,9 +58,10 @@ namespace extensions {
 class ActivityDatabase {
  public:
   // Need to call Init to actually use the ActivityDatabase.
-  ActivityDatabase();
+  explicit ActivityDatabase(ActivityPolicyCallbacks* callbacks);
 
-  // Opens the DB and creates tables as necessary.
+  // Opens the DB.  Calls DatabaseInitCallback in the policy callbacks, which
+  // is responsible for creating or updating the database schema if needed.
   void Init(const base::FilePath& db_name);
 
   // The ActivityLog should call this to kill the ActivityDatabase.
@@ -100,6 +124,7 @@ class ActivityDatabase {
   // For unit testing only.
   void RecordBatchedActionsWhileTesting();
 
+  ActivityPolicyCallbacks* policy_callbacks_;
   base::Clock* testing_clock_;
   sql::Connection db_;
   bool valid_db_;