TaskScheduler [11] Support ExecutionMode::SINGLE_THREADED.

base/task_scheduler/delayed_task_manager.h

Index: base/task_scheduler/delayed_task_manager.h
diff --git a/base/task_scheduler/delayed_task_manager.h b/base/task_scheduler/delayed_task_manager.h
index 9113fc561724469dd2ddbf042504938a185cb9e9..fcd7ec31ba6a3e3e5aea3187b1818b00bd738896 100644
--- a/base/task_scheduler/delayed_task_manager.h
+++ b/base/task_scheduler/delayed_task_manager.h
@@ -24,10 +24,10 @@ namespace base {
 namespace internal {
 
 class SchedulerThreadPool;
+class SchedulerWorkerThread;
 
 // A DelayedTaskManager holds delayed Tasks until they become ripe for
-// execution. When they become ripe for execution, it posts them to their
-// associated Sequence and SchedulerThreadPool. This class is thread-safe.
+// execution. This class is thread-safe.
 class BASE_EXPORT DelayedTaskManager {
  public:
   // |on_delayed_run_time_updated| is invoked when the delayed run time is
@@ -36,14 +36,16 @@ class BASE_EXPORT DelayedTaskManager {
   ~DelayedTaskManager();
 
   // Adds |task| to a queue of delayed tasks. The task will be posted to
-  // |thread_pool| as part of |sequence| the first time that PostReadyTasks() is
-  // called while Now() is passed |task->delayed_run_time|.
+  // |thread_pool| with |sequence| and |worker_thread| the first time that
+  // PostReadyTasks() is called while Now() is passed |task->delayed_run_time|.
+  // |worker_thread| is optional.

[gab, 2016/04/25 21:00:20]

Given the above comment says it will be "passed to |thread_pool|" it's weird to
now say it's optional.

How about "|worker_thread| may be nullptr"?

[fdoray, 2016/04/25 22:34:44]

Done.

   //
-  // TODO(robliao): Find a concrete way to manage |thread_pool|'s memory. It is
-  // never deleted in production, but it is better not to spread this assumption
-  // throughout the scheduler.
+  // TODO(robliao): Find a concrete way to manage the memory of |worker_thread|
+  // and |thread_pool|. These objects are never deleted in production, but it is
+  // better not to spread this assumption throughout the scheduler.
   void AddDelayedTask(std::unique_ptr<Task> task,
                       scoped_refptr<Sequence> sequence,
+                      SchedulerWorkerThread* worker_thread,

[gab, 2016/04/25 21:00:20]

I'm not a fan of having a |worker_thread| parameter here but I guess that's the
price to pay for deciding not to use Callbacks to Bind all the required
post-task state in it.

PS: Nothing to do here merely noting my observation...

[robliao, 2016/04/25 21:42:38]

Alternatively we could have two delayed tasks methods.
AddDelayedTask and AddSingleThreadedDelayedTask. I'm actually leaning towards
this since it avoids the nullptr fun.

[fdoray, 2016/04/25 22:34:44]

The 3 solutions are:
1. Keep this as-is.
2. Add a new method AddSingleThreadedDelayedTask.
3. Instead of passing multiple arguments to AddDelayedTask, expose DelayedTask
and pass a DelayedTask.

1. and 3. are good solutions. I'm less excited about 2. because:
- SchedulerThreadPool::PostTaskWithSequence will have to behave differently for
shared/single-threaded Tasks (or we'll need to add
PostSingleThreadedTaskWithSequence).
- We'll need an helper function for the code that is common between
AddDelayedTask/AddSingleThreadedDelayedTask (unless |worker_thread| stays
optional on AddDelayedTask?). An helper function makes it harder to understand
the post task flow.

[gab, 2016/04/26 11:46:27]

Meh, not sure about 3. either as DelayedTask's constructor will still need the
optionally nullptr argument and so will
SchedulerThreadPool::PosttaskWithSequence so we're not gaining much.

As I said in my original comment, not a fan of 1., but I think it's the lesser
evil (and not something likely to haunt us later given this is an internal API
and not a huge readability burden).

[robliao, 2016/04/26 16:58:56]

Exposing DelayedTask's constructor allows you to have a version that does not
take a WorkerThread argument. 

That constructor sets a default of nullptr and then funnels the call to the root
constructor that takes all the args. 

DelayedTask(std::unique_ptr<Task> task, scoped_refptr<Sequence> sequence,
SchedulerThreadPool* thread_pool, uint64_t index)
  : DelayedTask(task, sequence, nullptr, thread_pool, index) {}

[gab, 2016/04/26 20:30:58]

Good point, I'd be okay with that then. Maybe in a follow-up CL to avoid churn
all over this one again?

[fdoray, 2016/04/27 16:09:59]

If we want a single version of SchedulerThreadPoolImpl::PostTaskWithSequence(),
we don't need a constructor that does not take a WorkerThread argument. Knowing
that, do we still want to expose DelayedTask? I'm not a fan of setting |index|
after construction.

[robliao, 2016/04/27 17:43:45]

The thing that I like to avoid is forcing callers to use nullptr's in APIs. It
generally means that there probably should have been a different API surface,
especially if the nullptr version is the most common one.

With regards to setting the index, we're free to set it here as well.

[gab, 2016/04/27 18:11:43]

Either way PostTaskWithSequenceNow() will need to take a potentially null
SWorkerThread* so I prefer that DelayedTaskManager have the same API (otherwise
we have one API taking a pointer and the other one taking a DelayedTask with the
optional field -- which is really a disguised default argument which is banned
by the style guide for virtual functions [1] and thus can't be used for
PostTaskWithSequenceNow()).

[1] https://google.github.io/styleguide/cppguide.html#Default_Arguments

[robliao, 2016/04/27 18:21:03]

Yup, agreed on the default argument points. I'm fine with the nullptr as is for
now. Just replying to the comment.

[fdoray, 2016/04/27 19:17:04]

Keeping this discussion for another CL.

                       SchedulerThreadPool* thread_pool);
 
   // Posts delayed tasks that are ripe for execution.