Extension activity log database refactoring (step 1)

chrome/browser/extensions/activity_log/activity_database.cc

Index: chrome/browser/extensions/activity_log/activity_database.cc
diff --git a/chrome/browser/extensions/activity_log/activity_database.cc b/chrome/browser/extensions/activity_log/activity_database.cc
index 20f4b985b99276d5d22a523cdac13313f212385a..ee055a95d4db38cd83156a79935ea7b7e1b6276e 100644
--- a/chrome/browser/extensions/activity_log/activity_database.cc
+++ b/chrome/browser/extensions/activity_log/activity_database.cc
@@ -34,8 +34,9 @@ bool SortActionsByTime(const scoped_refptr<extensions::Action> a,
 
 namespace extensions {
 
-ActivityDatabase::ActivityDatabase()
-    : testing_clock_(NULL),
+ActivityDatabase::ActivityDatabase(ActivityPolicyCallbacks* callbacks)
+    : policy_callbacks_(callbacks),
+      testing_clock_(NULL),
       valid_db_(false),
       already_closed_(false),
       did_init_(false) {
@@ -76,16 +77,7 @@ void ActivityDatabase::Init(const base::FilePath& db_name) {
 
   db_.Preload();
 
-  // Create the DOMAction database.
-  if (!DOMAction::InitializeTable(&db_))
-    return LogInitFailure();
-
-  // Create the APIAction database.
-  if (!APIAction::InitializeTable(&db_))
-    return LogInitFailure();
-
-  // Create the BlockedAction database.
-  if (!BlockedAction::InitializeTable(&db_))
+  if (!policy_callbacks_->DatabaseInitCallback(&db_))
     return LogInitFailure();
 
   sql::InitStatus stat = committer.Commit() ? sql::INIT_OK : sql::INIT_FAILURE;
@@ -217,6 +209,7 @@ void ActivityDatabase::Close() {
   timer_.Stop();
   if (!already_closed_) {
     RecordBatchedActions();

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

Is it going to be a problem to call DatabaseCloseCallback on the DB thread, even
though the rest of a policy object's lifecycle happens on the UI thread?

One potential issue is that ActivityLogPolicy has its own RepeatingTimer, which
needs to be started and stopped on the same thread since it's not threadsafe. We
therefore need to destruct and construct ActivityLogPolicies on the same thread.
 (For that reason, I think this code as currently written should DCHECK.)
However, it seems like we can get rid of that second timer anyway.

Other than that, it seems like it would be OK... since no one else should be
calling any methods on the object at that point. (But I want to make sure you've
thought about it?)

[mvrable, 2013/07/09 23:03:22]

I think we want this to happen on the database thread is best, but I could be
missing some issues.

I'd forgotten about the ActivityLogPolicy having its own timer, but I agree that
we should get rid of that and let the database manage the timer.  I'll do that
(deleting the timer from the policy) as part of this patch for safety.

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

OK, can you add a comment to ~ActivityLogPolicy() and the callback method,
specifying that they are called from the Db thread?

[mvrable, 2013/07/10 17:49:17]

Done.

+    policy_callbacks_->DatabaseCloseCallback();

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

Note that if we've hit a HardFailureClose(), already_closed_ is set to true and
this code won't get called. already_closed_ currently refers to the status of
the database connection. I'd suggest (1) moving this outside of the if
statement, and (2) making a new boolean to track the status of calling ::Close()
if you think it's needed.

[mvrable, 2013/07/09 23:03:22]

You are right about already_closed_ and HardFailureClose().

I updated this but I don't think a boolean is needed: the lifetime of the policy
should match that of the database and so we can unconditionally call
DatabaseCloseCallback in Close().  Close() itself should be called exactly once
for the ActivityDatabase (otherwise we'd be seeing either memory leaks or double
frees already).

     db_.reset_error_callback();
   }
   valid_db_ = false;