Instantiate ScopedTaskEnvironment in net unittests.

Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests.

ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and
base/task_scheduler/post_task.h within its scope. It should be
instantiated in everytest that uses either of these APIs
(i.e. no test should instantiate a MessageLoop directly).

Switching from MessageLoopForIO to ScopedTaskEnvironment in
NetTestSuite allows Scoped(Async)TaskScheduler to be removed
from individual net unittests.

Motivation for ScopedTaskEnvironment can be found in:
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3Pkk/edit

BUG=708584
Review-Url: https://codereview.chromium.org/2839663002
Cr-Commit-Position: refs/heads/master@{#471073}
Committed: https://chromium.googlesource.com/chromium/src/+/f33fede528342549ee59c5cc2b8f4f966a3edec8

[fdoray, 2017-04-24 16:11:55]

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

[commit-bot: I haz the power, 2017-04-24 16:12:42]

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-04-24 16:38:04]

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

[commit-bot: I haz the power, 2017-04-24 16:38:05]

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

[fdoray, 2017-04-24 17:42:51]

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

[commit-bot: I haz the power, 2017-04-24 17:43:31]

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-04-24 18:19:48]

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

[commit-bot: I haz the power, 2017-04-24 18:19:49]

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

[fdoray, 2017-05-03 12:38:40]

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

[commit-bot: I haz the power, 2017-05-03 12:38:54]

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-05-03 13:11:49]

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

[commit-bot: I haz the power, 2017-05-03 13:11:50]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[fdoray, 2017-05-03 18:49:50]

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

[commit-bot: I haz the power, 2017-05-03 18:50:43]

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-05-03 19:01:17]

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

[commit-bot: I haz the power, 2017-05-03 19:01:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[fdoray, 2017-05-08 16:41:05]

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

[commit-bot: I haz the power, 2017-05-08 16:41:52]

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-05-08 16:53:07]

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

[commit-bot: I haz the power, 2017-05-08 16:53:07]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[fdoray, 2017-05-08 17:28:08]

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

[commit-bot: I haz the power, 2017-05-08 17:28: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-05-08 17:29:02]

Description was changed from

==========
Instantiate ScopedTaskEnvironment in net unittests.

BUG=
==========

to

==========
Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests.

ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and
base/task_scheduler/post_task.h within its scope. It should be
instantiated in everytest that uses either of these APIs
(i.e. no test should instantiate a MessageLoop directly).

Motivation for ScopedTaskEnvironment can be found in:
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

BUG=708584
==========

[commit-bot: I haz the power, 2017-05-08 17:41:04]

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

[commit-bot: I haz the power, 2017-05-08 17:41:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[fdoray, 2017-05-08 17:44:50]

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

[commit-bot: I haz the power, 2017-05-08 17:45:23]

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-05-08 18:23:19]

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

[commit-bot: I haz the power, 2017-05-08 18:23:20]

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

[fdoray, 2017-05-08 19:36:13]

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

[commit-bot: I haz the power, 2017-05-08 19:37: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-05-08 20:12:39]

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

[commit-bot: I haz the power, 2017-05-08 20:12:40]

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

[fdoray, 2017-05-08 20:23:51]

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

[commit-bot: I haz the power, 2017-05-08 20:25:16]

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-05-08 20:30:23]

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

[commit-bot: I haz the power, 2017-05-08 20:30:23]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[fdoray, 2017-05-09 14:45:16]

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

[fdoray, 2017-05-09 14:45:17]

Please take a look. Thanks.

[fdoray, 2017-05-09 15:05:32]

Description was changed from

==========
Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests.

ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and
base/task_scheduler/post_task.h within its scope. It should be
instantiated in everytest that uses either of these APIs
(i.e. no test should instantiate a MessageLoop directly).

Motivation for ScopedTaskEnvironment can be found in:
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

BUG=708584
==========

to

==========
Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests.

ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and
base/task_scheduler/post_task.h within its scope. It should be
instantiated in everytest that uses either of these APIs
(i.e. no test should instantiate a MessageLoop directly).

Switching from MessageLoopForIO to ScopedTaskEnvironment in
NetTestSuite allows Scoped(Async)TaskScheduler to be removed
from individual net unittests.

Motivation for ScopedTaskEnvironment can be found in:
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

BUG=708584
==========

[mmenke, 2017-05-09 15:40:46]

https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16792:
base::TaskScheduler::GetInstance()->FlushForTesting();
Why are both this and RunUntilIdle needed?

