Sync the browser test host resolver with the network process before a test runs.

Sync the browser test host resolver with the network process before a test runs.

This fixes the browser tests which use the in-process host resolver, which depends on globals in the network code.

Most of the tests already call host_resolver()->AddRule in their SetUpOnMainThread. This will be enforced once the remaining ones which don't do this are updated.

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

[jam, 2017-04-24 21:12:29]

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

[commit-bot: I haz the power, 2017-04-24 21:12:48]

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

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

[commit-bot: I haz the power, 2017-04-24 21:18:07]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jam, 2017-04-24 21:31:44]

Description was changed from

==========
Sync the browser test host resolver with the network process before a test runs.

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

to

==========
Sync the browser test host resolver with the network process before a test runs.

This fixes the browser tests which use the in-process host resolver, which
depends on globals in the network code.

Most of the tests already call host_resolver()->AddRule in their
SetUpOnMainThread. This will be enforced once the remaining ones which don't do
this are updated.

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

[jam, 2017-04-24 21:31:44]

jam@chromium.org changed reviewers:
+ mmenke@chromium.org, rockot@chromium.org, tsepez@chromium.org

[jam, 2017-04-24 21:32:26]

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

[jam, 2017-04-24 21:32:29]

Patchset #1 (id:1) has been deleted

[jam, 2017-04-24 21:33:27]

Matt: net/
Tom: mojom & manifest
Ken: the rest. This uses the temporary hooks we discussed until the service
manager has a way to hook this.

[commit-bot: I haz the power, 2017-04-24 21:33:27]

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-04-24 21:45:48]

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

[commit-bot: I haz the power, 2017-04-24 21:45:49]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-04-24 22:01:54]

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

[commit-bot: I haz the power, 2017-04-24 22:03:39]

Dry run: CQ is trying da patch.

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

[Tom Sepez, 2017-04-24 22:14:39]

On 2017/04/24 21:33:27, jam wrote:
> Matt: net/
> Tom: mojom & manifest
> Ken: the rest. This uses the temporary hooks we discussed until the service
> manager has a way to hook this.

LGTM

This should be OK since only the browser should be able to call this per the
manifest.
Ideally, this wouldn't be present in the shipping code?

[commit-bot: I haz the power, 2017-04-24 22:15:31]

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

[commit-bot: I haz the power, 2017-04-24 22:15:32]

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_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-04-24 22:20:17]

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

[commit-bot: I haz the power, 2017-04-24 22:20:50]

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-24 22:25:15]

On 2017/04/24 22:14:39, Tom Sepez wrote:
> On 2017/04/24 21:33:27, jam wrote:
> > Matt: net/
> > Tom: mojom & manifest
> > Ken: the rest. This uses the temporary hooks we discussed until the service
> > manager has a way to hook this.
> 
> LGTM
> 
> This should be OK since only the browser should be able to call this per the
> manifest.
> Ideally, this wouldn't be present in the shipping code?

The mojom has to be in the shipping code, but the caller/callee are only in test
code.

[commit-bot: I haz the power, 2017-04-24 22:33:23]

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

[commit-bot: I haz the power, 2017-04-24 22:33:25]

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

[jam, 2017-04-24 22:35:03]

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

[jam, 2017-04-24 22:35:09]

Patchset #3 (id:50001) has been deleted

[jam, 2017-04-24 22:35:14]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-04-24 22:35: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-04-24 23:10:05]

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

[commit-bot: I haz the power, 2017-04-24 23:10:06]

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

[jam, 2017-04-24 23:39:50]

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

[commit-bot: I haz the power, 2017-04-24 23:40:19]

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-04-25 00:44:19]

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

[commit-bot: I haz the power, 2017-04-25 00:44:20]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-04-25 15:14:43]

How do you see things going from here?  Since presumably we won't have a
HostResolver at all in the browser process at all at some point, and this is
still based on that, not sure where we'd be going from here.

[jam, 2017-04-25 16:18:10]

On 2017/04/25 15:14:43, mmenke wrote:
> How do you see things going from here?  Since presumably we won't have a
> HostResolver at all in the browser process at all at some point, 

