Use TaskScheduler instead of WorkerPool in platform_keys.cc.

Use TaskScheduler instead of WorkerPool in platform_keys.cc.

This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits(). The following traits are used:

Priority: BACKGROUND
  User won't notice if this task takes an arbitrarily long time
  to complete.

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

No File IO (default):
  The task does not perform synchronous IO operations.

No Wait (default):
  The task does not wait on things other than synchronous file IO
  operations (e.g. WaitableEvent, ConditionVariable, join a thread,
  join a process, blocking system call that doesn't involve
  interactions with the file system).

BUG=659191
Committed: https://crrev.com/7d18421543cc20d9d37a5709e4c9d08eaa5e8e62
Cr-Commit-Position: refs/heads/master@{#435057}

[fdoray, 2016-11-24 19:48:48]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 19:48:59]

Dry run: 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-24 20:00:07]

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

[commit-bot: I haz the power, 2016-11-24 20:00:08]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2016-11-25 13:21:15]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-25 13:21:24]

Dry run: 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-25 13:48:10]

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

[commit-bot: I haz the power, 2016-11-25 13:48:11]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-11-25 15:44:02]

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

[fdoray, 2016-11-25 15:44:03]

PTAL

[fdoray, 2016-11-25 16:39:45]

Description was changed from

==========
Use TaskScheduler instead of WorkerPool in platform_keys.cc.

This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits() call. The following traits are used:

Priority: BACKGROUND
  User won't notice if this task takes an arbitrarily long time
  to complete.

Shutdown behavior: SKIP_ON_SHUTDOWN (default)
  Tasks posted with this mode that have not started executing at
  shutdown will never run. However, any task that has already begun
  executing when shutdown is invoked will be allowed to continue and
  will block shutdown until completion.

No File IO (default):
  The task does not perform synchronous IO operations.
  It spends most of its time using the CPU.

BUG=659191
==========

to

==========
Use TaskScheduler instead of WorkerPool in platform_keys.cc.

This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits(). The following traits are used:

Priority: BACKGROUND
  User won't notice if this task takes an arbitrarily long time
  to complete.

Shutdown behavior: SKIP_ON_SHUTDOWN (default)
  Tasks posted with this mode that have not started executing at
  shutdown will never run. However, any task that has already begun
  executing when shutdown is invoked will be allowed to continue and
  will block shutdown until completion.

No File IO (default):
  The task does not perform synchronous IO operations.
  It spends most of its time using the CPU.

BUG=659191
==========

[gab, 2016-11-28 16:30:35]

lgtm though OWNERS should confirm, maybe this should be
TaskPriority::USER_VISIBLE

[gab, 2016-11-28 16:37:39]

On 2016/11/28 16:30:35, gab wrote:
> lgtm though OWNERS should confirm, maybe this should be
> TaskPriority::USER_VISIBLE

Actually all WorkerPool threads had CONTINUE_ON_SHUTDOWN semantics before,
should keep this behavior.

[fdoray, 2016-11-28 22:04:29]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 22:06:10]

Dry run: 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-29 00:16:25]

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

[commit-bot: I haz the power, 2016-11-29 00:16:27]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-11-29 16:25:28]

Description was changed from

==========
Use TaskScheduler instead of WorkerPool in platform_keys.cc.

This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits(). The following traits are used:

Priority: BACKGROUND
  User won't notice if this task takes an arbitrarily long time
  to complete.

Shutdown behavior: SKIP_ON_SHUTDOWN (default)
  Tasks posted with this mode that have not started executing at
  shutdown will never run. However, any task that has already begun
  executing when shutdown is invoked will be allowed to continue and
  will block shutdown until completion.

No File IO (default):
  The task does not perform synchronous IO operations.
  It spends most of its time using the CPU.

BUG=659191
==========

to

==========
Use TaskScheduler instead of WorkerPool in platform_keys.cc.

This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits(). The following traits are used:

Priority: BACKGROUND
  User won't notice if this task takes an arbitrarily long time
  to complete.

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

No File IO (default):
  The task does not perform synchronous IO operations.

No Wait (default):
  The task does not wait on things other than synchronous file IO
  operations (e.g. WaitableEvent, ConditionVariable, join a thread,
  join a process, blocking system call that doesn't involve
  interactions with the file system).

BUG=659191
==========

[fdoray, 2016-11-29 16:32:14]

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

[fdoray, 2016-11-29 16:32:14]

Please review this CL. You should verify that the traits mentioned in the CL
description make sense for this task. Thanks!

Why are you doing this? WorkerPool is being deprecated in favor of
TaskScheduler. Read the design doc
https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

Is a BACKGROUND priority correct? See all priorities in
base/task_scheduler/task_traits.h. Tell us if this task should be USER_VISIBLE
or USER_BLOCKING.

Is a CONTINUE_ON_SHUTDOWN shutdown behavior correct? It should, because it's how
the task was handled by base::WorkerPool before this CL.

Is no file IO / no wait correct? Yes if the task is always ready to run on the
CPU (except page faults and locks). Tell us if that's not the case.

[emaxx, 2016-11-29 17:25:04]

LGTM assuming that you agree with my reasoning (please see below).


On 2016/11/29 16:32:14, fdoray wrote:
> Is a BACKGROUND priority correct? See all priorities in
> base/task_scheduler/task_traits.h. Tell us if this task should be USER_VISIBLE
> or USER_BLOCKING.

