TaskScheduler: Remove base::ExecutionMode.

TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as argument,
post_task.h now exposes one task runner creation function per execution
mode. These functions return typed task runners.

  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

BUG=553459
Committed: https://crrev.com/e72adfa10a4c8c04c47b3004f73b8f694d82fcbf
Cr-Commit-Position: refs/heads/master@{#429276}

[fdoray, 2016-10-31 18:15:33]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org, robliao@chromium.org

[fdoray, 2016-10-31 18:15:33]

PTAL

[gab, 2016-10-31 18:52:55]

Add to CL description something like:

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

[gab, 2016-10-31 19:10:11]

lgtm % comments

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/del...
File base/task_scheduler/delayed_task_manager_unittest.cc (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/del...
base/task_scheduler/delayed_task_manager_unittest.cc:13: #include
"base/single_thread_task_runner.h"
Was previously missing task_runner.h

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/pos...
File base/task_scheduler/post_task.h (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/pos...
base/task_scheduler/post_task.h:106: // across tasks using thread-local storage
or you rely on a thread-affine API.
The last sentence is slightly too strict given what we've learnt from net/
folks. How about:

"Only use this if you rely on a thread-affine API (it might be safer to assume
thread-affinity when dealing with under-documented third-party APIs, e.g. other
OS') or share data across tasks using thread-local storage."

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (left):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:20: #include
"base/single_thread_task_runner.h"
(but here you'll want to keep them since you're using it beyond just the
overrides -- tricky details of IWYU ;-))

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:358: // threads that haven't
run tasks for a long time.
The second half of this comment is wrong with detach now (i.e. it's still fine
in practice but "alive" has been redefined)

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.h (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.h:21: #include
"base/single_thread_task_runner.h"
Do need to add includes solely used by overrides.

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl.h (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl.h:19: #include
"base/single_thread_task_runner.h"
rm

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:47: const char* execution_mode_;
const char* const

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:249: const char* execution_mode =
const char* const

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tes...
File base/task_scheduler/test_utils.h (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tes...
base/task_scheduler/test_utils.h:12: // Type of TaskRunner to which a task was
posted.
Change for something like:

// An enumeration of possible task_scheduler TaskRunner types. Used to
parametrize relevant task_scheduler tests.

[robliao, 2016-10-31 19:19:59]

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.cc (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.cc:50: // CoInitialized environment to
request one (via an upcoming trait).
I thought we were keeping COM as an execution mode since they effectively make
the task runner single-threaded. Are we no longer doing this?

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (left):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:20: #include
"base/single_thread_task_runner.h"
On 2016/10/31 19:10:11, gab wrote:
> (but here you'll want to keep them since you're using it beyond just the
> overrides -- tricky details of IWYU ;-))

Not sure I follow here. The style guide permits use of headers from the main
header.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
However, any includes present in the related header do not need to be included
again in the related cc (i.e., foo.cc can rely on foo.h's includes).

[gab, 2016-10-31 19:28:45]

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (left):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:20: #include
"base/single_thread_task_runner.h"
On 2016/10/31 19:19:59, robliao wrote:
> On 2016/10/31 19:10:11, gab wrote:
> > (but here you'll want to keep them since you're using it beyond just the
> > overrides -- tricky details of IWYU ;-))
> 
> Not sure I follow here. The style guide permits use of headers from the main
> header.
> 
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
> However, any includes present in the related header do not need to be included
> again in the related cc (i.e., foo.cc can rely on foo.h's includes).

Right but my other comment asks to remove from header per only being used in the
overrides (which can be assumed to be included by parent class).

[robliao, 2016-10-31 19:31:48]

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (left):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:20: #include
"base/single_thread_task_runner.h"
On 2016/10/31 19:28:45, gab wrote:
> On 2016/10/31 19:19:59, robliao wrote:
> > On 2016/10/31 19:10:11, gab wrote:
> > > (but here you'll want to keep them since you're using it beyond just the
> > > overrides -- tricky details of IWYU ;-))
> > 
> > Not sure I follow here. The style guide permits use of headers from the main
> > header.
> > 
> >
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
> > However, any includes present in the related header do not need to be
included
> > again in the related cc (i.e., foo.cc can rely on foo.h's includes).
> 
> Right but my other comment asks to remove from header per only being used in
the
> overrides (which can be assumed to be included by parent class).

Gotcha. That other comment makes a lot more sense now. +1 to that.

[fdoray, 2016-10-31 19:41:38]

Description was changed from

==========
TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as
argument, post_task.h now exposes one dedicated function per execution
mode:
  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

The main benefit of this change is that the returned TaskRunners
are typed according to their execution mode.

BUG=553459
==========

to

==========
TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as argument,
post_task.h now exposes one dedicated function per execution mode:
  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

BUG=553459
==========

[fdoray, 2016-10-31 19:44:52]

robliao@: PTAL

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/del...
File base/task_scheduler/delayed_task_manager_unittest.cc (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/del...
base/task_scheduler/delayed_task_manager_unittest.cc:13: #include
"base/single_thread_task_runner.h"
On 2016/10/31 19:10:11, gab wrote:
> Was previously missing task_runner.h

Done.

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/pos...
File base/task_scheduler/post_task.h (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/pos...
base/task_scheduler/post_task.h:106: // across tasks using thread-local storage
or you rely on a thread-affine API.
On 2016/10/31 19:10:11, gab wrote:
> The last sentence is slightly too strict given what we've learnt from net/
> folks. How about:
> 
> "Only use this if you rely on a thread-affine API (it might be safer to assume
> thread-affinity when dealing with under-documented third-party APIs, e.g.
other
> OS') or share data across tasks using thread-local storage."

Done.

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.cc (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.cc:50: // CoInitialized environment to
request one (via an upcoming trait).
On 2016/10/31 19:19:59, robliao wrote:
> I thought we were keeping COM as an execution mode since they effectively make
> the task runner single-threaded. Are we no longer doing this?

From Gab in "Typed TaskRunners and child TaskRunners" discussion: "As for
SEQUENCED+COM, I guess it will always be single-threaded for now in the
background but that doesn't break the API contract so we don't need to ban it."

If someone knows that their tasks don't have to run on the same thread (e.g.
they make independent COM calls), I think we should let them choose the
appropriate execution mode. They don't have to know that we'll run all COM tasks
posted to the same pool on the same thread.

Does that make sense?

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (left):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:20: #include
"base/single_thread_task_runner.h"
On 2016/10/31 19:31:48, robliao wrote:
> On 2016/10/31 19:28:45, gab wrote:
> > On 2016/10/31 19:19:59, robliao wrote:
> > > On 2016/10/31 19:10:11, gab wrote:
> > > > (but here you'll want to keep them since you're using it beyond just the
> > > > overrides -- tricky details of IWYU ;-))
> > > 
> > > Not sure I follow here. The style guide permits use of headers from the
main
> > > header.
> > > 
> > >
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
> > > However, any includes present in the related header do not need to be
> included
> > > again in the related cc (i.e., foo.cc can rely on foo.h's includes).
> > 
> > Right but my other comment asks to remove from header per only being used in
> the
> > overrides (which can be assumed to be included by parent class).
> 
> Gotcha. That other comment makes a lot more sense now. +1 to that.

Done.

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:358: // threads that haven't
run tasks for a long time.
On 2016/10/31 19:10:11, gab wrote:
> The second half of this comment is wrong with detach now (i.e. it's still fine
> in practice but "alive" has been redefined)

Removed second part of the comment since I don't believe that we'll take the
detach state into account. We'll decide what the best algorithm is when we
address this TODO.

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.h (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.h:21: #include
"base/single_thread_task_runner.h"
On 2016/10/31 19:10:11, gab wrote:
> Do need to add includes solely used by overrides.

Done.

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_scheduler_impl.h (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_scheduler_impl.h:19: #include
"base/single_thread_task_runner.h"
On 2016/10/31 19:10:11, gab wrote:
> rm

Done.

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:47: const char* execution_mode_;
On 2016/10/31 19:10:11, gab wrote:
> const char* const

Done.

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:249: const char* execution_mode =
On 2016/10/31 19:10:11, gab wrote:
> const char* const

Done.

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tes...
File base/task_scheduler/test_utils.h (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/tes...
base/task_scheduler/test_utils.h:12: // Type of TaskRunner to which a task was
posted.
On 2016/10/31 19:10:11, gab wrote:
> Change for something like:
> 
> // An enumeration of possible task_scheduler TaskRunner types. Used to
> parametrize relevant task_scheduler tests.

Done.

[robliao, 2016-10-31 19:50:46]

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.cc (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.cc:50: // CoInitialized environment to
request one (via an upcoming trait).
On 2016/10/31 19:44:51, fdoray wrote:
> On 2016/10/31 19:19:59, robliao wrote:
> > I thought we were keeping COM as an execution mode since they effectively
make
> > the task runner single-threaded. Are we no longer doing this?
> 
> From Gab in "Typed TaskRunners and child TaskRunners" discussion: "As for
> SEQUENCED+COM, I guess it will always be single-threaded for now in the
> background but that doesn't break the API contract so we don't need to ban
it."
> 
> If someone knows that their tasks don't have to run on the same thread (e.g.
> they make independent COM calls), I think we should let them choose the
> appropriate execution mode. They don't have to know that we'll run all COM
tasks
> posted to the same pool on the same thread.
> 
> Does that make sense?

In the Sequenced+COM case, it would be unexpected for tasks to post to one COM
apartment and then later on post to another COM apartment. That effectively
makes the task runner single threaded.

The only signal we have from the caller on whether or not it's safe tear down
the apartment is to have the TaskRunner go away.

[fdoray, 2016-10-31 20:42:28]

robliao@: PTAnL

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.cc (right):

https://codereview.chromium.org/2464963002/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.cc:50: // CoInitialized environment to
request one (via an upcoming trait).
On 2016/10/31 19:50:46, robliao wrote:
> On 2016/10/31 19:44:51, fdoray wrote:
> > On 2016/10/31 19:19:59, robliao wrote:
> > > I thought we were keeping COM as an execution mode since they effectively
> make
> > > the task runner single-threaded. Are we no longer doing this?
> > 
> > From Gab in "Typed TaskRunners and child TaskRunners" discussion: "As for
> > SEQUENCED+COM, I guess it will always be single-threaded for now in the
> > background but that doesn't break the API contract so we don't need to ban
> it."
> > 
> > If someone knows that their tasks don't have to run on the same thread (e.g.
> > they make independent COM calls), I think we should let them choose the
> > appropriate execution mode. They don't have to know that we'll run all COM
> tasks
> > posted to the same pool on the same thread.
> > 
> > Does that make sense?
> 
> In the Sequenced+COM case, it would be unexpected for tasks to post to one COM
> apartment and then later on post to another COM apartment. That effectively
> makes the task runner single threaded.
> 
> The only signal we have from the caller on whether or not it's safe tear down
> the apartment is to have the TaskRunner go away.

Changed "upcoming trait" to "upcoming execution mode" since this is outside the
scope of this CL.

We should discuss COM tomorrow. Do one-off COM tasks exist? If so, I think it
makes sense to support base::PostTask(..., TaskTraits().WithCOM()).

[robliao, 2016-11-01 00:33:21]

lgtm! Thanks. One of COM calls do exist as well as COM calls that could span
multiple tasks.

[fdoray, 2016-11-01 12:19:20]

Description was changed from

==========
TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as argument,
post_task.h now exposes one dedicated function per execution mode:
  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

BUG=553459
==========

to

==========
TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as argument,
post_task.h now exposes one dedicated function per execution mode:

  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

BUG=553459
==========

[fdoray, 2016-11-01 12:20:54]

Description was changed from

==========
TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as argument,
post_task.h now exposes one dedicated function per execution mode:

  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

BUG=553459
==========

to

==========
TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as argument,
post_task.h now exposes one task runner creation function per execution
mode. These functions return typed task runners.

  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

BUG=553459
==========

[fdoray, 2016-11-01 12:21:10]

fdoray@chromium.org changed reviewers:
+ danakj@chromium.org

[fdoray, 2016-11-01 12:21:10]

danakj@: PTAL

[danakj, 2016-11-01 20:32:11]

LGTM I like

https://codereview.chromium.org/2464963002/diff/60001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/2464963002/diff/60001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:28: constexpr char kParallelExecutionMode[]
= "parallel";
style guide says to prefer just const when possible (like here)

http://chromium-cpp.appspot.com/#core-whitelist

https://codereview.chromium.org/2464963002/diff/60001/base/task_scheduler/tes...
File base/task_scheduler/test_utils.h (right):

https://codereview.chromium.org/2464963002/diff/60001/base/task_scheduler/tes...
base/task_scheduler/test_utils.h:12: // An enumeration of possible
task_scheduler TaskRunner types. Used to
nit: just "task scheduler" (no underscore) maybe?

[fdoray, 2016-11-01 20:45:44]

https://codereview.chromium.org/2464963002/diff/60001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/2464963002/diff/60001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:28: constexpr char kParallelExecutionMode[]
= "parallel";
On 2016/11/01 20:32:11, danakj wrote:
> style guide says to prefer just const when possible (like here)
> 
> http://chromium-cpp.appspot.com/#core-whitelist

? Style guide says "Prefer **to** const for variables where possible."

https://codereview.chromium.org/2464963002/diff/60001/base/task_scheduler/tes...
File base/task_scheduler/test_utils.h (right):

https://codereview.chromium.org/2464963002/diff/60001/base/task_scheduler/tes...
base/task_scheduler/test_utils.h:12: // An enumeration of possible
task_scheduler TaskRunner types. Used to
On 2016/11/01 20:32:11, danakj wrote:
> nit: just "task scheduler" (no underscore) maybe?

Done.

[fdoray, 2016-11-01 20:45:53]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-11-01 20:45:53]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
robliao@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2464963002/#ps80001
(title: "CR danakj #17")

[commit-bot: I haz the power, 2016-11-01 20:46:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-11-01 21:53:32]

https://codereview.chromium.org/2464963002/diff/60001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/2464963002/diff/60001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:28: constexpr char kParallelExecutionMode[]
= "parallel";
On 2016/11/01 20:45:44, fdoray wrote:
> On 2016/11/01 20:32:11, danakj wrote:
> > style guide says to prefer just const when possible (like here)
> > 
> > http://chromium-cpp.appspot.com/#core-whitelist
> 
> ? Style guide says "Prefer **to** const for variables where possible."

lul i misread that the other day cool.

[commit-bot: I haz the power, 2016-11-01 22:47:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 22:47:53]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fdoray, 2016-11-02 12:34:47]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-11-02 12:35:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2016-11-02 12:35:03]

The CQ bit was unchecked by fdoray@chromium.org

[fdoray, 2016-11-02 12:36:04]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-11-02 12:36:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 14:35:49]

Description was changed from

==========
TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as argument,
post_task.h now exposes one task runner creation function per execution
mode. These functions return typed task runners.

  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

BUG=553459
==========

to

==========
TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as argument,
post_task.h now exposes one task runner creation function per execution
mode. These functions return typed task runners.

  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

BUG=553459
==========

[commit-bot: I haz the power, 2016-11-02 14:35:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-02 14:38:13]

Description was changed from

==========
TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as argument,
post_task.h now exposes one task runner creation function per execution
mode. These functions return typed task runners.

  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

BUG=553459
==========

to

==========
TaskScheduler: Remove base::ExecutionMode.

Instead of a single function that takes a base::ExecutionMode as argument,
post_task.h now exposes one task runner creation function per execution
mode. These functions return typed task runners.

  scoped_refptr<TaskRunner> CreateTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SequencedTaskRunner>
      CreateSequencedTaskRunnerWithTraits(TaskTraits);
  scoped_refptr<SingleThreadTaskRunner>
      CreateSingleThreadTaskRunnerWithTraits(TaskTraits);

We had originally explicitly opted against providing typed TaskRunners per them
lacking all other traits and thus merely being weakly typed (a compile time name
for every combination of trait would be ridiculous). It is however now becoming
obvious that the ExecutionMode is by far the most important type to spread
through the codebase as having a compile time guarantee that something is
single-threaded or sequenced is a pretty big deal and such usage is spread
throughout the codebase already. Other traits (e.g. file I/O allowed) are also
important but more easily enforced (e.g. AssertIOAllowed()) and we are thus fine
keeping the compile-time enforcement to the execution mode only.

This, in general, doesn't change the desire to have components get their own
TaskRunner instead of having it passed in in the TaskScheduler world.

BUG=553459

Committed: https://crrev.com/e72adfa10a4c8c04c47b3004f73b8f694d82fcbf
Cr-Commit-Position: refs/heads/master@{#429276}
==========

[commit-bot: I haz the power, 2016-11-02 14:38:14]

Patchset 5 (id:??) landed as
https://crrev.com/e72adfa10a4c8c04c47b3004f73b8f694d82fcbf
Cr-Commit-Position: refs/heads/master@{#429276}