Change WDS to use the DB thread rather than its own thread.

This cleanup was requested by brettw and was started to make it easier for the sync service to post tasks to the WDS thread (now the DB thread).  This simplifies the WDS a bit since it no longer has to manage its own thread, and can assume that the DB thread is running throughout its lifetime.

One change in behavior that is significant is that previous to this change, the WDS worker thread would always be joined when Shutdown() was called from Profile::~Profile().  Now the Shutdown() method schedules a task that can extend the lifetime of the WDS past the lifetime of the Profile instance.

Review URL: http://codereview.chromium.org/524003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@35339 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 34 insertions, 48 deletions

diff --git a/chrome/browser/search_engines/template_url_model_unittest.cc b/chrome/browser/search_engines/template_url_model_unittest.cc
index 1c50fc1..7218951 100644
--- a/chrome/browser/search_engines/template_url_model_unittest.cc
+++ b/chrome/browser/search_engines/template_url_model_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "base/path_service.h"
 #include "base/thread.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/webdata/web_database.h"
 #include "chrome/test/testing_profile.h"
@@ -34,6 +35,9 @@ class TemplateURLModelTestingProfile : public TestingProfile {
   TemplateURLModelTestingProfile() : TestingProfile() {}
 
   void SetUp() {
+    db_thread_.reset(new ChromeThread(ChromeThread::DB));
+    db_thread_->Start();
+
     // Name a subdirectory of the temp directory.
     ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &test_dir_));
     test_dir_ = test_dir_.AppendASCII("TemplateURLModelTest");
