Use a MojoProxyResolverFactory on Android.

Use a MojoProxyResolverFactory on Android.

For simplicity, this CL renames and reuses the UtilityProcessMojoProxyResolverFactory to create the in-process
factory on Android, and out-of-process one everywhere else, though
the paths may need to be separated out again in the future.

This CL will let us switch to using a URLRequestContextBuilderMojo to
create URLRequestContexts.

It does not really get us any closer to hooking up
MojoProxyResolverFactories created in the browser to the network
service, which is still a ways off.

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

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

Description was changed from

==========
Make URLRequestContextBuilderV8 Mojo-only.

This allows us to use an in-process Mojo resolver on Android, making
the transition to using a network service easier (NetworkService will
havet he same Mojo interface on Android, even if it's running in
process, and will be passed a Mojo ProxyResolverFactory there as well,
just as on other platforms. Only it will create in-process
ProxyResolvers, instead of ProxyResolvers in a utility process.

Also move some proxy settings from URLRequestContextBuilderV8 to
URLRequestContextBuilder.

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

to

==========
Use URLRequestContextBuilder to create system request context.

Use defaults where possible.

Also fix URLRequestContextBuilder so that some ProxyService
options are now applied regardless of the tyoe of ProxyService
created, and make DhcpProxyScriptFetcherFactory have virtual
methods, so we can create a ChromeOS subclass of it.

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

[mmenke, 2017-06-08 19:41:52]

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

[commit-bot: I haz the power, 2017-06-08 19:42:32]

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-06-08 20:19:58]

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

[commit-bot: I haz the power, 2017-06-08 20:19:58]

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-06-08 21:23:19]

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

[commit-bot: I haz the power, 2017-06-08 21:23:39]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-06-08 21:43:11]

Patchset #4 (id:60001) has been deleted

[mmenke, 2017-06-08 21:48:08]

Description was changed from

==========
Use URLRequestContextBuilder to create system request context.

Use defaults where possible.

Also fix URLRequestContextBuilder so that some ProxyService
options are now applied regardless of the tyoe of ProxyService
created, and make DhcpProxyScriptFetcherFactory have virtual
methods, so we can create a ChromeOS subclass of it.

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

to

==========
Use a MojoProxyResolverFactory on Android.

For simplicity, this CL renames and reuses the
UtilityProcessMojoProxyResolverFactory to create the in-process
factory on Android, and out-of-process one everywhere else, though
the paths may need to be separated out again in the future.

This CL will let us switch to using a URLRequestContextBuilderMojo to
create URLRequestContexts.

It does not really get us any closer to hooking up
MojoProxyResolverFactories created in the browser to the network
service, which is still a ways off.

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

[mmenke, 2017-06-08 22:17:11]

Patchset #5 (id:100001) has been deleted

[mmenke, 2017-06-08 22:17:45]

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

[commit-bot: I haz the power, 2017-06-08 22:18:11]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-06-08 22:32:40]

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

[mmenke, 2017-06-08 22:32:41]

Note that set 4 has basically passed all tests, and set 5 is identical, just
merged, so all tests are actually passing before I sent this one out.  :)

I'm thinking that for the NetworkService, we'll use something much like this
MojoProxyResolverFactory, but it takes a ProxyResolverFactoryPtr as input, and
when it wants to shut down the service temporarily, it calls a new method on the
factory (SafeToFreeResources or somesuch).

Then we run the ProxyResolverFactory in the browser process.  On Android, it's
just the basic in-process factory.  On desktop, it starts and stops the utility
process, forward ProxyResolver creation requests to that process (So there are 3
process hops to create a ProxyResolver, but only 2 to actually use it), manages
utility process crashes, etc.

I'm not going there on this CL mostly because I want to focus on more basic
per-profile configuration of the network service first, which I'm still quite a
ways from getting to (Want to get one shared method to initialize a
URLRequestContextBuilder for IOThread and PRofileIOData, and then share it with
the network service as well, and need this CL before anything can use the
URLRequestContextBuilder).  I also want to think a bit more about the approach,
and take a look at how other consumers create utility processes, so see if that
extra hop is really needed.

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
File chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc:75:
&ChromeMojoProxyResolverFactory::DestroyFactory, base::Unretained(this)));
It's not really clear to me if a "connection error" can happen to same-process
Mojo interfaces (Run out of memory/handles or something?  I dunno).  Since it's
no extra code, figure I'll play it safe for now, and worry about that again when
it comes to actually hooking up a ProxyResolverFactory to the NetworkService.

If you prefer, though, I can just make a separate MojoProxyResolverFactory for
Android (May even just be able to share and rename the
TestMojoProxyResolverFactory)

[eroman, 2017-06-08 23:01:43]

LGTM.

Also cc-ing sammc for a sanity check, since he is a mojo expert.

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
File chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc:47: void
ChromeMojoProxyResolverFactory::CreateProcessAndConnect() {
This function name is not really applicable on the android version now (i.e.
doesn't create process).

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc:75:
&ChromeMojoProxyResolverFactory::DestroyFactory, base::Unretained(this)));
On 2017/06/08 22:32:41, mmenke wrote:
> It's not really clear to me if a "connection error" can happen to same-process
> Mojo interfaces (Run out of memory/handles or something?  I dunno).  Since
it's
> no extra code, figure I'll play it safe for now, and worry about that again
when
> it comes to actually hooking up a ProxyResolverFactory to the NetworkService.
> 
> If you prefer, though, I can just make a separate MojoProxyResolverFactory for
> Android (May even just be able to share and rename the
> TestMojoProxyResolverFactory)

