Introduce SystemNetworkContextManager.

Introduce SystemNetworkContextManager.

The new class provides access to the SystemURLRequestContext via
an in-process NetworkService and the NetworkContext Mojo API.
Eventually, we should remove IOThread entirely, and have the
SystemNetworkContextManager perform all configuration of the
SystemRequestContext, and provide access to it as well.

The in-process network service currently just takes a
URLRequestContextBuilder from IOThread and uses that to create the
URLRequestContext, but this gives us a path to switch over
configuration so that it goes through the NetworkService API. It also
lets us switch consumers over to using the NetworkContext API without
having to create a wrapper that supports both the shiny new Mojo API
the legacy one.

When network service is enabled, the SystemNetworkContextManager will
use an out-of-process URLRequestContext instead of an in-process one,
while the IOThread will continue to use an in-process ones.

BUG=715695
Review-Url: https://codereview.chromium.org/2968293002
Cr-Commit-Position: refs/heads/master@{#486527}
Committed: https://chromium.googlesource.com/chromium/src/+/f73a51226b54539fc543cbb29784ab036b76dc40

[mmenke, 2017-07-06 21:43:33]

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

[commit-bot: I haz the power, 2017-07-06 21:43: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-07-06 21:50:28]

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

[commit-bot: I haz the power, 2017-07-06 21:50:28]

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

[mmenke, 2017-07-06 21:58:38]

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

[commit-bot: I haz the power, 2017-07-06 21:58:52]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-07-07 02:30:09]

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

[commit-bot: I haz the power, 2017-07-07 02:30: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-07-07 02:50:37]

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

[commit-bot: I haz the power, 2017-07-07 02:50:38]

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

[mmenke, 2017-07-07 17:50:31]

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

[commit-bot: I haz the power, 2017-07-07 17:50:45]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-07-07 17:52:49]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-07-07 18:03:03]

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

[commit-bot: I haz the power, 2017-07-07 18:03:03]

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

[mmenke, 2017-07-07 18:13:40]

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

[commit-bot: I haz the power, 2017-07-07 18:13:57]

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-07-07 18:30:56]

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

[commit-bot: I haz the power, 2017-07-07 18:30:57]

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

[mmenke, 2017-07-07 18:53:28]

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

[commit-bot: I haz the power, 2017-07-07 18:53:48]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-07-07 19:11:31]

Patchset #4 (id:50038) has been deleted

[commit-bot: I haz the power, 2017-07-07 19:46:42]

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

[commit-bot: I haz the power, 2017-07-07 19:46:43]

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

[mmenke, 2017-07-07 20:50:26]

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

[commit-bot: I haz the power, 2017-07-07 20:50:56]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-07-07 20:51:52]

mmenke@chromium.org changed reviewers:
+ asanka@chromium.org, jam@chromium.org

[mmenke, 2017-07-07 20:51:54]

[jam]:  PTAL
[asanka]:  Assume you don't know any of this code too well, but I'd like an
opinion from someone who knows net on this general direction.
[rdsmith]:  FYI

I basically want to work towards replacing IOThread with
SystemNetworkContextManager and replace ProfileIOData with some sort of
BrowserContextKeyedService.  As I move configuration from IOThread/ProfileIOData
over to the NetworkService, I want to add tests like the new browser_test to
make sure each thing is configured correctly for each NetworkContext, in both
the in-process and out-of-process cases.  I went with pushing the
URLRequestContextBuilder to the NetworkContext instead of using a callback
because it's more convenient, though it does mean once I add a configuration
option to the NetworkContext, I'll have to switch over all
URLRequestContextBuilders to using that option at once.

Suggested review order:
* io_thread / system_network_context_manager are the main things to look at
* network_service[_impl] / network_context
* network_service_instance / storage_partition_impl
* Everything else (excluding the new test) is just fallout from moving
test_url_loader_client and from the NetworkService rename.

