Remove SingleThreadTaskRunner Support from SchedulerWorkerPoolImpl

base/task_scheduler/scheduler_worker_pool_impl_unittest.cc

Index: base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
diff --git a/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc b/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
index 53b7e982ab03a8bfa346438174f74feba91973fe..35581bb58c8d073ca0b4aaa20d7dd39535711ec7 100644
--- a/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
+++ b/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
@@ -121,10 +121,11 @@ scoped_refptr<TaskRunner> CreateTaskRunnerWithExecutionMode(
       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;
 }
 
@@ -373,128 +374,6 @@ INSTANTIATE_TEST_CASE_P(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 {