Hook up ProfileIOData's URLRequestContext to a NetworkService.

Hook up ProfileIOData's URLRequestContext to a NetworkService.

This CL adds ProfileNetworkContextService, a BrowserContext
KeyedService, that wraps ProfileIOData's URLRequestContext in a
NetworkContext (Which ProfileIOData hooks up to IOThread's in-process
NetworkService), and provides NetworkContext configuration parameters
for it as well.

When the network service is disabled, the new service provides direct
access to the net NetworkContext. When the network service is enabled,
the new service configures and provides acess to the StoragePartition's
NetworkContext instance instead (Though it still sets up a
NetworkContext for ProfileIOData's URLRequestContext as well).

Bug=715695
Review-Url: https://codereview.chromium.org/2976323002
Cr-Commit-Position: refs/heads/master@{#488893}
Committed: https://chromium.googlesource.com/chromium/src/+/0d1d09c3691ba940649051a8d172b356cc1ef7b1

[mmenke, 2017-07-17 21:51:19]

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

[commit-bot: I haz the power, 2017-07-17 21:51:28]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-17 22:04:30]

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

[commit-bot: I haz the power, 2017-07-17 22:04:31]

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

[mmenke, 2017-07-17 22:07:55]

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

[commit-bot: I haz the power, 2017-07-17 22:08:06]

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-17 22:24:27]

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

[commit-bot: I haz the power, 2017-07-17 22:24:28]

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

[mmenke, 2017-07-18 15:32:53]

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

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

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-18 15:44:36]

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

[commit-bot: I haz the power, 2017-07-18 15:44:41]

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-18 16:01:32]

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

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

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

[mmenke, 2017-07-18 16:50:01]

Patchset #3 (id:40001) has been deleted

[mmenke, 2017-07-18 16:53:31]

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

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

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-18 17:02:25]

Patchset #3 (id:60001) has been deleted

[mmenke, 2017-07-18 17:03:52]

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

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

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-18 17:29:48]

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

[commit-bot: I haz the power, 2017-07-18 17:29: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-07-18 18:05:31]

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

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

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

[mmenke, 2017-07-18 18:05:55]

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

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

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

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

[commit-bot: I haz the power, 2017-07-18 18:21: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-07-18 18:41:04]

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

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

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

[mmenke, 2017-07-18 18:57:06]

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

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

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

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

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

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-18 19:52:50]

Patchset #5 (id:120001) has been deleted

[mmenke, 2017-07-18 19:53:14]

Patchset #6 (id:160001) has been deleted

[mmenke, 2017-07-18 19:53:24]

Patchset #5 (id:140001) has been deleted

[mmenke, 2017-07-18 20:02:37]

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

[mmenke, 2017-07-18 20:02:39]

Hopefully this will be the last of my huge CLs for a while (Modulo ones that
have a lot of test code).

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/net...
File chrome/browser/net/network_context_configuration_browsertest.cc (right):

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/net...
chrome/browser/net/network_context_configuration_browsertest.cc:44: // Tests the
system, profile, and incognito profile NetworkContexts.
Think having one set of tests for these will help ensure that the configuration
of each is reasonably well tested.

[mmenke, 2017-07-18 20:04:01]

And if anyone's wondering why it's using rietveld, that's because it's based on
top of another rietveld CL, and it was simpler to just use rietveld, rather than
move the other CL over, once I realized this would be a problem.

[Randy Smith (Not in Mondays), 2017-07-18 20:13:57]

Would you like both of us to review the whole thing, or are you expecting
specialization?

[mmenke, 2017-07-18 20:16:26]

On 2017/07/18 20:13:57, Randy Smith (Not in Mondays) wrote:
> Would you like both of us to review the whole thing, or are you expecting
> specialization?

Sorry about that.

[rdsmith]:  Please review review chrome/
[jam]:  Please review content/

[jam, 2017-07-18 20:26:27]

nit: can you add comments to NetworkContext to explain how it's used in the
scenarios that network service is enabled/disabled?