https://codereview.chromium.org/2839663002/diff/120001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/120001/net/ssl/channel_id_ser...
net/ssl/channel_id_service_unittest.cc:330: base::RunLoop().RunUntilIdle();
This means something different than the old code, right?  This fails if there's
a task that posts a task that does what we're waiting for?

https://codereview.chromium.org/2839663002/diff/120001/net/url_request/url_fe...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/120001/net/url_request/url_fe...
net/url_request/url_fetcher_impl_unittest.cc:537:
base::TaskScheduler::GetInstance()->FlushForTesting();
In most of the other tests you do a flush + RunUntilIdle together..Why aren't
you doing that here?

[fdoray, 2017-05-09 18:53:08]

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

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

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-05-09 19:08:05]

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

[commit-bot: I haz the power, 2017-05-09 19:08:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[fdoray, 2017-05-09 19:19:06]

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

[commit-bot: I haz the power, 2017-05-09 19:19:44]

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-05-09 20:29:43]

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

[commit-bot: I haz the power, 2017-05-09 20:29:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fdoray, 2017-05-09 21:35:12]

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

[commit-bot: I haz the power, 2017-05-09 21:36:16]

Dry run: CQ is trying da patch.

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

[fdoray, 2017-05-09 21:43:47]

Please take another look

https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16792:
base::TaskScheduler::GetInstance()->FlushForTesting();
On 2017/05/09 15:40:44, mmenke wrote:
> Why are both this and RunUntilIdle needed?

n/a with latest patch set

https://codereview.chromium.org/2839663002/diff/120001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/120001/net/ssl/channel_id_ser...
net/ssl/channel_id_service_unittest.cc:330: base::RunLoop().RunUntilIdle();
On 2017/05/09 15:40:44, mmenke wrote:
> This means something different than the old code, right?  This fails if
there's
> a task that posts a task that does what we're waiting for?

Addressed by calling ScopedTaskEnvironment::RunUntilIdle() instead.

https://codereview.chromium.org/2839663002/diff/120001/net/url_request/url_fe...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/120001/net/url_request/url_fe...
net/url_request/url_fetcher_impl_unittest.cc:537:
base::TaskScheduler::GetInstance()->FlushForTesting();
On 2017/05/09 15:40:44, mmenke wrote:
> In most of the other tests you do a flush + RunUntilIdle together..Why aren't
> you doing that here?

Because the RunLoop::RunUntilIdle() was not required to make the test pass.
Avoided confusion by calling
NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() instead.

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
net/url_request/url_fetcher_impl_unittest.cc:133:
task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
TaskScheduler tasks run on dedicated thread when they are posted within the
scope of a ScopedTaskEnvironment while they run on the main thread when posted
within the scope of a ScopedTaskScheduler. Switching from ScopedTaskEnvironment
to ScopedTaskScheduler means that OnURLFetchComplete is not called on the main
thread and a post task to |task_runner_| is required to quit the run loop on the
main thread.

[commit-bot: I haz the power, 2017-05-09 23:07:57]

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

[commit-bot: I haz the power, 2017-05-09 23:07:59]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mmenke, 2017-05-10 15:43:44]

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
net/url_request/url_fetcher_impl_unittest.cc:133:
task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
On 2017/05/09 21:43:47, fdoray wrote:
> TaskScheduler tasks run on dedicated thread when they are posted within the
> scope of a ScopedTaskEnvironment while they run on the main thread when posted
> within the scope of a ScopedTaskScheduler. Switching from
ScopedTaskEnvironment
> to ScopedTaskScheduler means that OnURLFetchComplete is not called on the main
> thread and a post task to |task_runner_| is required to quit the run loop on
the
> main thread.

I'm not really following, nor do I understand why that affects this test and no
others.  I'll try to work through it later today, but not sure I'll get to it.
Have a number of reviews on my plate today.

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
net/url_request/url_request_simple_job_unittest.cc:224:
NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle();
Why do only some RunUntilIdle calls need to be switched?  Just skimming over net
unittests, there are still a ton of RunUntilIdle calls.  Are we changing the
behavior of all of them?

