Fix SitePerProcessBrowserTest.ProcessTransferAfterError to not requiring modifying the mock HostRes…

Fix SitePerProcessBrowserTest.ProcessTransferAfterError to not requiring modifying the mock HostResolver when using the network service.

This allows us to turn on the check that the host resolver isn't modified after SetUpOnMainThread in a future change.

BUG=713847
Review-Url: https://codereview.chromium.org/2847003003
Cr-Commit-Position: refs/heads/master@{#468064}
Committed: https://chromium.googlesource.com/chromium/src/+/bcc67886fb387a582da6a03263ede2487479ae21

[jam, 2017-04-28 06:38:38]

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

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

Dry run: CQ is trying da patch.

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

[jam, 2017-04-28 06:40:55]

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

[commit-bot: I haz the power, 2017-04-28 06:41:10]

Dry run: CQ is trying da patch.

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

[jam, 2017-04-28 06:41:15]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-04-28 06:51:39]

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

[commit-bot: I haz the power, 2017-04-28 06:51:39]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jam, 2017-04-28 15:34:14]

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

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

Dry run: CQ is trying da patch.

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

[jam, 2017-04-28 15:56:50]

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

[jam, 2017-04-28 15:56:51]

FWIW I have audited all the usages outside of content, and this is still the
only test that does this. I'll fix the other tests by trivial moves and then we
can enable this.

[jam, 2017-04-28 16:33:30]

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

[commit-bot: I haz the power, 2017-04-28 16:34:23]

Dry run: CQ is trying da patch.

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

[scottmg, 2017-04-28 16:42:52]

lgtm

[jam, 2017-04-28 17:01:45]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-04-28 17:01:47]

The CQ bit was checked by jam@chromium.org

[jam, 2017-04-28 17:01:50]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-04-28 17:02:40]

CQ is trying da patch.

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

[Charlie Reis, 2017-04-28 18:08:14]

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

[Charlie Reis, 2017-04-28 18:08:14]

https://codereview.chromium.org/2847003003/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2847003003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2497:
host_resolver()->AddRule("*", "127.0.0.1");
Shouldn't there be an else branch here?  How would the request succeed the
second time?

[jam, 2017-04-28 18:09:53]

https://codereview.chromium.org/2847003003/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2847003003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2497:
host_resolver()->AddRule("*", "127.0.0.1");
On 2017/04/28 18:08:14, Charlie Reis wrote:
> Shouldn't there be an else branch here?  How would the request succeed the
> second time?

The URLLoaderFactory set above is only used for one request, after that it's
reset (might want to change this behavior later, but I'll leave that to be
driven by use cases)

[Charlie Reis, 2017-04-28 18:14:52]

https://codereview.chromium.org/2847003003/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2847003003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2497:
host_resolver()->AddRule("*", "127.0.0.1");
On 2017/04/28 18:09:53, jam wrote:
> On 2017/04/28 18:08:14, Charlie Reis wrote:
> > Shouldn't there be an else branch here?  How would the request succeed the
> > second time?
> 
> The URLLoaderFactory set above is only used for one request, after that it's
> reset (might want to change this behavior later, but I'll leave that to be
> driven by use cases)

Oh, ok.  That seems a bit non-obvious, but glad it's doing the right thing for
the test, at least.

[commit-bot: I haz the power, 2017-04-28 18:20:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493398910601920,
"parent_rev": "359564ee3ab7d63ea062089c12f1a9e6a60e6170", "commit_rev":
"bcc67886fb387a582da6a03263ede2487479ae21"}

[commit-bot: I haz the power, 2017-04-28 18:20:23]

Description was changed from

==========
Fix SitePerProcessBrowserTest.ProcessTransferAfterError to not requiring
modifying the mock HostResolver when using the network service.

This allows us to turn on the check that the host resolver isn't modified after
SetUpOnMainThread in a future change.

BUG=713847
==========

to

==========
Fix SitePerProcessBrowserTest.ProcessTransferAfterError to not requiring
modifying the mock HostResolver when using the network service.

This allows us to turn on the check that the host resolver isn't modified after
SetUpOnMainThread in a future change.

BUG=713847

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

[commit-bot: I haz the power, 2017-04-28 18:20:25]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/bcc67886fb387a582da6a03263ed...

[jam, 2017-04-28 19:25:24]

https://codereview.chromium.org/2847003003/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2847003003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2497:
host_resolver()->AddRule("*", "127.0.0.1");
On 2017/04/28 18:14:52, Charlie Reis wrote:
> On 2017/04/28 18:09:53, jam wrote:
> > On 2017/04/28 18:08:14, Charlie Reis wrote:
> > > Shouldn't there be an else branch here?  How would the request succeed the
> > > second time?
> > 
> > The URLLoaderFactory set above is only used for one request, after that it's
> > reset (might want to change this behavior later, but I'll leave that to be
> > driven by use cases)
> 
> Oh, ok.  That seems a bit non-obvious, but glad it's doing the right thing for
> the test, at least.

This was documented in the test method; but point taken that it would be easier
to understand if it was persistent. I'm quite sure this method will be used a
lot more soon, so I'll take that into account as I beef it up :)