https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
content/public/browser/content_browser_client.cc:497: return
ContentBrowserClient::CreateMainNetworkContext(context);
should this be setting up NetworkContextParams and calling 
GetNetworkService()->CreateNetworkContext it?

https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
content/public/browser/content_browser_client.h:836: // Creates the main
NetworkContext for a BrowserContext's StoragePartition. It
I'm having trouble seeing these two methods descriptions and understanding the
difference. Would it be enough to have one method that takes in the file path?
The callers could then concatenate the relative + profile path.

[commit-bot: I haz the power, 2017-07-18 20:34:25]

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

[commit-bot: I haz the power, 2017-07-18 20:34:26]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-07-18 20:49:52]

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

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

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-18 20:56:00]

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

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

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-07-18 20:57:34]

On 2017/07/18 20:26:27, jam wrote:
> nit: can you add comments to NetworkContext to explain how it's used in the
> scenarios that network service is enabled/disabled?

Which NetworkContext are you suggesting I add comments to?  I've beefed up/added
a number of comments in my changes to content/, but I may not have covered the
spot you were thinking of.

https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
content/public/browser/content_browser_client.cc:497: return
ContentBrowserClient::CreateMainNetworkContext(context);
On 2017/07/18 20:26:27, jam wrote:
> should this be setting up NetworkContextParams and calling 
> GetNetworkService()->CreateNetworkContext it?

Moot point, since I've merged it with CreateMainNetworkContext.

https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
content/public/browser/content_browser_client.h:836: // Creates the main
NetworkContext for a BrowserContext's StoragePartition. It
On 2017/07/18 20:26:27, jam wrote:
> I'm having trouble seeing these two methods descriptions and understanding the
> difference. Would it be enough to have one method that takes in the file path?
> The callers could then concatenate the relative + profile path.

I'm not sufficiently familiar with StoragePartitions to know if that's enough. 
I'll combine them for now, and we can separate them if needed.

[mmenke, 2017-07-19 15:29:51]

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

[commit-bot: I haz the power, 2017-07-19 15:29:55]

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

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

[commit-bot: I haz the power, 2017-07-19 18:04:12]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2017-07-19 19:31:11]

I'm afraid this is going to take a couple of rounds, mostly because there's a
learning curve I didn't realize I needed to climb :-}.  

It looks to me as if, in the non-network service enabled case, you've created a
NetworkContext associated with each ProfileIOData, but that that isn't
accessible anywhere.  It could be gotten through the
ProfileNetworkContextService::MainContext, but nothing currently does.  Is that
correct?  

So the primary effect of this CL is to shift the building and ownership of the
network context in the enable-network-service case over to the profile keyed
service, and to setup hooks that can be used to get a profile network context in
the non-enable-network-service case, but not yet use them.  That right?

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
File chrome/browser/net/profile_network_context_service.h (right):

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:23: // the IOThread's
in-process NetworkService). If the network service is
I think this description ("which the caller ...") only applies to the network
service being disabled, but its placement in the paragraph implies it applies to
both disabled and enabled situations.

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:25: // method, and use by
all the requests associated with a profile. If the
nit: "and used" (I think).

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:26: // network service is
enabled, this method is sitll used, but only legacy
nit: still

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:30: // storage, as the
network service will be using those files instead.
To confirm: This means that the legacy interface (URLRequestContext) gets the
files and the new interface (NetworkContext) will have no persistence?

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:32: // Must be called
before the system NetworkContext is first used.
Hmmm.  I'm having a hard time wrapping my brain around the message of this
interface contract.  Looking at the code, I think what's going on is that this
routine is setting up two parameters that will be used in configuring a
NetworkContext in the context of some NetworkService: A params argument to
define the configuration, and a Request end of a mojo pipe, to be bound to the
implementation of the Context.   It does do things differently in the service
enabled/disabled case--specifically, in the enabled case it sets up the params
to be an in-memory NetworkContext, to leave the legacy URLRequestContext alone. 