https://codereview.chromium.org/2968293002/diff/110001/content/public/browser...
File content/public/browser/network_service_instance.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/public/browser...
content/public/browser/network_service_instance.h:19: CONTENT_EXPORT
mojom::NetworkService* GetNetworkService();
Not sure if there's a better way to do this - basically both chrome/ and
content/ need access to the same mojom::NetworkService, to guarantee that any
initialization chrome/ does is done first.  And chrome/ needs to be able to
access it at all (i.e., can't just have it passed in when creating profiles). 
We could put an accessor in ContentBrowserClient, if we wanted to, though that's
kind of a grab bag of stuff, and it's unclear how we'd grab the service in
Chrome (Maybe have SystemNetworkContextManager own it as well?)

[mmenke, 2017-07-07 20:55:15]

Description was changed from

==========
Introduce SystemNetworkContextManager.

The new class provides access to the SystemURLRequestContext via the
an in-process NetworkService and the NetworkContext Mojo API.
Eventually, we should remove IOThread entirely, and have the
SystemNetworkContextManager perform all configuration of the
SystemRequestContext, and provide access to it as well.

The in-process network service currently just takes a
URLRequestContextBuilder from IOThread and uses that to create the
URLRequestContext, but this gives us a path to switch over
configuration so that it goes through the NetworkService API. It also
lets us switch consumers over to using the NetworkContext API without
having to create a wrapper that supports both the shiny new Mojo API
the legacy one.

When network service is enabled, the SystemNetworkContextManager will
use an out-of-process URLRequestContext instead of an in-process one,
while the IOThread will continue to use an in-process ones.

BUG=715695
==========

to

==========
Introduce SystemNetworkContextManager.

The new class provides access to the SystemURLRequestContext via
an in-process NetworkService and the NetworkContext Mojo API.
Eventually, we should remove IOThread entirely, and have the
SystemNetworkContextManager perform all configuration of the
SystemRequestContext, and provide access to it as well.

The in-process network service currently just takes a
URLRequestContextBuilder from IOThread and uses that to create the
URLRequestContext, but this gives us a path to switch over
configuration so that it goes through the NetworkService API. It also
lets us switch consumers over to using the NetworkContext API without
having to create a wrapper that supports both the shiny new Mojo API
the legacy one.

When network service is enabled, the SystemNetworkContextManager will
use an out-of-process URLRequestContext instead of an in-process one,
while the IOThread will continue to use an in-process ones.

BUG=715695
==========

[mmenke, 2017-07-07 20:56:55]

https://codereview.chromium.org/2968293002/diff/110001/content/public/browser...
File content/public/browser/network_service_instance.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/public/browser...
content/public/browser/network_service_instance.h:19: CONTENT_EXPORT
mojom::NetworkService* GetNetworkService();
On 2017/07/07 20:51:53, mmenke wrote:
> Not sure if there's a better way to do this - basically both chrome/ and
> content/ need access to the same mojom::NetworkService, to guarantee that any
> initialization chrome/ does is done first.  And chrome/ needs to be able to
> access it at all (i.e., can't just have it passed in when creating profiles). 
> We could put an accessor in ContentBrowserClient, if we wanted to, though
that's
> kind of a grab bag of stuff, and it's unclear how we'd grab the service in
> Chrome (Maybe have SystemNetworkContextManager own it as well?)

Sorry, that should be "chrome needs to be able to access it on its own"

[commit-bot: I haz the power, 2017-07-07 22:18:01]

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

[commit-bot: I haz the power, 2017-07-07 22:18:02]

Dry run: This issue passed the CQ dry run.

[jam, 2017-07-08 01:30:08]

nice, mostly one comment re future plans with OOP/in-process network service

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:91: # Allow in-proces network service. Remove once OOP
NetworkService ships.
nit: I think removing this wouldn't depend on the "OOP" part right? i.e. once
network service is shipped, we should use the same mojo codepaths even when it's
in-process. Then we can remove this deps entry right?

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.h (right):

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.h:23: // IOThread's
SystemURLRequestContext (If the OOP NetworkService is disabled) or
nit: here and below, I think OOP is not necessary as in my other comments right?
i.e. once we enable network service, even if it's running in-process then the
NetworkService path would be taken

https://codereview.chromium.org/2968293002/diff/110001/content/network/networ...
File content/network/network_service_impl.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/network/networ...
content/network/network_service_impl.h:53: friend class NetworkService;
why is this needed?

https://codereview.chromium.org/2968293002/diff/110001/content/public/browser...
File content/public/browser/network_service_instance.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/public/browser...
content/public/browser/network_service_instance.h:17: // Must only be called on
the UI thread. Must not be called if OOP
nit: "OOP" unnecessary?

https://codereview.chromium.org/2968293002/diff/110001/content/public/network...
File content/public/network/network_service.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/public/network...
content/public/network/network_service.h:48:
DISALLOW_COPY_AND_ASSIGN(NetworkService);
nit: not needed since interface isn't copyable

https://codereview.chromium.org/2968293002/diff/110001/content/public/test/te...
File content/public/test/test_url_loader_client.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/public/test/te...
content/public/test/test_url_loader_client.h:5: #ifndef
CONTENT_BROWSER_LOADER_TEST_URL_LOADER_CLIENT_H_
nit: update

[mmenke, 2017-07-08 01:53:56]

Quick response to the "OOP NetworkService" comments - in this patch, IOThread
instantiates a NetworkService object in-process on the IO thread, and I was
trying to distinguish that from the one created the ServiceManagerConnection. 
So as soon as soon as this code lands, we'll have an in-process NetworkService
(Even if it won't really be doing much).  I'm not sure how we'll handle a
self-contained in-process NetworkService on Android (or wherever else we need
it).  I'll try to update the comments to be clearer on Monday, though I'm open
to ideas until then on how to more clearly differentiate them.

[Randy Smith (Not in Mondays), 2017-07-08 22:58:49]

rdsmith@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2017-07-08 22:58:50]

https://codereview.chromium.org/2968293002/diff/110001/content/public/network...
File content/public/network/network_service.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/public/network...
content/public/network/network_service.h:21: // Allows an  in-process
NetworkService to be set up.
nit: extra space

[kinuko, 2017-07-10 08:10:32]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-07-10 08:10:33]

(Trying to learn the code around this)

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.cc (right):

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.cc:58: // Initialize() should
already have been called, and set the
ditto

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.cc:59: //
|system_network_context| if the OOP NetworkService is disabled.
system_network_context_

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.h (right):

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.h:45: // Returns the System
NetworkContext. May only be called after Initialize().
Wasn't sure what this Initialize points to, do you mean SetUp?

https://codereview.chromium.org/2968293002/diff/110001/content/network/networ...
File content/network/network_service_impl.cc (right):

https://codereview.chromium.org/2968293002/diff/110001/content/network/networ...
content/network/network_service_impl.cc:64: if (registry_) {
nit: might be still helpful to note when registry_ is null

https://codereview.chromium.org/2968293002/diff/110001/content/public/network...
File content/public/network/network_service.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/public/network...
content/public/network/network_service.h:24: // TODO(mmenke):  Take in a
NetworkServiceRequest.
(and extra space here too)

[mmenke, 2017-07-10 15:50:51]

Patchset #6 (id:130001) has been deleted

[mmenke, 2017-07-10 15:52:23]

Thanks for the feedback!

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:91: # Allow in-proces network service. Remove once OOP
NetworkService ships.
On 2017/07/08 01:30:08, jam wrote:
> nit: I think removing this wouldn't depend on the "OOP" part right? i.e. once
> network service is shipped, we should use the same mojo codepaths even when
it's
> in-process. Then we can remove this deps entry right?

Done.

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.cc (right):

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.cc:58: // Initialize() should
already have been called, and set the
On 2017/07/10 08:10:33, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.cc:59: //
|system_network_context| if the OOP NetworkService is disabled.
On 2017/07/10 08:10:33, kinuko wrote:
> system_network_context_

Done.

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.h (right):

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.h:23: // IOThread's
SystemURLRequestContext (If the OOP NetworkService is disabled) or
On 2017/07/08 01:30:08, jam wrote:
> nit: here and below, I think OOP is not necessary as in my other comments
right?
> i.e. once we enable network service, even if it's running in-process then the
> NetworkService path would be taken

I've removed all instances of OOP (I now refer to it as the network service [No
caps], and in a couple places try to make the distinction between the IOThread's
NetworkService and the actual network service clear.

https://codereview.chromium.org/2968293002/diff/110001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.h:45: // Returns the System
NetworkContext. May only be called after Initialize().
On 2017/07/10 08:10:33, kinuko wrote:
> Wasn't sure what this Initialize points to, do you mean SetUp?

Done, thanks (Renamed the method itself once I move initialization into
GetContext() in the network service case)

https://codereview.chromium.org/2968293002/diff/110001/content/network/networ...
File content/network/network_service_impl.cc (right):

https://codereview.chromium.org/2968293002/diff/110001/content/network/networ...
content/network/network_service_impl.cc:64: if (registry_) {
On 2017/07/10 08:10:33, kinuko wrote:
> nit: might be still helpful to note when registry_ is null

Done.

https://codereview.chromium.org/2968293002/diff/110001/content/network/networ...
File content/network/network_service_impl.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/network/networ...
content/network/network_service_impl.h:53: friend class NetworkService;
On 2017/07/08 01:30:08, jam wrote:
> why is this needed?

Removed.  It's not, added it for a new private constructor, which I decided not
to keep.

https://codereview.chromium.org/2968293002/diff/110001/content/public/browser...
File content/public/browser/network_service_instance.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/public/browser...
content/public/browser/network_service_instance.h:17: // Must only be called on
the UI thread. Must not be called if OOP
On 2017/07/08 01:30:08, jam wrote:
> nit: "OOP" unnecessary?

Done.

https://codereview.chromium.org/2968293002/diff/110001/content/public/network...
File content/public/network/network_service.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/public/network...
content/public/network/network_service.h:21: // Allows an  in-process
NetworkService to be set up.
On 2017/07/08 22:58:50, Randy Smith (Not in Mondays) wrote:
> nit: extra space

Done.

https://codereview.chromium.org/2968293002/diff/110001/content/public/network...
content/public/network/network_service.h:24: // TODO(mmenke):  Take in a
NetworkServiceRequest.
On 2017/07/10 08:10:33, kinuko wrote:
> (and extra space here too)

Done.

https://codereview.chromium.org/2968293002/diff/110001/content/public/network...
content/public/network/network_service.h:48:
DISALLOW_COPY_AND_ASSIGN(NetworkService);
On 2017/07/08 01:30:08, jam wrote:
> nit: not needed since interface isn't copyable

Done, though I'm not certain I agree.  I feel it's a better idea to keep them,
to avoid regressions if an abstract type ever becomes non-abstract.  The style
guide says "If you do not want to support copy/move operations on your type,
explicitly disable them" (And there is no exceptions for abstract classes /
interfaces that I see)

https://codereview.chromium.org/2968293002/diff/110001/content/public/test/te...
File content/public/test/test_url_loader_client.h (right):

https://codereview.chromium.org/2968293002/diff/110001/content/public/test/te...
content/public/test/test_url_loader_client.h:5: #ifndef
CONTENT_BROWSER_LOADER_TEST_URL_LOADER_CLIENT_H_
On 2017/07/08 01:30:08, jam wrote:
> nit: update

Done.  Nice catch!  Wish we had a checked for these.  :(

[mmenke, 2017-07-10 15:52:31]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-07-10 15:52:41]

The CQ bit was unchecked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-07-10 15:52: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-07-10 15:52:44]

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

