Reduce/remove usage of BrowserThread in content/browser/loader.

Reduce/remove usage of BrowserThread in content/browser/loader.

This patch removes usage of BrowserThread for the UI and IO threads from a number of files
in content/browser/loader. I left the navigation_resource_throttle.cc/.h, navigation_url_loader_impl.cc,
navigation_url_loader_impl_core.cc/.h as is. The expectation being these files will not be part of the
network service.

Following changes in this patch.
1. ResourceDispatcherHostImpl maintains a scoped reference to the task runners for the UI and IO threads.

2. We pass the task runners to the classes  which need them like ResourceMessageFilter, URLLoaderFactoryImpl, etc.

3. The other changes are mostly removing usage of DCHECK_CURRENTLY_ON and PostTask for BrowserThread. I left BrowserThread::FILE
   as is as that is going away soon.

BUG=598073
Review-Url: https://codereview.chromium.org/2785523002
Cr-Commit-Position: refs/heads/master@{#461625}
Committed: https://chromium.googlesource.com/chromium/src/+/a094fc10467ca10732cf02d489565c4d4322890d

[ananta, 2017-03-28 20:56:38]

ananta@chromium.org changed reviewers:
+ jam@chromium.org

[ananta, 2017-03-28 21:15:36]

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

[commit-bot: I haz the power, 2017-03-28 21:16:29]

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-03-28 21:16:51]

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

[commit-bot: I haz the power, 2017-03-28 21:16:52]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[ananta, 2017-03-28 21:59:57]

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

[commit-bot: I haz the power, 2017-03-28 22:00:09]

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-03-28 22:48:35]

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

[commit-bot: I haz the power, 2017-03-28 22:48:36]

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

[ananta, 2017-03-29 00:20:55]

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

[commit-bot: I haz the power, 2017-03-29 00:21:33]

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-03-29 00:57:13]

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

[commit-bot: I haz the power, 2017-03-29 00:57:14]

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

[ananta, 2017-03-29 05:33:54]

Description was changed from

==========
Reduce/remove usage of BrowserThread in content/browser/loader.

This patch removes usage of BrowserThread for the UI and IO threads from a
number of files
in content/browser/loader. I left the navigation_resource_throttle.cc/.h,
navigation_url_loader_impl.cc,
navigation_url_loader_impl_core.cc/.h as is. The expectation being these files
will not be part of the
network service.

Following changes in this patch.
1. A new class LoaderGlobals which lives in
content/browser/loader/loader_globals.cc/.h. This class is instantiated
   with the taskrunners for the UI and IO threads. It provides access to these
runners via getters. Additionally the
   class has only one instance active at any give point.

2. ResourceDispatcherHostImpl maintains a scoped reference to the LoaderGlobals
class instance. This helps ensure that
   the instance is destroyed when ResourceDispatcherHostImpl is destroyed.

3. The other changes are mostly removing usage of DCHECK_CURRENTLY_ON and
PostTask for BrowserThread. I left BrowserThread::FILE
   as is as that is going away soon.

BUG=598073
==========

to

==========
Reduce/remove usage of BrowserThread in content/browser/loader.

This patch removes usage of BrowserThread for the UI and IO threads from a
number of files
in content/browser/loader. I left the navigation_resource_throttle.cc/.h,
navigation_url_loader_impl.cc,
navigation_url_loader_impl_core.cc/.h as is. The expectation being these files
will not be part of the
network service.

Following changes in this patch.
1. A new class LoaderGlobals which lives in
content/browser/loader/loader_globals.cc/.h. This class is instantiated
   with the taskrunners for the UI and IO threads. It provides access to these
runners via getters. Additionally the
   class has only one instance active at any give point.

2. ResourceDispatcherHostImpl maintains a scoped reference to the LoaderGlobals
class instance. This helps ensure that
   the instance is destroyed when ResourceDispatcherHostImpl is destroyed.

3. The other changes are mostly removing usage of DCHECK_CURRENTLY_ON and
PostTask for BrowserThread. I left BrowserThread::FILE
   as is as that is going away soon.

4. For content_unittests to pass I had to instantiate dummy instances of the
LoaderGlobals class in some places like test_renderer_host.cc, etc. I added
comments in these places to rework or ideally remove the hack.


BUG=598073
==========

[ananta, 2017-03-29 05:34:18]

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

[commit-bot: I haz the power, 2017-03-29 05:34:49]

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-03-29 05:41:37]

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

