Separate the create and start phases in TaskSchedulerImpl.

base/task_scheduler/task_scheduler.h

 // 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.
 
 #ifndef BASE_TASK_SCHEDULER_TASK_SCHEDULER_H_
 #define BASE_TASK_SCHEDULER_TASK_SCHEDULER_H_
 
 #include <memory>
 #include <vector>
 
@@ skipping 15 matching lines @@
 
 namespace tracked_objects {
 class Location;
 }
 
 namespace base {
 
 class HistogramBase;
 
 // Interface for a task scheduler and static methods to manage the instance used
-// by the post_task.h API. Note: all base/task_scheduler users should go through
-// post_task.h instead of TaskScheduler except for the one callsite per process
-// which manages the process' instance.
+// by the post_task.h API.
+//
+// The task scheduler doesn't create threads before Start() is called. Tasks can
+// be posted before Start() is called, but they don't run until Start() is
+// called.
+//
+// The non-static methods of this class are thread-safe.

[robliao, 2017/04/24 22:32:19]

Nit: non-static -> instance

[fdoray, 2017/04/25 18:43:34]

Done.

+//
+// Note: all base/task_scheduler users should go through post_task.h instead of

[robliao, 2017/04/24 22:32:19]

Nit: all -> All

[gab, 2017/04/25 15:16:15]

I don't think this is required (and IMO capital letter here looks weird):
http://data.grammarbook.com/blog/commas/capitalization-with-colons/

"Capitalization rule with sentences after colons: If only one sentence follows
the colon, it is often not necessary to capitalize the first word of the new
sentence. If two or more sentences follow the colon, capitalize the first word
of each sentence following."

[robliao, 2017/04/25 17:37:28]

http://apvschicago.com/2011/04/capitalization-after-colons.html

Since the structure here is Note: [Complete Sentence and more], the sentence
should be capitalized.

[fdoray, 2017/04/25 18:43:34]

Done.

+// TaskScheduler except for the one callsite per process which manages the
+// process' instance.

[robliao, 2017/04/24 22:32:19]

Nit: process's

[gab, 2017/04/25 15:16:15]

Don't think this is required either (I sure always omit after 's' and have
committed many comments as such):
http://www.grammarbook.com/punctuation/apostro.asp

[robliao, 2017/04/25 17:37:27]

In this case, because process is singular, process's is the way to go. 
See
https://english.stackexchange.com/questions/1073/what-is-the-correct-possessi...

[fdoray, 2017/04/25 18:43:34]

Done.

 class BASE_EXPORT TaskScheduler {
  public:
   struct BASE_EXPORT InitParams {
     InitParams(
         const SchedulerWorkerPoolParams& background_worker_pool_params_in,
         const SchedulerWorkerPoolParams&
             background_blocking_worker_pool_params_in,
         const SchedulerWorkerPoolParams& foreground_worker_pool_params_in,
         const SchedulerWorkerPoolParams&
             foreground_blocking_worker_pool_params_in);
     ~InitParams();
 
     const SchedulerWorkerPoolParams background_worker_pool_params;
     const SchedulerWorkerPoolParams background_blocking_worker_pool_params;
     const SchedulerWorkerPoolParams foreground_worker_pool_params;
     const SchedulerWorkerPoolParams foreground_blocking_worker_pool_params;
   };
 
   // Destroying a TaskScheduler is not allowed in production; it is always
   // leaked. In tests, it should only be destroyed after JoinForTesting() has
   // returned.
   virtual ~TaskScheduler() = default;
 
+  // Allows the task scheduler to create threads and run tasks following the
+  // |init_params| specification. CHECKs on failure.
+  virtual void Start(const InitParams& init_params) = 0;
+
   // Posts |task| with a |delay| and specific |traits|. |delay| can be zero.
   // For one off tasks that don't require a TaskRunner.
   virtual void PostDelayedTaskWithTraits(
       const tracked_objects::Location& from_here,
       const TaskTraits& traits,
       OnceClosure task,
       TimeDelta delay) = 0;
 
   // Returns a TaskRunner whose PostTask invocations result in scheduling tasks
   // using |traits|. Tasks may run in any order and in parallel.
@@ skipping 65 matching lines @@
   // label threads and histograms. It should identify the component that calls
   // this. CHECKs on failure. For tests, prefer base::test::ScopedTaskScheduler
   // (ensures isolation).
   static void CreateAndSetSimpleTaskScheduler(StringPiece name);
 #endif  // !defined(OS_NACL)
 
   // Creates and sets a task scheduler using custom params. |name| is used to
   // label threads and histograms. It should identify the component that creates
   // the TaskScheduler. |init_params| is used to initialize the worker pools.
   // CHECKs on failure. For tests, prefer base::test::ScopedTaskScheduler
   // (ensures isolation).

[gab, 2017/04/25 15:20:01]

Actually, this is the API that needs to be updated right? How will we enforce
that existing callers properly start? Will we instead introduce another call
that implies you explicitly want a non-started version (still started by default
to avoid affecting existing callers)?

PS: I think this voids some of my previous comments, I somehow thought some
callers were doing SetInstance with a TaskSchedulerImpl.

[gab, 2017/04/25 15:21:11]

Nvm, found answer @ https://codereview.chromium.org/2836033002 :)

This also voids my comment on task_scheduler_impl

[fdoray, 2017/04/25 18:43:34]

Yes.

   static void CreateAndSetDefaultTaskScheduler(StringPiece name,
                                                const InitParams& init_params);
 
   // Registers |task_scheduler| to handle tasks posted through the post_task.h
   // API for this process. For tests, prefer base::test::ScopedTaskScheduler
   // (ensures isolation).
   static void SetInstance(std::unique_ptr<TaskScheduler> task_scheduler);
 
   // Retrieve the TaskScheduler set via SetInstance() or
   // CreateAndSet(Simple|Default)TaskScheduler(). This should be used very
@@ skipping 19 matching lines @@
   // each process n/GetMaxConcurrentTasksWithTraitsDeprecated() items.
   //
   // TODO(fdoray): Remove this method. https://crbug.com/687264
   virtual int GetMaxConcurrentTasksWithTraitsDeprecated(
       const TaskTraits& traits) const = 0;
 };
 
 }  // namespace base
 
 #endif  // BASE_TASK_SCHEDULER_TASK_SCHEDULER_H_