[commit-bot: I haz the power, 2017-07-10 15:52:46]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[mmenke, 2017-07-10 15:53:00]

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

[commit-bot: I haz the power, 2017-07-10 15:53:16]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-07-10 15:53:29]

On 2017/07/10 15:52:46, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet.
> CQ run can only be started once the patch has received an L-G-T-M from a full
> committer.
> Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
> full super star committer.
> Committers are members of the group "project-chromium-committers".
> Note that this has nothing to do with OWNERS files.

Oops.  Meant to do a tryjob and misclicked (Caught it myself, but it beat me to
it)

[commit-bot: I haz the power, 2017-07-10 17:22:32]

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

[commit-bot: I haz the power, 2017-07-10 17:22:34]

Dry run: This issue passed the CQ dry run.

[jam, 2017-07-10 19:17:25]

lgtm

[kinuko, 2017-07-11 08:26:41]

Some nits, and supplemental lgtm. Looks sensible to me.

https://codereview.chromium.org/2968293002/diff/150001/chrome/browser/io_thre...
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/2968293002/diff/150001/chrome/browser/io_thre...
chrome/browser/io_thread.h:134: // the actual network service is disabled.
nit: might be helpful to have a forwarding pointer to the detailed comment, e.g.
"see SystemNetworkContextManager's header comment for more details"