From an abstraction perspective seems like should always handle connection
errors, so I would say this is desirable as written.

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc:88: // If there's still
no factory, then factor creation failed so
factory

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc:118: // On desktop,
where a utliity process is used, if the
utility

[commit-bot: I haz the power, 2017-06-08 23:59:01]

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

[commit-bot: I haz the power, 2017-06-08 23:59:03]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-06-09 15:20:41]

Thanks for the feedback!  I'll plan to tonight or Monday unless sammc chimes in
with comments.

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
File chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc:47: void
ChromeMojoProxyResolverFactory::CreateProcessAndConnect() {
On 2017/06/08 23:01:42, eroman wrote:
> This function name is not really applicable on the android version now (i.e.
> doesn't create process).

Good point.  Renamed.  (Also reordered things so they match the order in the
header)

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc:88: // If there's still
no factory, then factor creation failed so
On 2017/06/08 23:01:43, eroman wrote:
> factory

Done.  Also shorted the sentence a bit (4 clauses is a bit much).

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc:118: // On desktop,
where a utliity process is used, if the
On 2017/06/08 23:01:42, eroman wrote:
> utility

Done.  Also re-flowed comment (Not sure why git cl format didn't do that itself)

[mmenke, 2017-06-09 15:25:02]

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

[commit-bot: I haz the power, 2017-06-09 15:25:28]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-06-09 15:26:50]

Response to another comment.

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
File chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/2930743002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_mojo_proxy_resolver_factory.cc:75:
&ChromeMojoProxyResolverFactory::DestroyFactory, base::Unretained(this)));
On 2017/06/08 23:01:43, eroman wrote:
> On 2017/06/08 22:32:41, mmenke wrote:
> > It's not really clear to me if a "connection error" can happen to
same-process
> > Mojo interfaces (Run out of memory/handles or something?  I dunno).  Since
> it's
> > no extra code, figure I'll play it safe for now, and worry about that again
> when
> > it comes to actually hooking up a ProxyResolverFactory to the
NetworkService.
> > 
> > If you prefer, though, I can just make a separate MojoProxyResolverFactory
for
> > Android (May even just be able to share and rename the
> > TestMojoProxyResolverFactory)
> 
> From an abstraction perspective seems like should always handle connection
> errors, so I would say this is desirable as written.

I don't think it matters from an abstraction perspective, actually - this class
is the one that chooses to run it in process, so seems like it can strongly
depend on running it in-process on Android.

Other classes, of course, should not depend on that.

[commit-bot: I haz the power, 2017-06-09 15:37:00]

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

[commit-bot: I haz the power, 2017-06-09 15:37:01]

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-06-09 16:32:33]

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

[commit-bot: I haz the power, 2017-06-09 16:32:53]

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-06-09 16:44:02]

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

[commit-bot: I haz the power, 2017-06-09 16:44:03]

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

[mmenke, 2017-06-09 16:53:14]

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

[commit-bot: I haz the power, 2017-06-09 16:53:26]

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-06-09 18:18:28]

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

[commit-bot: I haz the power, 2017-06-09 18:18:29]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-06-09 22:16:51]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-06-09 22:16:52]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2930743002/#ps180001
(title: "Oops")

[commit-bot: I haz the power, 2017-06-09 22:17:16]

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-06-09 22:22:08]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1497046611529650,
"parent_rev": "6ef29daef4aa32b5c744c645891ac4595bc4d073", "commit_rev":
"1bb4d98b866875041d80b5134eff3e64529382d5"}

[commit-bot: I haz the power, 2017-06-09 22:22:20]

Description was changed from

==========
Use a MojoProxyResolverFactory on Android.

For simplicity, this CL renames and reuses the
UtilityProcessMojoProxyResolverFactory to create the in-process
factory on Android, and out-of-process one everywhere else, though
the paths may need to be separated out again in the future.

This CL will let us switch to using a URLRequestContextBuilderMojo to
create URLRequestContexts.

It does not really get us any closer to hooking up
MojoProxyResolverFactories created in the browser to the network
service, which is still a ways off.

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

to

==========
Use a MojoProxyResolverFactory on Android.

For simplicity, this CL renames and reuses the
UtilityProcessMojoProxyResolverFactory to create the in-process
factory on Android, and out-of-process one everywhere else, though
the paths may need to be separated out again in the future.

This CL will let us switch to using a URLRequestContextBuilderMojo to
create URLRequestContexts.

It does not really get us any closer to hooking up
MojoProxyResolverFactories created in the browser to the network
service, which is still a ways off.

BUG=715695

Review-Url: https://codereview.chromium.org/2930743002
Cr-Commit-Position: refs/heads/master@{#478435}
Committed:
https://chromium.googlesource.com/chromium/src/+/1bb4d98b866875041d80b5134eff...
==========

[commit-bot: I haz the power, 2017-06-09 22:22:22]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/1bb4d98b866875041d80b5134eff...