Simplify threading in browser thread by making only ChromeThread deal with different thread lifetimes.The rest of the code doesn't get MessageLoop pointers since they're not thread-safe and instead just call PostTask on ChromeThread. If the target thread is not alive, then the task is simply deleted.In a followup change, I'll remove any remaining MessageLoop* caching. With this change, there's little to be gained by caching since no locks are involved if the target MessageLoop is guaranteed to outlive the current thread (inferred automatically by the order of the chrome_threads_ array).BUG=25354

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

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

3 files changed, 30 insertions, 42 deletions

diff --git a/chrome/browser/sync/glue/http_bridge.cc b/chrome/browser/sync/glue/http_bridge.cc
index 3448755..c019cba 100644
--- a/chrome/browser/sync/glue/http_bridge.cc
+++ b/chrome/browser/sync/glue/http_bridge.cc
@@ -10,6 +10,7 @@
 #include "base/string_util.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/profile.h"
+#include "chrome/browser/chrome_thread.h"
 #include "net/base/cookie_monster.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_cache.h"
@@ -53,15 +54,15 @@ HttpBridgeFactory::HttpBridgeFactory(
 HttpBridgeFactory::~HttpBridgeFactory() {
   if (request_context_getter_) {
     // Clean up request context getter on IO thread.
-    ChromeThread::GetMessageLoop(ChromeThread::IO)->ReleaseSoon(FROM_HERE,
-        request_context_getter_);
+    bool posted = ChromeThread::ReleaseSoon(
+        ChromeThread::IO, FROM_HERE, request_context_getter_);
+    DCHECK(posted);
     request_context_getter_ = NULL;
   }
 }
 
 sync_api::HttpPostProviderInterface* HttpBridgeFactory::Create() {
-  HttpBridge* http = new HttpBridge(request_context_getter_,
-      ChromeThread::GetMessageLoop(ChromeThread::IO));
+  HttpBridge* http = new HttpBridge(request_context_getter_);
   http->AddRef();
   return http;
 }
@@ -109,22 +110,21 @@ HttpBridge::RequestContext::~RequestContext() {
   delete http_transaction_factory_;
 }
 
-HttpBridge::HttpBridge(HttpBridge::RequestContextGetter* context_getter,
-                       MessageLoop* io_loop)
+HttpBridge::HttpBridge(HttpBridge::RequestContextGetter* context_getter)
     : context_getter_for_request_(context_getter),
       url_poster_(NULL),
       created_on_loop_(MessageLoop::current()),
-      io_loop_(io_loop),
       request_completed_(false),
       request_succeeded_(false),
       http_response_code_(-1),
-      http_post_completed_(false, false),
-      use_io_loop_for_testing_(false) {
+      http_post_completed_(false, false) {
   context_getter_for_request_->AddRef();
 }
 
 HttpBridge::~HttpBridge() {
-  io_loop_->ReleaseSoon(FROM_HERE, context_getter_for_request_);
+  bool posted = ChromeThread::ReleaseSoon(
+      ChromeThread::IO, FROM_HERE, context_getter_for_request_);
+  DCHECK(posted);
 }
 
 void HttpBridge::SetUserAgent(const char* user_agent) {
@@ -176,8 +176,9 @@ bool HttpBridge::MakeSynchronousPost(int* os_error_code, int* response_code) {
   DCHECK(url_for_request_.is_valid()) << "Invalid URL for request";
   DCHECK(!content_type_.empty()) << "Payload not set";
 
-  io_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
-                                &HttpBridge::CallMakeAsynchronousPost));
+  ChromeThread::PostTask(
+      ChromeThread::IO, FROM_HERE,
+      NewRunnableMethod(this, &HttpBridge::CallMakeAsynchronousPost));
 
   if (!http_post_completed_.Wait())  // Block until network request completes.
     NOTREACHED();                    // See OnURLFetchComplete.
@@ -189,17 +190,13 @@ bool HttpBridge::MakeSynchronousPost(int* os_error_code, int* response_code) {
 }
 
 void HttpBridge::MakeAsynchronousPost() {
-  DCHECK_EQ(MessageLoop::current(), io_loop_);
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   DCHECK(!request_completed_);
 
   url_poster_ = new URLFetcher(url_for_request_, URLFetcher::POST, this);
   url_poster_->set_request_context(context_getter_for_request_);
   url_poster_->set_upload_data(content_type_, request_content_);
   url_poster_->set_extra_request_headers(extra_headers_);
-
-  if (use_io_loop_for_testing_)
-    url_poster_->set_io_loop(io_loop_);
-
   url_poster_->Start();
 }
 
@@ -220,7 +217,7 @@ void HttpBridge::OnURLFetchComplete(const URLFetcher *source, const GURL &url,
                                     int response_code,
                                     const ResponseCookies &cookies,
                                     const std::string &data) {
-  DCHECK_EQ(MessageLoop::current(), io_loop_);
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
   request_completed_ = true;
   request_succeeded_ = (URLRequestStatus::SUCCESS == status.status());
@@ -229,10 +226,10 @@ void HttpBridge::OnURLFetchComplete(const URLFetcher *source, const GURL &url,
 
   response_content_ = data;
 
-  // End of the line for url_poster_. It lives only on the io_loop.
+  // End of the line for url_poster_. It lives only on the IO loop.
   // We defer deletion because we're inside a callback from a component of the
   // URLFetcher, so it seems most natural / "polite" to let the stack unwind.
-  io_loop_->DeleteSoon(FROM_HERE, url_poster_);
+  MessageLoop::current()->DeleteSoon(FROM_HERE, url_poster_);
   url_poster_ = NULL;
 
   // Wake the blocked syncer thread in MakeSynchronousPost.
diff --git a/chrome/browser/sync/glue/http_bridge.h b/chrome/browser/sync/glue/http_bridge.h
index 7e1d967..12500ec 100644
--- a/chrome/browser/sync/glue/http_bridge.h
+++ b/chrome/browser/sync/glue/http_bridge.h
@@ -97,7 +97,7 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
     DISALLOW_COPY_AND_ASSIGN(RequestContextGetter);
   };
 
-  HttpBridge(RequestContextGetter* context, MessageLoop* io_loop);
+  HttpBridge(RequestContextGetter* context);
   virtual ~HttpBridge();
 
   // sync_api::HttpPostProvider implementation.
@@ -135,7 +135,7 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
  private:
   friend class ::HttpBridgeTest;
 
-  // Called on the io_loop_ to issue the network request. The extra level
+  // Called on the IO loop to issue the network request. The extra level
   // of indirection is so that the unit test can override this behavior but we
   // still have a function to statically pass to PostTask.
   void CallMakeAsynchronousPost() { MakeAsynchronousPost(); }
@@ -148,7 +148,7 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
   // so we can block created_on_loop_ while the fetch is in progress.
   // NOTE: This is not a scoped_ptr for a reason. It must be deleted on the same
   // thread that created it, which isn't the same thread |this| gets deleted on.
-  // We must manually delete url_poster_ on the io_loop_.
+  // We must manually delete url_poster_ on the IO loop.
   URLFetcher* url_poster_;
 
   // The message loop of the thread we were created on. This is the thread that
@@ -158,10 +158,6 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
   // on network IO through curl_easy_perform.
   MessageLoop* const created_on_loop_;
 
-  // Member variable for the IO loop instead of asking ChromeThread directly,
-  // done this way for testability.
-  MessageLoop* const io_loop_;
-
   // The URL to POST to.
   GURL url_for_request_;
 
@@ -178,15 +174,10 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>,
   std::string response_content_;
 
   // A waitable event we use to provide blocking semantics to
-  // MakeSynchronousPost. We block created_on_loop_ while the io_loop_ fetches
+  // MakeSynchronousPost. We block created_on_loop_ while the IO loop fetches
   // network request.
   base::WaitableEvent http_post_completed_;
 
-  // This is here so that the unit test subclass can force our URLFetcher to
-  // use the io_loop_ passed on construction for network requests, rather than
-  // ChromeThread::IO's message loop (which won't exist in testing).
-  bool use_io_loop_for_testing_;
-
   DISALLOW_COPY_AND_ASSIGN(HttpBridge);
 };
 
diff --git a/chrome/browser/sync/glue/http_bridge_unittest.cc b/chrome/browser/sync/glue/http_bridge_unittest.cc
index da3eb67..c34ad25 100644
--- a/chrome/browser/sync/glue/http_bridge_unittest.cc
+++ b/chrome/browser/sync/glue/http_bridge_unittest.cc
@@ -5,6 +5,7 @@
 #if defined(BROWSER_SYNC)
 
 #include "base/thread.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/sync/glue/http_bridge.h"
 #include "net/url_request/url_request_unittest.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -32,7 +33,7 @@ class HttpBridgeTest : public testing::Test {
  public:
   HttpBridgeTest()
       : fake_default_request_context_getter_(NULL),
-        io_thread_("HttpBridgeTest IO thread") {
+        io_thread_(ChromeThread::IO) {
   }
 
   virtual void SetUp() {
@@ -55,9 +56,7 @@ class HttpBridgeTest : public testing::Test {
     }
     HttpBridge* bridge = new HttpBridge(
         new HttpBridge::RequestContextGetter(
-            fake_default_request_context_getter_),
-        io_thread_loop());
-    bridge->use_io_loop_for_testing_ = true;
+            fake_default_request_context_getter_));
     return bridge;
   }
 
@@ -74,7 +73,8 @@ class HttpBridgeTest : public testing::Test {
   TestURLRequestContextGetter* fake_default_request_context_getter_;
 
   // Separate thread for IO used by the HttpBridge.
-  base::Thread io_thread_;
+  ChromeThread io_thread_;
+
 };
 
 // An HttpBridge that doesn't actually make network requests and just calls
@@ -82,10 +82,10 @@ class HttpBridgeTest : public testing::Test {
 class ShuntedHttpBridge : public HttpBridge {
  public:
   ShuntedHttpBridge(URLRequestContextGetter* baseline_context_getter,
-                    MessageLoop* io_loop, HttpBridgeTest* test)
+                    HttpBridgeTest* test)
       : HttpBridge(new HttpBridge::RequestContextGetter(
-                       baseline_context_getter),
-                   io_loop), test_(test) { }
+                       baseline_context_getter)),
+                   test_(test) { }
  protected:
   virtual void MakeAsynchronousPost() {
     ASSERT_TRUE(MessageLoop::current() == test_->io_thread_loop());
@@ -124,7 +124,7 @@ TEST_F(HttpBridgeTest, TestMakeSynchronousPostShunted) {
   scoped_refptr<URLRequestContextGetter> ctx_getter(
       new TestURLRequestContextGetter());
   scoped_refptr<HttpBridge> http_bridge(new ShuntedHttpBridge(
-      ctx_getter, io_thread_loop(), this));
+      ctx_getter, this));
   http_bridge->SetUserAgent("bob");
   http_bridge->SetURL("http://www.google.com", 9999);
   http_bridge->SetPostPayload("text/plain", 2, " ");