Use MessageLoopProxy instead of MessageLoop to dispatch notifications in ObserverListThreadsafe.

base/observer_list_threadsafe.h

Index: base/observer_list_threadsafe.h
diff --git a/base/observer_list_threadsafe.h b/base/observer_list_threadsafe.h
index f4ff68d7c942513dddcb79f2c26bd90549eeab5e..eb3ed19575f5f028483cd9c4e91e5343171bb242 100644
--- a/base/observer_list_threadsafe.h
+++ b/base/observer_list_threadsafe.h
@@ -14,6 +14,7 @@
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop.h"
+#include "base/message_loop_proxy.h"
 #include "base/observer_list.h"
 #include "base/task.h"
 
@@ -86,7 +87,7 @@ class ObserverListThreadSafe
   // Add an observer to the list.  An observer should not be added to
   // the same list more than once.
   void AddObserver(ObserverType* obs) {
-    ObserverList<ObserverType>* list = NULL;
+    ObserverListContext* context = NULL;
     MessageLoop* loop = MessageLoop::current();
     // TODO(mbelshe): Get rid of this check.  Its needed right now because
     //                Time currently triggers usage of the ObserverList.
@@ -95,11 +96,16 @@ class ObserverListThreadSafe
       return;  // Some unittests may access this without a message loop.
     {
       base::AutoLock lock(list_lock_);
-      if (observer_lists_.find(loop) == observer_lists_.end())
-        observer_lists_[loop] = new ObserverList<ObserverType>(type_);
-      list = observer_lists_[loop];
+      typename ObserversListMap::iterator it = observer_lists_.find(loop);

[darin (slow to review), 2011/08/08 19:33:21]

nit: maybe it is not worth it, but you could just do the insert without the
find.  to avoid allocating an ObseverListContext each time, you could only set
that field if need be.

  std::pair<typename ObserversListMap::iterator, bool> result =
      observer_lists_.insert(std::make_pair(loop, NULL));
  if (result.second)
    result.first->second = new ObserverListContext();

[adamk, 2011/08/08 19:50:04]

I've made the code do this.  Certainly not a big deal on performance, but easy
enough to do.

+      if (it == observer_lists_.end()) {
+        std::pair<typename ObserversListMap::iterator, bool> result =
+            observer_lists_.insert(
+                std::make_pair(loop, new ObserverListContext));
+        it = result.first;
+      }
+      context = it->second;
     }
-    list->AddObserver(obs);
+    context->list().AddObserver(obs);
   }
 
   // Remove an observer from the list if it is in the list.
@@ -108,7 +114,7 @@ class ObserverListThreadSafe
   // If the observer to be removed is in the list, RemoveObserver MUST
   // be called from the same thread which called AddObserver.
   void RemoveObserver(ObserverType* obs) {
-    ObserverList<ObserverType>* list = NULL;
+    ObserverListContext* context = NULL;
     MessageLoop* loop = MessageLoop::current();
     if (!loop)
       return;  // On shutdown, it is possible that current() is already null.
@@ -116,24 +122,25 @@ class ObserverListThreadSafe
       base::AutoLock lock(list_lock_);
       typename ObserversListMap::iterator it = observer_lists_.find(loop);
       if (it == observer_lists_.end()) {
-        // This may happen if we try to remove an observer on a thread
+        // This will happen if we try to remove an observer on a thread
         // we never added an observer for.
         return;
       }
-      list = it->second;
+      context = it->second;
 
       // If we're about to remove the last observer from the list,
       // then we can remove this observer_list entirely.
-      if (list->HasObserver(obs) && list->size() == 1)
+      if (context->list().HasObserver(obs) &&
+          context->list().size() == 1)
         observer_lists_.erase(it);
     }
-    list->RemoveObserver(obs);
+    context->list().RemoveObserver(obs);
 
     // If RemoveObserver is called from a notification, the size will be
     // nonzero.  Instead of deleting here, the NotifyWrapper will delete
     // when it finishes iterating.
