Use MessageLoopProxy instead of MessageLoop to dispatch notifications in ObserverListThreadsafe.

base/observer_list_threadsafe.h

 // Copyright (c) 2011 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.
 
 #ifndef BASE_OBSERVER_LIST_THREADSAFE_H_
 #define BASE_OBSERVER_LIST_THREADSAFE_H_
 #pragma once
 
 #include <algorithm>
 #include <map>
 
 #include "base/basictypes.h"
 #include "base/callback_old.h"
 #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"
 
 ///////////////////////////////////////////////////////////////////////////////
 //
 // OVERVIEW:
 //
 //   A thread-safe container for a list of observers.
 //   This is similar to the observer_list (see observer_list.h), but it
 //   is more robust for multi-threaded situations.
@@ skipping 52 matching lines @@
   typedef typename ObserverList<ObserverType>::NotificationType
       NotificationType;
 
   ObserverListThreadSafe()
       : type_(ObserverListBase<ObserverType>::NOTIFY_ALL) {}
   explicit ObserverListThreadSafe(NotificationType type) : type_(type) {}
 
   // 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.
     //                And unittests use time without a MessageLoop.
     if (!loop)
       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.
   // If there are pending notifications in-transit to the observer, they will
   // be aborted.
   // 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.
     {
       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.
   // Make a thread-safe callback to each Observer in the list.
   // Note, these calls are effectively asynchronous.  You cannot assume
   // that at the completion of the Notify call that all Observers have
   // been Notified.  The notification may still be pending delivery.
   template <class Method>
   void Notify(Method m) {
     UnboundMethod<ObserverType, Method, Tuple0> method(m, MakeTuple());
@@ skipping 26 matching lines @@
         m, MakeTuple(a, b, c, d));
     Notify<Method, Tuple4<A, B, C, D> >(method);
   }
 
   // TODO(mbelshe):  Add more wrappers for Notify() with more arguments.
 
  private:
   // 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)
       delete (*it).second;
     observer_lists_.clear();
   }
 
   template <class Method, class Params>
   void Notify(const UnboundMethod<ObserverType, Method, Params>& method) {
     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));
     }
   }
 
   // Wrapper which is called to fire the notifications for each thread's
   // 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.
     {
       base::AutoLock lock(list_lock_);
       typename ObserversListMap::iterator it =
           observer_lists_.find(MessageLoop::current());
 
       // The ObserverList could have been removed already.  In fact, it could
       // 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.
         // This can happen if multiple observers got removed in a notification.
         // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(ObserverListThreadSafe);
 };
 
 #endif  // BASE_OBSERVER_LIST_THREADSAFE_H_