Extension activity log database refactoring (step 1)

chrome/browser/extensions/activity_log/activity_database.cc

 // Copyright (c) 2012 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.
 
 #include <string>
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread.h"
@@ skipping 16 matching lines @@
 
 bool SortActionsByTime(const scoped_refptr<extensions::Action> a,
                        const scoped_refptr<extensions::Action> b) {
   return a->time() > b->time();
 }
 
 }  // namespace
 
 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) {
   // We don't batch commits when in testing mode.
   batch_mode_ = !(CommandLine::ForCurrentProcess()->
       HasSwitch(switches::kEnableExtensionActivityLogTesting));
 }
 
 ActivityDatabase::~ActivityDatabase() {}
 
@@ skipping 20 matching lines @@
   if (!committer.Begin())
     return LogInitFailure();
 
 #if defined(OS_MACOSX)
   // Exclude the database from backups.
   base::mac::SetFileBackupExclusion(db_name);
 #endif
 
   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;
   if (stat != sql::INIT_OK)
     return LogInitFailure();
 
   valid_db_ = true;
   timer_.Start(FROM_HERE,
                base::TimeDelta::FromMinutes(2),
                this,
@@ skipping 110 matching lines @@
     actions->push_back(action);
   }
   // Sort by time (from newest to oldest).
   std::sort(actions->begin(), actions->end(), SortActionsByTime);
   return actions.Pass();
 }
 
 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;
   already_closed_ = true;
   delete this;
 }
 
 void ActivityDatabase::HardFailureClose() {
   if (already_closed_) return;
   valid_db_ = false;
@@ skipping 30 matching lines @@
 
 void ActivityDatabase::SetTimerForTesting(int ms) {
   timer_.Stop();
   timer_.Start(FROM_HERE,
                base::TimeDelta::FromMilliseconds(ms),
                this,
                &ActivityDatabase::RecordBatchedActionsWhileTesting);
 }
 
 }  // namespace extensions