[net] Add DNS-related signals and NetLog to NetworkChangeNotifier.

[net] Add DNS-related signals and NetLog to NetworkChangeNotifier.

Copy DNS-related signals from DnsConfigService to NetworkChangeNotifier
Adds DNS watching to Windows and Mac implementations.

BUG=57102, 114827
TEST=Change DNS settings. Name resolutions should complete correctly.

[cbentzel, 2012-02-29 21:51:58]

I didn't delve into the implementations - how do you anticipate callers will use
this? Is it specifically for HostResolver?

http://codereview.chromium.org/9540011/diff/8014/net/base/network_change_noti...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/9540011/diff/8014/net/base/network_change_noti...
net/base/network_change_notifier.h:91: virtual bool IsCurrentlyWatchingDNS()
const;
Which consumers are using this?

http://codereview.chromium.org/9540011/diff/8014/net/base/network_change_noti...
net/base/network_change_notifier.h:91: virtual bool IsCurrentlyWatchingDNS()
const;
why not pure virtual? should also include similar notes as IsCurrentlyOffline
about expected behaviors.

[szym, 2012-02-29 22:03:49]

HostResolverImpl will be a DNSObserver. OnDNSCanged, it will retract its current
DnsConfig (and bypass DnsTask) and tell its DnsConfigService to (asynchronously)
obtain updated config. If !NCN::IsWatchingDNS() then it will cancel
DnsConfigService and there will be no support for DnsTask until the browser is
restarted.

http://codereview.chromium.org/9540011/diff/8014/net/base/network_change_noti...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/9540011/diff/8014/net/base/network_change_noti...
net/base/network_change_notifier.h:91: virtual bool IsCurrentlyWatchingDNS()
const;
On 2012/02/29 21:51:58, cbentzel wrote:
> Which consumers are using this? 

Any DNSObserver that must know if it can expect to receive OnDNSChanged signals
in the future. For example HostResolverImpl must not use
DnsConfig/DnsTransaction if it cannot reliably expect future OnDNSChanged
signals.

This requirement is very strict but the aim is to err on the side of correctness
for now.

I have considered adding a CHANGE_DNS_FAIL flag to |detail| in OnDNSChanged, but
a DNSObserver added after this notification is sent will miss it.

http://codereview.chromium.org/9540011/diff/8014/net/base/network_change_noti...
net/base/network_change_notifier.h:91: virtual bool IsCurrentlyWatchingDNS()
const;
On 2012/02/29 21:51:58, cbentzel wrote:
> why not pure virtual? should also include similar notes as IsCurrentlyOffline
> about expected behaviors.

Not pure virtual, because this is not an essential functionality and if some
platform (e.g. CrOS) implements its own NetworkChangeNotifier without DNS
signals, it will return false by default.

[cbentzel, 2012-02-29 22:05:32]

Should it be a parameter to OnDnsChanged instead?

On Wed, Feb 29, 2012 at 5:03 PM, <szym@chromium.org> wrote:

> HostResolverImpl will be a DNSObserver. OnDNSCanged, it will retract its
> current
> DnsConfig (and bypass DnsTask) and tell its DnsConfigService to
> (asynchronously)
> obtain updated config. If !NCN::IsWatchingDNS() then it will cancel
> DnsConfigService and there will be no support for DnsTask until the
> browser is
> restarted.
>
>
>
>
> http://codereview.chromium.**org/9540011/diff/8014/net/**
>
base/network_change_notifier.h<http://codereview.chromium.org/9540011/diff/8014/net/base/network_change_notifier.h>
> File net/base/network_change_**notifier.h (right):
>
> http://codereview.chromium.**org/9540011/diff/8014/net/**
>
base/network_change_notifier.**h#newcode91<http://codereview.chromium.org/9540011/diff/8014/net/base/network_change_notifier.h#newcode91>
> net/base/network_change_**notifier.h:91: virtual bool
> IsCurrentlyWatchingDNS() const;
> On 2012/02/29 21:51:58, cbentzel wrote:
>
>> Which consumers are using this?
>>
>
> Any DNSObserver that must know if it can expect to receive OnDNSChanged
> signals in the future. For example HostResolverImpl must not use
> DnsConfig/DnsTransaction if it cannot reliably expect future
> OnDNSChanged signals.
>
> This requirement is very strict but the aim is to err on the side of
> correctness for now.
>
> I have considered adding a CHANGE_DNS_FAIL flag to |detail| in
> OnDNSChanged, but a DNSObserver added after this notification is sent
> will miss it.
>
>
> http://codereview.chromium.**org/9540011/diff/8014/net/**
>
base/network_change_notifier.**h#newcode91<http://codereview.chromium.org/9540011/diff/8014/net/base/network_change_notifier.h#newcode91>
> net/base/network_change_**notifier.h:91: virtual bool
> IsCurrentlyWatchingDNS() const;
> On 2012/02/29 21:51:58, cbentzel wrote:
>
>> why not pure virtual? should also include similar notes as
>>
> IsCurrentlyOffline
>
>> about expected behaviors.
>>
>
> Not pure virtual, because this is not an essential functionality and if
> some platform (e.g. CrOS) implements its own NetworkChangeNotifier
> without DNS signals, it will return false by default.
>
>
http://codereview.chromium.**org/9540011/<http://codereview.chromium.org/9540...
>

