Allow ObserverListThreadSafe to be used from sequenced tasks.

base/observer_list_unittest.cc

 // Copyright (c) 2012 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.
 
 #include "base/observer_list.h"
-#include "base/observer_list_threadsafe.h"

[gab, 2017/01/12 16:24:06]

Keep it up here I'd say (though it's weird to conflate testing of two cc's in
one unittest.cc but that's not worth changing in this CL)

[fdoray, 2017/01/12 17:17:57]

Done.

 
+#include <utility>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list_threadsafe.h"
 #include "base/run_loop.h"
+#include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/test/scoped_task_scheduler.h"
 #include "base/threading/platform_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
 namespace {
 
 class Foo {
  public:
   virtual void Observe(int x) = 0;
   virtual ~Foo() {}
@@ skipping 297 matching lines @@
   // Should also do nothing.
   observer_list->RemoveObserver(&b);
 
   observer_list->Notify(FROM_HERE, &Foo::Observe, 10);
   RunLoop().RunUntilIdle();
 
   EXPECT_EQ(10, a.total);
   EXPECT_EQ(0, b.total);
 }
 
-TEST(ObserverListThreadSafeTest, WithoutMessageLoop) {
+TEST(ObserverListThreadSafeTest, WithoutSequence) {
   scoped_refptr<ObserverListThreadSafe<Foo> > observer_list(
       new ObserverListThreadSafe<Foo>);
 
   Adder a(1), b(1), c(1);
 
-  // No MessageLoop, so these should not be added.
+  // No sequence, so these should not be added.
   observer_list->AddObserver(&a);
   observer_list->AddObserver(&b);
 
   {
-    // Add c when there's a loop.
+    // Add c when there's a sequence.
     MessageLoop loop;
     observer_list->AddObserver(&c);
 
     observer_list->Notify(FROM_HERE, &Foo::Observe, 10);
     RunLoop().RunUntilIdle();
 
     EXPECT_EQ(0, a.total);
     EXPECT_EQ(0, b.total);
     EXPECT_EQ(10, c.total);
 
-    // Now add a when there's a loop.
+    // Now add a when there's a sequence.
     observer_list->AddObserver(&a);
 
-    // Remove c when there's a loop.
+    // Remove c when there's a sequence.
     observer_list->RemoveObserver(&c);
 
     // Notify again.
     observer_list->Notify(FROM_HERE, &Foo::Observe, 20);
     RunLoop().RunUntilIdle();
 
     EXPECT_EQ(20, a.total);
     EXPECT_EQ(0, b.total);
     EXPECT_EQ(10, c.total);
   }
 
-  // Removing should always succeed with or without a loop.
+  // Removing should always succeed with or without a sequence.
   observer_list->RemoveObserver(&a);
 
   // Notifying should not fail but should also be a no-op.
   MessageLoop loop;
   observer_list->AddObserver(&b);
   observer_list->Notify(FROM_HERE, &Foo::Observe, 30);
   RunLoop().RunUntilIdle();
 
   EXPECT_EQ(20, a.total);
   EXPECT_EQ(30, b.total);
@@ skipping 104 matching lines @@
   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(FROM_HERE, &Foo::Observe, 1);
 }
 
+namespace {
+
+class SequenceVerificationObserver : public Foo {
+ public:
+  explicit SequenceVerificationObserver(
+      scoped_refptr<SequencedTaskRunner> task_runner)
+      : task_runner_(std::move(task_runner)) {}
+  ~SequenceVerificationObserver() override = default;
+
+  void Observe(int x) override {
+    called_on_valid_sequence_ = task_runner_->RunsTasksOnCurrentThread();
+  }
+
+  bool called_on_valid_sequence() const { return called_on_valid_sequence_; }
+
+ private:
+  const scoped_refptr<SequencedTaskRunner> task_runner_;
+  bool called_on_valid_sequence_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(SequenceVerificationObserver);
+};
+
+}  // namespace
+
+// Verify that observers are notified on the correct sequence.
+TEST(ObserverListThreadSafeTest, NotificationOnValidSequence) {
+  using ObserverListType = ObserverListThreadSafe<Foo>;
+
+  test::ScopedTaskScheduler scoped_task_scheduler;
+
+  auto task_runner_1 = CreateSequencedTaskRunnerWithTraits(TaskTraits());
+  auto task_runner_2 = CreateSequencedTaskRunnerWithTraits(TaskTraits());
+
+  auto observer_list = make_scoped_refptr(new ObserverListType());
+
+  SequenceVerificationObserver observer_1(task_runner_1);
+  SequenceVerificationObserver observer_2(task_runner_2);
+
+  task_runner_1->PostTask(FROM_HERE,
+                          Bind(&ObserverListType::AddObserver, observer_list,
+                               Unretained(&observer_1)));
+  task_runner_2->PostTask(FROM_HERE,
+                          Bind(&ObserverListType::AddObserver, observer_list,
+                               Unretained(&observer_2)));
+
+  RunLoop().RunUntilIdle();
+
+  observer_list->Notify(FROM_HERE, &Foo::Observe, 1);
+
+  RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(observer_1.called_on_valid_sequence());
+  EXPECT_TRUE(observer_2.called_on_valid_sequence());
+}
+
+// Verify that when an observer is added to a NOTIFY_ALL ObserverListThreadSafe
+// from a notification, it is itself notified.
+TEST(ObserverListThreadSafeTest, AddObserverFromNotificationNotifyAll) {
+  using ObserverListType = ObserverListThreadSafe<Foo>;

[gab, 2017/01/12 16:24:06]

Don't think the using is worth it for two sites, it merely obscures types used
(same above)

[fdoray, 2017/01/12 17:17:57]

Done.

+
+  MessageLoop message_loop;
+  auto observer_list = make_scoped_refptr(new ObserverListType());
+
+  Adder observer_added_from_notification(1);
+
+  AddInObserve<ObserverListType> initial_observer(observer_list.get());
+  initial_observer.SetToAdd(&observer_added_from_notification);
+  observer_list->AddObserver(&initial_observer);
+
+  observer_list->Notify(FROM_HERE, &Foo::Observe, 1);
+
+  RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1, observer_added_from_notification.GetValue());
+}
+
 TEST(ObserverListTest, Existing) {
   ObserverList<Foo> observer_list(ObserverList<Foo>::NOTIFY_EXISTING_ONLY);
   Adder a(1);
   AddInObserve<ObserverList<Foo> > b(&observer_list);
   Adder c(1);
   b.SetToAdd(&c);
 
   observer_list.AddObserver(&a);
   observer_list.AddObserver(&b);
 
@@ skipping 422 matching lines @@
     // However, the first Observe() call will add a second observer: at this
     // point, it != observer_list.end() should be true, and Observe() should be
     // called on the newly added observer on the next iteration of the loop.
     observer.Observe(10);
   }
 
   EXPECT_EQ(-10, b.total);
 }
 
 }  // namespace base