Use TaskScheduler instead of blocking pool in chrome_views_delegate.cc.

Use TaskScheduler instead of blocking pool in chrome_views_delegate.cc.

The blocking pool is being deprecated in favor of TaskScheduler.

BUG=667892
R=pkasting@chromium.org

Review-Url: https://codereview.chromium.org/2670353003
Cr-Commit-Position: refs/heads/master@{#451604}
Committed: https://chromium.googlesource.com/chromium/src/+/b44a323dde8ccb75455736ede5986e191b890eb2

[fdoray, 2017-02-03 17:49:08]

Hi! I'm a Python script responsible for migrating tasks from the
blocking pool to TaskScheduler. In this CL, I used these traits to post to
TaskScheduler a task that was previously posted to the blocking pool:

.WithPriority(base::TaskPriority::BACKGROUND)
  User won't notice if the task takes an arbitrarily long time to complete.
  Making your task BACKGROUND allows non-BACKGROUND tasks to run faster :)

*No* .WithShutdownBehavior()

.MayBlock()
  The task may block. This includes but is not limited to tasks that wait on
  synchronous file I/O operations: read or write a file from disk, interact
  with a pipe or a socket, rename or delete a file, enumerate files in a
  directory, etc. This trait isn't required for the mere use of locks.

*No* .WithBaseSyncPrimitives()
  The task is not allowed to use these functions:
  - base::WaitableEvent::Wait
  - base::ConditionVariable::Wait
  - base::PlatformThread::Join
  - base::PlatformThread::Sleep
  - base::Process::WaitForExit
  - base::Process::WaitForExitWithTimeout

If you think this is correct, please LGTM and CQ this CL.

Otherwise, tell me which traits I should have used and I'll automatically
upload a new patch. You can find documentation at
https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h

WithPriority()
[default/BACKGROUND/USER_VISIBLE/USER_BLOCKING]:

WithShutdownBehavior()
[default/CONTINUE_ON_SHUTDOWN/SKIP_ON_SHUTDOWN/BLOCK_SHUTDOWN]:

MayBlock()
[yes/no]:

WithBaseSyncPrimitives()
[yes/no]:

Comments for a human:
[A human will read this]

FAQ:

What should I do if the CQ dry run didn't pass?
You can ignore this CL. A human will take a look and get back to you.

Why are you doing this?
Browser threads, the blocking pool and base::WorkerPool are being deprecated
in favor of TaskScheduler. Design doc:
https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

What is the default priority?
A task posted to TaskScheduler without an explicit priority inherits its
priority from the calling context (e.g. a task posted to TaskScheduler
from a BACKGROUND task without an explicit .WithPriority() will have a
BACKGROUND priority).

What is the default shutdown behavior?
It is not documented on purpose. If your task shouldn't be skipped on shutdown
(e.g. a task that persists user data on disk), it should have an explicit
BLOCK_SHUTDOWN shutdown behavior. If it's important not to wait on your task
on shutdown (e.g. it takes a long time to run and could cause a shutdown
hang), it should have an explicit CONTINUE_ON_SHUTDOWN shutdown behavior.

[fdoray, 2017-02-03 17:49:09]

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

[commit-bot: I haz the power, 2017-02-03 17:49:30]

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, 2017-02-03 18:35:12]

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

[commit-bot: I haz the power, 2017-02-03 18:35:13]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-02-11 04:16:15]

On 2017/02/03 17:49:08, fdoray wrote:
> WithPriority()
> [default/BACKGROUND/USER_VISIBLE/USER_BLOCKING]:

USER_BLOCKING

> Comments for a human:

It's unclear whether this can actually block.

However, it's critically important that it not be run on the main thread, to
avoid reentrancy from nested message loops.  What we really want is a way to
indicate "must be on different thread" as opposed to "blocks", but the task
scheduler design doc says the notion of threads is explicitly excluded from the
API.  In this case, that's a problem :/

[fdoray, 2017-02-17 20:47:24]

PTAnL

On 2017/02/11 04:16:15, Peter Kasting wrote:
> On 2017/02/03 17:49:08, fdoray wrote:
> > WithPriority()
> > [default/BACKGROUND/USER_VISIBLE/USER_BLOCKING]:
> 
> USER_BLOCKING

Done

> 
> > Comments for a human:
> 
> It's unclear whether this can actually block.
> 
> However, it's critically important that it not be run on the main thread, to
> avoid reentrancy from nested message loops.  What we really want is a way to
> indicate "must be on different thread" as opposed to "blocks", but the task
> scheduler design doc says the notion of threads is explicitly excluded from
the
> API.  In this case, that's a problem :/

I updated the documentation to clarify that TaskScheduler only runs tasks on
threads that it owns (i.e. not the UI thread):
https://cs.chromium.org/chromium/src/base/task_scheduler/post_task.h?l=61

In the existing world, it's not always clear why a task is posted to the "FILE"
thread: is it because it cannot run at the same time as other FILE thread tasks
or because someone thought that a FILE thread was appropriate for any file
operation? That's why we want people to think about TaskTraits and Sequences
rather than Threads. But you're still allowed to know that your tasks don't run
on non-TaskScheduler threads :)

https://codereview.chromium.org/2670353003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2670353003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.cc:510: //
https://crbug.com/662122
Currently, all TaskScheduler threads are CoInitialized. We plan to limit this to
a few threads and to introduce a WithCOM() trait for tasks that need to be
scheduled on a CoInitialized thread.

[fdoray, 2017-02-17 20:47:31]

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

[commit-bot: I haz the power, 2017-02-17 20:48:10]

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

[Peter Kasting, 2017-02-17 21:51:43]

LGTM

[commit-bot: I haz the power, 2017-02-18 02:09:04]

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

[commit-bot: I haz the power, 2017-02-18 02:09:05]

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

[fdoray, 2017-02-20 11:57:16]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-02-20 11:57: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, 2017-02-20 12:44:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487591836241420,
"parent_rev": "ad0db3749c27fd3c427f32d4c75d782540b40fba", "commit_rev":
"b44a323dde8ccb75455736ede5986e191b890eb2"}

[commit-bot: I haz the power, 2017-02-20 12:45:31]

Description was changed from

==========
Use TaskScheduler instead of blocking pool in chrome_views_delegate.cc.

The blocking pool is being deprecated in favor of TaskScheduler.

BUG=667892
R=pkasting@chromium.org
==========

to

==========
Use TaskScheduler instead of blocking pool in chrome_views_delegate.cc.

The blocking pool is being deprecated in favor of TaskScheduler.

BUG=667892
R=pkasting@chromium.org

Review-Url: https://codereview.chromium.org/2670353003
Cr-Commit-Position: refs/heads/master@{#451604}
Committed:
https://chromium.googlesource.com/chromium/src/+/b44a323dde8ccb75455736ede598...
==========

[commit-bot: I haz the power, 2017-02-20 12:45:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b44a323dde8ccb75455736ede598...