Use TaskScheduler instead of WorkerPool in url_request_simple_job.cc.

Use TaskScheduler instead of WorkerPool in url_request_simple_job.cc.

The following traits are used to post a task to TaskScheduler:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

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).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

Does Not Block (default):
  Tasks without the MayBlock() and WithBaseSyncPrimitives() traits
  may not block.

BUG=659191
Review-Url: https://codereview.chromium.org/2679583004
Cr-Commit-Position: refs/heads/master@{#449977}
Committed: https://chromium.googlesource.com/chromium/src/+/cb664bc9e179ed2ac87ce32a1d862160243be48b

[fdoray, 2017-02-06 21:12:37]

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

[commit-bot: I haz the power, 2017-02-06 21:13:28]

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-06 21:37:41]

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

[commit-bot: I haz the power, 2017-02-06 21:37:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fdoray, 2017-02-07 16:06:00]

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

[commit-bot: I haz the power, 2017-02-07 16:06:20]

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-07 16:34:42]

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

[commit-bot: I haz the power, 2017-02-07 16:34:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fdoray, 2017-02-08 13:42:13]

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

[commit-bot: I haz the power, 2017-02-08 13:42:25]

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-08 15:15:42]

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

[commit-bot: I haz the power, 2017-02-08 15:15:43]

Dry run: This issue passed the CQ dry run.

[fdoray, 2017-02-09 12:52:19]

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

[fdoray, 2017-02-09 12:52:20]

PTAL

[xunjieli, 2017-02-09 16:43:08]

net/ LGTM with nits. You need to find reviewers for other files.

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
net/url_request/url_request_simple_job.cc:74: // Do memory copy asynchronously.
See crbug.com/422489.
Can you retain the comment about the "background thread" ?
Something like: Do a memory copy on a background thread asynchronously.


I have read the design doc. Just to double check,
PostTaskWithTraitsAndReply(...CONTINUE_ON_SHUTDOWN) will post to a thread that
is not the IO thread, right?

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
net/url_request/url_request_simple_job_unittest.cc:135: private:
nit: private after protected

[fdoray, 2017-02-10 16:08:50]

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
net/url_request/url_request_simple_job.cc:74: // Do memory copy asynchronously.
See crbug.com/422489.
On 2017/02/09 16:43:08, xunjieli wrote:
> Can you retain the comment about the "background thread" ?
> Something like: Do a memory copy on a background thread asynchronously.
> 

TaskScheduler has "background" threads on which tasks posted with the BACKGROUND
priority run slowly. Since this task is not posted with the BACKGROUND priority,
I'm reluctant to use the term "background thread".

> 
> I have read the design doc. Just to double check,
> PostTaskWithTraitsAndReply(...CONTINUE_ON_SHUTDOWN) will post to a thread that
> is not the IO thread, right?

Right.

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
net/url_request/url_request_simple_job_unittest.cc:135: private:
On 2017/02/09 16:43:08, xunjieli wrote:
> nit: private after protected

We recommend keeping ScopedTaskScheduler first because it allows other members
to post to it in their constructor and destructor. I'll update the documentation
of ScopedTaskScheduler to make this explicit.

Keeping |scoped_task_scheduler_| private makes it clear that no test uses it
directly.

[xunjieli, 2017-02-10 16:15:07]

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
net/url_request/url_request_simple_job.cc:74: // Do memory copy asynchronously.
See crbug.com/422489.
On 2017/02/10 16:08:50, fdoray wrote:
> On 2017/02/09 16:43:08, xunjieli wrote:
> > Can you retain the comment about the "background thread" ?
> > Something like: Do a memory copy on a background thread asynchronously.
> > 
> 
> TaskScheduler has "background" threads on which tasks posted with the
BACKGROUND
> priority run slowly. Since this task is not posted with the BACKGROUND
priority,
> I'm reluctant to use the term "background thread".
> 
> > 
> > I have read the design doc. Just to double check,
> > PostTaskWithTraitsAndReply(...CONTINUE_ON_SHUTDOWN) will post to a thread
that
> > is not the IO thread, right?
> 
> Right.

Can you add a comment here mentioning that the memcpy is on a different thread?

Something like: "Do memory copy asynchronously on a thread that is not the
network thread."

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
net/url_request/url_request_simple_job_unittest.cc:135: private:
On 2017/02/10 16:08:50, fdoray wrote:
> On 2017/02/09 16:43:08, xunjieli wrote:
> > nit: private after protected
> 
> We recommend keeping ScopedTaskScheduler first because it allows other members
> to post to it in their constructor and destructor. I'll update the
documentation
> of ScopedTaskScheduler to make this explicit.
> 
> Keeping |scoped_task_scheduler_| private makes it clear that no test uses it
> directly.
> 

