Allow ObserverListThreadSafe to be used from sequenced tasks.

base/observer_list_unittest.cc

Index: base/observer_list_unittest.cc
diff --git a/base/observer_list_unittest.cc b/base/observer_list_unittest.cc
index c5e556bd9da4e76f65ad9965fb256d02932e672b..7482d81c692c6ce889d959271edb31febc495258 100644
--- a/base/observer_list_unittest.cc
+++ b/base/observer_list_unittest.cc
@@ -3,15 +3,20 @@
 // 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"
 
@@ -329,18 +334,18 @@ TEST(ObserverListThreadSafeTest, RemoveObserver) {
   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);
 
@@ -351,10 +356,10 @@ TEST(ObserverListThreadSafeTest, WithoutMessageLoop) {
     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.
@@ -366,7 +371,7 @@ TEST(ObserverListThreadSafeTest, WithoutMessageLoop) {
     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.
@@ -491,6 +496,82 @@ TEST(ObserverListThreadSafeTest, OutlivesMessageLoop) {
   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);