Remove SingleThreadTaskRunner Support from SchedulerWorkerPoolImpl

base/task_scheduler/scheduler_worker_pool_impl_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_pool_impl.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <unordered_set>
@@ skipping 103 matching lines @@
 scoped_refptr<TaskRunner> CreateTaskRunnerWithExecutionMode(
     SchedulerWorkerPoolImpl* worker_pool,
     test::ExecutionMode execution_mode) {
   // Allow tasks posted to the returned TaskRunner to wait on a WaitableEvent.
   const TaskTraits traits = TaskTraits().WithBaseSyncPrimitives();
   switch (execution_mode) {
     case test::ExecutionMode::PARALLEL:
       return worker_pool->CreateTaskRunnerWithTraits(traits);
     case test::ExecutionMode::SEQUENCED:
       return worker_pool->CreateSequencedTaskRunnerWithTraits(traits);
-    case test::ExecutionMode::SINGLE_THREADED:
-      return worker_pool->CreateSingleThreadTaskRunnerWithTraits(traits);
+    default:

[fdoray, 2017/03/14 18:35:47]

If you have no default and you forget to handle an enum value, clang complains.
E.g.:
error: enumeration value 'SEQUENCED' not handled in switch [-Werror,-Wswitch]

I would not add a default to make sure that we are warned at compile time if we
forget to handle a value.

[robliao, 2017/03/14 22:58:28]

Indeed. I would agree if the goal here is to be exhaustive. Now that we're no
longer handling the SINGLE_THREADED execution mode, it's likely we won't handle
others (e.g. the COM execution mode if there is one).

Since it's not clear this should be an exhaustive enumeration anymore, and
having clang complain just because we added an enumeration value seems to be
noisy.

As a general practice, all new code is generally associated with tests, so it's
unlikely we would miss this when adding a new task runner to
SchedulerWorkerPoolImpl.

Happy to add it back if you feel strongly about it.

[gab, 2017/03/16 15:34:45]

In that case we can keep default but we should still NOTREACHED if it's called
with an unhandled case

[robliao, 2017/03/16 22:49:03]

That suggests we should be using NOTREACHED() at the bottom instead of
ADD_FAILURE(). Thoughts?

[gab, 2017/03/20 16:40:58]

Ah, had missed this was in a unittest. ADD_FAILURE() is preferred to
NOTREACHED() then. But I'd prefer to move both the ADD_FAILURE() and return
nullptr to the default: case (I think that will compile and not complain about
missing return statement?)

+      // Fall through.
+      break;
   }
-  ADD_FAILURE() << "Unknown ExecutionMode";
+  ADD_FAILURE() << "Unexpected ExecutionMode";
   return nullptr;
 }
 
 using PostNestedTask = test::TestTaskFactory::PostNestedTask;
 
 class ThreadPostingTasks : public SimpleThread {
  public:
   enum class WaitBeforePostTask {
     NO_WAIT,
     WAIT_FOR_ALL_WORKERS_IDLE,
@@ skipping 228 matching lines @@
           sequenced_task_runner, Unretained(&task_ran)));
   task_ran.Wait();
 }
 
 INSTANTIATE_TEST_CASE_P(Parallel,
                         TaskSchedulerWorkerPoolImplTest,
                         ::testing::Values(test::ExecutionMode::PARALLEL));
 INSTANTIATE_TEST_CASE_P(Sequenced,
                         TaskSchedulerWorkerPoolImplTest,
                         ::testing::Values(test::ExecutionMode::SEQUENCED));
