Hook up ProfileIOData's URLRequestContext to a NetworkService.

chrome/browser/net/profile_network_context_service.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_NET_PROFILE_NETWORK_CONTEXT_SERVICE_H_
+#define CHROME_BROWSER_NET_PROFILE_NETWORK_CONTEXT_SERVICE_H_
+
+#include "base/macros.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "content/public/common/network_service.mojom.h"
+
+class Profile;
+
+// KeyedService that initializes and provides access to the NetworkContexts for
+// a Profile. This will eventually replace ProfileIOData.
+class ProfileNetworkContextService : public KeyedService {
+ public:
+  explicit ProfileNetworkContextService(Profile* profile);
+  ~ProfileNetworkContextService() override;
+
+  // 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). If the network service is

[Randy Smith (Not in Mondays), 2017/07/19 19:31:10]

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.

[mmenke, 2017/07/19 20:35:38]

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)

+  // disabled, this will be the NetworkContext returned by the MainContext()
+  // method, and use by all the requests associated with a profile. If the

[Randy Smith (Not in Mondays), 2017/07/19 19:31:10]

nit: "and used" (I think).

[mmenke, 2017/07/19 20:35:38]

Done.

+  // network service is enabled, this method is sitll used, but only legacy

[Randy Smith (Not in Mondays), 2017/07/19 19:31:10]

nit: still

[mmenke, 2017/07/19 20:35:38]

Done.

+  // 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.

[Randy Smith (Not in Mondays), 2017/07/19 19:31:10]

To confirm: This means that the legacy interface (URLRequestContext) gets the
files and the new interface (NetworkContext) will have no persistence?

[mmenke, 2017/07/19 20:35:37]

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

+  //
+  // Must be called before the system NetworkContext is first used.

[Randy Smith (Not in Mondays), 2017/07/19 19:31:10]

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?

[mmenke, 2017/07/19 20:35:38]

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.

+  void SetUpProfileIODataMainContext(
+      content::mojom::NetworkContextRequest* network_context_request,
+      content::mojom::NetworkContextParamsPtr* network_context_params);
+
+  // Returns the main NetworkContext for the BrowserContext.
+  content::mojom::NetworkContext* MainContext();
+
+  // Creates the main NetworkContext for the BrowserContext. Only called when

[Randy Smith (Not in Mondays), 2017/07/19 19:31:10]

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

[mmenke, 2017/07/19 20:35:38]

Done.

+  // the network service is enabled.
+  content::mojom::NetworkContextPtr CreateMainNetworkContext();

[Randy Smith (Not in Mondays), 2017/07/19 19:31:10]

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?
 

[mmenke, 2017/07/19 20:35:38]

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

+
+ private:
+  Profile* const profile_;
+
+  // This is a NetworkContext that wraps ProfileIOData's main URLRequestContext.
+  // Always initialized in SetUpProfileIODataMainContext, but it's only returned
+  // by Context() when the network service is disabled.
+  content::mojom::NetworkContextPtr profile_io_data_main_network_context_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProfileNetworkContextService);
+};
+
+#endif  // CHROME_BROWSER_NET_PROFILE_NETWORK_CONTEXT_SERVICE_H_