[cros/net] Add DnsConfigService to NetworkChangeNotifierChromeos.

[cros/net] Add DnsConfigService to NetworkChangeNotifierChromeos.

Unlike DnsConfigServicePosix, the CrOS implementation uses signals from
NetworkManager (via NetworkLibrary), rather than watching files in /etc.

BUG=125877
TEST=Enable "Built-in Asynchronous DNS". Check if chrome://net-internals/#dns shows DNS config.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156055

[szym, 2012-09-04 04:42:10]

I'm still working on building a bootable CrOS image, so I can't verify this will
actually detect network changes, but I'd appreciate any early feedback on the
approach.

[zel, 2012-09-04 17:16:11]

http://codereview.chromium.org/10909034/diff/1/chrome/browser/chromeos/net/ne...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/10909034/diff/1/chrome/browser/chromeos/net/ne...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:143:
dns_config_service_->OnNetworkChange();
This will fire way too often for what you are trying to detect.

You should probably move this to line 165.

[szym, 2012-09-04 17:29:30]

http://codereview.chromium.org/10909034/diff/1/chrome/browser/chromeos/net/ne...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/10909034/diff/1/chrome/browser/chromeos/net/ne...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:143:
dns_config_service_->OnNetworkChange();
On 2012/09/04 17:16:11, zel wrote:
> This will fire way too often for what you are trying to detect.
> 
> You should probably move this to line 165.  

How often does this fire?

If I correctly understand the condition in 146-149, it will filter out changes,
unless it's a change of active network or the IP address changes. What if the
user changes the preferred DNS provider? Is it a different network?

[zel, 2012-09-05 19:26:59]

http://codereview.chromium.org/10909034/diff/1/chrome/browser/chromeos/net/ne...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/10909034/diff/1/chrome/browser/chromeos/net/ne...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:143:
dns_config_service_->OnNetworkChange();
On 2012/09/04 17:29:31, szym wrote:
> On 2012/09/04 17:16:11, zel wrote:
> > This will fire way too often for what you are trying to detect.
> > 
> > You should probably move this to line 165.  
> 
> How often does this fire?
> 
> If I correctly understand the condition in 146-149, it will filter out
changes,
> unless it's a change of active network or the IP address changes. What if the
> user changes the preferred DNS provider? Is it a different network?

For an active network this will fire when any of it's properties changes (i.e.
signal strength change for WiFi/mobile network), so you you will be dropping DNS
cache all the times.

Yes, DNS settings can also change when the active network didn't change. You
should inspect results of

lib->GetIPConfigs(network->service_path(), ...)

which will return the list of NetworkIPConfig structs that contain name_servers
member.

[szym, 2012-09-05 19:48:29]

http://codereview.chromium.org/10909034/diff/1/chrome/browser/chromeos/net/ne...
File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):

http://codereview.chromium.org/10909034/diff/1/chrome/browser/chromeos/net/ne...
chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:143:
dns_config_service_->OnNetworkChange();
On 2012/09/05 19:26:59, zel wrote:
> For an active network this will fire when any of it's properties changes (i.e.
> signal strength change for WiFi/mobile network), so you you will be dropping
DNS
> cache all the times.

That is indeed a bit too often, but note that this is not a direct signal to
HostResolverImpl. Instead, on signal DnsConfigService will run res_ninit to
determine if the change actually affects DNS configuration, i.e., if nameservers
or suffix search list changed. Only then NetworkChangeNotifier will broadcast
OnDNSChanged to the observers. So, DnsConfigService serves as a filter for
signals from NetworkLibrary.
 
> Yes, DNS settings can also change when the active network didn't change. You
> should inspect results of
> 
> lib->GetIPConfigs(network->service_path(), ...)
> 
> which will return the list of NetworkIPConfig structs that contain
name_servers
> member.

Why is there more than one NetworkIPConfig per service? Which of the configs
should I be looking at for the current DNS configuration?

[szym, 2012-09-10 21:33:45]

I have carefully studied NetworkLibrary and realized that some refactoring will
be needed to get to the static DNS configuration. So for now I moved
OnNetworkChanged to where we generate OnIPAddressChanged.

PTAL.

On 2012/09/05 19:48:29, szym wrote:
>
http://codereview.chromium.org/10909034/diff/1/chrome/browser/chromeos/net/ne...
> File chrome/browser/chromeos/net/network_change_notifier_chromeos.cc (right):
> 
>
http://codereview.chromium.org/10909034/diff/1/chrome/browser/chromeos/net/ne...
> chrome/browser/chromeos/net/network_change_notifier_chromeos.cc:143:
> dns_config_service_->OnNetworkChange();
> On 2012/09/05 19:26:59, zel wrote:
> > For an active network this will fire when any of it's properties changes
(i.e.
> > signal strength change for WiFi/mobile network), so you you will be dropping
> DNS
> > cache all the times.
> 
> That is indeed a bit too often, but note that this is not a direct signal to
> HostResolverImpl. Instead, on signal DnsConfigService will run res_ninit to
> determine if the change actually affects DNS configuration, i.e., if
nameservers
> or suffix search list changed. Only then NetworkChangeNotifier will broadcast
> OnDNSChanged to the observers. So, DnsConfigService serves as a filter for
> signals from NetworkLibrary.
>  
> > Yes, DNS settings can also change when the active network didn't change. You
> > should inspect results of
> > 
> > lib->GetIPConfigs(network->service_path(), ...)
> > 
> > which will return the list of NetworkIPConfig structs that contain
> name_servers
> > member.
> 
> Why is there more than one NetworkIPConfig per service? Which of the configs
> should I be looking at for the current DNS configuration?

[zel, 2012-09-10 22:57:31]

ok, even if this does not cover/detect all DNS config changes as you said, it
will catch the most common cases. So, please create a separate work item for
detecting DNS changes in active NW.

LGTM

[szym, 2012-09-11 15:03:56]

sky: please approve changes in chrome/browser/about_flags.cc

[sky, 2012-09-11 16:11:12]

LGTM