-INSTANTIATE_TEST_CASE_P(
-    SingleThreaded,
-    TaskSchedulerWorkerPoolImplTest,
-    ::testing::Values(test::ExecutionMode::SINGLE_THREADED));
 
 namespace {
 
-// Same as TaskSchedulerWorkerPoolImplTest but its SchedulerWorkerPoolImpl
-// instance uses |max_threads == 1|.
-class TaskSchedulerWorkerPoolImplSingleWorkerTest
-    : public TaskSchedulerWorkerPoolImplTest {
- public:
-  TaskSchedulerWorkerPoolImplSingleWorkerTest() = default;
-
- protected:
-  void SetUp() override {
-    InitializeWorkerPool(TimeDelta::Max(), 1);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TaskSchedulerWorkerPoolImplSingleWorkerTest);
-};
-
-}  // namespace
-
-// Verify that the RunsTasksOnCurrentThread() method of a
-// SchedulerSingleThreadTaskRunner returns false when called from a task that
-// isn't part of its sequence even though it's running on that
-// SchedulerSingleThreadTaskRunner's assigned worker. Note: Tests that use
-// TestTaskFactory already verify that RunsTasksOnCurrentThread() returns true
-// when appropriate so this method complements it to get full coverage of that
-// method.
-TEST_P(TaskSchedulerWorkerPoolImplSingleWorkerTest,
-       SingleThreadRunsTasksOnCurrentThread) {
-  scoped_refptr<TaskRunner> task_runner(
-      CreateTaskRunnerWithExecutionMode(worker_pool_.get(), GetParam()));
-  scoped_refptr<SingleThreadTaskRunner> single_thread_task_runner(
-      worker_pool_->CreateSingleThreadTaskRunnerWithTraits(TaskTraits()));
-
-  WaitableEvent task_ran(WaitableEvent::ResetPolicy::MANUAL,
-                         WaitableEvent::InitialState::NOT_SIGNALED);
-  task_runner->PostTask(
-      FROM_HERE,
-      Bind(
-          [](scoped_refptr<TaskRunner> single_thread_task_runner,
-             WaitableEvent* task_ran) {
-            EXPECT_FALSE(single_thread_task_runner->RunsTasksOnCurrentThread());
-            task_ran->Signal();
-          },
-          single_thread_task_runner, Unretained(&task_ran)));
-  task_ran.Wait();
-}
-
-INSTANTIATE_TEST_CASE_P(Parallel,
-                        TaskSchedulerWorkerPoolImplSingleWorkerTest,
-                        ::testing::Values(test::ExecutionMode::PARALLEL));
-INSTANTIATE_TEST_CASE_P(Sequenced,
-                        TaskSchedulerWorkerPoolImplSingleWorkerTest,
-                        ::testing::Values(test::ExecutionMode::SEQUENCED));
-INSTANTIATE_TEST_CASE_P(
-    SingleThreaded,
-    TaskSchedulerWorkerPoolImplSingleWorkerTest,
-    ::testing::Values(test::ExecutionMode::SINGLE_THREADED));
-
-namespace {
-
-class TaskSchedulerWorkerPoolSingleThreadedTest
-    : public TaskSchedulerWorkerPoolImplTest {
- public:
-  void InitializeThreadChecker() {
-    thread_checker_.reset(new ThreadCheckerImpl());
-  }
-
-  void CheckValidThread() {
-    EXPECT_TRUE(thread_checker_->CalledOnValidThread());
-  }
-
- protected:
-  void SetUp() override {
-    InitializeWorkerPool(kReclaimTimeForDetachTests, kNumWorkersInWorkerPool);
-  }
-
-  TaskSchedulerWorkerPoolSingleThreadedTest() = default;
-
- private:
-  std::unique_ptr<ThreadCheckerImpl> thread_checker_;
-
-  DISALLOW_COPY_AND_ASSIGN(TaskSchedulerWorkerPoolSingleThreadedTest);
-};
-
-}  // namespace
-
-// Verify that thread resources for a single thread remain.
-TEST_F(TaskSchedulerWorkerPoolSingleThreadedTest, SingleThreadTask) {
-  auto single_thread_task_runner =
-      worker_pool_->CreateSingleThreadTaskRunnerWithTraits(
-          TaskTraits().WithShutdownBehavior(
-              TaskShutdownBehavior::BLOCK_SHUTDOWN));
-  single_thread_task_runner->PostTask(
-      FROM_HERE,
-      Bind(&TaskSchedulerWorkerPoolSingleThreadedTest::InitializeThreadChecker,
-           Unretained(this)));
-  WaitableEvent task_waiter(WaitableEvent::ResetPolicy::AUTOMATIC,
-                            WaitableEvent::InitialState::NOT_SIGNALED);
-  single_thread_task_runner->PostTask(
-      FROM_HERE, Bind(&WaitableEvent::Signal, Unretained(&task_waiter)));
-  task_waiter.Wait();
-  worker_pool_->WaitForAllWorkersIdleForTesting();
-
-  // Give the worker pool a chance to reclaim its threads.
-  PlatformThread::Sleep(kReclaimTimeForDetachTests + kExtraTimeToWaitForDetach);
-
-  worker_pool_->DisallowWorkerDetachmentForTesting();
-
-  single_thread_task_runner->PostTask(
-      FROM_HERE,
-      Bind(&TaskSchedulerWorkerPoolSingleThreadedTest::CheckValidThread,
-           Unretained(this)));
-  single_thread_task_runner->PostTask(
-      FROM_HERE, Bind(&WaitableEvent::Signal, Unretained(&task_waiter)));
-  task_waiter.Wait();
-}
-
-namespace {
-
 constexpr size_t kMagicTlsValue = 42;
 
 class TaskSchedulerWorkerPoolCheckTlsReuse
     : public TaskSchedulerWorkerPoolImplTest {
  public:
   void SetTlsValueAndWait() {
     slot_.Set(reinterpret_cast<void*>(kMagicTlsValue));
     waiter_.Wait();
   }
 
@@ skipping 340 matching lines @@
                                 StandbyThreadPolicy::ONE, 8U, TimeDelta::Max()),
       Bind(&NotReachedReEnqueueSequenceCallback), &task_tracker,
       &delayed_task_manager);
   ASSERT_TRUE(worker_pool);
   EXPECT_EQ(1U, worker_pool->NumberOfAliveWorkersForTesting());
   worker_pool->JoinForTesting();
 }
 
 }  // namespace internal
 }  // namespace base