Simplify threading in browser thread by making only ChromeThread deal with di...

chrome/browser/chrome_thread.cc

Index: chrome/browser/chrome_thread.cc
===================================================================
--- chrome/browser/chrome_thread.cc	(revision 30037)
+++ chrome/browser/chrome_thread.cc	(working copy)
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 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.
 
@@ -7,10 +7,10 @@
 // Friendly names for the well-known threads.
 static const char* chrome_thread_names[ChromeThread::ID_COUNT] = {
   "",  // UI (name assembled in browser_main.cc).
-  "Chrome_IOThread",  // IO
-  "Chrome_FileThread",  // FILE
   "Chrome_DBThread",  // DB
   "Chrome_WebKitThread",  // WEBKIT
+  "Chrome_FileThread",  // FILE
+  "Chrome_IOThread",  // IO
 #if defined(OS_LINUX)
   "Chrome_Background_X11Thread",  // BACKGROUND_X11
 #endif
@@ -18,16 +18,7 @@
 
 Lock ChromeThread::lock_;
 
-ChromeThread* ChromeThread::chrome_threads_[ID_COUNT] = {
-  NULL,  // UI
-  NULL,  // IO
-  NULL,  // FILE
-  NULL,  // DB
-  NULL,  // WEBKIT
-#if defined(OS_LINUX)
-  NULL,  // BACKGROUND_X11
-#endif
-};
+ChromeThread* ChromeThread::chrome_threads_[ID_COUNT];
 
 ChromeThread::ChromeThread(ChromeThread::ID identifier)
     : Thread(chrome_thread_names[identifier]),
@@ -35,10 +26,10 @@
   Initialize();
 }
 
-ChromeThread::ChromeThread()
-    : Thread(MessageLoop::current()->thread_name().c_str()),
-      identifier_(UI) {
-  set_message_loop(MessageLoop::current());
+ChromeThread::ChromeThread(ID identifier, MessageLoop* message_loop)
+    : Thread(message_loop->thread_name().c_str()),
+      identifier_(identifier) {
+  set_message_loop(message_loop);
   Initialize();
 }
 
@@ -52,26 +43,108 @@
 ChromeThread::~ChromeThread() {
   AutoLock lock(lock_);
   chrome_threads_[identifier_] = NULL;
+#ifndef NDEBUG
+  // Double check that the threads are ordererd correctly in the enumeration.
+  for (int i = identifier_ + 1; i < ID_COUNT; ++i) {
+    DCHECK(!chrome_threads_[i]) <<
+        "Threads must be listed in the reverse order that they die";
+  }
+#endif
 }
 
 // static
-MessageLoop* ChromeThread::GetMessageLoop(ID identifier) {
+bool ChromeThread::CurrentlyOn(ID identifier) {
+  // chrome_threads_[identifier] will be NULL if none is running.  This is often
+  // true when running under unit tests.  This behavior actually works out
+  // pretty conveniently but it's worth noting here.
   AutoLock lock(lock_);
   DCHECK(identifier >= 0 && identifier < ID_COUNT);
+  return chrome_threads_[identifier] &&
+         chrome_threads_[identifier]->message_loop() == MessageLoop::current();
+}
 
-  if (chrome_threads_[identifier])
-    return chrome_threads_[identifier]->message_loop();
+// static
+bool ChromeThread::PostTask(ID identifier,
+                            const tracked_objects::Location& from_here,
+                            Task* task) {
+  return PostTaskHelper(identifier, from_here, task, 0, true);
+}
 
-  return NULL;
+// static
+bool ChromeThread::PostDelayedTask(ID identifier,
+                                   const tracked_objects::Location& from_here,
+                                   Task* task,
+                                   int64 delay_ms) {
+  return PostTaskHelper(identifier, from_here, task, delay_ms, true);
 }
 
 // static
-bool ChromeThread::CurrentlyOn(ID identifier) {
-  // MessageLoop::current() will return NULL if none is running.  This is often
-  // true when running under unit tests.  This behavior actually works out
-  // pretty convienently (as is mentioned in the header file comment), but it's
-  // worth noting here.
-  MessageLoop* message_loop = GetMessageLoop(identifier);
-  return MessageLoop::current() == message_loop;
+bool ChromeThread::PostNonNestableTask(
+    ID identifier,
+    const tracked_objects::Location& from_here,
+    Task* task) {
+  return PostTaskHelper(identifier, from_here, task, 0, false);
 }
 
+// static
+bool ChromeThread::PostNonNestableDelayedTask(
+    ID identifier,
+    const tracked_objects::Location& from_here,
+    Task* task,
+    int64 delay_ms) {
+  return PostTaskHelper(identifier, from_here, task, delay_ms, false);
+}
+
+// static
+bool ChromeThread::GetCurrentThreadIdentifier(ID* identifier) {
+  MessageLoop* cur_message_loop = MessageLoop::current();
+  for (int i = 0; i < ID_COUNT; ++i) {
+    if (chrome_threads_[i] &&
+        chrome_threads_[i]->message_loop() == cur_message_loop) {
+      *identifier = chrome_threads_[i]->identifier_;
+      return true;
+    }
+  }
+
+  return false;
+}
+
+// static
+bool ChromeThread::PostTaskHelper(
+    ID identifier,
+    const tracked_objects::Location& from_here,
+    Task* task,
+    int64 delay_ms,
+    bool nestable) {
+  DCHECK(identifier >= 0 && identifier < ID_COUNT);
+  // Optimization: to avoid unnecessary locks, we listed the ID enumeration in
+  // order of lifetime.  So no need to lock if we know that the other thread
+  // outlives this one.
+  // Note: since the array is so small, ok to loop instead of creating a map,
+  // which would require a lock because std::map isn't thread safe, defeating
+  // the whole purpose of this optimization.
+  ID current_thread;
+  bool guaranteed_to_outlive_target_thread =
+      GetCurrentThreadIdentifier(&current_thread) &&
+      current_thread >= identifier;
+
+  if (!guaranteed_to_outlive_target_thread)
+    lock_.Acquire();
+
+  MessageLoop* message_loop = chrome_threads_[identifier] ?
+      chrome_threads_[identifier]->message_loop() : NULL;
+  if (message_loop) {
+    if (nestable) {
+      message_loop->PostDelayedTask(from_here, task, delay_ms);
+    } else {
+      message_loop->PostNonNestableDelayedTask(from_here, task, delay_ms);
+    }
+  } else {
+    delete task;

[darin (slow to review), 2009/10/27 00:06:52]

maybe this should have a DLOG(INFO)?  or would that be too spammy for linux
developers?

[jam, 2009/10/27 02:38:18]

I don't know how spammy it would be so I'm hesitant to add it.  But even if it's
not spammy, would there be any actionable thing that a developer would do?  If
it's harmless, it would just clog up the output and hide more important
statements, no?

[darin (slow to review), 2009/10/27 04:43:33]

True... I think we'll likely end up with some valgrind / purify suppressions as
a result of this.  That kind of sucks.  I wish we had a better solution that
didn't leak memory on shutdown.

+  }
+
+  if (!guaranteed_to_outlive_target_thread)
+    lock_.Release();
+
+  return !!message_loop;
+}