TaskScheduler: Make SchedulerWorkerThread own its delegate.

base/task_scheduler/scheduler_worker_thread_unittest.cc

Index: base/task_scheduler/scheduler_worker_thread_unittest.cc
diff --git a/base/task_scheduler/scheduler_worker_thread_unittest.cc b/base/task_scheduler/scheduler_worker_thread_unittest.cc
index 0131dcf0d7b07f9989ebf46d7247fd9b247fb29c..a3df98ea54f8ba0009c0b563a393eeda2cd6f3d1 100644
--- a/base/task_scheduler/scheduler_worker_thread_unittest.cc
+++ b/base/task_scheduler/scheduler_worker_thread_unittest.cc
@@ -27,16 +27,17 @@ namespace {
 const size_t kNumSequencesPerTest = 150;
 
 // The test parameter is the number of Tasks per Sequence returned by GetWork().
-class TaskSchedulerWorkerThreadTest : public testing::TestWithParam<size_t>,
-                                      public SchedulerWorkerThread::Delegate {
+class TaskSchedulerWorkerThreadTest : public testing::TestWithParam<size_t> {
  protected:
   TaskSchedulerWorkerThreadTest()
       : main_entry_called_(true, false),
         num_get_work_cv_(lock_.CreateConditionVariable()) {}
 
   void SetUp() override {
-    worker_thread_ = SchedulerWorkerThread::CreateSchedulerWorkerThread(
-        ThreadPriority::NORMAL, this, &task_tracker_);
+    worker_thread_ = SchedulerWorkerThread::CreateWorkerThread(
+        ThreadPriority::NORMAL,
+        WrapUnique(new SchedulerWorkerThreadDelegateImpl(this)),

[gab, 2016/04/22 19:30:29]

This compiles without a fwd-decl of SchedulerWorkerThreadDelegateTestImpl?

[fdoray, 2016/04/22 22:46:46]

Yes. We can use any type defined in the class in the methods of the class, even
if the definition of the type is after the method.

+        &task_tracker_);
     ASSERT_TRUE(worker_thread_);
     main_entry_called_.Wait();
   }
@@ -83,74 +84,86 @@ class TaskSchedulerWorkerThreadTest : public testing::TestWithParam<size_t>,
   std::unique_ptr<SchedulerWorkerThread> worker_thread_;
 
  private:
-  // SchedulerWorkerThread::Delegate:
-  void OnMainEntry() override {
-    // Without this |auto_lock|, OnMainEntry() could be called twice without
-    // generating an error.
-    AutoSchedulerLock auto_lock(lock_);
-    EXPECT_FALSE(main_entry_called_.IsSignaled());
-    main_entry_called_.Signal();
-  }
-
-  scoped_refptr<Sequence> GetWork(
-      SchedulerWorkerThread* worker_thread) override {
-    EXPECT_EQ(worker_thread_.get(), worker_thread);
-
-    {
-      AutoSchedulerLock auto_lock(lock_);
-
-      // Increment the number of times that this method has been called.
-      ++num_get_work_;
-      num_get_work_cv_->Signal();
-
-      // Verify that this method isn't called more times than expected.
-      EXPECT_LE(num_get_work_, max_get_work_);
-
-      // Check if a Sequence should be returned.
-      if (num_sequences_to_create_ == 0)
-        return nullptr;
-      --num_sequences_to_create_;
+  class SchedulerWorkerThreadDelegateImpl

[gab, 2016/04/22 19:30:29]

Name this differently than main impl, e.g. SchedulerWorkerThreadDelegateTestImpl

[danakj, 2016/04/22 20:16:33]

nit: not a fan of "test" in class names. Suggest following the naming scheme
here: http://martinfowler.com/articles/mocksArentStubs.html

"FakeSchedulerWorkerThreadDelegate" or "FakeWorkerThreadDelegate" or something
is my suggestion.

Also suggest making this not nested, and expose things on the outer class as
needed for it.

[robliao, 2016/04/22 20:30:48]

There is a reasonable precedent in Chrome to name things with Test at the
beginning, so maybe

TestSchedulerWorkerThreadDelegate

[fdoray, 2016/04/22 22:46:46]

Done. Renamed to TestSchedulerWorkerThreadDelegate.

+      : public SchedulerWorkerThread::Delegate {
+   public:
+    SchedulerWorkerThreadDelegateImpl(TaskSchedulerWorkerThreadTest* outer)
+        : outer_(outer) {}
+
+    // SchedulerWorkerThread::Delegate:
+    void OnMainEntry() override {
+      // Without this |auto_lock|, OnMainEntry() could be called twice without

[robliao, 2016/04/22 20:30:48]

Nit: Without synchronization

[fdoray, 2016/04/22 22:46:46]

Done.

+      // generating an error.
+      AutoSchedulerLock auto_lock(outer_->lock_);
+      EXPECT_FALSE(outer_->main_entry_called_.IsSignaled());
+      outer_->main_entry_called_.Signal();
     }
 
-    // Create a Sequence with TasksPerSequence() Tasks.
-    scoped_refptr<Sequence> sequence(new Sequence);
-    for (size_t i = 0; i < TasksPerSequence(); ++i) {
-      std::unique_ptr<Task> task(new Task(
-          FROM_HERE, Bind(&TaskSchedulerWorkerThreadTest::RunTaskCallback,
-                          Unretained(this)),
-          TaskTraits(), TimeTicks()));
-      EXPECT_TRUE(task_tracker_.WillPostTask(task.get()));
-      sequence->PushTask(std::move(task));
+    scoped_refptr<Sequence> GetWork(
+        SchedulerWorkerThread* worker_thread) override {
+      EXPECT_EQ(outer_->worker_thread_.get(), worker_thread);
+
+      {
+        AutoSchedulerLock auto_lock(outer_->lock_);
+
+        // Increment the number of times that this method has been called.
+        ++outer_->num_get_work_;
+        outer_->num_get_work_cv_->Signal();
+
+        // Verify that this method isn't called more times than expected.
+        EXPECT_LE(outer_->num_get_work_, outer_->max_get_work_);
+
+        // Check if a Sequence should be returned.
+        if (outer_->num_sequences_to_create_ == 0)
+          return nullptr;
+        --outer_->num_sequences_to_create_;
+      }
+
+      // Create a Sequence with TasksPerSequence() Tasks.
+      scoped_refptr<Sequence> sequence(new Sequence);
+      for (size_t i = 0; i < outer_->TasksPerSequence(); ++i) {
+        std::unique_ptr<Task> task(new Task(
+            FROM_HERE, Bind(&TaskSchedulerWorkerThreadTest::RunTaskCallback,
+                            Unretained(outer_)),
+            TaskTraits(), TimeTicks()));
+        EXPECT_TRUE(outer_->task_tracker_.WillPostTask(task.get()));
+        sequence->PushTask(std::move(task));
+      }
+
+      {
+        // Add the Sequence to the vector of created Sequences.
+        AutoSchedulerLock auto_lock(outer_->lock_);
+        outer_->created_sequences_.push_back(sequence);
+      }
+
+      return sequence;
     }
 
-    {
-      // Add the Sequence to the vector of created Sequences.
-      AutoSchedulerLock auto_lock(lock_);
-      created_sequences_.push_back(sequence);
+    // This override verifies that |sequence| contains the expected number of
+    // Tasks and adds it to |enqueued_sequences_|. Unlike a normal
+    // EnqueueSequence
+    // implementation, it doesn't reinsert |sequence| into a queue for further
+    // execution.
+    void EnqueueSequence(scoped_refptr<Sequence> sequence) override {
+      EXPECT_GT(outer_->TasksPerSequence(), 1U);
+
+      // Verify that |sequence| contains TasksPerSequence() - 1 Tasks.
+      for (size_t i = 0; i < outer_->TasksPerSequence() - 1; ++i) {
+        EXPECT_TRUE(sequence->PeekTask());
+        sequence->PopTask();
+      }
+      EXPECT_FALSE(sequence->PeekTask());
+
+      // Add |sequence| to |enqueued_sequences_|.
+      AutoSchedulerLock auto_lock(outer_->lock_);
+      outer_->enqueued_sequences_.push_back(std::move(sequence));
+      EXPECT_LE(outer_->enqueued_sequences_.size(),
+                outer_->created_sequences_.size());
     }
 
-    return sequence;
-  }
-
-  // This override verifies that |sequence| contains the expected number of
-  // Tasks and adds it to |enqueued_sequences_|. Unlike a normal EnqueueSequence
-  // implementation, it doesn't reinsert |sequence| into a queue for further
-  // execution.
-  void EnqueueSequence(scoped_refptr<Sequence> sequence) override {
-    EXPECT_GT(TasksPerSequence(), 1U);
-
-    // Verify that |sequence| contains TasksPerSequence() - 1 Tasks.
-    for (size_t i = 0; i < TasksPerSequence() - 1; ++i) {
-      EXPECT_TRUE(sequence->PeekTask());
-      sequence->PopTask();
-    }
-    EXPECT_FALSE(sequence->PeekTask());
-
-    // Add |sequence| to |enqueued_sequences_|.
-    AutoSchedulerLock auto_lock(lock_);
-    enqueued_sequences_.push_back(std::move(sequence));
-    EXPECT_LE(enqueued_sequences_.size(), created_sequences_.size());
-  }
+   private:
+    TaskSchedulerWorkerThreadTest* outer_;
+  };
 
   void RunTaskCallback() {
     AutoSchedulerLock auto_lock(lock_);