Change Cronet's "disable_ipv6" to "disable_ipv6_on_wifi"

Change Cronet's "disable_ipv6" to "disable_ipv6_on_wifi"

The new option is implemented in HostResolverImpl as an assumption that
IPv6 is unreachable when the current connection is wifi. In order to
make this testable, the tests that used to override IsIPv6Reachable()
now override IsGloballyReachable().

BUG=696569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2811183003
Cr-Commit-Position: refs/heads/master@{#464522}
Committed: https://chromium.googlesource.com/chromium/src/+/af9a9230d0afc8c8ba34f30a2a12fafc3ae144e4

[mgersh, 2017-04-11 22:31:27]

Description was changed from

==========
WIP
==========

to

==========
WIP
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mgersh, 2017-04-12 14:51:06]

Description was changed from

==========
WIP
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
The new option is implemented in HostResolverImpl as an assumption that
IPv6 is unreachable when the current connection is wifi. In order to
make this testable, the tests that used to override IsIPv6Reachable()
now override IsGloballyReachable().

BUG=696569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mgersh, 2017-04-12 14:51:25]

Patchset #1 (id:1) has been deleted

[mgersh, 2017-04-12 14:54:29]

Description was changed from

==========
The new option is implemented in HostResolverImpl as an assumption that
IPv6 is unreachable when the current connection is wifi. In order to
make this testable, the tests that used to override IsIPv6Reachable()
now override IsGloballyReachable().

BUG=696569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Change Cronet's "disable_ipv6" to "disable_ipv6_on_wifi"

The new option is implemented in HostResolverImpl as an assumption that
IPv6 is unreachable when the current connection is wifi. In order to
make this testable, the tests that used to override IsIPv6Reachable()
now override IsGloballyReachable().

BUG=696569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mgersh, 2017-04-12 14:55:57]

mgersh@chromium.org changed reviewers:
+ pauljensen@chromium.org

[mgersh, 2017-04-12 14:55:58]