[fdoray, 2017-05-10 16:16:12]

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
net/url_request/url_fetcher_impl_unittest.cc:133:
task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
On 2017/05/10 15:43:44, mmenke wrote:
> On 2017/05/09 21:43:47, fdoray wrote:
> > TaskScheduler tasks run on dedicated thread when they are posted within the
> > scope of a ScopedTaskEnvironment while they run on the main thread when
posted
> > within the scope of a ScopedTaskScheduler. Switching from
> ScopedTaskEnvironment
> > to ScopedTaskScheduler means that OnURLFetchComplete is not called on the
main
> > thread and a post task to |task_runner_| is required to quit the run loop on
> the
> > main thread.
> 
> I'm not really following, nor do I understand why that affects this test and
no
> others.  I'll try to work through it later today, but not sure I'll get to it.
> Have a number of reviews on my plate today.

- run_loop_.Quit() and run_loop_.Run() must be called on the same thread.
- This test calls run_loop_.Run() on the main thread.
- OnURLFetchComplete() is called as part of a TaskScheduler task.
- Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler
tasks are scheduled on the main thread. Therefore, before this CL,
OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was valid
to call run_loop_.Quit() inline.
- Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler
tasks are scheduled on a dedicated thread. Therefore, it is no longer valid to
call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the main
thread.

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
net/url_request/url_request_simple_job_unittest.cc:224:
NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle();
On 2017/05/10 15:43:44, mmenke wrote:
> Why do only some RunUntilIdle calls need to be switched?  Just skimming over
net
> unittests, there are still a ton of RunUntilIdle calls.  Are we changing the
> behavior of all of them?

This CL removes ScopedTaskScheduler from individual net/ tests and adds a global
ScopedTaskEnvironment.

Within the scope of a ScopedTaskScheduler (deprecated):

  RunLoop().RunUntilIdle()
    Runs TaskScheduler tasks: Yes
    Runs main MessageLoop tasks: Yes
  NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle()
    Runs TaskScheduler tasks: Yes
    Runs main MessageLoop tasks: Yes

Within the scope of a ScopedTaskEnvironment:

  RunLoop().RunUntilIdle()
    Runs TaskScheduler tasks: *NO*
    Runs main MessageLoop tasks: Yes
  NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle()
    Runs TaskScheduler tasks: *YES*
    Runs main MessageLoop tasks: Yes

Replacing RunLoop().RunUntilIdle() with
NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() is not necessary if we
don't expect to run TaskScheduler tasks.

[mmenke, 2017-05-10 16:24:44]

On 2017/05/10 16:16:12, fdoray wrote:
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> File net/url_request/url_fetcher_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> net/url_request/url_fetcher_impl_unittest.cc:133:
> task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
> On 2017/05/10 15:43:44, mmenke wrote:
> > On 2017/05/09 21:43:47, fdoray wrote:
> > > TaskScheduler tasks run on dedicated thread when they are posted within
the
> > > scope of a ScopedTaskEnvironment while they run on the main thread when
> posted
> > > within the scope of a ScopedTaskScheduler. Switching from
> > ScopedTaskEnvironment
> > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on the
> main
> > > thread and a post task to |task_runner_| is required to quit the run loop
on
> > the
> > > main thread.
> > 
> > I'm not really following, nor do I understand why that affects this test and
> no
> > others.  I'll try to work through it later today, but not sure I'll get to
it.
> > Have a number of reviews on my plate today.
> 
> - run_loop_.Quit() and run_loop_.Run() must be called on the same thread.
> - This test calls run_loop_.Run() on the main thread.
> - OnURLFetchComplete() is called as part of a TaskScheduler task.
> - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler
> tasks are scheduled on the main thread. Therefore, before this CL,
> OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was
valid
> to call run_loop_.Quit() inline.
> - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler
> tasks are scheduled on a dedicated thread. Therefore, it is no longer valid to
> call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the
main
> thread.
> 
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
> File net/url_request/url_request_simple_job_unittest.cc (right):
> 
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
> net/url_request/url_request_simple_job_unittest.cc:224:
> NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle();
> On 2017/05/10 15:43:44, mmenke wrote:
> > Why do only some RunUntilIdle calls need to be switched?  Just skimming over
> net
> > unittests, there are still a ton of RunUntilIdle calls.  Are we changing the
> > behavior of all of them?
> 
> This CL removes ScopedTaskScheduler from individual net/ tests and adds a
global
> ScopedTaskEnvironment.
> 
> Within the scope of a ScopedTaskScheduler (deprecated):
> 
>   RunLoop().RunUntilIdle()
>     Runs TaskScheduler tasks: Yes
>     Runs main MessageLoop tasks: Yes
>   NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle()
>     Runs TaskScheduler tasks: Yes
>     Runs main MessageLoop tasks: Yes
> 
> Within the scope of a ScopedTaskEnvironment:
> 
>   RunLoop().RunUntilIdle()
>     Runs TaskScheduler tasks: *NO*
>     Runs main MessageLoop tasks: Yes
>   NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle()
>     Runs TaskScheduler tasks: *YES*
>     Runs main MessageLoop tasks: Yes
> 
> Replacing RunLoop().RunUntilIdle() with
> NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() is not necessary if
we
> don't expect to run TaskScheduler tasks.

