Revert 96013 - Use MessageLoopProxy instead of MessageLoop to dispatch...

Revert 96013 - Use MessageLoopProxy instead of MessageLoop to dispatch notifications in ObserverListThreadsafe.

BUG=91589

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

TBR=adamk@chromium.org
Review URL: http://codereview.chromium.org/7605002

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

base/observer_list_threadsafe.h

@@ -14,7 +14,6 @@
 #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"
 
@@ -96,12 +95,9 @@ class ObserverListThreadSafe
       return;  // Some unittests may access this without a message loop.
     {
       base::AutoLock lock(list_lock_);
-      std::pair<typename ObserversListMap::iterator, bool> result =
-          observer_lists_.insert(
-              typename ObserversListMap::value_type(loop, NULL));
-      if (result.second)
-        result.first->second = new ObserverListContext(type_);
-      list = &(result.first->second->list);
+      if (observer_lists_.find(loop) == observer_lists_.end())
+        observer_lists_[loop] = new ObserverList<ObserverType>(type_);
+      list = observer_lists_[loop];
     }
     list->AddObserver(obs);
   }
@@ -112,7 +108,6 @@ 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) {
-    ObserverListContext* context = NULL;
     ObserverList<ObserverType>* list = NULL;
     MessageLoop* loop = MessageLoop::current();
     if (!loop)
@@ -121,12 +116,11 @@ class ObserverListThreadSafe
       base::AutoLock lock(list_lock_);
       typename ObserversListMap::iterator it = observer_lists_.find(loop);
       if (it == observer_lists_.end()) {
-        // This will happen if we try to remove an observer on a thread
+        // This may happen if we try to remove an observer on a thread
         // we never added an observer for.
         return;
       }
-      context = it->second;
-      list = &context->list;
+      list = it->second;
 
       // If we're about to remove the last observer from the list,
       // then we can remove this observer_list entirely.
@@ -139,7 +133,7 @@ class ObserverListThreadSafe
     // nonzero.  Instead of deleting here, the NotifyWrapper will delete
     // when it finishes iterating.
     if (list->size() == 0)
-      delete context;
+      delete list;
   }
 
   // Notify methods.
@@ -186,18 +180,6 @@ class ObserverListThreadSafe
   // See comment above ObserverListThreadSafeTraits' definition.
   friend struct ObserverListThreadSafeTraits<ObserverType>;
 
-  struct ObserverListContext {
-    explicit ObserverListContext(NotificationType type)
-        : loop(base::MessageLoopProxy::CreateForCurrentThread()),
-          list(type) {
-    }
-
-    scoped_refptr<base::MessageLoopProxy> loop;
-    ObserverList<ObserverType> list;
-
-    DISALLOW_COPY_AND_ASSIGN(ObserverListContext);
-  };
-
   ~ObserverListThreadSafe() {
     typename ObserversListMap::const_iterator it;
     for (it = observer_lists_.begin(); it != observer_lists_.end(); ++it)
@@ -210,12 +192,13 @@ class ObserverListThreadSafe
     base::AutoLock lock(list_lock_);
     typename ObserversListMap::iterator it;
     for (it = observer_lists_.begin(); it != observer_lists_.end(); ++it) {
-      ObserverListContext* context = (*it).second;
-      context->loop->PostTask(
+      MessageLoop* loop = (*it).first;
+      ObserverList<ObserverType>* list = (*it).second;
+      loop->PostTask(
           FROM_HERE,
           NewRunnableMethod(this,
               &ObserverListThreadSafe<ObserverType>::
-                 template NotifyWrapper<Method, Params>, context, method));
+                 template NotifyWrapper<Method, Params>, list, method));
     }
   }
 
@@ -223,7 +206,7 @@ class ObserverListThreadSafe
   // ObserverList.  This function MUST be called on the thread which owns
   // the unsafe ObserverList.
   template <class Method, class Params>