Is that accurate?  If so, I'd find documentation patterned along those lines
(output args to be used in creating a NetworkService, description of output
args, information about feature conditionalization).  Willing to consider that?

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:40: // Creates the main
NetworkContext for the BrowserContext. Only called when
nit, suggestion: "May only be called".  I take header file comments as
specifying conditions for the consumer, not as describing status of consumer
code (which is out of its control).

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:42:
content::mojom::NetworkContextPtr CreateMainNetworkContext();
As might be suggested by my confused questions over IM, I'm not sure this
interface is going to be intuitive to people who aren't steeped in Mojo.  A
couple of points, happy to have pushback since this is clearly partially based
in my confusion:

* It isn't clear that CreateMainNetworkContext() is passing back an owning
pointer that the caller will need to store somewhere.  

* By the interface names it sounds like the same entity is being returned by
both of these two.  But it really doesn't look like
CreateMainNetworkContext().get() will return the same thing as MainContext(). 
Maybe the documentation could clarify the difference between those two entities?

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
File chrome/browser/net/profile_network_context_service_factory.cc (right):

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service_factory.cc:38: // Create
separate service for incognito profiles.
Is this a TODO?

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
File chrome/browser/net/profile_network_context_service_factory.h (right):

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service_factory.h:23: // |profile|.
nit: Comment doesn't match actual argument.

[mmenke, 2017-07-19 20:32:31]

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

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

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-19 20:35:38]

Thanks for the feedback!

On 2017/07/19 19:31:11, Randy Smith (Not in Mondays) wrote:
> I'm afraid this is going to take a couple of rounds, mostly because there's a
> learning curve I didn't realize I needed to climb :-}.  
> 
> It looks to me as if, in the non-network service enabled case, you've created
a
> NetworkContext associated with each ProfileIOData, but that that isn't
> accessible anywhere.  It could be gotten through the
> ProfileNetworkContextService::MainContext, but nothing currently does.  Is
that
> correct?  

I think those two sentences are contradictory.  It is accessible (To chrome
code) through ProfileNetworkContextService::MainContext, but it isn't currently
accessed anywhere, other than in tests.  It is not accessible in content/, nor
is it accessible when the network service is enabled.

> So the primary effect of this CL is to shift the building and ownership of the
> network context in the enable-network-service case over to the profile keyed
> service, and to setup hooks that can be used to get a profile network context
in
> the non-enable-network-service case, but not yet use them.  That right?

Mostly right.  In the service enabled case, the NetworkService is still owned by
StoragePartition, but the ProfileKeyedService will eventually own chrome-side
configuration objects that talk to it (proxy service, things that watch prefs,
unless we use a pref service for those, etc).

Conveniently, it shouldn't matter which is destroyed first, the KeyedService or
the StoragePartition, once we start hooking things up, but that's a concern for
another day, anyways.

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
File chrome/browser/net/profile_network_context_service.h (right):

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:23: // the IOThread's
in-process NetworkService). If the network service is
On 2017/07/19 19:31:10, Randy Smith (Not in Mondays) wrote:
> I think this description ("which the caller ...") only applies to the network
> service being disabled, but its placement in the paragraph implies it applies
to
> both disabled and enabled situations.

It applies to both cases.  When the network service is disabled, we still need
an in-process URLRequestContext for legacy code, and it's still set up through
the IOThread's NetworkService (Though it will need to use different
configuration parameters, to avoid fighting over file access)

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:25: // method, and use by
all the requests associated with a profile. If the
On 2017/07/19 19:31:10, Randy Smith (Not in Mondays) wrote:
> nit: "and used" (I think).

Done.

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:26: // network service is
enabled, this method is sitll used, but only legacy
On 2017/07/19 19:31:10, Randy Smith (Not in Mondays) wrote:
> nit: still

Done.

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:30: // storage, as the
network service will be using those files instead.
On 2017/07/19 19:31:10, Randy Smith (Not in Mondays) wrote:
> To confirm: This means that the legacy interface (URLRequestContext) gets the
> files and the new interface (NetworkContext) will have no persistence?