I'm not really seeing the difference here.  No net/ production code references
TaskScheduler.  So why do some net tests need to run TaskScheduler tests, and
others are fine with just MessageLoop ones?

[mmenke, 2017-05-10 16:25:25]

On 2017/05/10 16:24:44, mmenke wrote:
> On 2017/05/10 16:16:12, fdoray wrote:
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> > File net/url_request/url_fetcher_impl_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> > net/url_request/url_fetcher_impl_unittest.cc:133:
> > task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
> > On 2017/05/10 15:43:44, mmenke wrote:
> > > On 2017/05/09 21:43:47, fdoray wrote:
> > > > TaskScheduler tasks run on dedicated thread when they are posted within
> the
> > > > scope of a ScopedTaskEnvironment while they run on the main thread when
> > posted
> > > > within the scope of a ScopedTaskScheduler. Switching from
> > > ScopedTaskEnvironment
> > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on
the
> > main
> > > > thread and a post task to |task_runner_| is required to quit the run
loop
> on
> > > the
> > > > main thread.
> > > 
> > > I'm not really following, nor do I understand why that affects this test
and
> > no
> > > others.  I'll try to work through it later today, but not sure I'll get to
> it.
> > > Have a number of reviews on my plate today.
> > 
> > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread.
> > - This test calls run_loop_.Run() on the main thread.
> > - OnURLFetchComplete() is called as part of a TaskScheduler task.
> > - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler
> > tasks are scheduled on the main thread. Therefore, before this CL,
> > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was
> valid
> > to call run_loop_.Quit() inline.
> > - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler
> > tasks are scheduled on a dedicated thread. Therefore, it is no longer valid
to
> > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the
> main
> > thread.
> > 
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
> > File net/url_request/url_request_simple_job_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
> > net/url_request/url_request_simple_job_unittest.cc:224:
> > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle();
> > On 2017/05/10 15:43:44, mmenke wrote:
> > > Why do only some RunUntilIdle calls need to be switched?  Just skimming
over
> > net
> > > unittests, there are still a ton of RunUntilIdle calls.  Are we changing
the
> > > behavior of all of them?
> > 
> > This CL removes ScopedTaskScheduler from individual net/ tests and adds a
> global
> > ScopedTaskEnvironment.
> > 
> > Within the scope of a ScopedTaskScheduler (deprecated):
> > 
> >   RunLoop().RunUntilIdle()
> >     Runs TaskScheduler tasks: Yes
> >     Runs main MessageLoop tasks: Yes
> >   NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle()
> >     Runs TaskScheduler tasks: Yes
> >     Runs main MessageLoop tasks: Yes
> > 
> > Within the scope of a ScopedTaskEnvironment:
> > 
> >   RunLoop().RunUntilIdle()
> >     Runs TaskScheduler tasks: *NO*
> >     Runs main MessageLoop tasks: Yes
> >   NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle()
> >     Runs TaskScheduler tasks: *YES*
> >     Runs main MessageLoop tasks: Yes
> > 
> > Replacing RunLoop().RunUntilIdle() with
> > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() is not necessary if
> we
> > don't expect to run TaskScheduler tasks.
> 
> I'm not really seeing the difference here.  No net/ production code references
> TaskScheduler.  So why do some net tests need to run TaskScheduler tests, and
> others are fine with just MessageLoop ones?

*Run TaskScheduler tasks, rather

[fdoray, 2017-05-10 16:49:14]

