Extension activity log database refactoring (step 1)

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"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/synchronization/lock.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/extensions/activity_log/api_actions.h"
 #include "chrome/browser/extensions/activity_log/blocked_actions.h"
 #include "chrome/browser/extensions/activity_log/dom_actions.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/browser_thread.h"
 #include "sql/connection.h"
 #include "sql/init_status.h"
 
 namespace base {
 class Clock;
 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
 // called on the DB thread. For this reason, the ActivityLog calls Close from
 // its destructor instead of destructing its ActivityDatabase object.
 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.
   void Close();
 
   void LogInitFailure();
 
   // Record a DOMction in the database.
   void RecordDOMAction(scoped_refptr<DOMAction> action);
 
@@ skipping 42 matching lines @@
   // 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();
 
   // For unit testing only.
   void RecordBatchedActionsWhileTesting();
 
+  ActivityPolicyCallbacks* policy_callbacks_;
   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_