Acknowledged. Please add a one-line comment here as well so it won't be
accidentally formatted.

[fdoray, 2017-02-10 16:15:27]

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

[commit-bot: I haz the power, 2017-02-10 16:15:51]

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

[fdoray, 2017-02-10 16:17:24]

fdoray@chromium.org changed reviewers:
+ csharrison@chromium.org, sorin@chromium.org

[fdoray, 2017-02-10 16:17:24]

sorin@: Please review changes in components/update_client/

csharrison@: Please review changes in content/browser/loader/

Thanks!

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/2679583004/diff/40001/net/url_request/url_req...
net/url_request/url_request_simple_job_unittest.cc:135: private:
On 2017/02/10 16:08:50, fdoray wrote:
> On 2017/02/09 16:43:08, xunjieli wrote:
> > nit: private after protected
> 
> We recommend keeping ScopedTaskScheduler first because it allows other members
> to post to it in their constructor and destructor. I'll update the
documentation
> of ScopedTaskScheduler to make this explicit.
> 
> Keeping |scoped_task_scheduler_| private makes it clear that no test uses it
> directly.
> 

After reflection, I decided to move it to the end since there is no good reason
to keep it first in this particular test.

[xunjieli, 2017-02-10 16:30:19]

The fact that memcpy needs to be off network thread is very important. Please
put back that part of the comment.

https://codereview.chromium.org/2679583004/diff/60001/net/url_request/url_req...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/2679583004/diff/60001/net/url_request/url_req...
net/url_request/url_request_simple_job.cc:74: // Do memory copy asynchronously.
See crbug.com/422489.
Please edit this comment per previous comment.

"Do memory copy asynchronously on a thread that is not the network thread."

[commit-bot: I haz the power, 2017-02-10 17:30:34]

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

[commit-bot: I haz the power, 2017-02-10 17:30:35]

Dry run: This issue passed the CQ dry run.

[Sorin Jianu, 2017-02-10 23:06:06]

lgtm

update_client lgtm

[Sorin Jianu, 2017-02-10 23:08:58]

On 2017/02/10 23:06:06, Sorin Jianu wrote:
> lgtm
> 
> update_client lgtm

How does the code in update_client work? I see a class member is instantiated
but how is it used in the test? Thank you!

[Charlie Harrison, 2017-02-11 03:26:39]

c/b/l lgtm

[fdoray, 2017-02-13 15:37:51]

> How does the code in update_client work? I see a class member is instantiated
> but how is it used in the test? Thank you!

base::test::ScopedTaskScheduler and base::test::ScopedAsyncTaskScheduler allow
usage of the functions in base/task_scheduler/post_task.h within their scope. A
base::test::ScopedTaskScheduler queues tasks and runs them synchronously when
RunLoop::Run() or RunLoop::RunUntilIdle() is called. A
base::test::ScopedAsyncTaskScheduler runs tasks on a separate thread as they are
posted.

https://codereview.chromium.org/2679583004/diff/60001/net/url_request/url_req...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/2679583004/diff/60001/net/url_request/url_req...
net/url_request/url_request_simple_job.cc:74: // Do memory copy asynchronously.
See crbug.com/422489.
On 2017/02/10 16:30:19, xunjieli wrote:
> Please edit this comment per previous comment.
> 
> "Do memory copy asynchronously on a thread that is not the network thread."

Done.

[fdoray, 2017-02-13 15:38:55]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-02-13 15:38:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org, sorin@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2679583004/#ps80001
(title: "separate thread comment")

[commit-bot: I haz the power, 2017-02-13 15:39:16]

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

[xunjieli, 2017-02-13 16:06:38]

net/ lgtm

[commit-bot: I haz the power, 2017-02-13 16:45:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487000335857810,
"parent_rev": "b83be4ca0e00b50b17619adf7f7de275455e9852", "commit_rev":
"cb664bc9e179ed2ac87ce32a1d862160243be48b"}

[commit-bot: I haz the power, 2017-02-13 16:45:44]

Description was changed from

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

The following traits are used to post a task to TaskScheduler:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

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).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

Does Not Block (default):
  Tasks without the MayBlock() and WithBaseSyncPrimitives() traits
  may not block.

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

to

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

The following traits are used to post a task to TaskScheduler:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

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).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

Does Not Block (default):
  Tasks without the MayBlock() and WithBaseSyncPrimitives() traits
  may not block.

BUG=659191

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

[commit-bot: I haz the power, 2017-02-13 16:45:46]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cb664bc9e179ed2ac87ce32a1d86...