-    if (list->size() == 0)
-      delete list;
+    if (context->list().size() == 0)
+      delete context;
   }
 
   // Notify methods.
@@ -180,6 +187,23 @@ class ObserverListThreadSafe
   // See comment above ObserverListThreadSafeTraits' definition.
   friend struct ObserverListThreadSafeTraits<ObserverType>;
 
+  class ObserverListContext {
+   public:
+    ObserverListContext()
+        : message_loop_proxy_(
+              base::MessageLoopProxy::CreateForCurrentThread()) {
+    }
+
+    base::MessageLoopProxy& loop() { return *message_loop_proxy_; }

[darin (slow to review), 2011/08/08 19:33:21]

perhaps there is no point in these member variables being private?

[adamk, 2011/08/08 19:50:04]

I suppose not.  Made this a struct instead (though left it non-copyable).

+    ObserverList<ObserverType>& list() { return observer_list_; }
+
+   private:
+    scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
+    ObserverList<ObserverType> observer_list_;
+
+    DISALLOW_COPY_AND_ASSIGN(ObserverListContext);
+  };
+
   ~ObserverListThreadSafe() {
     typename ObserversListMap::const_iterator it;
     for (it = observer_lists_.begin(); it != observer_lists_.end(); ++it)
@@ -192,13 +216,12 @@ class ObserverListThreadSafe
     base::AutoLock lock(list_lock_);
     typename ObserversListMap::iterator it;
     for (it = observer_lists_.begin(); it != observer_lists_.end(); ++it) {
-      MessageLoop* loop = (*it).first;
-      ObserverList<ObserverType>* list = (*it).second;
-      loop->PostTask(
+      ObserverListContext* context = (*it).second;
+      context->loop().PostTask(
           FROM_HERE,
           NewRunnableMethod(this,
               &ObserverListThreadSafe<ObserverType>::
-                 template NotifyWrapper<Method, Params>, list, method));
+                 template NotifyWrapper<Method, Params>, context, method));
     }
   }
 
@@ -206,7 +229,7 @@ class ObserverListThreadSafe
   // ObserverList.  This function MUST be called on the thread which owns
   // the unsafe ObserverList.
   template <class Method, class Params>
-  void NotifyWrapper(ObserverList<ObserverType>* list,
+  void NotifyWrapper(ObserverListContext* context,
       const UnboundMethod<ObserverType, Method, Params>& method) {
 
     // Check that this list still needs notifications.
@@ -219,19 +242,19 @@ class ObserverListThreadSafe
       // have been removed and then re-added!  If the master list's loop
       // does not match this one, then we do not need to finish this
       // notification.
-      if (it == observer_lists_.end() || it->second != list)
+      if (it == observer_lists_.end() || it->second != context)
         return;
     }
 
     {
-      typename ObserverList<ObserverType>::Iterator it(*list);
+      typename ObserverList<ObserverType>::Iterator it(context->list());
       ObserverType* obs;
       while ((obs = it.GetNext()) != NULL)
         method.Run(obs);
     }
 
     // If there are no more observers on the list, we can now delete it.
-    if (list->size() == 0) {
+    if (context->list().size() == 0) {
       {
         base::AutoLock lock(list_lock_);
         // Remove |list| if it's not already removed.
@@ -239,16 +262,15 @@ class ObserverListThreadSafe
         // See http://crbug.com/55725.
         typename ObserversListMap::iterator it =
             observer_lists_.find(MessageLoop::current());
-        if (it != observer_lists_.end() && it->second == list)
+        if (it != observer_lists_.end() && it->second == context)
           observer_lists_.erase(it);
       }
-      delete list;
+      delete context;
     }
   }
 
-  typedef std::map<MessageLoop*, ObserverList<ObserverType>*> ObserversListMap;
+  typedef std::map<MessageLoop*, ObserverListContext*> ObserversListMap;
 
-  // These are marked mutable to facilitate having NotifyAll be const.
   base::Lock list_lock_;  // Protects the observer_lists_.
   ObserversListMap observer_lists_;
   const NotificationType type_;