[commit-bot: I haz the power, 2017-03-29 05:41:38]

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

[ananta, 2017-03-29 13:06:49]

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

[commit-bot: I haz the power, 2017-03-29 13:07:04]

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-03-29 13:33:42]

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

[commit-bot: I haz the power, 2017-03-29 13:33:43]

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

[ananta, 2017-03-29 14:53:04]

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

[commit-bot: I haz the power, 2017-03-29 14:53: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-03-29 15:24:16]

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

[commit-bot: I haz the power, 2017-03-29 15:24:17]

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

[jam, 2017-03-29 15:44:33]

I think a bunch of these places don't need to change; left comments.

the remaining callsites are fewer, and since we already have a global
(ResourceDispatcherHost), why not pass the IO thread to it on creation (it can
deduce the main thread since that's where it's created)? Then the remaining
sites can just call to it to get the threads

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler_browsertest.cc (left):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler_browsertest.cc:49:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
this file isn't going to moveee (browser tests need all the code in content), so
it can use BrowerThread

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/loader_io_thread_notifier.cc (left):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:10: #include
"content/public/browser/browser_thread.h"
this file isn't going to move, since it's using RenderFrameHost. so no need to
change it

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (left):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_browsertest.cc:63:
BrowserThread::PostTask(
ditto, this is a browser test so it won't move. no need to change it.

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader.cc:448: void
ResourceLoader::CancelSSLRequest(int error,
ResourceLoader is created from RDH, so we know it's created on the right thread.
so it should have a base::ThreadChecker member and it can use it in these
methods

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_message_filter.cc (left):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_message_filter.cc:49:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
can use ThreadChecker that's initialized here

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.cc (right):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.cc:24:
DCHECK(LoaderGlobals::Get()
seems like any code that calls WebContents is by definition not moving

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_requester_info.cc (right):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_requester_info.cc:53:
LoaderGlobals::Get()->io_thread_task_runner()->BelongsToCurrentThread());
i think we can take this out, there's one caller inside content and that one
already checks its thread (and it's somewhat redundant anyways)

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/temporary_file_stream.cc (right):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/temporary_file_stream.cc:57:
LoaderGlobals::Get()->io_thread_task_runner()->BelongsToCurrentThread());
this is only called by redirect_to_file_resource_handler.cc which isn't planned
to move, so it can use BrowserThread

[ananta, 2017-03-29 19:41:04]

Thanks for the review. I should have looked at this
https://docs.google.com/spreadsheets/d/1ot0WOgC2TrMY5VADo2ijmx98Z792FojUKJncG...
before sending this patch out. 

Regarding keeping the runners in RDHI vs a new class, I thought a new class
would be better. This is mostly because having a task runner queried out of RDHI
did not seem intuitive to me. It is plausible that the LoaderState class or some
variant of this may be needed to hold global state in the n/w service.

Thanks

https://codereview.chromium.org/2785523002/diff/60001/content/browser/loader/...
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2785523002/diff/60001/content/browser/loader/...
content/browser/loader/url_loader_factory_impl_unittest.cc:135:
std::unique_ptr<TestBrowserThreadBundle> thread_bundle_;
This change was required to ensure that the thread bundle gets destroyed
before the other members of this class like ResourceDispatcherHostImpl, etc. Not
doing this causes dchecks to fire in the code due to a null LoaderGlobals
instance.

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler_browsertest.cc (left):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler_browsertest.cc:49:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2017/03/29 15:44:32, jam wrote:
> this file isn't going to moveee (browser tests need all the code in content),
so
> it can use BrowerThread

ok. thanks. done

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/loader_io_thread_notifier.cc (left):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:10: #include
"content/public/browser/browser_thread.h"
On 2017/03/29 15:44:32, jam wrote:
> this file isn't going to move, since it's using RenderFrameHost. so no need to
> change it

Thanks. done

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (left):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_browsertest.cc:63:
BrowserThread::PostTask(
On 2017/03/29 15:44:32, jam wrote:
> ditto, this is a browser test so it won't move. no need to change it.

Done.

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader.cc:448: void
ResourceLoader::CancelSSLRequest(int error,
On 2017/03/29 15:44:32, jam wrote:
> ResourceLoader is created from RDH, so we know it's created on the right
thread.
> so it should have a base::ThreadChecker member and it can use it in these
> methods

Done.

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_message_filter.cc (left):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_message_filter.cc:49:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2017/03/29 15:44:33, jam wrote:
> can use ThreadChecker that's initialized here

Leaving this as is as we need the runner in some places in this file.

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.cc (right):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.cc:24:
DCHECK(LoaderGlobals::Get()
On 2017/03/29 15:44:33, jam wrote:
> seems like any code that calls WebContents is by definition not moving

This function needs to be moved. We can revisit. Reverted this change

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_requester_info.cc (right):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_requester_info.cc:53:
LoaderGlobals::Get()->io_thread_task_runner()->BelongsToCurrentThread());
On 2017/03/29 15:44:33, jam wrote:
> i think we can take this out, there's one caller inside content and that one
> already checks its thread (and it's somewhat redundant anyways)

Done.

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/temporary_file_stream.cc (right):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/temporary_file_stream.cc:57:
LoaderGlobals::Get()->io_thread_task_runner()->BelongsToCurrentThread());
On 2017/03/29 15:44:33, jam wrote:
> this is only called by redirect_to_file_resource_handler.cc which isn't
planned
> to move, so it can use BrowserThread

Done.

[ananta, 2017-03-29 19:43:55]

https://codereview.chromium.org/2785523002/diff/120001/content/browser/loader...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/2785523002/diff/120001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:42: DCHECK(
ThreadChecker can be used here. However static functions below need the global
object. Left this as is for consistency.

[ananta, 2017-03-29 19:46:02]

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

[commit-bot: I haz the power, 2017-03-29 19:46:47]

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-03-29 21:13:21]

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

[commit-bot: I haz the power, 2017-03-29 21:13:22]

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

[ananta, 2017-03-29 21:44:54]

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

[commit-bot: I haz the power, 2017-03-29 21:45:46]

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

[ananta, 2017-03-29 23:11:46]

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

[commit-bot: I haz the power, 2017-03-29 23:13:06]

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

[ananta, 2017-03-30 00:28:33]

Description was changed from

==========
Reduce/remove usage of BrowserThread in content/browser/loader.

This patch removes usage of BrowserThread for the UI and IO threads from a
number of files
in content/browser/loader. I left the navigation_resource_throttle.cc/.h,
navigation_url_loader_impl.cc,
navigation_url_loader_impl_core.cc/.h as is. The expectation being these files
will not be part of the
network service.

Following changes in this patch.
1. A new class LoaderGlobals which lives in
content/browser/loader/loader_globals.cc/.h. This class is instantiated
   with the taskrunners for the UI and IO threads. It provides access to these
runners via getters. Additionally the
   class has only one instance active at any give point.

2. ResourceDispatcherHostImpl maintains a scoped reference to the LoaderGlobals
class instance. This helps ensure that
   the instance is destroyed when ResourceDispatcherHostImpl is destroyed.

3. The other changes are mostly removing usage of DCHECK_CURRENTLY_ON and
PostTask for BrowserThread. I left BrowserThread::FILE
   as is as that is going away soon.

4. For content_unittests to pass I had to instantiate dummy instances of the
LoaderGlobals class in some places like test_renderer_host.cc, etc. I added
comments in these places to rework or ideally remove the hack.


BUG=598073
==========

to

==========
Reduce/remove usage of BrowserThread in content/browser/loader.

This patch removes usage of BrowserThread for the UI and IO threads from a
number of files
in content/browser/loader. I left the navigation_resource_throttle.cc/.h,
navigation_url_loader_impl.cc,
navigation_url_loader_impl_core.cc/.h as is. The expectation being these files
will not be part of the
network service.

Following changes in this patch.
1. A new class LoaderGlobals which lives in
content/browser/loader/loader_globals.cc/.h. This class is instantiated
   with the taskrunners for the UI and IO threads. It provides access to these
runners via getters. Additionally the
   class has only one instance active at any give point.

2. ResourceDispatcherHostImpl maintains a scoped reference to the LoaderGlobals
class instance. This helps ensure that
   the instance is destroyed when ResourceDispatcherHostImpl is destroyed.

3. The other changes are mostly removing usage of DCHECK_CURRENTLY_ON and
PostTask for BrowserThread. I left BrowserThread::FILE
   as is as that is going away soon.

BUG=598073
==========

[commit-bot: I haz the power, 2017-03-30 04:12:25]

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

[commit-bot: I haz the power, 2017-03-30 04:12:26]

Dry run: This issue passed the CQ dry run.

[jam, 2017-03-30 17:34:09]

On 2017/03/29 19:41:04, ananta wrote:
> Thanks for the review. I should have looked at this
>
https://docs.google.com/spreadsheets/d/1ot0WOgC2TrMY5VADo2ijmx98Z792FojUKJncG...
> before sending this patch out. 
> 
> Regarding keeping the runners in RDHI vs a new class, I thought a new class
> would be better. This is mostly because having a task runner queried out of
RDHI
> did not seem intuitive to me. It is plausible that the LoaderState class or
some
> variant of this may be needed to hold global state in the n/w service.
> 
> Thanks
> 

some more comments.

Thanks for the iteration. Looking more, I think the use cases here can be
removed by passing the IO task runner down to the few remaining users of the new
global. Then we don't need it anymore. In general we avoid globals, and it seems
if we can avoid adding yet another one, that's a good goal.

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_message_filter.cc:10: #include
"content/browser/loader/loader_globals.h"
can we just pass the IO task runner to this class' constructor?
RenderProcessHost can use BrowserThread for that, and once we run it in a
different process, whoever starts it then will also have it

https://codereview.chromium.org/2785523002/diff/150001/content/browser/loader...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/2785523002/diff/150001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:43:
LoaderGlobals::Get()->io_thread_task_runner()->BelongsToCurrentThread());
this can get the IO thread task runner from whoever creates it (RMF, or SW
code). they both have it

[ananta, 2017-03-30 22:50:41]

Description was changed from

==========
Reduce/remove usage of BrowserThread in content/browser/loader.

This patch removes usage of BrowserThread for the UI and IO threads from a
number of files
in content/browser/loader. I left the navigation_resource_throttle.cc/.h,
navigation_url_loader_impl.cc,
navigation_url_loader_impl_core.cc/.h as is. The expectation being these files
will not be part of the
network service.

Following changes in this patch.
1. A new class LoaderGlobals which lives in
content/browser/loader/loader_globals.cc/.h. This class is instantiated
   with the taskrunners for the UI and IO threads. It provides access to these
runners via getters. Additionally the
   class has only one instance active at any give point.

2. ResourceDispatcherHostImpl maintains a scoped reference to the LoaderGlobals
class instance. This helps ensure that
   the instance is destroyed when ResourceDispatcherHostImpl is destroyed.

3. The other changes are mostly removing usage of DCHECK_CURRENTLY_ON and
PostTask for BrowserThread. I left BrowserThread::FILE
   as is as that is going away soon.

BUG=598073
==========

to

==========
Reduce/remove usage of BrowserThread in content/browser/loader.

This patch removes usage of BrowserThread for the UI and IO threads from a
number of files
in content/browser/loader. I left the navigation_resource_throttle.cc/.h,
navigation_url_loader_impl.cc,
navigation_url_loader_impl_core.cc/.h as is. The expectation being these files
will not be part of the
network service.

Following changes in this patch.
1. ResourceDispatcherHostImpl maintains a scoped reference to the task runners
for the UI and IO threads.

2. We pass the task runners to the classes  which need them like
ResourceMessageFilter, URLLoaderFactoryImpl, etc.

3. The other changes are mostly removing usage of DCHECK_CURRENTLY_ON and
PostTask for BrowserThread. I left BrowserThread::FILE
   as is as that is going away soon.

BUG=598073
==========

[ananta, 2017-03-30 22:51:22]

I removed the LoaderGlobals class and moved the runners to RDHI.

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/2785523002/diff/80001/content/browser/loader/...
content/browser/loader/resource_message_filter.cc:10: #include
"content/browser/loader/loader_globals.h"
On 2017/03/30 17:34:09, jam wrote:
> can we just pass the IO task runner to this class' constructor?
> RenderProcessHost can use BrowserThread for that, and once we run it in a
> different process, whoever starts it then will also have it

Done.

https://codereview.chromium.org/2785523002/diff/150001/content/browser/loader...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/2785523002/diff/150001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:43:
LoaderGlobals::Get()->io_thread_task_runner()->BelongsToCurrentThread());
On 2017/03/30 17:34:09, jam wrote:
> this can get the IO thread task runner from whoever creates it (RMF, or SW
> code). they both have it

Done.

[ananta, 2017-03-30 22:52:03]

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

[commit-bot: I haz the power, 2017-03-30 22:53: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, 2017-03-31 00:35:05]

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

[commit-bot: I haz the power, 2017-03-31 00:35:06]

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

[jam, 2017-04-03 14:47:03]

lgtm with comments
sorry for the delay

https://codereview.chromium.org/2785523002/diff/210001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2785523002/diff/210001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:343: const
scoped_refptr<base::SingleThreadTaskRunner>& main_thread_runner,
since RDH is always created on the main thread, no need to pass in the main
thread runner. it can deduce this itself in its constructor

https://codereview.chromium.org/2785523002/diff/210001/content/browser/loader...
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/2785523002/diff/210001/content/browser/loader...
content/browser/loader/resource_message_filter.cc:27: const
scoped_refptr<base::SingleThreadTaskRunner>& main_thread_runner,
no need to pass this one in, we can remove the dcheck in the constructor. all
message filters are created on the UI thread.

[ananta, 2017-04-03 22:58:19]

https://codereview.chromium.org/2785523002/diff/210001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2785523002/diff/210001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:343: const
scoped_refptr<base::SingleThreadTaskRunner>& main_thread_runner,
On 2017/04/03 14:47:03, jam (busy till April 4) wrote:
> since RDH is always created on the main thread, no need to pass in the main
> thread runner. it can deduce this itself in its constructor

Done.

https://codereview.chromium.org/2785523002/diff/210001/content/browser/loader...
File content/browser/loader/resource_message_filter.cc (right):

https://codereview.chromium.org/2785523002/diff/210001/content/browser/loader...
content/browser/loader/resource_message_filter.cc:27: const
scoped_refptr<base::SingleThreadTaskRunner>& main_thread_runner,
On 2017/04/03 14:47:03, jam (busy till April 4) wrote:
> no need to pass this one in, we can remove the dcheck in the constructor. all
> message filters are created on the UI thread.

Done.

[ananta, 2017-04-03 23:03:05]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-04-03 23:03:06]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2785523002/#ps250001
(title: "Remove DCHECK")