On 2017/05/10 16:25:25, mmenke wrote:
> On 2017/05/10 16:24:44, mmenke wrote:
> > On 2017/05/10 16:16:12, fdoray wrote:
> > >
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> > > File net/url_request/url_fetcher_impl_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> > > net/url_request/url_fetcher_impl_unittest.cc:133:
> > > task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
> > > On 2017/05/10 15:43:44, mmenke wrote:
> > > > On 2017/05/09 21:43:47, fdoray wrote:
> > > > > TaskScheduler tasks run on dedicated thread when they are posted
within
> > the
> > > > > scope of a ScopedTaskEnvironment while they run on the main thread
when
> > > posted
> > > > > within the scope of a ScopedTaskScheduler. Switching from
> > > > ScopedTaskEnvironment
> > > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on
> the
> > > main
> > > > > thread and a post task to |task_runner_| is required to quit the run
> loop
> > on
> > > > the
> > > > > main thread.
> > > > 
> > > > I'm not really following, nor do I understand why that affects this test
> and
> > > no
> > > > others.  I'll try to work through it later today, but not sure I'll get
to
> > it.
> > > > Have a number of reviews on my plate today.
> > > 
> > > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread.
> > > - This test calls run_loop_.Run() on the main thread.
> > > - OnURLFetchComplete() is called as part of a TaskScheduler task.
> > > - Within the scope of a ScopedTaskScheduler (before this CL),
TaskScheduler
> > > tasks are scheduled on the main thread. Therefore, before this CL,
> > > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was
> > valid
> > > to call run_loop_.Quit() inline.
> > > - Within the scope of a ScopedTaskEnvironment (with this CL),
TaskScheduler
> > > tasks are scheduled on a dedicated thread. Therefore, it is no longer
valid
> to
> > > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the
> > main
> > > thread.
> > > 
> > >
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
> > > File net/url_request/url_request_simple_job_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
> > > net/url_request/url_request_simple_job_unittest.cc:224:
> > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle();
> > > On 2017/05/10 15:43:44, mmenke wrote:
> > > > Why do only some RunUntilIdle calls need to be switched?  Just skimming
> over
> > > net
> > > > unittests, there are still a ton of RunUntilIdle calls.  Are we changing
> the
> > > > behavior of all of them?
> > > 
> > > This CL removes ScopedTaskScheduler from individual net/ tests and adds a
> > global
> > > ScopedTaskEnvironment.
> > > 
> > > Within the scope of a ScopedTaskScheduler (deprecated):
> > > 
> > >   RunLoop().RunUntilIdle()
> > >     Runs TaskScheduler tasks: Yes
> > >     Runs main MessageLoop tasks: Yes
> > >   NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle()
> > >     Runs TaskScheduler tasks: Yes
> > >     Runs main MessageLoop tasks: Yes
> > > 
> > > Within the scope of a ScopedTaskEnvironment:
> > > 
> > >   RunLoop().RunUntilIdle()
> > >     Runs TaskScheduler tasks: *NO*
> > >     Runs main MessageLoop tasks: Yes
> > >   NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle()
> > >     Runs TaskScheduler tasks: *YES*
> > >     Runs main MessageLoop tasks: Yes
> > > 
> > > Replacing RunLoop().RunUntilIdle() with
> > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() is not necessary
if
> > we
> > > don't expect to run TaskScheduler tasks.
> > 
> > I'm not really seeing the difference here.  No net/ production code
references
> > TaskScheduler.  So why do some net tests need to run TaskScheduler tests,
and
> > others are fine with just MessageLoop ones?
> 
> *Run TaskScheduler tasks, rather

Yes, net/ production code references TaskScheduler
https://cs.chromium.org/search/?q=task_scheduler/post_task.h+file:%5Esrc/net/...
Every call to NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() in this
CL actually runs a TaskScheduler task that is required for a test to pass.

[mmenke, 2017-05-10 17:08:30]