Right, since the hostresolver is used by /net, once calls that make requests
through net/ moves out-of-process then there'd be no hostresolver in the
browser. of course, we're quite a long way from that. but for now, this is to
get more of content_browsertests passing with OOP network service.

> and this is
> still based on that, not sure where we'd be going from here.

I'm not quite sure what you mean by this statement, can you expand what you mean
by "this is still based on that"?

If you mean tests, I think for browser_tests, using host_resolver()->AddRule is
still the easy way for tests to set things up. Once we only have network
service, we could change host_resolver() to just be a simple object that does
mojo calls.

[mmenke, 2017-04-25 16:22:34]

On 2017/04/25 16:18:10, jam wrote:
> On 2017/04/25 15:14:43, mmenke wrote:
> > How do you see things going from here?  Since presumably we won't have a
> > HostResolver at all in the browser process at all at some point, 
> 
> Right, since the hostresolver is used by /net, once calls that make requests
> through net/ moves out-of-process then there'd be no hostresolver in the
> browser. of course, we're quite a long way from that. but for now, this is to
> get more of content_browsertests passing with OOP network service.
> 
> > and this is
> > still based on that, not sure where we'd be going from here.
> 
> I'm not quite sure what you mean by this statement, can you expand what you
mean
> by "this is still based on that"?
> 
> If you mean tests, I think for browser_tests, using host_resolver()->AddRule
is
> still the easy way for tests to set things up. Once we only have network
> service, we could change host_resolver() to just be a simple object that does
> mojo calls.

So you're thinking the test fixture would expose a mojo-ified HostResolver
interface interface, which would just pass stuff over to the network process? 
Ok, that works.  I had imagined some sort of sidechannel, just for configuring
it, which would involve more intrusive changes, but that approach makes perfect
sense.

[jam, 2017-04-25 16:30:18]

On 2017/04/25 16:22:34, mmenke wrote:
> On 2017/04/25 16:18:10, jam wrote:
> > On 2017/04/25 15:14:43, mmenke wrote:
> > > How do you see things going from here?  Since presumably we won't have a
> > > HostResolver at all in the browser process at all at some point, 
> > 
> > Right, since the hostresolver is used by /net, once calls that make requests
> > through net/ moves out-of-process then there'd be no hostresolver in the
> > browser. of course, we're quite a long way from that. but for now, this is
to
> > get more of content_browsertests passing with OOP network service.
> > 
> > > and this is
> > > still based on that, not sure where we'd be going from here.
> > 
> > I'm not quite sure what you mean by this statement, can you expand what you
> mean
> > by "this is still based on that"?
> > 
> > If you mean tests, I think for browser_tests, using host_resolver()->AddRule
> is
> > still the easy way for tests to set things up. Once we only have network
> > service, we could change host_resolver() to just be a simple object that
does
> > mojo calls.
> 
> So you're thinking the test fixture would expose a mojo-ified HostResolver
> interface interface, which would just pass stuff over to the network process? 
> Ok, that works.  I had imagined some sort of sidechannel, just for configuring
> it, which would involve more intrusive changes, but that approach makes
perfect
> sense.

yep, that's what I was thinking.

[mmenke, 2017-04-25 16:37:58]

https://codereview.chromium.org/2837813004/diff/90001/content/public/test/bro...
File content/public/test/browser_test_base.cc (right):