[commit-bot: I haz the power, 2017-04-03 23:03:50]

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-04 00:33:17]

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

[commit-bot: I haz the power, 2017-04-04 00:33:19]

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

[ananta, 2017-04-04 00:52:58]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2017-04-04 00:53:28]

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-04 04:10:46]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1491267178002850,
"parent_rev": "59016e60a48d96c7b7d9bf71cf61307ea1df0903", "commit_rev":
"a094fc10467ca10732cf02d489565c4d4322890d"}

[commit-bot: I haz the power, 2017-04-04 04:12:06]

Description was changed from

==========
Reduce/remove usage of BrowserThread in content/browser/loader.

This patch removes usage of BrowserThread for the UI and IO threads from a
number of files
in content/browser/loader. I left the navigation_resource_throttle.cc/.h,
navigation_url_loader_impl.cc,
navigation_url_loader_impl_core.cc/.h as is. The expectation being these files
will not be part of the
network service.

Following changes in this patch.
1. ResourceDispatcherHostImpl maintains a scoped reference to the task runners
for the UI and IO threads.

2. We pass the task runners to the classes  which need them like
ResourceMessageFilter, URLLoaderFactoryImpl, etc.

3. The other changes are mostly removing usage of DCHECK_CURRENTLY_ON and
PostTask for BrowserThread. I left BrowserThread::FILE
   as is as that is going away soon.

BUG=598073
==========

to

==========
Reduce/remove usage of BrowserThread in content/browser/loader.

This patch removes usage of BrowserThread for the UI and IO threads from a
number of files
in content/browser/loader. I left the navigation_resource_throttle.cc/.h,
navigation_url_loader_impl.cc,
navigation_url_loader_impl_core.cc/.h as is. The expectation being these files
will not be part of the
network service.

Following changes in this patch.
1. ResourceDispatcherHostImpl maintains a scoped reference to the task runners
for the UI and IO threads.

2. We pass the task runners to the classes  which need them like
ResourceMessageFilter, URLLoaderFactoryImpl, etc.

3. The other changes are mostly removing usage of DCHECK_CURRENTLY_ON and
PostTask for BrowserThread. I left BrowserThread::FILE
   as is as that is going away soon.

BUG=598073

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

[commit-bot: I haz the power, 2017-04-04 04:12:08]

Committed patchset #14 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/a094fc10467ca10732cf02d48956...