On 2017/05/10 16:49:14, fdoray wrote:
> On 2017/05/10 16:25:25, mmenke wrote:
> > On 2017/05/10 16:24:44, mmenke wrote:
> > > On 2017/05/10 16:16:12, fdoray wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> > > > File net/url_request/url_fetcher_impl_unittest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> > > > net/url_request/url_fetcher_impl_unittest.cc:133:
> > > > task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
> > > > On 2017/05/10 15:43:44, mmenke wrote:
> > > > > On 2017/05/09 21:43:47, fdoray wrote:
> > > > > > TaskScheduler tasks run on dedicated thread when they are posted
> within
> > > the
> > > > > > scope of a ScopedTaskEnvironment while they run on the main thread
> when
> > > > posted
> > > > > > within the scope of a ScopedTaskScheduler. Switching from
> > > > > ScopedTaskEnvironment
> > > > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called
on
> > the
> > > > main
> > > > > > thread and a post task to |task_runner_| is required to quit the run
> > loop
> > > on
> > > > > the
> > > > > > main thread.
> > > > > 
> > > > > I'm not really following, nor do I understand why that affects this
test
> > and
> > > > no
> > > > > others.  I'll try to work through it later today, but not sure I'll
get
> to
> > > it.
> > > > > Have a number of reviews on my plate today.
> > > > 
> > > > - run_loop_.Quit() and run_loop_.Run() must be called on the same
thread.
> > > > - This test calls run_loop_.Run() on the main thread.
> > > > - OnURLFetchComplete() is called as part of a TaskScheduler task.
> > > > - Within the scope of a ScopedTaskScheduler (before this CL),
> TaskScheduler
> > > > tasks are scheduled on the main thread. Therefore, before this CL,
> > > > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it
was
> > > valid
> > > > to call run_loop_.Quit() inline.
> > > > - Within the scope of a ScopedTaskEnvironment (with this CL),
> TaskScheduler
> > > > tasks are scheduled on a dedicated thread. Therefore, it is no longer
> valid
> > to
> > > > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to
the
> > > main
> > > > thread.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
> > > > File net/url_request/url_request_simple_job_unittest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re...
> > > > net/url_request/url_request_simple_job_unittest.cc:224:
> > > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle();
> > > > On 2017/05/10 15:43:44, mmenke wrote:
> > > > > Why do only some RunUntilIdle calls need to be switched?  Just
skimming
> > over
> > > > net
> > > > > unittests, there are still a ton of RunUntilIdle calls.  Are we
changing
> > the
> > > > > behavior of all of them?
> > > > 
> > > > This CL removes ScopedTaskScheduler from individual net/ tests and adds
a
> > > global
> > > > ScopedTaskEnvironment.
> > > > 
> > > > Within the scope of a ScopedTaskScheduler (deprecated):
> > > > 
> > > >   RunLoop().RunUntilIdle()
> > > >     Runs TaskScheduler tasks: Yes
> > > >     Runs main MessageLoop tasks: Yes
> > > >   NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle()
> > > >     Runs TaskScheduler tasks: Yes
> > > >     Runs main MessageLoop tasks: Yes
> > > > 
> > > > Within the scope of a ScopedTaskEnvironment:
> > > > 
> > > >   RunLoop().RunUntilIdle()
> > > >     Runs TaskScheduler tasks: *NO*
> > > >     Runs main MessageLoop tasks: Yes
> > > >   NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle()
> > > >     Runs TaskScheduler tasks: *YES*
> > > >     Runs main MessageLoop tasks: Yes
> > > > 
> > > > Replacing RunLoop().RunUntilIdle() with
> > > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() is not
necessary
> if
> > > we
> > > > don't expect to run TaskScheduler tasks.
> > > 
> > > I'm not really seeing the difference here.  No net/ production code
> references
> > > TaskScheduler.  So why do some net tests need to run TaskScheduler tests,
> and
> > > others are fine with just MessageLoop ones?
> > 
> > *Run TaskScheduler tasks, rather
> 
> Yes, net/ production code references TaskScheduler
>
https://cs.chromium.org/search/?q=task_scheduler/post_task.h+file:%5Esrc/net/...
> Every call to NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() in this
> CL actually runs a TaskScheduler task that is required for a test to pass.

Ahh, task scheduler methods just don't have the string "TaskScheduler" in them,
which was confusing me.

[mmenke, 2017-05-11 15:30:40]

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
net/url_request/url_fetcher_impl_unittest.cc:133:
task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
On 2017/05/10 16:16:12, fdoray wrote:
> On 2017/05/10 15:43:44, mmenke wrote:
> > On 2017/05/09 21:43:47, fdoray wrote:
> > > TaskScheduler tasks run on dedicated thread when they are posted within
the
> > > scope of a ScopedTaskEnvironment while they run on the main thread when
> posted
> > > within the scope of a ScopedTaskScheduler. Switching from
> > ScopedTaskEnvironment
> > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on the
> main
> > > thread and a post task to |task_runner_| is required to quit the run loop
on
> > the
> > > main thread.
> > 
> > I'm not really following, nor do I understand why that affects this test and
> no
> > others.  I'll try to work through it later today, but not sure I'll get to
it.
> > Have a number of reviews on my plate today.
> 
> - run_loop_.Quit() and run_loop_.Run() must be called on the same thread.
> - This test calls run_loop_.Run() on the main thread.
> - OnURLFetchComplete() is called as part of a TaskScheduler task.
> - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler
> tasks are scheduled on the main thread. Therefore, before this CL,
> OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was
valid
> to call run_loop_.Quit() inline.
> - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler
> tasks are scheduled on a dedicated thread. Therefore, it is no longer valid to
> call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the
main
> thread.