https://codereview.chromium.org/2968293002/diff/150001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.h (right):

https://codereview.chromium.org/2968293002/diff/150001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.h:37: // help set up the
IOTHread's in-process URLRequestContext, and
nit: IOTHread -> IOThread

[mmenke, 2017-07-11 18:36:26]

Thanks!

https://codereview.chromium.org/2968293002/diff/150001/chrome/browser/io_thre...
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/2968293002/diff/150001/chrome/browser/io_thre...
chrome/browser/io_thread.h:134: // the actual network service is disabled.
On 2017/07/11 08:26:41, kinuko wrote:
> nit: might be helpful to have a forwarding pointer to the detailed comment,
e.g.
> "see SystemNetworkContextManager's header comment for more details"

Done.

https://codereview.chromium.org/2968293002/diff/150001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.h (right):

https://codereview.chromium.org/2968293002/diff/150001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.h:37: // help set up the
IOTHread's in-process URLRequestContext, and
On 2017/07/11 08:26:41, kinuko wrote:
> nit: IOTHread -> IOThread

Done.

[mmenke, 2017-07-11 18:42:17]

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

[commit-bot: I haz the power, 2017-07-11 18:42:40]

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-07-11 18:45:41]

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

[commit-bot: I haz the power, 2017-07-11 18:45:43]

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