This code is used by the chrome.[enterprise.]platformKeys extensions API
implementation, and is triggered by the API calls performed by extensions.
So BACKGROUND seems to be the correct priority.


> Is a CONTINUE_ON_SHUTDOWN shutdown behavior correct? It should, because it's
how
> the task was handled by base::WorkerPool before this CL.

This seems to be fine, as the job uses only net::X509Certificate methods, which
themselves use the NSS library methods (AFAICS they only globals that they use
are
PODs).


> Is no file IO / no wait correct? Yes if the task is always ready to run on the
> CPU (except page faults and locks). Tell us if that's not the case.

AFAICS, the involved methods of the net::X509Certificate class and functions of
the NSS library perform neither IO nor waiting on synchronous operations.

[fdoray, 2016-11-29 18:25:49]

On 2016/11/29 17:25:04, emaxx wrote:
> LGTM assuming that you agree with my reasoning (please see below).
> 
> 
> On 2016/11/29 16:32:14, fdoray wrote:
> > Is a BACKGROUND priority correct? See all priorities in
> > base/task_scheduler/task_traits.h. Tell us if this task should be
USER_VISIBLE
> > or USER_BLOCKING.
> 
> This code is used by the chrome.[enterprise.]platformKeys extensions API
> implementation, and is triggered by the API calls performed by extensions.
> So BACKGROUND seems to be the correct priority.
> 
> 
> > Is a CONTINUE_ON_SHUTDOWN shutdown behavior correct? It should, because it's
> how
> > the task was handled by base::WorkerPool before this CL.
> 
> This seems to be fine, as the job uses only net::X509Certificate methods,
which
> themselves use the NSS library methods (AFAICS they only globals that they use
> are
> PODs).
> 
> 
> > Is no file IO / no wait correct? Yes if the task is always ready to run on
the
> > CPU (except page faults and locks). Tell us if that's not the case.
> 
> AFAICS, the involved methods of the net::X509Certificate class and functions
of
> the NSS library perform neither IO nor waiting on synchronous operations.

Thanks for the write-up. I added a comment about the TaskPriority in code.

[fdoray, 2016-11-29 18:29:09]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-11-29 18:29:09]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2534513002/#ps60001
(title: "add comment about priority")

[commit-bot: I haz the power, 2016-11-29 18:29:30]

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-29 19:49:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480444149435030,
"parent_rev": "1f957c4c86149d441e70bdf0a6c069601c83e23c", "commit_rev":
"78c47355b98f3ab8dfafa5e65b363409c643dc3e"}

[commit-bot: I haz the power, 2016-11-29 19:49:26]

Description was changed from

==========
Use TaskScheduler instead of WorkerPool in platform_keys.cc.

This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits(). The following traits are used:

Priority: BACKGROUND
  User won't notice if this task takes an arbitrarily long time
  to complete.

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

No File IO (default):
  The task does not perform synchronous IO operations.

No Wait (default):
  The task does not wait on things other than synchronous file IO
  operations (e.g. WaitableEvent, ConditionVariable, join a thread,
  join a process, blocking system call that doesn't involve
  interactions with the file system).

BUG=659191
==========

to

==========
Use TaskScheduler instead of WorkerPool in platform_keys.cc.

This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits(). The following traits are used:

Priority: BACKGROUND
  User won't notice if this task takes an arbitrarily long time
  to complete.

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

No File IO (default):
  The task does not perform synchronous IO operations.

No Wait (default):
  The task does not wait on things other than synchronous file IO
  operations (e.g. WaitableEvent, ConditionVariable, join a thread,
  join a process, blocking system call that doesn't involve
  interactions with the file system).

BUG=659191
==========

[commit-bot: I haz the power, 2016-11-29 19:49:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-29 19:53:31]

Description was changed from

==========
Use TaskScheduler instead of WorkerPool in platform_keys.cc.

This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits(). The following traits are used:

Priority: BACKGROUND
  User won't notice if this task takes an arbitrarily long time
  to complete.

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

No File IO (default):
  The task does not perform synchronous IO operations.

No Wait (default):
  The task does not wait on things other than synchronous file IO
  operations (e.g. WaitableEvent, ConditionVariable, join a thread,
  join a process, blocking system call that doesn't involve
  interactions with the file system).

BUG=659191
==========

to

==========
Use TaskScheduler instead of WorkerPool in platform_keys.cc.

This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits(). The following traits are used:

Priority: BACKGROUND
  User won't notice if this task takes an arbitrarily long time
  to complete.

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

No File IO (default):
  The task does not perform synchronous IO operations.

No Wait (default):
  The task does not wait on things other than synchronous file IO
  operations (e.g. WaitableEvent, ConditionVariable, join a thread,
  join a process, blocking system call that doesn't involve
  interactions with the file system).

BUG=659191

Committed: https://crrev.com/7d18421543cc20d9d37a5709e4c9d08eaa5e8e62
Cr-Commit-Position: refs/heads/master@{#435057}
==========

[commit-bot: I haz the power, 2016-11-29 19:53:32]

Patchset 4 (id:??) landed as
https://crrev.com/7d18421543cc20d9d37a5709e4c9d08eaa5e8e62
Cr-Commit-Position: refs/heads/master@{#435057}