@@ -50,6 +54,11 @@ class TemplateURLModelTestingProfile : public TestingProfile {
   void TearDown() {
     // Clean up the test directory.
     service_->Shutdown();
+    // Note that we must ensure the DB thread is stopped after WDS
+    // shutdown (so it can commit pending transactions) but before
+    // deleting the test profile directory, otherwise we may not be
+    // able to delete it due to an open transaction.
+    db_thread_->Stop();
     ASSERT_TRUE(file_util::Delete(test_dir_, true));
     ASSERT_FALSE(file_util::PathExists(test_dir_));
   }
@@ -61,6 +70,7 @@ class TemplateURLModelTestingProfile : public TestingProfile {
  private:
   scoped_refptr<WebDataService> service_;
   FilePath test_dir_;
+  scoped_ptr<ChromeThread> db_thread_;
 };
 
 // Trivial subclass of TemplateURLModel that records the last invocation of
@@ -142,10 +152,9 @@ class TemplateURLModelTest : public testing::Test,
   // Blocks the caller until the service has finished servicing all pending
   // requests.
   void BlockTillServiceProcessesRequests() {
-    // Schedule a task on the background thread that is processed after all
-    // pending requests on the background thread.
-    profile_->GetWebDataService(Profile::EXPLICIT_ACCESS)->thread()->
-        message_loop()->PostTask(FROM_HERE, new QuitTask2());
+    // Schedule a task on the DB thread that is processed after all
+    // pending requests on the DB thread.
+    ChromeThread::PostTask(ChromeThread::DB, FROM_HERE, new QuitTask2());
     // Run the current message loop. QuitTask2, when run, invokes Quit,
     // which unblocks this.
     MessageLoop::current()->Run();
@@ -756,4 +765,3 @@ TEST_F(TemplateURLModelTest, FailedInit) {
 
   ASSERT_TRUE(model_->GetDefaultSearchProvider());
 }
-
diff --git a/chrome/browser/webdata/web_data_service.cc b/chrome/browser/webdata/web_data_service.cc
index 4407cdd..d25492b 100644
--- a/chrome/browser/webdata/web_data_service.cc
+++ b/chrome/browser/webdata/web_data_service.cc
@@ -29,7 +29,7 @@ using webkit_glue::FormField;
 using webkit_glue::PasswordForm;
 
 WebDataService::WebDataService()
-  : thread_(NULL),
+  : is_running_(false),
     db_(NULL),
     failed_init_(false),
     should_commit_(false),
@@ -38,8 +38,8 @@ WebDataService::WebDataService()
 }
 
 WebDataService::~WebDataService() {
-  if (thread_) {
-    Shutdown();
+  if (is_running_ && db_) {
+    DLOG_ASSERT("WebDataService dtor called without Shutdown");
   }
 }
 
@@ -51,47 +51,18 @@ bool WebDataService::Init(const FilePath& profile_path) {
 
 bool WebDataService::InitWithPath(const FilePath& path) {
   path_ = path;
-
-  thread_ = new base::Thread("Chrome_WebDataThread");
-  if (!thread_->Start()) {
-    delete thread_;
-    thread_ = NULL;
-    return false;
-  }
-
+  is_running_ = true;
   ScheduleTask(NewRunnableMethod(this,
       &WebDataService::InitializeDatabaseIfNecessary));
   return true;
 }
 
-class ShutdownTask : public Task {
- public:
-  explicit ShutdownTask(WebDataService* wds) : service_(wds) {
-  }
-  virtual void Run() {
-    service_->ShutdownDatabase();
-  }
-
- private:
-
-  WebDataService* service_;
-};
-
 void WebDataService::Shutdown() {
-  if (thread_) {
-    // We cannot use NewRunnableMethod() because this can be called from our
-    // destructor. NewRunnableMethod() would AddRef() this instance.
-    ScheduleTask(new ShutdownTask(this));
-
-    // The thread destructor sends a message to terminate the thread and waits
-    // until the thread has exited.
-    delete thread_;
-    thread_ = NULL;
-  }
+  UnloadDatabase();
 }
 
 bool WebDataService::IsRunning() const {
-  return thread_ != NULL;
+  return is_running_;
 }
 
 void WebDataService::UnloadDatabase() {
@@ -106,8 +77,8 @@ void WebDataService::ScheduleCommit() {
 }
 
 void WebDataService::ScheduleTask(Task* t) {
-  if (thread_)
-    thread_->message_loop()->PostTask(FROM_HERE, t);
+  if (is_running_)
+    ChromeThread::PostTask(ChromeThread::DB, FROM_HERE, t);
   else
     NOTREACHED() << "Task scheduled after Shutdown()";
 }
diff --git a/chrome/browser/webdata/web_data_service.h b/chrome/browser/webdata/web_data_service.h
index 3a6c83b..4421b71 100644
--- a/chrome/browser/webdata/web_data_service.h
+++ b/chrome/browser/webdata/web_data_service.h
@@ -12,6 +12,7 @@
 #include "base/file_path.h"
 #include "base/lock.h"
 #include "base/ref_counted.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "webkit/glue/form_field.h"
@@ -145,7 +146,9 @@ template <class T> class WDObjectResult : public WDTypedResult {
 
 class WebDataServiceConsumer;
 
-class WebDataService : public base::RefCountedThreadSafe<WebDataService> {
+class WebDataService
+    : public base::RefCountedThreadSafe<WebDataService,
+                                        ChromeThread::DeleteOnUIThread> {
  public:
 
   // All requests return an opaque handle of the following type.
@@ -429,6 +432,8 @@ class WebDataService : public base::RefCountedThreadSafe<WebDataService> {
   //////////////////////////////////////////////////////////////////////////////
  private:
   friend class base::RefCountedThreadSafe<WebDataService>;
+  friend class ChromeThread;
+  friend class DeleteTask<WebDataService>;
   friend class ShutdownTask;
 
   typedef GenericRequest2<std::vector<const TemplateURL*>,
@@ -507,8 +512,6 @@ class WebDataService : public base::RefCountedThreadSafe<WebDataService> {
 
   void GetWebAppImagesImpl(GenericRequest<GURL>* request);
 
-  base::Thread* thread() { return thread_; }
-
   // Schedule a task on our worker thread.
   void ScheduleTask(Task* t);
 
@@ -518,8 +521,8 @@ class WebDataService : public base::RefCountedThreadSafe<WebDataService> {
   // Return the next request handle.
   int GetNextRequestHandle();
 
-  // Our worker thread. All requests are processed from that thread.
-  base::Thread* thread_;
+  // True once initialization has started.
+  bool is_running_;
 
   // The path with which to initialize the database.
   FilePath path_;
diff --git a/chrome/browser/webdata/web_data_service_unittest.cc b/chrome/browser/webdata/web_data_service_unittest.cc
index 1e26539..7c92725 100644
--- a/chrome/browser/webdata/web_data_service_unittest.cc
+++ b/chrome/browser/webdata/web_data_service_unittest.cc
@@ -71,10 +71,12 @@ class AutofillWebDataServiceConsumer: public WebDataServiceConsumer {
 class WebDataServiceTest : public testing::Test {
  public:
   WebDataServiceTest()
-      : ui_thread_(ChromeThread::UI, &message_loop_) {}
+      : ui_thread_(ChromeThread::UI, &message_loop_),
+        db_thread_(ChromeThread::DB) {}
 
  protected:
   virtual void SetUp() {
+    db_thread_.Start();
     name1_ = ASCIIToUTF16("name1");
     name2_ = ASCIIToUTF16("name2");
     value1_ = ASCIIToUTF16("value1");
@@ -94,6 +96,7 @@ class WebDataServiceTest : public testing::Test {
       wds_->Shutdown();
     file_util::Delete(profile_dir_, true);
 
+    db_thread_.Stop();
     MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask);
     MessageLoop::current()->Run();
   }
@@ -110,6 +113,7 @@ class WebDataServiceTest : public testing::Test {
 
   MessageLoopForUI message_loop_;
   ChromeThread ui_thread_;
+  ChromeThread db_thread_;
   string16 name1_;
   string16 name2_;
   string16 value1_;