[mmenke, 2017-07-11 19:03:32]

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

[commit-bot: I haz the power, 2017-07-11 19:04:03]

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-07-11 19:10:27]

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

[commit-bot: I haz the power, 2017-07-11 19:10:28]

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-07-12 20:16:01]

asanka:  Friendly ping.

[asanka, 2017-07-13 01:52:29]

LGTM % nits. It took me a while to come up to speed on mojo and wrap my head
around this code. Sorry about the delay.

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/io_thre...
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/io_thre...
chrome/browser/io_thread.h:134: // the actual network service is disabled. See
SystemNetworkContextManager's
s/actual/out-of-process/ ?

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/io_thre...
chrome/browser/io_thread.h:335: // These are set on the IOThread, and then
consumed during initialization on
... are set on the UI thread, and then ... on the IO thread.
                   ~~~~~~~~~                      ~~~~~~~~~

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.cc (right):

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.cc:63:
content::mojom::NetworkContextParams::New();
did you mean to call CreateNetworkContextParams() here instead and use
|context_params| in the CreateNetworkContext() call below?

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.h (right):

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.h:63: // IOThread's
SystemURLRequestContext.
The fact that system_network_context_ is bound to the IOThread's network context
while io_thread_network_context_ is nil was quite confusing to me.

It would've been easier to understand if io_thread_network_context_ is
unconditionally wrapping the IOThread's NetworkContext and have
SystemNetworkContextManager::Context() bind via the out-of-process network
service depending on the feature state. That way we can call this
out_of_process_network_context_ or something.

