SequenceManager: Clean up TaskQueue Creation

 1. Remove references to TaskQueueImpl in blink scheduler and create
    (Main|NonMain)ThreadTaskQueues directly, rather than routing
    through SequenceManager. This simplifies the construction path,
    fixes layering, and is needed if we move blink TQs oilpan.

 2. Remove MainThreadTaskQueueForTest in blink since it wasn't used.
    This also means task queues are always created with a TaskQueueImpl,
    so require this in the TaskQueue constructor and remove
    NullTaskRunner.

 3. Use plain TaskQueue instead of TestTaskQueue everywhere except
    SequenceManager unit tests. TestTaskQueue only exists to get at
    the TaskQueueImpl and test lifetime (via a weak ptr), so it
    generally isn't needed elsewhere.

 4. Finally, remove SequenceManager::CreateTaskQueueWithType()

Bug: 1143007
Change-Id: If40c62e17f5dda301f0ce729c6a692440608b031
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4336340
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Carlos Caballero Grolimund <carlscab@google.com>
Cr-Commit-Position: refs/heads/main@{#1118767}

base/task/sequence_manager/sequence_manager.h

@@ -28,6 +28,9 @@ class MessagePump;
 class TaskObserver;
 
 namespace sequence_manager {
+namespace internal {
+class TestTaskQueue;
+}  // namespace internal
 
 class TimeDomain;
 
@@ -283,17 +286,6 @@ class BASE_EXPORT SequenceManager {
 
   virtual TaskQueue::QueuePriority GetPriorityCount() const = 0;
 
-  // Creates a task queue with the given type, `spec` and args.
-  // Must be called on the main thread.
-  // TODO(scheduler-dev): SequenceManager should not create TaskQueues.
-  template <typename TaskQueueType, typename... Args>
-  scoped_refptr<TaskQueueType> CreateTaskQueueWithType(
-      const TaskQueue::Spec& spec,
-      Args&&... args) {
-    return WrapRefCounted(new TaskQueueType(CreateTaskQueueImpl(spec), spec,
-                                            std::forward<Args>(args)...));
-  }
-
   // Creates a vanilla TaskQueue rather than a user type derived from it. This
   // should be used if you don't wish to sub class TaskQueue.
   // Must be called on the main thread.
@@ -328,6 +320,8 @@ class BASE_EXPORT SequenceManager {
   virtual void RemoveTaskObserver(TaskObserver* task_observer) = 0;
 
  protected:
+  friend class internal::TestTaskQueue;  // For CreateTaskQueueImpl().
+
   virtual std::unique_ptr<internal::TaskQueueImpl> CreateTaskQueueImpl(
       const TaskQueue::Spec& spec) = 0;
 };

base/task/sequence_manager/sequence_manager_impl_unittest.cc

@@ -37,7 +37,6 @@
 #include "base/task/sequence_manager/test/mock_time_domain.h"
 #include "base/task/sequence_manager/test/mock_time_message_pump.h"
 #include "base/task/sequence_manager/test/sequence_manager_for_test.h"
-#include "base/task/sequence_manager/test/test_task_queue.h"
 #include "base/task/sequence_manager/test/test_task_time_observer.h"
 #include "base/task/sequence_manager/thread_controller_with_message_pump_impl.h"
 #include "base/task/sequence_manager/work_queue.h"
@@ -82,6 +81,22 @@ namespace base {
 namespace sequence_manager {
 namespace internal {
 
+class TestTaskQueue : public TaskQueue {
+ public:
+  TestTaskQueue(SequenceManager& sequence_manager, const TaskQueue::Spec& spec)
+      : TaskQueue(sequence_manager.CreateTaskQueueImpl(spec), spec) {}
+
+  using TaskQueue::GetTaskQueueImpl;
+
+  WeakPtr<TestTaskQueue> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }
+
+ private:
+  ~TestTaskQueue() override = default;  // Ref-counted.
+
+  // Used to ensure that task queue is deleted in tests.
+  WeakPtrFactory<TestTaskQueue> weak_factory_{this};
+};
+
 namespace {
 
 enum class RunnerType {
@@ -398,7 +413,7 @@ class SequenceManagerTest
 
   scoped_refptr<TestTaskQueue> CreateTaskQueue(
       TaskQueue::Spec spec = TaskQueue::Spec(QueueName::TEST_TQ)) {
-    return sequence_manager()->CreateTaskQueueWithType<TestTaskQueue>(spec);
+    return MakeRefCounted<TestTaskQueue>(*sequence_manager(), spec);
   }
 
   std::vector<scoped_refptr<TestTaskQueue>> CreateTaskQueues(

base/task/sequence_manager/sequence_manager_perftest.cc

@@ -15,10 +15,9 @@
 #include "base/run_loop.h"
 #include "base/sequence_checker.h"
 #include "base/synchronization/condition_variable.h"
-#include "base/task/sequence_manager/task_queue_impl.h"
+#include "base/task/sequence_manager/task_queue.h"
 #include "base/task/sequence_manager/test/mock_time_domain.h"
 #include "base/task/sequence_manager/test/sequence_manager_for_test.h"
-#include "base/task/sequence_manager/test/test_task_queue.h"
 #include "base/task/sequence_manager/test/test_task_time_observer.h"
 #include "base/task/sequence_manager/thread_controller_with_message_pump_impl.h"
 #include "base/task/single_thread_task_runner.h"
@@ -111,9 +110,8 @@ class BaseSequenceManagerPerfTestDelegate : public PerfTestDelegate {
   bool MultipleQueuesSupported() const override { return true; }
 
   scoped_refptr<TaskRunner> CreateTaskRunner() override {
-    scoped_refptr<TestTaskQueue> task_queue =
-        manager_->CreateTaskQueueWithType<TestTaskQueue>(
-            TaskQueue::Spec(QueueName::TEST_TQ));
+    scoped_refptr<TaskQueue> task_queue =
+        manager_->CreateTaskQueue(TaskQueue::Spec(QueueName::TEST_TQ));
     owned_task_queues_.push_back(task_queue);
     return task_queue->task_runner();
   }
@@ -143,7 +141,7 @@ class BaseSequenceManagerPerfTestDelegate : public PerfTestDelegate {
   std::unique_ptr<SequenceManager> manager_;
   std::unique_ptr<TimeDomain> time_domain_;
   std::unique_ptr<RunLoop> run_loop_;
-  std::vector<scoped_refptr<TestTaskQueue>> owned_task_queues_;
+  std::vector<scoped_refptr<TaskQueue>> owned_task_queues_;
 };
 
 class SequenceManagerWithMessagePumpPerfTestDelegate
@@ -166,8 +164,7 @@ class SequenceManagerWithMessagePumpPerfTestDelegate
     // ThreadControllerWithMessagePumpImpl doesn't provide a default task
     // runner.
     scoped_refptr<TaskQueue> default_task_queue =
-        GetManager()->template CreateTaskQueueWithType<TestTaskQueue>(
-            TaskQueue::Spec(QueueName::DEFAULT_TQ));
+        GetManager()->CreateTaskQueue(TaskQueue::Spec(QueueName::DEFAULT_TQ));
     GetManager()->SetDefaultTaskRunner(default_task_queue->task_runner());
   }
 

base/task/sequence_manager/task_queue.cc

@@ -20,42 +20,6 @@
 namespace base {
 namespace sequence_manager {
 
-namespace {
-
-class NullTaskRunner final : public SingleThreadTaskRunner {
- public:
-  NullTaskRunner() {}
-
-  bool PostDelayedTask(const Location& location,
-                       OnceClosure callback,
-                       TimeDelta delay) override {
-    return false;
-  }
-
-  bool PostNonNestableDelayedTask(const Location& location,
-                                  OnceClosure callback,
-                                  TimeDelta delay) override {
-    return false;
-  }
-
-  bool RunsTasksInCurrentSequence() const override {
-    return thread_checker_.CalledOnValidThread();
-  }
-
- private:
-  // Ref-counted
-  ~NullTaskRunner() override = default;
-
-  ThreadCheckerImpl thread_checker_;
-};
-
-// TODO(kraynov): Move NullTaskRunner from //base/test to //base.
-scoped_refptr<SingleThreadTaskRunner> CreateNullTaskRunner() {
-  return MakeRefCounted<NullTaskRunner>();
-}
-
-}  // namespace
-
 TaskQueue::QueueEnabledVoter::QueueEnabledVoter(
     scoped_refptr<TaskQueue> task_queue)
     : task_queue_(std::move(task_queue)), enabled_(true) {
@@ -125,13 +89,12 @@ void TaskQueue::OnQueueEnabledVoteChanged(bool enabled) {
 TaskQueue::TaskQueue(std::unique_ptr<internal::TaskQueueImpl> impl,
                      const TaskQueue::Spec& spec)
     : impl_(std::move(impl)),
-      sequence_manager_(impl_ ? impl_->GetSequenceManagerWeakPtr() : nullptr),
-      associated_thread_((impl_ && impl_->sequence_manager())
+      sequence_manager_(impl_->GetSequenceManagerWeakPtr()),
+      associated_thread_((impl_->sequence_manager())
                              ? impl_->sequence_manager()->associated_thread()
                              : MakeRefCounted<internal::AssociatedThreadId>()),
-      default_task_runner_(impl_ ? impl_->CreateTaskRunner(kTaskTypeNone)
-                                 : CreateNullTaskRunner()),
-      name_(impl_ ? impl_->GetProtoName() : QueueName::UNKNOWN_TQ) {}
+      default_task_runner_(impl_->CreateTaskRunner(kTaskTypeNone)),
+      name_(impl_->GetProtoName()) {}
 
 TaskQueue::~TaskQueue() {
   ShutdownTaskQueueGracefully();

base/test/BUILD.gn

@@ -39,8 +39,6 @@ static_library("test_support") {
     "../task/sequence_manager/test/mock_time_message_pump.h",
     "../task/sequence_manager/test/sequence_manager_for_test.cc",
     "../task/sequence_manager/test/sequence_manager_for_test.h",
-    "../task/sequence_manager/test/test_task_queue.cc",
-    "../task/sequence_manager/test/test_task_queue.h",
     "../task/sequence_manager/test/test_task_time_observer.h",
     "../timer/mock_timer.cc",
     "../timer/mock_timer.h",

base/threading/thread_unittest.cc

@@ -564,7 +564,7 @@ class SequenceManagerThreadDelegate : public Thread::Delegate {
   SequenceManagerThreadDelegate()
       : sequence_manager_(
             base::sequence_manager::CreateUnboundSequenceManager()),
-        task_queue_(sequence_manager_->CreateTaskQueueWithType<TaskQueue>(
+        task_queue_(sequence_manager_->CreateTaskQueue(
             TaskQueue::Spec(base::sequence_manager::QueueName::DEFAULT_TQ))) {
     sequence_manager_->SetDefaultTaskRunner(GetDefaultTaskRunner());
   }

codelabs/threading_and_scheduling/04-multiple-threads.cc

@@ -87,11 +87,9 @@ int main() {
 
   // Create a default TaskQueue that feeds into the SequenceManager.
   scoped_refptr<base::sequence_manager::TaskQueue> main_task_queue =
-      sequence_manager
-          ->CreateTaskQueueWithType<base::sequence_manager::TaskQueue>(
-              base::sequence_manager::TaskQueue::Spec(
-                  base::sequence_manager::TaskQueue::Spec(
-                      base::sequence_manager::QueueName::DEFAULT_TQ)));
+      sequence_manager->CreateTaskQueue(base::sequence_manager::TaskQueue::Spec(
+          base::sequence_manager::TaskQueue::Spec(
+              base::sequence_manager::QueueName::DEFAULT_TQ)));
 
   // Get a default TaskRunner for the main (UI) thread.
   scoped_refptr<base::SingleThreadTaskRunner> default_task_runner =

third_party/blink/renderer/platform/scheduler/common/auto_advancing_virtual_time_domain_unittest.cc

@@ -10,7 +10,6 @@
 #include "base/message_loop/message_pump.h"
 #include "base/run_loop.h"
 #include "base/task/sequence_manager/sequence_manager.h"
-#include "base/task/sequence_manager/test/test_task_queue.h"
 #include "base/task/sequence_manager/test/test_task_time_observer.h"
 #include "base/time/time.h"
 #include "testing/gmock/include/gmock/gmock.h"

third_party/blink/renderer/platform/scheduler/main_thread/idle_time_estimator_unittest.cc

@@ -9,8 +9,8 @@
 #include "base/functional/callback.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/task/sequence_manager/sequence_manager.h"
+#include "base/task/sequence_manager/task_queue.h"
 #include "base/task/sequence_manager/test/sequence_manager_for_test.h"
-#include "base/task/sequence_manager/test/test_task_queue.h"
 #include "base/task/sequence_manager/test/test_task_time_observer.h"
 #include "base/task/single_thread_task_runner.h"
 #include "base/test/task_environment.h"
@@ -53,7 +53,8 @@ class IdleTimeEstimatorTest : public testing::Test {
   }
 
   scoped_refptr<MainThreadTaskQueue> NewTaskQueue() {
-    return manager_->CreateTaskQueueWithType<MainThreadTaskQueue>(
+    return base::MakeRefCounted<MainThreadTaskQueue>(
+        *manager_.get(),
         base::sequence_manager::TaskQueue::Spec(
             base::sequence_manager::QueueName::TEST_TQ),
         MainThreadTaskQueue::QueueCreationParams(

third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_helper.cc

@@ -62,10 +62,8 @@ MainThreadSchedulerHelper::DeprecatedDefaultTaskRunner() {
 
 scoped_refptr<MainThreadTaskQueue> MainThreadSchedulerHelper::NewTaskQueue(
     const MainThreadTaskQueue::QueueCreationParams& params) {
-  scoped_refptr<MainThreadTaskQueue> task_queue =
-      sequence_manager_->CreateTaskQueueWithType<MainThreadTaskQueue>(
-          params.spec, params, main_thread_scheduler_);
-  return task_queue;
+  return base::MakeRefCounted<MainThreadTaskQueue>(
+      *sequence_manager_, params.spec, params, main_thread_scheduler_);
 }
 
 void MainThreadSchedulerHelper::ShutdownAllQueues() {

third_party/blink/renderer/platform/scheduler/main_thread/main_thread_task_queue.cc

@@ -112,18 +112,18 @@ bool MainThreadTaskQueue::IsPerFrameTaskQueue(
 }
 
 MainThreadTaskQueue::MainThreadTaskQueue(
-    std::unique_ptr<base::sequence_manager::internal::TaskQueueImpl> impl,
+    base::sequence_manager::SequenceManager& sequence_manager,
     const TaskQueue::Spec& spec,
     const QueueCreationParams& params,
     MainThreadSchedulerImpl* main_thread_scheduler)
-    : queue_type_(params.queue_type),
+    : task_queue_(sequence_manager.CreateTaskQueue(spec)),
+      queue_type_(params.queue_type),
       queue_traits_(params.queue_traits),
       web_scheduling_queue_type_(params.web_scheduling_queue_type),
       web_scheduling_priority_(params.web_scheduling_priority),
       main_thread_scheduler_(main_thread_scheduler),
       agent_group_scheduler_(params.agent_group_scheduler),
       frame_scheduler_(params.frame_scheduler) {
-  task_queue_ = base::MakeRefCounted<TaskQueue>(std::move(impl), spec);
   task_runner_with_default_task_type_ =
       base::FeatureList::IsEnabled(
           features::kUseBlinkSchedulerTaskRunnerWithCustomDeleter)
@@ -141,7 +141,6 @@ MainThreadTaskQueue::MainThreadTaskQueue(
       throttler_.emplace(task_queue_.get(),
                          main_thread_scheduler_->GetTickClock());
     }
-    // TaskQueueImpl may be null for tests.
     // TODO(scheduler-dev): Consider mapping directly to
     // MainThreadSchedulerImpl::OnTaskStarted/Completed. At the moment this
     // is not possible due to task queue being created inside

third_party/blink/renderer/platform/scheduler/main_thread/main_thread_task_queue.h

@@ -383,6 +383,11 @@ class PLATFORM_EXPORT MainThreadTaskQueue
     }
   };
 
+  MainThreadTaskQueue(base::sequence_manager::SequenceManager& sequence_manager,
+                      const TaskQueue::Spec& spec,
+                      const QueueCreationParams& params,
+                      MainThreadSchedulerImpl* main_thread_scheduler);
+
   QueueType queue_type() const { return queue_type_; }
 
   bool CanBeDeferred() const { return queue_traits_.can_be_deferred; }
@@ -528,22 +533,13 @@ class PLATFORM_EXPORT MainThreadTaskQueue
  protected:
   void SetFrameSchedulerForTest(FrameSchedulerImpl* frame_scheduler);
 
-  // TODO(crbug.com/1143007): Remove references to TaskQueueImpl once
-  // TaskQueueImpl inherits from TaskQueue.
-  MainThreadTaskQueue(
-      std::unique_ptr<base::sequence_manager::internal::TaskQueueImpl> impl,
-      const TaskQueue::Spec& spec,
-      const QueueCreationParams& params,
-      MainThreadSchedulerImpl* main_thread_scheduler);
-
   MainThreadTaskQueue(const MainThreadTaskQueue&) = delete;
   MainThreadTaskQueue& operator=(const MainThreadTaskQueue&) = delete;
 
   ~MainThreadTaskQueue();
 
  private:
   friend class base::RefCountedThreadSafe<MainThreadTaskQueue>;
-  friend class base::sequence_manager::SequenceManager;
   friend class blink::scheduler::main_thread_scheduler_impl_unittest::
       MainThreadSchedulerImplTest;