[szym, 2012-02-29 22:07:17]

On 2012/02/29 22:05:32, cbentzel wrote:
> Should it be a parameter to OnDnsChanged instead?

My concern is:
 
> > I have considered adding a CHANGE_DNS_FAIL flag to |detail| in
> > OnDNSChanged, but a DNSObserver added after this notification is sent
> > will miss it.

[cbentzel, 2012-02-29 22:10:58]

On Wed, Feb 29, 2012 at 5:07 PM, <szym@chromium.org> wrote:

> On 2012/02/29 22:05:32, cbentzel wrote:
>
>> Should it be a parameter to OnDnsChanged instead?
>>
>
> My concern is:
>
>
>  > I have considered adding a CHANGE_DNS_FAIL flag to |detail| in
>> > OnDNSChanged, but a DNSObserver added after this notification is sent
>> > will miss it.
>>
>
Ahh, thanks.


>
>
>
http://codereview.chromium.**org/9540011/<http://codereview.chromium.org/9540...
>

[cbentzel, 2012-02-29 22:12:03]

Actually - would it be better if newly added observers get an OnDnsChanged
event immediately with the current state?

It would be great if we had edge-triggered ways of indicating this only.

On Wed, Feb 29, 2012 at 5:10 PM, Chris Bentzel <cbentzel@chromium.org>wrote:

>
>
> On Wed, Feb 29, 2012 at 5:07 PM, <szym@chromium.org> wrote:
>
>> On 2012/02/29 22:05:32, cbentzel wrote:
>>
>>> Should it be a parameter to OnDnsChanged instead?
>>>
>>
>> My concern is:
>>
>>
>>  > I have considered adding a CHANGE_DNS_FAIL flag to |detail| in
>>> > OnDNSChanged, but a DNSObserver added after this notification is sent
>>> > will miss it.
>>>
>>
> Ahh, thanks.
>
>
>>
>>
>>
http://codereview.chromium.**org/9540011/<http://codereview.chromium.org/9540...
>>
>
>

[szym, 2012-02-29 22:14:36]

On 2012/02/29 22:12:03, cbentzel wrote:
> Actually - would it be better if newly added observers get an OnDnsChanged
> event immediately with the current state?
> 
> It would be great if we had edge-triggered ways of indicating this only.

I wouldn't mind an interface like this, and it would simplify the consumers.
However, it won't really affect the implementation. AddDNSObserver is
implemented in the base class NetworkChangeNotifier.

[szym, 2012-03-01 18:18:44]

The reason mac trybots fail is that FilePathWatcher requires an IO message loop
and NetworkChangeNotifier is created on the browser (UI) thread.
NetworkChangeNotifierLinux spawns a separate IO thread to avoid this problem.
NetworkChangeNotifierMac currently spawns a separate UI thread, so we would need
yet another thread. Windows does not see this problem because FilePathWatcherWin
does not require an IO loop.

It feels wasteful to spawn a separate IO thread for asynchronous operations when
the primary customers of DNSObserver live on another IO thread. HostResolverImpl
is essentially the only customer of DNSObserver -- directly and through
DnsReloader (not clear on the plans for DnsRRResolver). So it makes sense to
move DNSObserver out of NetworkChangeNotifier and make it owned by
HostResolverImpl. 

This would mean staying closer to what DnsConfigService is today, and probably
making it even simpler by replacing Observer with Delegate. The main difference
would be that DnsConfigService would be used regardless of "--enable-async-dns".

On 2012/02/29 22:14:36, szym wrote:
> On 2012/02/29 22:12:03, cbentzel wrote:
> > Actually - would it be better if newly added observers get an OnDnsChanged
> > event immediately with the current state?
> > 
> > It would be great if we had edge-triggered ways of indicating this only.
> 
> I wouldn't mind an interface like this, and it would simplify the consumers.
> However, it won't really affect the implementation. AddDNSObserver is
> implemented in the base class NetworkChangeNotifier.