PTAL. I'm going to revert the default address family stuff
(https://codereview.chromium.org/2709393007/) in a separate CL once this lands.

[pauljensen, 2017-04-12 18:16:40]

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2445: bool
HostResolverImpl::IsGloballyReachable(const IPAddress& dest,
why is this moved?

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.h (right):

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:363: // True if IPv6 should not be attempted when
on a wifi connection. See
nit: wifi->WiFi

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:364: // http://crbug.com/696569 for further
context.
nit: http->https

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:368: NetworkChangeNotifier::ConnectionType
current_connection_type_;
Can we remove this?  Caches are tricky and it doesn't look like it's actually
used for testing.

[mgersh, 2017-04-12 18:26:43]

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2445: bool
HostResolverImpl::IsGloballyReachable(const IPAddress& dest,
On 2017/04/12 18:16:40, pauljensen wrote:
> why is this moved?

As I said in the CL description, I wanted tests to override this rather than
IsIPv6Reachable() so that IsIPv6Reachable() becomes testable.

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.h (right):

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:363: // True if IPv6 should not be attempted when
on a wifi connection. See
On 2017/04/12 18:16:40, pauljensen wrote:
> nit: wifi->WiFi

Done.

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:364: // http://crbug.com/696569 for further
context.
On 2017/04/12 18:16:40, pauljensen wrote:
> nit: http->https

Done.

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:368: NetworkChangeNotifier::ConnectionType
current_connection_type_;
On 2017/04/12 18:16:40, pauljensen wrote:
> Can we remove this?  Caches are tricky and it doesn't look like it's actually
> used for testing.

It's actually used for testing. If I do it this way, I can tell
NetworkChangeNotifier to simulate a network change and then HostResolverImpl
thinks I'm on that kind of network. If I don't, I have to deal with mocking
NetworkChangeNotifier. Keeping it updated is one line of code.

[pauljensen, 2017-04-12 18:58:11]

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.h (right):

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:368: NetworkChangeNotifier::ConnectionType
current_connection_type_;
On 2017/04/12 18:26:42, mgersh wrote:
> On 2017/04/12 18:16:40, pauljensen wrote:
> > Can we remove this?  Caches are tricky and it doesn't look like it's
actually
> > used for testing.
> 
> It's actually used for testing. If I do it this way, I can tell
> NetworkChangeNotifier to simulate a network change and then HostResolverImpl
> thinks I'm on that kind of network. If I don't, I have to deal with mocking
> NetworkChangeNotifier. Keeping it updated is one line of code.

How about net::test::MockNetworkChangeNotifier
(https://cs.chromium.org/chromium/src/net/quic/chromium/mock_network_change_no...

[mgersh, 2017-04-13 14:57:10]

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.h (right):

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:368: NetworkChangeNotifier::ConnectionType
current_connection_type_;
On 2017/04/12 18:58:11, pauljensen wrote:
> On 2017/04/12 18:26:42, mgersh wrote:
> > On 2017/04/12 18:16:40, pauljensen wrote:
> > > Can we remove this?  Caches are tricky and it doesn't look like it's
> actually
> > > used for testing.
> > 
> > It's actually used for testing. If I do it this way, I can tell
> > NetworkChangeNotifier to simulate a network change and then HostResolverImpl
> > thinks I'm on that kind of network. If I don't, I have to deal with mocking
> > NetworkChangeNotifier. Keeping it updated is one line of code.
> 
> How about net::test::MockNetworkChangeNotifier
>
(https://cs.chromium.org/chromium/src/net/quic/chromium/mock_network_change_no...)

Done.

[pauljensen, 2017-04-13 17:17:02]

lgtm module some simple comments

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2811183003/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2445: bool
HostResolverImpl::IsGloballyReachable(const IPAddress& dest,
On 2017/04/12 18:26:42, mgersh wrote:
> On 2017/04/12 18:16:40, pauljensen wrote:
> > why is this moved?
> 
> As I said in the CL description, I wanted tests to override this rather than
> IsIPv6Reachable() so that IsIPv6Reachable() becomes testable.

Ah, okay, sorry I didn't notice the "HostResolverImpl::" addition.

https://codereview.chromium.org/2811183003/diff/60001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2811183003/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:85: // Disable IPv6. This should
almost never be necessary because the network stack
this comment should be augmented to note that it only takes effect on WiFi.

https://codereview.chromium.org/2811183003/diff/60001/components/cronet/url_r...
File components/cronet/url_request_context_config_unittest.cc (right):

https://codereview.chromium.org/2811183003/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config_unittest.cc:119: EXPECT_EQ(true,
context->host_resolver()->GetNoIPv6OnWifi());
EXPECT_TRUE

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver.h...
net/dns/host_resolver.h:225: // connection. See http://crbug.com/696569 for
further context.
nit:https

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver.h...
net/dns/host_resolver.h:226: virtual void SetNoIPv6OnWifi();
This should probably take a bool.  Calling it "enabled" may be quite confusing;
perhaps "no_ipv6_on_wifi"

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2422: // Don't bother checking if we're on wifi
and we're assuming that IPv6 doesn't
nit: I'm not sure which style-guide recommends it, but generally avoiding "I" or
"we" in comments is helpful to make it clear who/what is motivating something.
For example you could reword this line: "Don't bother checking if the device is
on wifi and |assume_ipv6_failure_on_wifi_| indicates that IPv6 doesn't"

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2422: // Don't bother checking if we're on wifi
and we're assuming that IPv6 doesn't
nit (and on next line): wifi->WiFi

[mgersh, 2017-04-13 19:07:20]

Thanks!

https://codereview.chromium.org/2811183003/diff/60001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2811183003/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:85: // Disable IPv6. This should
almost never be necessary because the network stack
On 2017/04/13 17:17:01, pauljensen wrote:
> this comment should be augmented to note that it only takes effect on WiFi.

Done.

https://codereview.chromium.org/2811183003/diff/60001/components/cronet/url_r...
File components/cronet/url_request_context_config_unittest.cc (right):

https://codereview.chromium.org/2811183003/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config_unittest.cc:119: EXPECT_EQ(true,
context->host_resolver()->GetNoIPv6OnWifi());
On 2017/04/13 17:17:01, pauljensen wrote:
> EXPECT_TRUE

Done.

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver.h...
net/dns/host_resolver.h:225: // connection. See http://crbug.com/696569 for
further context.
On 2017/04/13 17:17:01, pauljensen wrote:
> nit:https

Done.

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver.h...
net/dns/host_resolver.h:226: virtual void SetNoIPv6OnWifi();
On 2017/04/13 17:17:01, pauljensen wrote:
> This should probably take a bool.  Calling it "enabled" may be quite
confusing;
> perhaps "no_ipv6_on_wifi"

Done.

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2422: // Don't bother checking if we're on wifi
and we're assuming that IPv6 doesn't
On 2017/04/13 17:17:01, pauljensen wrote:
> nit: I'm not sure which style-guide recommends it, but generally avoiding "I"
or
> "we" in comments is helpful to make it clear who/what is motivating something.
> For example you could reword this line: "Don't bother checking if the device
is
> on wifi and |assume_ipv6_failure_on_wifi_| indicates that IPv6 doesn't"

Done.

https://codereview.chromium.org/2811183003/diff/60001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2422: // Don't bother checking if we're on wifi
and we're assuming that IPv6 doesn't
On 2017/04/13 17:17:01, pauljensen wrote:
> nit (and on next line): wifi->WiFi

Done.

[mgersh, 2017-04-13 19:07:30]

The CQ bit was checked by mgersh@chromium.org

[mgersh, 2017-04-13 19:07:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/2811183003/#ps80001
(title: "more comments")

[commit-bot: I haz the power, 2017-04-13 19:07:59]

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-13 20:19:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492110450798630,
"parent_rev": "63957e7f711d2b15cd1974b4dd4d3abedc2ae596", "commit_rev":
"af9a9230d0afc8c8ba34f30a2a12fafc3ae144e4"}

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

Description was changed from

==========
Change Cronet's "disable_ipv6" to "disable_ipv6_on_wifi"

The new option is implemented in HostResolverImpl as an assumption that
IPv6 is unreachable when the current connection is wifi. In order to
make this testable, the tests that used to override IsIPv6Reachable()
now override IsGloballyReachable().

BUG=696569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Change Cronet's "disable_ipv6" to "disable_ipv6_on_wifi"

The new option is implemented in HostResolverImpl as an assumption that
IPv6 is unreachable when the current connection is wifi. In order to
make this testable, the tests that used to override IsIPv6Reachable()
now override IsGloballyReachable().

BUG=696569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

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

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

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/af9a9230d0afc8c8ba34f30a2a12...

[mrtegg, 2017-04-13 20:24:17]

mrtegg@googlemail.com changed reviewers:
+ mrtegg@googlemail.com

[mrtegg, 2017-04-13 20:24:18]

lgtm