Separate the create and start phases in SchedulerSingleThreadTaskRunnerManager.

base/task_scheduler/scheduler_worker.h

Index: base/task_scheduler/scheduler_worker.h
diff --git a/base/task_scheduler/scheduler_worker.h b/base/task_scheduler/scheduler_worker.h
index 0fc595d4e48ad5347b9d3ba7baeca653fdd43fd6..e2cd386a8e4b23bf2409d7d6be3f58251cb8a81a 100644
--- a/base/task_scheduler/scheduler_worker.h
+++ b/base/task_scheduler/scheduler_worker.h
@@ -98,27 +98,31 @@ class BASE_EXPORT SchedulerWorker
   enum class InitialState { ALIVE, DETACHED };
 
   // Creates a SchedulerWorker that runs Tasks from Sequences returned by
-  // |delegate|. |priority_hint| is the preferred thread priority; the actual
-  // thread priority depends on shutdown state and platform capabilities.
-  // |task_tracker| is used to handle shutdown behavior of Tasks. If
-  // |worker_state| is DETACHED, the thread will be created upon a WakeUp().
-  // Returns nullptr if creating the underlying platform thread fails during
-  // Create(). |backward_compatibility| indicates whether backward compatibility
-  // is enabled. Either JoinForTesting() or Cleanup() must be called before
+  // |delegate|. No actual thread will be created for this SchedulerWorker
+  // before Start() is called. |priority_hint| is the preferred thread priority;
+  // the actual thread priority depends on shutdown state and platform
+  // capabilities. |task_tracker| is used to handle shutdown behavior of Tasks.
+  // |backward_compatibility| indicates whether backward compatibility is
+  // enabled. Either JoinForTesting() or Cleanup() must be called before
   // releasing the last external reference.
-  static scoped_refptr<SchedulerWorker> Create(
-      ThreadPriority priority_hint,
-      std::unique_ptr<Delegate> delegate,
-      TaskTracker* task_tracker,
-      InitialState initial_state,
-      SchedulerBackwardCompatibility backward_compatibility =
-          SchedulerBackwardCompatibility::DISABLED);
-
-  // Wakes up this SchedulerWorker if it wasn't already awake. After this
-  // is called, this SchedulerWorker will run Tasks from Sequences
-  // returned by the GetWork() method of its delegate until it returns nullptr.
-  // WakeUp() may fail if the worker is detached and it fails to allocate a new
-  // worker. If this happens, there will be no call to GetWork().
+  SchedulerWorker(ThreadPriority priority_hint,
+                  std::unique_ptr<Delegate> delegate,
+                  TaskTracker* task_tracker,
+                  SchedulerBackwardCompatibility backward_compatibility =
+                      SchedulerBackwardCompatibility::DISABLED);
+
+  // Allows this SchedulerWorker to be backed by a thread. If |initial_state| is
+  // ALIVE and Cleanup() wasn't called, a thread is created immediately (in a
+  // wait state pending a WakeUp() call). Otherwise, creation is delayed until
+  // the next WakeUp(). Returns true on success.
+  bool Start(InitialState initial_state = InitialState::ALIVE);

[robliao, 2017/04/10 20:52:48]

Initial state seems better placed at the constructor. Start just allows this
object to start doing work. Callers then don't have to remember the initial
state after they've created the SchedulerWorker.

[fdoray, 2017/04/11 15:27:24]

Did you have in mind a solution that doesn't involve adding a |initial_state_|
member to SchedulerWorker?

[robliao, 2017/04/13 16:42:30]

In this CL, we create the SchedulerWorker and then immediately start it in both
SchedulerWorkerPoolImpl and SchedulerSingleThreadTaskRunnerManager. Why can't
initial_state_ be in the constructor?

[fdoray, 2017/04/13 19:53:15]

No. SchedulerWorkers created by SchedulerSingleThreadTaskRunnerManager before
SchedulerSingleThreadTaskRunnerManager::Start() are started here
https://codereview.chromium.org/2806413002/diff/100001/base/task_scheduler/sc...
(not in the method from which they were instantiated).

+
+  /// Wakes up this SchedulerWorker if it wasn't already awake. After this is

[robliao, 2017/04/10 20:52:48]

Remove extra leading slash.

[fdoray, 2017/04/11 15:27:24]

Done.

+  // called, this SchedulerWorker will run Tasks from Sequences returned by the
+  // GetWork() method of its delegate until it returns nullptr. WakeUp() may
+  // fail if the worker is detached and it fails to allocate a new worker. If
+  // this happens, there will be no call to GetWork(). No-op if Start() wasn't
+  // called.
   void WakeUp();
 
   SchedulerWorker::Delegate* delegate() { return delegate_.get(); }
@@ -155,10 +159,6 @@ class BASE_EXPORT SchedulerWorker
     DELEGATE,
   };
 
-  SchedulerWorker(ThreadPriority thread_priority,
-                  std::unique_ptr<Delegate> delegate,
-                  TaskTracker* task_tracker,
-                  SchedulerBackwardCompatibility backward_compatibility);
   ~SchedulerWorker();
 
   // Returns ownership of the thread instance when appropriate so that it can be
@@ -168,23 +168,22 @@ class BASE_EXPORT SchedulerWorker
       DetachNotify detach_notify);
 
   void CreateThread();
-
-  void CreateThreadAssertSynchronized();
-
   bool ShouldExit();
 
-  // Synchronizes access to |thread_| (read+write) as well as |should_exit_|
-  // (write-only). See Cleanup() for details.
+  // Synchronizes access to |thread_| (read+write), |started_| (read+write) and
+  // |should_exit_| (write-only). See Cleanup() for details.
   mutable SchedulerLock thread_lock_;
 
-  AtomicFlag should_exit_;
-
   // The underlying thread for this SchedulerWorker.
   // The thread object will be cleaned up by the running thread unless we join
   // against the thread. Joining requires the thread object to remain alive for
   // the Thread::Join() call.
   std::unique_ptr<Thread> thread_;
 
+  bool started_ = false;
+
+  AtomicFlag should_exit_;
+
   const ThreadPriority priority_hint_;
 
   const std::unique_ptr<Delegate> delegate_;