Better ActivityLog error handling

chrome/browser/extensions/activity_log/activity_log.cc

Index: chrome/browser/extensions/activity_log/activity_log.cc
diff --git a/chrome/browser/extensions/activity_log/activity_log.cc b/chrome/browser/extensions/activity_log/activity_log.cc
index 59108407e366eb9b3771a056361e549c5e0e542a..9403318e2f53e710fbcbd18069b2e858356bf717 100644
--- a/chrome/browser/extensions/activity_log/activity_log.cc
+++ b/chrome/browser/extensions/activity_log/activity_log.cc
@@ -21,7 +21,6 @@
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/web_contents.h"
 #include "googleurl/src/gurl.h"
-#include "sql/error_delegate_util.h"
 #include "third_party/re2/re2/re2.h"
 
 namespace {
@@ -131,13 +130,14 @@ ActivityLog::ActivityLog(Profile* profile) : profile_(profile) {
   base::FilePath base_dir = profile->GetPath();
   base::FilePath database_name = base_dir.Append(
       chrome::kExtensionActivityLogFilename);
-  db_->SetErrorCallback(base::Bind(&ActivityLog::DatabaseErrorCallback,
-                                   base::Unretained(this)));
   ScheduleAndForget(&ActivityDatabase::Init, database_name);
 }
 
 ActivityLog::~ActivityLog() {
-  ScheduleAndForget(&ActivityDatabase::Close);
+  if (dispatch_thread_ == BrowserThread::UI)  // Cleanup fast in a unittest.

[Matt Perry, 2013/06/07 20:58:01]

I think it would be better for the unit tests to flush the message loop (using
RunLoop().RunUntilIdle()) than to put special cases in the production code.

[Matt Perry, 2013/06/07 20:58:01]

OK, I see you're already doing RunUntilIdle. Are you adding this code
speculatively? I'd be surprised if RunUntilIdle is not enough.

[felt, 2013/06/08 00:01:48]

the problem is in other people's unit tests. our constructor and destructor get
called in every test where someone has a Profile. sometimes other tests lack the
right threads, or even worse, threads get prematurely/unexpectedly made and
destroyed. it's really frustrating to have to accommodate these cases.

On 2013/06/07 20:58:01, Matt Perry wrote:

[Matt Perry, 2013/06/08 00:34:17]

Ah, that is unfortunate. I don't have a general solution for that.

On 2013/06/08 00:01:48, felt wrote:

+    db_->Close();
+  else
+    ScheduleAndForget(&ActivityDatabase::Close);
 }
 
 void ActivityLog::SetArgumentLoggingForTesting(bool log_arguments) {
@@ -359,9 +359,4 @@ void ActivityLog::OnScriptsExecuted(
   }
 }
 
-void ActivityLog::DatabaseErrorCallback(int error, sql::Statement* stmt) {
-  if (sql::IsErrorCatastrophic(error))
-    ScheduleAndForget(&ActivityDatabase::KillDatabase);
-}
-
 }  // namespace extensions