URLFetchers are cross-thread objects.  You start them on one thread, it fetches
on another thread (Which may be the starting thread), and then they notify of
completion on the original thread.

The URLFetcher is being started on the main thread, so it should also be
notified of completion on the main thread.  So if this isn't being called on the
main thread, we may have a problem.

[mmenke, 2017-05-11 15:50:53]

On 2017/05/11 15:30:40, mmenke wrote:
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> File net/url_request/url_fetcher_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> net/url_request/url_fetcher_impl_unittest.cc:133:
> task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
> On 2017/05/10 16:16:12, fdoray wrote:
> > On 2017/05/10 15:43:44, mmenke wrote:
> > > On 2017/05/09 21:43:47, fdoray wrote:
> > > > TaskScheduler tasks run on dedicated thread when they are posted within
> the
> > > > scope of a ScopedTaskEnvironment while they run on the main thread when
> > posted
> > > > within the scope of a ScopedTaskScheduler. Switching from
> > > ScopedTaskEnvironment
> > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on
the
> > main
> > > > thread and a post task to |task_runner_| is required to quit the run
loop
> on
> > > the
> > > > main thread.
> > > 
> > > I'm not really following, nor do I understand why that affects this test
and
> > no
> > > others.  I'll try to work through it later today, but not sure I'll get to
> it.
> > > Have a number of reviews on my plate today.
> > 
> > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread.
> > - This test calls run_loop_.Run() on the main thread.
> > - OnURLFetchComplete() is called as part of a TaskScheduler task.
> > - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler
> > tasks are scheduled on the main thread. Therefore, before this CL,
> > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was
> valid
> > to call run_loop_.Quit() inline.
> > - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler
> > tasks are scheduled on a dedicated thread. Therefore, it is no longer valid
to
> > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the
> main
> > thread.
> 
> URLFetchers are cross-thread objects.  You start them on one thread, it
fetches
> on another thread (Which may be the starting thread), and then they notify of
> completion on the original thread.
> 
> The URLFetcher is being started on the main thread, so it should also be
> notified of completion on the main thread.  So if this isn't being called on
the
> main thread, we may have a problem.

(If you want to be precise, substitute "task runner" for "thread")

[fdoray, 2017-05-11 16:15:08]

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
net/url_request/url_fetcher_impl_unittest.cc:133:
task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
On 2017/05/11 15:30:39, mmenke wrote:
> On 2017/05/10 16:16:12, fdoray wrote:
> > On 2017/05/10 15:43:44, mmenke wrote:
> > > On 2017/05/09 21:43:47, fdoray wrote:
> > > > TaskScheduler tasks run on dedicated thread when they are posted within
> the
> > > > scope of a ScopedTaskEnvironment while they run on the main thread when
> > posted
> > > > within the scope of a ScopedTaskScheduler. Switching from
> > > ScopedTaskEnvironment
> > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on
the
> > main
> > > > thread and a post task to |task_runner_| is required to quit the run
loop
> on
> > > the
> > > > main thread.
> > > 
> > > I'm not really following, nor do I understand why that affects this test
and
> > no
> > > others.  I'll try to work through it later today, but not sure I'll get to
> it.
> > > Have a number of reviews on my plate today.
> > 
> > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread.
> > - This test calls run_loop_.Run() on the main thread.
> > - OnURLFetchComplete() is called as part of a TaskScheduler task.
> > - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler
> > tasks are scheduled on the main thread. Therefore, before this CL,
> > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was
> valid
> > to call run_loop_.Quit() inline.
> > - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler
> > tasks are scheduled on a dedicated thread. Therefore, it is no longer valid
to
> > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the
> main
> > thread.
> 
> URLFetchers are cross-thread objects.  You start them on one thread, it
fetches
> on another thread (Which may be the starting thread), and then they notify of
> completion on the original thread.
> 
> The URLFetcher is being started on the main thread, so it should also be
> notified of completion on the main thread.  So if this isn't being called on
the
> main thread, we may have a problem.

URLFetcherTest.SequencedTaskTest creates a URLFetcher in  TaskScheduler.
OnURLFetchComplete() will therefore be invoked in TaskScheduler.

URLFetcherTest.SequencedTaskTest also calls StartFetcherAndWait(), which ends up
calling run_loop_.Run(), on the main thread.