Other way around, so we can test the new interface is correct set up to use the
cache and the like.

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:32: // Must be called
before the system NetworkContext is first used.
On 2017/07/19 19:31:10, Randy Smith (Not in Mondays) wrote:
> Hmmm.  I'm having a hard time wrapping my brain around the message of this
> interface contract.  Looking at the code, I think what's going on is that this
> routine is setting up two parameters that will be used in configuring a
> NetworkContext in the context of some NetworkService: A params argument to
> define the configuration, and a Request end of a mojo pipe, to be bound to the
> implementation of the Context.   It does do things differently in the service
> enabled/disabled case--specifically, in the enabled case it sets up the params
> to be an in-memory NetworkContext, to leave the legacy URLRequestContext
alone. 
> 
> 
> Is that accurate?  If so, I'd find documentation patterned along those lines
> (output args to be used in creating a NetworkService, description of output
> args, information about feature conditionalization).  Willing to consider
that?

That's what the above paragraph was attempting to explain:

"Initializes |network_context_params| as needed to set up the
ProfileIOData's main NetworkContext (Which the caller will need to send to
the IOThread's in-process NetworkService)."

-> attempts to explain it applies only to legacy URLRequestContext (That
NetworkContext is a typo, switched to URLRequestContext)

"If the network service is enabled, this method is still used, but only legacy
requests going through the URLRequestContext interface instead of the
NetworkContext interface will use the profile's NetworkContext, and the
ProfileIOData's main network context will be configured not to use on-disk
storage, as the network service will be using those files instead."

->  Attempts to summarize conditionalization behavior and rationale (Have now
replaced profile's NetworkContext with ProfileIOData's URLRequestContext, to try
to improve it a little).

I'm not sure how to improve on that, but I'm open to suggestions.  Part of the
problem is we have "profile" "ProfileIOData", two "main" contexts, two
NetworkContexts and a URLRequestContext, two NetworkService's (one out of
process) and a network service, which may be enabled and disabled.

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:40: // Creates the main
NetworkContext for the BrowserContext. Only called when
On 2017/07/19 19:31:10, Randy Smith (Not in Mondays) wrote:
> nit, suggestion: "May only be called".  I take header file comments as
> specifying conditions for the consumer, not as describing status of consumer
> code (which is out of its control).

Done.

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:42:
content::mojom::NetworkContextPtr CreateMainNetworkContext();
On 2017/07/19 19:31:10, Randy Smith (Not in Mondays) wrote:
> As might be suggested by my confused questions over IM, I'm not sure this
> interface is going to be intuitive to people who aren't steeped in Mojo.  A
> couple of points, happy to have pushback since this is clearly partially based
> in my confusion:
> 
> * It isn't clear that CreateMainNetworkContext() is passing back an owning
> pointer that the caller will need to store somewhere.  

I don't think every use of a mojom::*Ptr should have documentation that it's an
owning pointer, any more than std::unique_ptr does.  If this is a problem, this
seems the wrong layer to handle it at - the issue is the Mojo naming scheme, and
it should be fixed there.

I'm fine with using mojo::InterfacePtr<content::mojom::NetworkContext>
explicitly instead, though not sure how common that notation is.

> * By the interface names it sounds like the same entity is being returned by
> both of these two.  But it really doesn't look like
> CreateMainNetworkContext().get() will return the same thing as MainContext(). 
> Maybe the documentation could clarify the difference between those two
entities?

It actually will return the same thing, when the network service is enabled,
with the caveat that the result of CreateMainNetworkContext() must be passed to
the StoragePartition - this is basically part of a private interface that no one
other than ChromeContentBrowserClient should be calling.  Once it actually does
something interesting (Stashes a URLLoaderFactory, starts watching prefs, etc),
we can make it DCHECK if it's called more than once, but now, we can't, without
adding a state variable solely for the purpose of DCHECKing.