-  void NotifyWrapper(ObserverListContext* context,
+  void NotifyWrapper(ObserverList<ObserverType>* list,
       const UnboundMethod<ObserverType, Method, Params>& method) {
 
     // Check that this list still needs notifications.
@@ -236,19 +219,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 != context)
+      if (it == observer_lists_.end() || it->second != list)
         return;
     }
 
     {
-      typename ObserverList<ObserverType>::Iterator it(context->list);
+      typename ObserverList<ObserverType>::Iterator it(*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 (context->list.size() == 0) {
+    if (list->size() == 0) {
       {
         base::AutoLock lock(list_lock_);
         // Remove |list| if it's not already removed.
@@ -256,15 +239,16 @@ class ObserverListThreadSafe
         // See http://crbug.com/55725.
         typename ObserversListMap::iterator it =
             observer_lists_.find(MessageLoop::current());
-        if (it != observer_lists_.end() && it->second == context)
+        if (it != observer_lists_.end() && it->second == list)
           observer_lists_.erase(it);
       }
-      delete context;
+      delete list;
     }
   }
 
-  typedef std::map<MessageLoop*, ObserverListContext*> ObserversListMap;
+  typedef std::map<MessageLoop*, ObserverList<ObserverType>*> 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_;

base/observer_list_unittest.cc

@@ -61,10 +61,9 @@ class ThreadSafeDisrupter : public Foo {
   Foo* doomed_;
 };
 
-template <typename ObserverListType>
 class AddInObserve : public Foo {
  public:
-  explicit AddInObserve(ObserverListType* observer_list)
+  explicit AddInObserve(ObserverList<Foo>* observer_list)
       : added(false),
         observer_list(observer_list),
         adder(1) {
@@ -77,11 +76,14 @@ class AddInObserve : public Foo {
   }
 
   bool added;
-  ObserverListType* observer_list;
+  ObserverList<Foo>* observer_list;
   Adder adder;
 };
 
 
+class ObserverListThreadSafeTest : public testing::Test {
+};
+
 static const int kThreadRunTime = 2000;  // ms to run the multi-threaded test.
 
 // A thread for use in the ThreadSafeObserver test
@@ -355,22 +357,10 @@ TEST(ObserverListThreadSafeTest, CrossThreadNotifications) {
   ThreadSafeObserverHarness(3, true);
 }
 
-TEST(ObserverListThreadSafeTest, OutlivesMessageLoop) {
-  MessageLoop* loop = new MessageLoop;
-  scoped_refptr<ObserverListThreadSafe<Foo> > observer_list(
-      new ObserverListThreadSafe<Foo>);
-
-  Adder a(1);
-  observer_list->AddObserver(&a);
-  delete loop;
-  // Test passes if we don't crash here.
-  observer_list->Notify(&Foo::Observe, 1);
-}
-
 TEST(ObserverListTest, Existing) {
   ObserverList<Foo> observer_list(ObserverList<Foo>::NOTIFY_EXISTING_ONLY);
   Adder a(1);
-  AddInObserve<ObserverList<Foo> > b(&observer_list);
+  AddInObserve b(&observer_list);
 
   observer_list.AddObserver(&a);
   observer_list.AddObserver(&b);
@@ -387,31 +377,6 @@ TEST(ObserverListTest, Existing) {
   EXPECT_EQ(1, b.adder.total);
 }
 
-// Same as above, but for ObserverListThreadSafe
-TEST(ObserverListThreadSafeTest, Existing) {
-  MessageLoop loop;
-  scoped_refptr<ObserverListThreadSafe<Foo> > observer_list(
-      new ObserverListThreadSafe<Foo>(ObserverList<Foo>::NOTIFY_EXISTING_ONLY));
-  Adder a(1);
-  AddInObserve<ObserverListThreadSafe<Foo> > b(observer_list.get());
-
-  observer_list->AddObserver(&a);
-  observer_list->AddObserver(&b);
-
-  observer_list->Notify(&Foo::Observe, 1);
-  loop.RunAllPending();
-
-  EXPECT_TRUE(b.added);
-  // B's adder should not have been notified because it was added during
-  // notificaiton.
-  EXPECT_EQ(0, b.adder.total);
-
-  // Notify again to make sure b's adder is notified.
-  observer_list->Notify(&Foo::Observe, 1);
-  loop.RunAllPending();
-  EXPECT_EQ(1, b.adder.total);
-}
-
 class AddInClearObserve : public Foo {
  public:
   explicit AddInClearObserve(ObserverList<Foo>* list)