Fix bug in ObserverListThreadsafe::RemoveObserver

Like ObserverList, ObserverListThreadSafe::RemoveObserver should do
nothing if the observer isn't in the list.  Instead, it causes weird
bugs if the list has 0 or 1 existing elements.

Add comments clarifying semantics of AddObserver/RemoveObserver.

Change AddObserver to do nothing in release mode if it tries to add an existing observer.

BUG=84922
TEST=


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

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

1 files changed, 34 insertions, 1 deletions

diff --git a/base/observer_list_unittest.cc b/base/observer_list_unittest.cc
index d313367..8315a2b 100644
--- a/base/observer_list_unittest.cc
+++ b/base/observer_list_unittest.cc
@@ -175,7 +175,7 @@ class AddRemoveThread : public PlatformThread::Delegate,
 
 TEST(ObserverListTest, BasicTest) {
   ObserverList<Foo> observer_list;
-  Adder a(1), b(-1), c(1), d(-1);
+  Adder a(1), b(-1), c(1), d(-1), e(-1);
   Disrupter evil(&observer_list, &c);
 
   observer_list.AddObserver(&a);
@@ -187,12 +187,16 @@ TEST(ObserverListTest, BasicTest) {
   observer_list.AddObserver(&c);
   observer_list.AddObserver(&d);
 
+  // Removing an observer not in the list should do nothing.
+  observer_list.RemoveObserver(&e);
+
   FOR_EACH_OBSERVER(Foo, observer_list, Observe(10));
 
   EXPECT_EQ(a.total, 20);
   EXPECT_EQ(b.total, -20);
   EXPECT_EQ(c.total, 0);
   EXPECT_EQ(d.total, -10);
+  EXPECT_EQ(e.total, 0);
 }
 
 TEST(ObserverListThreadSafeTest, BasicTest) {
@@ -225,6 +229,35 @@ TEST(ObserverListThreadSafeTest, BasicTest) {
   EXPECT_EQ(d.total, -10);
 }
 
+TEST(ObserverListThreadSafeTest, RemoveObserver) {
+  MessageLoop loop;
+
+  scoped_refptr<ObserverListThreadSafe<Foo> > observer_list(
+      new ObserverListThreadSafe<Foo>);
+  Adder a(1), b(1);
+
+  // Should do nothing.
+  observer_list->RemoveObserver(&a);
+  observer_list->RemoveObserver(&b);
+
+  observer_list->Notify(&Foo::Observe, 10);
+  loop.RunAllPending();
+
+  EXPECT_EQ(a.total, 0);
+  EXPECT_EQ(b.total, 0);
+
+  observer_list->AddObserver(&a);
+
+  // Should also do nothing.
+  observer_list->RemoveObserver(&b);
+
+  observer_list->Notify(&Foo::Observe, 10);
+  loop.RunAllPending();
+
+  EXPECT_EQ(a.total, 10);
+  EXPECT_EQ(b.total, 0);
+}
+
 class FooRemover : public Foo {
  public:
   explicit FooRemover(ObserverListThreadSafe<Foo>* list) : list_(list) {}