Anyhow, I've tried to clarify usage.

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
File chrome/browser/net/profile_network_context_service_factory.cc (right):

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service_factory.cc:38: // Create
separate service for incognito profiles.
On 2017/07/19 19:31:10, Randy Smith (Not in Mondays) wrote:
> Is this a TODO?

No, that's what this line of code does.  The default implementation either
returns the main context given an incognito one or just returns nullptr in the
incognito case (Can't recall which).  This returns the (possibly incognito)
profile passed in to it, indicating that incognito BrowserContexts are lucky
enough to get their own service.

No test currently verifies they're different, but once I add cache configuration
tests and the like, those will give us some coverage, at least.

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
File chrome/browser/net/profile_network_context_service_factory.h (right):

https://codereview.chromium.org/2976323002/diff/170001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service_factory.h:23: // |profile|.
On 2017/07/19 19:31:11, Randy Smith (Not in Mondays) wrote:
> nit: Comment doesn't match actual argument.

Done.

[jam, 2017-07-19 20:42:54]

lgmt

On 2017/07/18 20:57:34, mmenke wrote:

> On 2017/07/18 20:26:27, jam wrote:
> > nit: can you add comments to NetworkContext to explain how it's used in the
> > scenarios that network service is enabled/disabled?
> 
> Which NetworkContext are you suggesting I add comments to?  

I meant content/network/network_context.h

>I've beefed up/added
> a number of comments in my changes to content/, but I may not have covered the
> spot you were thinking of.
> 
>
https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
> File content/public/browser/content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
> content/public/browser/content_browser_client.cc:497: return
> ContentBrowserClient::CreateMainNetworkContext(context);
> On 2017/07/18 20:26:27, jam wrote:
> > should this be setting up NetworkContextParams and calling 
> > GetNetworkService()->CreateNetworkContext it?
> 
> Moot point, since I've merged it with CreateMainNetworkContext.
> 
>
https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
> File content/public/browser/content_browser_client.h (right):
> 
>
https://codereview.chromium.org/2976323002/diff/170001/content/public/browser...
> content/public/browser/content_browser_client.h:836: // Creates the main
> NetworkContext for a BrowserContext's StoragePartition. It
> On 2017/07/18 20:26:27, jam wrote:
> > I'm having trouble seeing these two methods descriptions and understanding
the
> > difference. Would it be enough to have one method that takes in the file
path?
> > The callers could then concatenate the relative + profile path.
> 
> I'm not sufficiently familiar with StoragePartitions to know if that's enough.

> I'll combine them for now, and we can separate them if needed.

[mmenke, 2017-07-19 21:02:25]

On 2017/07/19 20:42:54, jam wrote:
> lgmt

I think this way a typo?  :)

> On 2017/07/18 20:57:34, mmenke wrote:
> 
> > On 2017/07/18 20:26:27, jam wrote:
> > > nit: can you add comments to NetworkContext to explain how it's used in
the
> > > scenarios that network service is enabled/disabled?
> > 
> > Which NetworkContext are you suggesting I add comments to?  
> 
> I meant content/network/network_context.h

Should have realized that.  Done.

[jam, 2017-07-19 21:57:36]

On 2017/07/19 21:02:25, mmenke wrote:
> On 2017/07/19 20:42:54, jam wrote:
> > lgmt
> 
> I think this way a typo?  :)

agh sorry
lgtm

> 
> > On 2017/07/18 20:57:34, mmenke wrote:
> > 
> > > On 2017/07/18 20:26:27, jam wrote:
> > > > nit: can you add comments to NetworkContext to explain how it's used in
> the
> > > > scenarios that network service is enabled/disabled?
> > > 
> > > Which NetworkContext are you suggesting I add comments to?  
> > 
> > I meant content/network/network_context.h
> 
> Should have realized that.  Done.

[mmenke, 2017-07-19 21:59:00]

On 2017/07/19 21:57:36, jam wrote:
> On 2017/07/19 21:02:25, mmenke wrote:
> > On 2017/07/19 20:42:54, jam wrote:
> > > lgmt
> > 
> > I think this way a typo?  :)
> 
> agh sorry
> lgtm