Also is it possible to bind out_of_process_network_context_ at
SystemNetworkContextManager::SetUp()? If so SetUp() can return the
NetworkContextRequest for io_thread_network_context_ and, if the feature is
enabled, bind out_of_process_network_context_. This way
SystemNetworkContextManager::GetContext() is ready to fire without fiddling with
bindings.

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager_browsertest.cc (right):

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager_browsertest.cc:42:
command_line->AppendSwitchASCII(switches::kEnableFeatures,
I believe the new hotness is to use ScopedFeatureList. See
https://cs.chromium.org/chromium/src/base/test/scoped_feature_list.h and
https://crbug.com/727044

[mmenke, 2017-07-13 16:15:18]

Going to land this as-is, but I think I'll switch this from a singleton to being
owned by the browser_process in a followup CL.  Most things get the
IOThread/system_request_context through the BrowserProcess, already, and
singletons are kinda ugly.

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/io_thre...
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/io_thre...
chrome/browser/io_thread.h:134: // the actual network service is disabled. See
SystemNetworkContextManager's
On 2017/07/13 01:52:29, asanka wrote:
> s/actual/out-of-process/ ?

Except it's not necessarily out of process, as John pointed out.  It's just the
one created by the ServiceManager (Which will be in process on Android,
presumably).  Reworded slightly.

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/io_thre...
chrome/browser/io_thread.h:335: // These are set on the IOThread, and then
consumed during initialization on
On 2017/07/13 01:52:29, asanka wrote:
> ... are set on the UI thread, and then ... on the IO thread.
>                    ~~~~~~~~~                      ~~~~~~~~~

Done.

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.cc (right):

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.cc:63:
content::mojom::NetworkContextParams::New();
On 2017/07/13 01:52:29, asanka wrote:
> did you mean to call CreateNetworkContextParams() here instead and use
> |context_params| in the CreateNetworkContext() call below?

Actually, I meant to just remove this line after I decided to inline the call,
and forgot to do that.  Inlined.

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager.h (right):

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager.h:63: // IOThread's
SystemURLRequestContext.
On 2017/07/13 01:52:29, asanka wrote:
> The fact that system_network_context_ is bound to the IOThread's network
context
> while io_thread_network_context_ is nil was quite confusing to me.
> 
> It would've been easier to understand if io_thread_network_context_ is
> unconditionally wrapping the IOThread's NetworkContext and have
> SystemNetworkContextManager::Context() bind via the out-of-process network
> service depending on the feature state. That way we can call this
> out_of_process_network_context_ or something.

Done.  I called it the network_service_network_context_, though.

> Also is it possible to bind out_of_process_network_context_ at
> SystemNetworkContextManager::SetUp()? If so SetUp() can return the
> NetworkContextRequest for io_thread_network_context_ and, if the feature is
> enabled, bind out_of_process_network_context_. This way
> SystemNetworkContextManager::GetContext() is ready to fire without fiddling
with
> bindings.

I tried this initially.  Unfortunately, the service manager is set up after the
IOThread is.

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager_browsertest.cc (right):

https://codereview.chromium.org/2968293002/diff/190001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager_browsertest.cc:42:
command_line->AppendSwitchASCII(switches::kEnableFeatures,
On 2017/07/13 01:52:29, asanka wrote:
> I believe the new hotness is to use ScopedFeatureList. See
> https://cs.chromium.org/chromium/src/base/test/scoped_feature_list.h and
> https://crbug.com/727044

Done.

[mmenke, 2017-07-13 16:15:44]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-07-13 16:15:44]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
jam@chromium.org, asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/2968293002/#ps210001
(title: "Response to comments")

[commit-bot: I haz the power, 2017-07-13 16:15:58]

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-07-13 18:38:30]

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

[commit-bot: I haz the power, 2017-07-13 18:38:31]

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

[mmenke, 2017-07-13 18:40:14]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-07-13 18:40:29]

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-07-13 23:27:46]

CQ is committing da patch.

Bot data: {"patchset_id": 210001, "attempt_start_ts": 1499971214717210,
"parent_rev": "ecfb7892e75a1da32e60b9ca812066b7e18d81bb", "commit_rev":
"f73a51226b54539fc543cbb29784ab036b76dc40"}

[commit-bot: I haz the power, 2017-07-13 23:28:01]

Description was changed from

==========
Introduce SystemNetworkContextManager.

The new class provides access to the SystemURLRequestContext via
an in-process NetworkService and the NetworkContext Mojo API.
Eventually, we should remove IOThread entirely, and have the
SystemNetworkContextManager perform all configuration of the
SystemRequestContext, and provide access to it as well.

The in-process network service currently just takes a
URLRequestContextBuilder from IOThread and uses that to create the
URLRequestContext, but this gives us a path to switch over
configuration so that it goes through the NetworkService API. It also
lets us switch consumers over to using the NetworkContext API without
having to create a wrapper that supports both the shiny new Mojo API
the legacy one.

When network service is enabled, the SystemNetworkContextManager will
use an out-of-process URLRequestContext instead of an in-process one,
while the IOThread will continue to use an in-process ones.

BUG=715695
==========

to

==========
Introduce SystemNetworkContextManager.

The new class provides access to the SystemURLRequestContext via
an in-process NetworkService and the NetworkContext Mojo API.
Eventually, we should remove IOThread entirely, and have the
SystemNetworkContextManager perform all configuration of the
SystemRequestContext, and provide access to it as well.

The in-process network service currently just takes a
URLRequestContextBuilder from IOThread and uses that to create the
URLRequestContext, but this gives us a path to switch over
configuration so that it goes through the NetworkService API. It also
lets us switch consumers over to using the NetworkContext API without
having to create a wrapper that supports both the shiny new Mojo API
the legacy one.

When network service is enabled, the SystemNetworkContextManager will
use an out-of-process URLRequestContext instead of an in-process one,
while the IOThread will continue to use an in-process ones.

BUG=715695

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

[commit-bot: I haz the power, 2017-07-13 23:28:02]

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