https://codereview.chromium.org/2837813004/diff/90001/content/public/test/bro...
content/public/test/browser_test_base.cc:378: return;
add braces

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
File net/dns/mock_host_resolver.cc (right):

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.cc:358: CHECK(!locked_);
include base/logging.h

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
File net/dns/mock_host_resolver.h (right):

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:215: // Asserts if any changes are made to the
rules after this call.
Could you clarify that this makes calls that add/remove/modify rules assert?  As
opposed to this method itself asserting.

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:216: void Lock();
Can we not call this lock?  Commit() or Initialize(), maybe?  (I don't like lock
because this doesn't have anything to do with a base::Lock objects)

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:216: void Lock();
Add a TODO to remove this, once it's no longer used, and to make Rule private
again when that happens?  I assume we won't need either of those, once we're
using a Mojo interface for this.

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:237: std::string canonical_name;
include <string> (Should have been included already)

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:247: Rule(const Rule& other);
Per style guide, constructor/destructor should go before member variables.

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:265: bool locked_;
Hrm...Should this be protected by the lock as well?  Currently, it's actually
safe to add a rule on any thread.  This CL changes that.  That's probably not an
expected use-case of this class, but I'm not sure whether or not any test is
actually doing it.

[jam, 2017-04-25 17:04:41]

https://codereview.chromium.org/2837813004/diff/90001/content/public/test/bro...
File content/public/test/browser_test_base.cc (right):

https://codereview.chromium.org/2837813004/diff/90001/content/public/test/bro...
content/public/test/browser_test_base.cc:378: return;
On 2017/04/25 16:37:51, mmenke wrote:
> add braces

This is fine according to style guide afaik

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
File net/dns/mock_host_resolver.cc (right):

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.cc:358: CHECK(!locked_);
On 2017/04/25 16:37:51, mmenke wrote:
> include base/logging.h

Done.

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
File net/dns/mock_host_resolver.h (right):

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:215: // Asserts if any changes are made to the
rules after this call.
On 2017/04/25 16:37:53, mmenke wrote:
> Could you clarify that this makes calls that add/remove/modify rules assert? 
As
> opposed to this method itself asserting.

"after this call" isn't clear enough?

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:216: void Lock();
On 2017/04/25 16:37:55, mmenke wrote:
> Add a TODO to remove this, once it's no longer used, and to make Rule private
> again when that happens?  I assume we won't need either of those, once we're
> using a Mojo interface for this.

Done.

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:216: void Lock();
On 2017/04/25 16:37:55, mmenke wrote:
> Can we not call this lock?  Commit() or Initialize(), maybe?  (I don't like
lock
> because this doesn't have anything to do with a base::Lock objects)

Done.

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:237: std::string canonical_name;
On 2017/04/25 16:37:54, mmenke wrote:
> include <string> (Should have been included already)

Done.

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:247: Rule(const Rule& other);
On 2017/04/25 16:37:56, mmenke wrote:
> Per style guide, constructor/destructor should go before member variables.

Done.

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:265: bool locked_;
On 2017/04/25 16:37:55, mmenke wrote:
> Hrm...Should this be protected by the lock as well?  Currently, it's actually
> safe to add a rule on any thread.  This CL changes that.  That's probably not
an
> expected use-case of this class, but I'm not sure whether or not any test is
> actually doing it.

this will be modified before any threading happens by definition (since if there
are races with locking and new rules being added, then the network service would
miss some rules)

[jam, 2017-04-25 17:04:41]

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

[commit-bot: I haz the power, 2017-04-25 17:05: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-04-25 17:29:47]

LGTM

https://codereview.chromium.org/2837813004/diff/90001/content/public/test/bro...
File content/public/test/browser_test_base.cc (right):

https://codereview.chromium.org/2837813004/diff/90001/content/public/test/bro...
content/public/test/browser_test_base.cc:378: return;
On 2017/04/25 17:04:40, jam wrote:
> On 2017/04/25 16:37:51, mmenke wrote:
> > add braces
> 
> This is fine according to style guide afaik

Hrm...  "In general, curly braces are not required for single-line statements,
but they are allowed if you like them; conditional or loop statements with
complex conditions or statements may be more readable with curly braces. Some
projects require that an if must always always have an accompanying brace."

So I think it depends on your definition of "single-line statements". If it
means just the body, then yes.  If it means the body and condition lines are
each single-line, then no.  Anyhow, not going to fight over it.

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
File net/dns/mock_host_resolver.h (right):

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:265: bool locked_;
On 2017/04/25 17:04:40, jam wrote:
> On 2017/04/25 16:37:55, mmenke wrote:
> > Hrm...Should this be protected by the lock as well?  Currently, it's
actually
> > safe to add a rule on any thread.  This CL changes that.  That's probably
not
> an
> > expected use-case of this class, but I'm not sure whether or not any test is
> > actually doing it.
> 
> this will be modified before any threading happens by definition (since if
there
> are races with locking and new rules being added, then the network service
would
> miss some rules)

SGTM (Particularly with the above TODO)

https://codereview.chromium.org/2837813004/diff/110001/net/dns/mock_host_reso...
File net/dns/mock_host_resolver.h (right):

https://codereview.chromium.org/2837813004/diff/110001/net/dns/mock_host_reso...
net/dns/mock_host_resolver.h:217: // TODO: once this class isn't used by tests
that use an out of process
I believe TODO(name) is the preferred style here?  Feel free to use "mmenke"
instead of "jam".

[Ken Rockot(use gerrit already), 2017-04-25 17:36:09]

lgtm

https://codereview.chromium.org/2837813004/diff/110001/content/shell/utility/...
File content/shell/utility/shell_content_utility_client.h (right):

https://codereview.chromium.org/2837813004/diff/110001/content/shell/utility/...
content/shell/utility/shell_content_utility_client.h:25: void
RegisterNetworkServices(
nit: RegisterNetworkBinders?

https://codereview.chromium.org/2837813004/diff/110001/content/shell/utility/...
content/shell/utility/shell_content_utility_client.h:29: void Create(const
service_manager::Identity& remote_identity,
nit: Could be private. Also having a Create method is kinda weird on a class
that isn't dedicated to the interface(s) it's creating, so I'd kinda prefer we
just used Bind internally instead of InterfaceFactory. Your call.

https://codereview.chromium.org/2837813004/diff/110001/content/utility/utilit...
File content/utility/utility_service_factory.h (right):

https://codereview.chromium.org/2837813004/diff/110001/content/utility/utilit...
content/utility/utility_service_factory.h:30:
std::unique_ptr<service_manager::BinderRegistry> network_registry_;
nit: Document what this is?

[mmenke, 2017-04-25 17:44:44]

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
File net/dns/mock_host_resolver.h (right):

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:215: // Asserts if any changes are made to the
rules after this call.
On 2017/04/25 17:04:40, jam wrote:
> On 2017/04/25 16:37:53, mmenke wrote:
> > Could you clarify that this makes calls that add/remove/modify rules assert?

> As
> > opposed to this method itself asserting.
> 
> "after this call" isn't clear enough?

Oops, forgot to respond to this.  My feeling is that for clarity, methods should
have comments along the lines what they do when called, not what they will do in
the future (Or worse, what they cause other, unspecified methods to do in the
future).

So I think it's best to prefer something like "Causes/tells/makes class to
assert if <....>", for instance, to more clearly say what the method does when
called.  This also more closely matches format of other method descriptions.

[mmenke, 2017-04-25 18:10:09]

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
File net/dns/mock_host_resolver.h (right):

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:215: // Asserts if any changes are made to the
rules after this call.
On 2017/04/25 17:44:44, mmenke wrote:
> On 2017/04/25 17:04:40, jam wrote:
> > On 2017/04/25 16:37:53, mmenke wrote:
> > > Could you clarify that this makes calls that add/remove/modify rules
assert?
> 
> > As
> > > opposed to this method itself asserting.
> > 
> > "after this call" isn't clear enough?
> 
> Oops, forgot to respond to this.  My feeling is that for clarity, methods
should
> have comments along the lines what they do when called, not what they will do
in
> the future (Or worse, what they cause other, unspecified methods to do in the
> future).
> 
> So I think it's best to prefer something like "Causes/tells/makes class to
> assert if <....>", for instance, to more clearly say what the method does when
> called.  This also more closely matches format of other method descriptions.

A better explanation:  This method does not "Assert if any changes are made to
the rules after this call".  It makes other methods assert if changes are made
after it is called.

[commit-bot: I haz the power, 2017-04-25 18:11:12]

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

[commit-bot: I haz the power, 2017-04-25 18:11:13]

Dry run: This issue passed the CQ dry run.

[jam, 2017-04-25 18:19:03]

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

[jam, 2017-04-25 18:19:37]

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
File net/dns/mock_host_resolver.h (right):

https://codereview.chromium.org/2837813004/diff/90001/net/dns/mock_host_resol...
net/dns/mock_host_resolver.h:215: // Asserts if any changes are made to the
rules after this call.
On 2017/04/25 17:44:44, mmenke wrote:
> On 2017/04/25 17:04:40, jam wrote:
> > On 2017/04/25 16:37:53, mmenke wrote:
> > > Could you clarify that this makes calls that add/remove/modify rules
assert?
> 
> > As
> > > opposed to this method itself asserting.
> > 
> > "after this call" isn't clear enough?
> 
> Oops, forgot to respond to this.  My feeling is that for clarity, methods
should
> have comments along the lines what they do when called, not what they will do
in
> the future (Or worse, what they cause other, unspecified methods to do in the
> future).
> 
> So I think it's best to prefer something like "Causes/tells/makes class to
> assert if <....>", for instance, to more clearly say what the method does when
> called.  This also more closely matches format of other method descriptions.

Done.

https://codereview.chromium.org/2837813004/diff/110001/content/shell/utility/...
File content/shell/utility/shell_content_utility_client.h (right):

https://codereview.chromium.org/2837813004/diff/110001/content/shell/utility/...
content/shell/utility/shell_content_utility_client.h:25: void
RegisterNetworkServices(
On 2017/04/25 17:36:09, Ken Rockot wrote:
> nit: RegisterNetworkBinders?

Done.

https://codereview.chromium.org/2837813004/diff/110001/content/shell/utility/...
content/shell/utility/shell_content_utility_client.h:29: void Create(const
service_manager::Identity& remote_identity,
On 2017/04/25 17:36:09, Ken Rockot wrote:
> nit: Could be private. Also having a Create method is kinda weird on a class
> that isn't dedicated to the interface(s) it's creating, so I'd kinda prefer we
> just used Bind internally instead of InterfaceFactory. Your call.

Done.

https://codereview.chromium.org/2837813004/diff/110001/net/dns/mock_host_reso...
File net/dns/mock_host_resolver.h (right):

https://codereview.chromium.org/2837813004/diff/110001/net/dns/mock_host_reso...
net/dns/mock_host_resolver.h:217: // TODO: once this class isn't used by tests
that use an out of process
On 2017/04/25 17:29:47, mmenke wrote:
> I believe TODO(name) is the preferred style here?  Feel free to use "mmenke"
> instead of "jam".

Done.

[commit-bot: I haz the power, 2017-04-25 18:19:50]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-04-25 18:21:01]

net/ still LGTM. Thanks!

[commit-bot: I haz the power, 2017-04-25 19:33:26]

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

[commit-bot: I haz the power, 2017-04-25 19:33:27]

Dry run: This issue passed the CQ dry run.

[jam, 2017-04-25 19:56:47]

The CQ bit was checked by jam@chromium.org

[jam, 2017-04-25 19:56:48]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2837813004/#ps130001
(title: "review comments")

[commit-bot: I haz the power, 2017-04-25 19:57:14]

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-04-25 20:04:24]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1493150207589120,
"parent_rev": "b16ffdebce9e4724eb3e415c8760abc751d1c035", "commit_rev":
"700d86f404cceb4d49e2a1ed0b6f584839e43fde"}

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

Description was changed from

==========
Sync the browser test host resolver with the network process before a test runs.

This fixes the browser tests which use the in-process host resolver, which
depends on globals in the network code.

Most of the tests already call host_resolver()->AddRule in their
SetUpOnMainThread. This will be enforced once the remaining ones which don't do
this are updated.

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

to

==========
Sync the browser test host resolver with the network process before a test runs.

This fixes the browser tests which use the in-process host resolver, which
depends on globals in the network code.

Most of the tests already call host_resolver()->AddRule in their
SetUpOnMainThread. This will be enforced once the remaining ones which don't do
this are updated.

BUG=713847

Review-Url: https://codereview.chromium.org/2837813004
Cr-Commit-Position: refs/heads/master@{#467082}
Committed:
https://chromium.googlesource.com/chromium/src/+/700d86f404cceb4d49e2a1ed0b6f...
==========

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

Committed patchset #5 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/700d86f404cceb4d49e2a1ed0b6f...