Thanks!  And of course, I make a typo myself when asking about it.  :)

[Randy Smith (Not in Mondays), 2017-07-20 17:38:34]

So I'm happy to leave everything below to your judgement, so LGTM conditional on
my "Confirming this TODO" question (because if I'm wrong, it means there's
something I'm not understanding that might leave to more need for review).  

But I'm also happy to have more rounds with you on documentation (or anything
else you'd like to hash out) if that would have value to you.

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2700: // TODO(mmenke): 
Implement this.
nit, suggestion: Enhance TODO to indicate what's missing from this path.

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2700: // TODO(mmenke): 
Implement this.
Confirming that this TODO is basically parallel to the TODO that used to be in
StoragePartitionImpl::Create() around filling in context_param?

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/pro...
File chrome/browser/net/profile_network_context_service.h (right):

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:31: // instead.
Ok, I do find this clearer.  It's still somewhat hard going for me, but I'm not
sure if that's a failure of comprehension on my part, or documentation changes
would be useful in general.  So the following are suggestions, take them or not
as you wish.

* Change the first sentence to mention network_context_request.  

* Add after that sentence something that makes explicit that
|*network_context_request| is one end of a pipe and the class is retaining the
other end.

* Change the "If the network service is enabled" sentence to be parallel in
structure with the disabled sentence. 

I envision something like:

----
Initializes |*network_context_params| as needed to set up the ProfileIOData's
main URLRequestContext and |*network_context_request| as one end of a Mojo pipe
to be bound to the NetworkContext for that URLRequestContext.  (The caller will
need to send these parameters to the IOThread's in-process NetworkService.) .
The class retains the other end of the |*network_context_request| pipe for later
vending.

If the network service is disabled, MainContext() will return that end of the
pipe.  In this case, all requests associated with this profile will use the
associated URLRequestContext (either through MainContext() or directly).  

If the network service is enabled, MainContext() will instead return a network
context vended by the NetworkService.  In this case, the ProfileIOData's
URLRequest context will be configured not to use on-disk storage (so as not to
conflict with the NetworkService vended context), and will only be used for
legacy requests.  

Must be called before the system NetworkContext is first used.
---

but up to you.

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:38: // Returns the main
NetworkContext for the BrowserContext.
suggestion: Add "If the network service is disabled, this will be the
ProfileIOData NetworkContext setup above.  Otherwise, it will be a
NetworkContext vended from the network service."

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:41: // Creates the main
NetworkContext for the BrowserContext. May only be called
Suggest adding to first sentence ", by requesting it from the network service."

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager_browsertest.cc (left):

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager_browsertest.cc:5: #include
"chrome/browser/net/system_network_context_manager.h"
So I'm puzzled by the deletion of this file.  I can't find it in ToT, and your
CL doesn't appear to remove it from any lists of tests.  Is this actually part
of your CL, or is it a ghost of some sort?

https://codereview.chromium.org/2976323002/diff/230001/content/network/networ...
File content/network/network_context.h (right):

https://codereview.chromium.org/2976323002/diff/230001/content/network/networ...
content/network/network_context.h:39: // URLRequestContext. When that happens,
the consumer takes owners the
nit: owners -> ownership of

https://codereview.chromium.org/2976323002/diff/230001/content/network/networ...
content/network/network_context.h:41: // responsible for destroying it before
the NetworkService.
Thank you for this--when I first hit this file (earlier in the CL) there was
some frustration over the lack of these paragraphs.

[mmenke, 2017-07-20 18:31:31]

Happy to spend more time hashing out comments, but unless I hear back from you,
I'll plan to land it on Monday (Today's branch, and while I'm pretty confident
this will stick, the Friday after branch just doesn't seem a great time for big
patches).

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2700: // TODO(mmenke): 
Implement this.
On 2017/07/20 17:38:34, Randy Smith (Not in Mondays) wrote:
> nit, suggestion: Enhance TODO to indicate what's missing from this path.

Done.  Left this pretty general ("Implement this once
ProfileNetworkContextServiceFactory can create a fully functional NetworkContext
for Apps when the network service is disabled.").  We could implement it before
then, but I'm not sure how much benefit there would be.

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2700: // TODO(mmenke): 
Implement this.
On 2017/07/20 17:38:34, Randy Smith (Not in Mondays) wrote:
> Confirming that this TODO is basically parallel to the TODO that used to be in
> StoragePartitionImpl::Create() around filling in context_param?

Not quite analogous - I'm not sure it will even make sense to populate them in
content/ (As opposed to having individual embedders populate them), though not
at all certain of that.

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/pro...
File chrome/browser/net/profile_network_context_service.h (right):

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:31: // instead.
On 2017/07/20 17:38:34, Randy Smith (Not in Mondays) wrote:
> Ok, I do find this clearer.  It's still somewhat hard going for me, but I'm
not
> sure if that's a failure of comprehension on my part, or documentation changes
> would be useful in general.  So the following are suggestions, take them or
not
> as you wish.
> 
> * Change the first sentence to mention network_context_request.  
> 
> * Add after that sentence something that makes explicit that
> |*network_context_request| is one end of a pipe and the class is retaining the
> other end.
> 
> * Change the "If the network service is enabled" sentence to be parallel in
> structure with the disabled sentence. 
> 
> I envision something like:
> 
> ----
> Initializes |*network_context_params| as needed to set up the ProfileIOData's
> main URLRequestContext and |*network_context_request| as one end of a Mojo
pipe
> to be bound to the NetworkContext for that URLRequestContext.  (The caller
will
> need to send these parameters to the IOThread's in-process NetworkService.) .
> The class retains the other end of the |*network_context_request| pipe for
later
> vending.
> 
> If the network service is disabled, MainContext() will return that end of the
> pipe.  In this case, all requests associated with this profile will use the
> associated URLRequestContext (either through MainContext() or directly).  
> 
> If the network service is enabled, MainContext() will instead return a network
> context vended by the NetworkService.  In this case, the ProfileIOData's
> URLRequest context will be configured not to use on-disk storage (so as not to
> conflict with the NetworkService vended context), and will only be used for
> legacy requests.  
> 
> Must be called before the system NetworkContext is first used.
> ---
> 
> but up to you.
> 

Thanks so much for the detailed text - I think it's a lot better, and have
switched to it (With a minor change or two).

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:38: // Returns the main
NetworkContext for the BrowserContext.
On 2017/07/20 17:38:34, Randy Smith (Not in Mondays) wrote:
> suggestion: Add "If the network service is disabled, this will be the
> ProfileIOData NetworkContext setup above.  Otherwise, it will be a
> NetworkContext vended from the network service."

Done.

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/pro...
chrome/browser/net/profile_network_context_service.h:41: // Creates the main
NetworkContext for the BrowserContext. May only be called
On 2017/07/20 17:38:34, Randy Smith (Not in Mondays) wrote:
> Suggest adding to first sentence ", by requesting it from the network
service."

Done (Added "using the network service", "requesting" has a bit of an
implication of requesting something that already exists)

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/sys...
File chrome/browser/net/system_network_context_manager_browsertest.cc (left):

https://codereview.chromium.org/2976323002/diff/230001/chrome/browser/net/sys...
chrome/browser/net/system_network_context_manager_browsertest.cc:5: #include
"chrome/browser/net/system_network_context_manager.h"
On 2017/07/20 17:38:34, Randy Smith (Not in Mondays) wrote:
> So I'm puzzled by the deletion of this file.  I can't find it in ToT, and your
> CL doesn't appear to remove it from any lists of tests.  Is this actually part
> of your CL, or is it a ghost of some sort?

This was moved into NetworkContextConfigurationBrowserTest, which includes
running this test on the SystemNetworkContextManager, but also on two Profile
NetworkContexts (Note that it's an "A+" file, showing this one as the original
file).  Having one browsertest file test two different classes is a bit unusual,
but it ensures we run the same set of configuration tests on all the main
NetworkContexts, so think it's the way to go (We could also go with templated
tests, but that seems unnecessary, in this case).

https://codereview.chromium.org/2976323002/diff/230001/content/network/networ...
File content/network/network_context.h (right):

https://codereview.chromium.org/2976323002/diff/230001/content/network/networ...
content/network/network_context.h:39: // URLRequestContext. When that happens,
the consumer takes owners the
On 2017/07/20 17:38:34, Randy Smith (Not in Mondays) wrote:
> nit: owners -> ownership of

Done.

https://codereview.chromium.org/2976323002/diff/230001/content/network/networ...
content/network/network_context.h:41: // responsible for destroying it before
the NetworkService.
On 2017/07/20 17:38:34, Randy Smith (Not in Mondays) wrote:
> Thank you for this--when I first hit this file (earlier in the CL) there was
> some frustration over the lack of these paragraphs.

No problem, though credit goes to John for suggesting adding comments here.

[mmenke, 2017-07-20 18:31:53]

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

[commit-bot: I haz the power, 2017-07-20 18:32:15]

Dry run: CQ is trying da patch.

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

[Randy Smith (Not in Mondays), 2017-07-20 18:33:11]

Still LGTM.  Thanks!

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

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

[commit-bot: I haz the power, 2017-07-20 20:13:32]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-07-23 18:11:43]

On 2017/07/20 18:33:11, Randy Smith (Not in Mondays) wrote:
> Still LGTM.  Thanks!

Landing now - some of the stuff I'll be working on next week depends on this, so
life is simplest if I have this landed before then.  If some reason the branch
61 cut ends up being later than expected, I'll revert this from M61.

[mmenke, 2017-07-23 18:11:50]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-07-23 18:11:50]

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/2976323002/#ps250001
(title: "Response to comments")