The call to run_loop_.Quit() must happen on the main thread (same thread as
run_loop_.Run()). Therefore, it cannot happen synchronously in
OnURLFetchComplete(). A post task back to the main thread is required.

[mmenke, 2017-05-11 18:40:44]

On 2017/05/11 16:15:08, fdoray wrote:
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> File net/url_request/url_fetcher_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe...
> net/url_request/url_fetcher_impl_unittest.cc:133:
> task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
> On 2017/05/11 15:30:39, mmenke wrote:
> > On 2017/05/10 16:16:12, fdoray wrote:
> > > On 2017/05/10 15:43:44, mmenke wrote:
> > > > On 2017/05/09 21:43:47, fdoray wrote:
> > > > > TaskScheduler tasks run on dedicated thread when they are posted
within
> > the
> > > > > scope of a ScopedTaskEnvironment while they run on the main thread
when
> > > posted
> > > > > within the scope of a ScopedTaskScheduler. Switching from
> > > > ScopedTaskEnvironment
> > > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on
> the
> > > main
> > > > > thread and a post task to |task_runner_| is required to quit the run
> loop
> > on
> > > > the
> > > > > main thread.
> > > > 
> > > > I'm not really following, nor do I understand why that affects this test
> and
> > > no
> > > > others.  I'll try to work through it later today, but not sure I'll get
to
> > it.
> > > > Have a number of reviews on my plate today.
> > > 
> > > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread.
> > > - This test calls run_loop_.Run() on the main thread.
> > > - OnURLFetchComplete() is called as part of a TaskScheduler task.
> > > - Within the scope of a ScopedTaskScheduler (before this CL),
TaskScheduler
> > > tasks are scheduled on the main thread. Therefore, before this CL,
> > > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was
> > valid
> > > to call run_loop_.Quit() inline.
> > > - Within the scope of a ScopedTaskEnvironment (with this CL),
TaskScheduler
> > > tasks are scheduled on a dedicated thread. Therefore, it is no longer
valid
> to
> > > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the
> > main
> > > thread.
> > 
> > URLFetchers are cross-thread objects.  You start them on one thread, it
> fetches
> > on another thread (Which may be the starting thread), and then they notify
of
> > completion on the original thread.
> > 
> > The URLFetcher is being started on the main thread, so it should also be
> > notified of completion on the main thread.  So if this isn't being called on
> the
> > main thread, we may have a problem.
> 
> URLFetcherTest.SequencedTaskTest creates a URLFetcher in  TaskScheduler.
> OnURLFetchComplete() will therefore be invoked in TaskScheduler.
> 
> URLFetcherTest.SequencedTaskTest also calls StartFetcherAndWait(), which ends
up
> calling run_loop_.Run(), on the main thread.
> 
> The call to run_loop_.Quit() must happen on the main thread (same thread as
> run_loop_.Run()). Therefore, it cannot happen synchronously in
> OnURLFetchComplete(). A post task back to the main thread is required.

Ahh, missed that some tests were running (Or pretending to run) fetchers on
another task runner.  LGTM, thanks for bearing with me!

[fdoray, 2017-05-11 18:49:45]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-05-11 18:50:43]

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-05-11 21:18:13]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494528585321100,
"parent_rev": "5641ee8c6260a7ac4fddf464ed97ea81e74116d2", "commit_rev":
"f33fede528342549ee59c5cc2b8f4f966a3edec8"}

[commit-bot: I haz the power, 2017-05-11 21:18:26]

Description was changed from

==========
Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests.

ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and
base/task_scheduler/post_task.h within its scope. It should be
instantiated in everytest that uses either of these APIs
(i.e. no test should instantiate a MessageLoop directly).

Switching from MessageLoopForIO to ScopedTaskEnvironment in
NetTestSuite allows Scoped(Async)TaskScheduler to be removed
from individual net unittests.

Motivation for ScopedTaskEnvironment can be found in:
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

BUG=708584
==========

to

==========
Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests.

ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and
base/task_scheduler/post_task.h within its scope. It should be
instantiated in everytest that uses either of these APIs
(i.e. no test should instantiate a MessageLoop directly).

Switching from MessageLoopForIO to ScopedTaskEnvironment in
NetTestSuite allows Scoped(Async)TaskScheduler to be removed
from individual net unittests.

Motivation for ScopedTaskEnvironment can be found in:
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

BUG=708584

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

[commit-bot: I haz the power, 2017-05-11 21:18:27]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/f33fede528342549ee59c5cc2b8f...