TaskScheduler: Delegate instead of callbacks for SchedulerWorkerThread.

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 fcccda182633678a4a45543e8f7c4274f8d7aa71..6a9eb1ce774abc0b7e0e60b7dce73b8463a0d766 100644
--- a/base/task_scheduler/scheduler_worker_thread_unittest.cc
+++ b/base/task_scheduler/scheduler_worker_thread_unittest.cc
@@ -15,6 +15,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/task_scheduler/scheduler_lock.h"
+#include "base/task_scheduler/scheduler_worker_thread_delegate.h"
 #include "base/task_scheduler/sequence.h"
 #include "base/task_scheduler/task.h"
 #include "base/task_scheduler/task_tracker.h"
@@ -26,51 +27,50 @@ namespace {
 
 const size_t kNumSequencesPerTest = 150;
 
-class TaskSchedulerWorkerThreadTest : public testing::Test {
+class TaskSchedulerWorkerThreadTest : public testing::Test,
+                                      public SchedulerWorkerThreadDelegate {
  protected:
   TaskSchedulerWorkerThreadTest()
-      : num_main_entry_callback_cv_(lock_.CreateConditionVariable()),
-        num_get_work_callback_cv_(lock_.CreateConditionVariable()),
+      : num_main_entry_cv_(lock_.CreateConditionVariable()),
+        num_get_work_cv_(lock_.CreateConditionVariable()),
         run_sequences_cv_(lock_.CreateConditionVariable()) {}
 
   void SetUp() override {
     worker_thread_ = SchedulerWorkerThread::CreateSchedulerWorkerThread(
-        ThreadPriority::NORMAL,
-        Bind(&TaskSchedulerWorkerThreadTest::MainEntryCallback,
-             Unretained(this)),
-        Bind(&TaskSchedulerWorkerThreadTest::GetWorkCallback, Unretained(this)),
-        Bind(&TaskSchedulerWorkerThreadTest::RanTaskFromSequenceCallback,
-             Unretained(this)),
-        &task_tracker_);
+        ThreadPriority::NORMAL, this, &task_tracker_);
     ASSERT_TRUE(worker_thread_);
-    WaitForNumMainEntryCallback(1);
+    WaitForMainEntry();
   }
 
   void TearDown() override {
+    {
+      AutoSchedulerLock auto_lock(lock_);
+      EXPECT_EQ(0U, num_main_exit_);
+    }
+
     worker_thread_->JoinForTesting();
 
     AutoSchedulerLock auto_lock(lock_);
-    EXPECT_EQ(1U, num_main_entry_callback_);
+    EXPECT_EQ(1U, num_main_entry_);

[gab, 2016/04/07 16:42:08]

Move to test before Join?

[fdoray, 2016/04/07 17:59:33]

I no longer need this in TearDown(). SetUp() verifies that OnMainEntry() is
called at least once and OnMainEntry() verifies that it isn't called more than
once.

+    EXPECT_EQ(1U, num_main_exit_);
   }
 
-  // Wait until MainEntryCallback() has been called |num_main_entry_callback|
-  // times.
-  void WaitForNumMainEntryCallback(size_t num_main_entry_callback) {
+  // Wait until OnMainEntry() has been called once.
+  void WaitForMainEntry() {
     AutoSchedulerLock auto_lock(lock_);
-    while (num_main_entry_callback_ < num_main_entry_callback)
-      num_main_entry_callback_cv_->Wait();
+    while (num_main_entry_ < 1)

[gab, 2016/04/07 16:42:08]

Make |num_main_entry_| a bool (and thus probably a WaitableEvent)? 

Same for |num_main_exit_|.

[fdoray, 2016/04/07 17:59:33]

Done.

+      num_main_entry_cv_->Wait();
   }
 
-  // Wait until GetWorkCallback() has been called |num_get_work_callback| times.
-  void WaitForNumGetWorkCallback(size_t num_get_work_callback) {
+  // Wait until GetWork() has been called |num_get_work| times.
+  void WaitForNumGetWork(size_t num_get_work) {
     AutoSchedulerLock auto_lock(lock_);
-    while (num_get_work_callback_ < num_get_work_callback)
-      num_get_work_callback_cv_->Wait();
+    while (num_get_work_ < num_get_work)
+      num_get_work_cv_->Wait();
   }
 
-  // Wait until there is no more Sequences to create and
-  // RanTaskFromSequenceCallback() has been invoked once for each Sequence
-  // returned by GetWorkCallback().
+  // Wait until there are no more Sequences to create and RanTaskFromSequence()
+  // has been invoked once for each Sequence returned by GetWork().
   void WaitForAllSequencesToRun() {
     AutoSchedulerLock auto_lock(lock_);
 
@@ -79,12 +79,12 @@ class TaskSchedulerWorkerThreadTest : public testing::Test {
       run_sequences_cv_->Wait();
     }
 
-    // Verify that RanTaskFromSequenceCallback() has been invoked with the
-    // same Sequences that were returned by GetWorkCallback().
+    // Verify that RanTaskFromSequence() has been invoked with the same
+    // Sequences that were returned by GetWork().
     EXPECT_EQ(created_sequences_, run_sequences_);
 
     // Verify that RunTaskCallback() has been invoked once for each Sequence
-    // returned by GetWorkCallback().
+    // returned by GetWork().
     EXPECT_EQ(created_sequences_.size(), num_run_tasks_);
   }
 
@@ -94,32 +94,33 @@ class TaskSchedulerWorkerThreadTest : public testing::Test {
     num_sequences_to_create_ = num_sequences_to_create;
   }
 
-  size_t NumGetWorkCallback() const {
+  size_t NumGetWork() const {
     AutoSchedulerLock auto_lock(lock_);
-    return num_get_work_callback_;
+    return num_get_work_;
   }
 
   std::unique_ptr<SchedulerWorkerThread> worker_thread_;
 
  private:
-  void MainEntryCallback() {
+  // SchedulerWorkerThreadDelegate:
+  void OnMainEntry() override {
     AutoSchedulerLock auto_lock(lock_);
-    ++num_main_entry_callback_;
-    num_main_entry_callback_cv_->Signal();
+    ++num_main_entry_;
+    num_main_entry_cv_->Signal();
   }
 
-  // Returns a Sequence that contains 1 Task if |num_sequences_to_create_| is
-  // greater than 0.
-  scoped_refptr<Sequence> GetWorkCallback(
-      SchedulerWorkerThread* worker_thread) {
-    EXPECT_EQ(worker_thread_.get(), worker_thread);
+  void OnMainExit() override {
+    AutoSchedulerLock auto_lock(lock_);
+    ++num_main_exit_;
+  }
 
+  scoped_refptr<Sequence> GetWork() override {
     {
       AutoSchedulerLock auto_lock(lock_);
 
-      // Increment the number of times that this callback has been invoked.
-      ++num_get_work_callback_;
-      num_get_work_callback_cv_->Signal();
+      // Increment the number of times that this method has been called.
+      ++num_get_work_;
+      num_get_work_cv_->Signal();
 
       // Check if a Sequence should be returned.
       if (num_sequences_to_create_ == 0)
@@ -145,10 +146,9 @@ class TaskSchedulerWorkerThreadTest : public testing::Test {
     return sequence;
   }
 
-  void RanTaskFromSequenceCallback(const SchedulerWorkerThread* worker_thread,
-                                   scoped_refptr<Sequence> sequence) {
-    EXPECT_EQ(worker_thread_.get(), worker_thread);
-
+  // Called after the SchedulerWorkerThread has tried to run a Task from
+  // |sequence| (a TaskTracker might have prevented the Task from running).

[gab, 2016/04/07 16:42:08]

No need to re-state comment from override.

[fdoray, 2016/04/07 17:59:33]

Done.

+  void RanTaskFromSequence(scoped_refptr<Sequence> sequence) override {
     AutoSchedulerLock auto_lock(lock_);
     run_sequences_.push_back(std::move(sequence));
     run_sequences_cv_->Signal();
@@ -164,26 +164,29 @@ class TaskSchedulerWorkerThreadTest : public testing::Test {
   // Synchronizes access to all members below.
   mutable SchedulerLock lock_;
 
-  // Number of times that MainEntryCallback() has been called.
-  size_t num_main_entry_callback_ = 0;
+  // Number of times that OnMainEntry() has been called.
+  size_t num_main_entry_ = 0;
+
+  // Condition variable signaled when |num_main_entry_| is incremented.
+  std::unique_ptr<ConditionVariable> num_main_entry_cv_;
 
-  // Condition variable signaled when |num_main_entry_callback_| is incremented.
-  std::unique_ptr<ConditionVariable> num_main_entry_callback_cv_;
+  // Number of times that OnMainExit() has been called.
+  size_t num_main_exit_ = 0;
 
-  // Number of Sequences that should be created by GetWorkCallback(). When this
-  // is 0, GetWorkCallback() returns nullptr.
+  // Number of Sequences that should be created by GetWork(). When this
+  // is 0, GetWork() returns nullptr.
   size_t num_sequences_to_create_ = 0;
 
-  // Number of times that GetWorkCallback() has been called.
-  size_t num_get_work_callback_ = 0;
+  // Number of times that GetWork() has been called.
+  size_t num_get_work_ = 0;
 
-  // Condition variable signaled when |num_get_work_callback_| is incremented.
-  std::unique_ptr<ConditionVariable> num_get_work_callback_cv_;
+  // Condition variable signaled when |num_get_work_| is incremented.
+  std::unique_ptr<ConditionVariable> num_get_work_cv_;
 
-  // Sequences created by GetWorkCallback().
+  // Sequences created by GetWork().
   std::vector<scoped_refptr<Sequence>> created_sequences_;
 
-  // Sequences passed to RanTaskFromSequenceCallback().
+  // Sequences passed to RanTaskFromSequence().
   std::vector<scoped_refptr<Sequence>> run_sequences_;
 
   // Condition variable signaled when a Sequence is added to |run_sequences_|.
@@ -195,45 +198,44 @@ class TaskSchedulerWorkerThreadTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(TaskSchedulerWorkerThreadTest);
 };
 
-// Verify that when GetWorkCallback() continuously returns Sequences, all Tasks
-// in these Sequences run successfully. The SchedulerWorkerThread is woken up
-// once.
+// Verify that when GetWork() continuously returns Sequences, all Tasks in these
+// Sequences run successfully. The SchedulerWorkerThread is woken up once.
 TEST_F(TaskSchedulerWorkerThreadTest, ContinousWork) {
-  // Set GetWorkCallback() to return |kNumSequencesPerTest| Sequences before
-  // starting to return nullptr.
+  // Set GetWork() to return |kNumSequencesPerTest| Sequences before starting to
+  // return nullptr.
   SetNumSequencesToCreate(kNumSequencesPerTest);
 
   // Wake up |worker_thread_| and wait until it has run all the Tasks returned
-  // by GetWorkCallback().
+  // by GetWork().
   worker_thread_->WakeUp();
   WaitForAllSequencesToRun();
 
-  // Expect |kNumSequencesPerTest| calls to GetWorkCallback() in which it
-  // returned a Sequence and 1 call in which it returned nullptr.
-  const size_t expected_num_get_work_callback = kNumSequencesPerTest + 1;
-  WaitForNumGetWorkCallback(expected_num_get_work_callback);
-  EXPECT_EQ(expected_num_get_work_callback, NumGetWorkCallback());
+  // Expect |kNumSequencesPerTest| calls to GetWork() in which it returned a
+  // Sequence and 1 call in which it returned nullptr.
+  const size_t expected_num_get_work = kNumSequencesPerTest + 1;
+  WaitForNumGetWork(expected_num_get_work);
+  EXPECT_EQ(expected_num_get_work, NumGetWork());
 }
 
-// Verify that when GetWorkCallback() alternates between returning a Sequence
-// and returning nullptr, all Tasks in the returned Sequences run successfully.
-// The SchedulerWorkerThread is woken up once for each Sequence.
+// Verify that when GetWork() alternates between returning a Sequence and
+// returning nullptr, all Tasks in the returned Sequences run successfully. The
+// SchedulerWorkerThread is woken up once for each Sequence.
 TEST_F(TaskSchedulerWorkerThreadTest, IntermittentWork) {
   for (size_t i = 0; i < kNumSequencesPerTest; ++i) {
-    // Set GetWorkCallback() to return 1 Sequence before starting to return
+    // Set GetWork() to return 1 Sequence before starting to return
     // nullptr.
     SetNumSequencesToCreate(1);
 
     // Wake up |worker_thread_| and wait until it has run all the Tasks returned

[gab, 2016/04/07 16:42:08]

s/all the Tasks/the Task/ ?

[fdoray, 2016/04/07 17:59:33]

Done.

-    // by GetWorkCallback().
+    // by GetWork().
     worker_thread_->WakeUp();
     WaitForAllSequencesToRun();
 
-    // Expect |i| calls to GetWorkCallback() in which it returned a Sequence and
+    // Expect |i| calls to GetWork() in which it returned a Sequence and
     // |i| calls in which it returned nullptr.
-    const size_t expected_num_get_work_callback = 2 * (i + 1);
-    WaitForNumGetWorkCallback(expected_num_get_work_callback);
-    EXPECT_EQ(expected_num_get_work_callback, NumGetWorkCallback());
+    const size_t expected_num_get_work = 2 * (i + 1);
+    WaitForNumGetWork(expected_num_get_work);
+    EXPECT_EQ(expected_num_get_work, NumGetWork());
   }
 }