[commit-bot: I haz the power, 2017-07-23 18:11:54]

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-23 20:19:16]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1500833510275930,
"parent_rev": "d47528294061f99e303dc031b86b0f7179cec4a5", "commit_rev":
"0d1d09c3691ba940649051a8d172b356cc1ef7b1"}

[commit-bot: I haz the power, 2017-07-23 20:19:28]

Description was changed from

==========
Hook up ProfileIOData's URLRequestContext to a NetworkService.

This CL adds ProfileNetworkContextService, a BrowserContext
KeyedService, that wraps ProfileIOData's URLRequestContext in a
NetworkContext (Which ProfileIOData hooks up to IOThread's in-process
NetworkService), and provides NetworkContext configuration parameters
for it as well.

When the network service is disabled, the new service provides direct
access to the net NetworkContext. When the network service is enabled,
the new service configures and provides acess to the StoragePartition's
NetworkContext instance instead (Though it still sets up a
NetworkContext for ProfileIOData's URLRequestContext as well).

Bug=715695
==========

to

==========
Hook up ProfileIOData's URLRequestContext to a NetworkService.

This CL adds ProfileNetworkContextService, a BrowserContext
KeyedService, that wraps ProfileIOData's URLRequestContext in a
NetworkContext (Which ProfileIOData hooks up to IOThread's in-process
NetworkService), and provides NetworkContext configuration parameters
for it as well.

When the network service is disabled, the new service provides direct
access to the net NetworkContext. When the network service is enabled,
the new service configures and provides acess to the StoragePartition's
NetworkContext instance instead (Though it still sets up a
NetworkContext for ProfileIOData's URLRequestContext as well).

Bug=715695

Review-Url: https://codereview.chromium.org/2976323002
Cr-Commit-Position: refs/heads/master@{#488893}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d1d09c3691ba940649051a8d172...
==========

[commit-bot: I haz the power, 2017-07-23 20:19:30]

Committed patchset #9 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/0d1d09c3691ba940649051a8d172...