TaskScheduler: Make SchedulerWorkerThread own its delegate.

base/task_scheduler/scheduler_worker_thread_unittest.cc

 // Copyright 2016 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/task_scheduler/scheduler_worker_thread.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/task_scheduler/scheduler_lock.h"
 #include "base/task_scheduler/sequence.h"
 #include "base/task_scheduler/task.h"
 #include "base/task_scheduler/task_tracker.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
 namespace internal {
 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();
   }
 
   void TearDown() override {
     worker_thread_->JoinForTesting();
   }
 
   size_t TasksPerSequence() const { return GetParam(); }
 
@@ skipping 26 matching lines @@
   }
 
   std::vector<scoped_refptr<Sequence>> EnqueuedSequences() {
     AutoSchedulerLock auto_lock(lock_);
     return enqueued_sequences_;
   }
 
   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();
-  }
+  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) {}
 
-  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_;
+    // 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_);
     ++num_run_tasks_;
     EXPECT_LE(num_run_tasks_, created_sequences_.size());
   }
 
   TaskTracker task_tracker_;
 
   // Synchronizes access to all members below.
@@ skipping 91 matching lines @@
 INSTANTIATE_TEST_CASE_P(OneTaskPerSequence,
                         TaskSchedulerWorkerThreadTest,
                         ::testing::Values(1));
 INSTANTIATE_TEST_CASE_P(TwoTasksPerSequence,
                         TaskSchedulerWorkerThreadTest,
                         ::testing::Values(2));
 
 }  // namespace
 }  // namespace internal
 }  // namespace base