Add the ability for MDnsListener to actively refresh records

Add the ability for MDnsListener to actively refresh records

If the MDnsListener is listening for a record we care about, actively refresh
it.

BUG=336883
R=vitalybuka@chromium.org,szym@chromium.org

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

[szym, 2014-01-23 11:24:03]

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
net/dns/mdns_client_impl.cc:29: const double kListenerRefreshPercentage1 = 0.85;
Percentage suggests that this is divided by 100, i.e., percentage=0.85 means
0.85% or 0.0085. Suggest ListenerRefreshRatio.

Even then the name is unclear so this needs a comment.

// Refresh ratio is the time before the record expiration at which
// the listener should refresh it expressed as a fraction of
// the record TTL. For example, with refresh ratio of 0.85, a record
// with announced TTL of 100s should be refreshed 15s before expiration.

What's the reasoning for using time relative to TTL instead of absolute? I'm
concerned that the TTL received in DNS response is always the remaining TTL, so
it will be less if it's sent from a cache.

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
net/dns/mdns_client_impl.cc:518: void MDnsListenerImpl::ScheduleNextRefresh() {
This function silently assumes that if active_refresh_ then last_update_ !=
Time().

I suggest you either add DCHECK or explicit check and exit if last_update_ ==
Time().

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
net/dns/mdns_client_impl.cc:519: if (active_refresh_) {
By convention this should be:
if (!active_refresh_)
  return;
...

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
net/dns/mdns_client_impl.cc:533: base::Time next_refresh2 = last_update_ +
base::TimeDelta::FromMilliseconds(
You need a comment somewhere why two refresh tasks are scheduled at the same
time.

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.h#n...
net/dns/mdns_client_impl.h:237: base::CancelableCallback<void()> next_refresh_;
base::CancelableClosure

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_unittest.cc
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_unittest...
net/dns/mdns_client_unittest.cc:1046: .Times(4);
Add comment explaining why 4.

[Noam Samuel, 2014-01-23 17:50:41]

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
net/dns/mdns_client_impl.cc:29: const double kListenerRefreshPercentage1 = 0.85;
On 2014/01/23 11:24:03, szym wrote:
> Percentage suggests that this is divided by 100, i.e., percentage=0.85 means
> 0.85% or 0.0085. Suggest ListenerRefreshRatio.
> 
> Even then the name is unclear so this needs a comment.
> 
> // Refresh ratio is the time before the record expiration at which
> // the listener should refresh it expressed as a fraction of
> // the record TTL. For example, with refresh ratio of 0.85, a record
> // with announced TTL of 100s should be refreshed 15s before expiration.
> 
> What's the reasoning for using time relative to TTL instead of absolute? I'm
> concerned that the TTL received in DNS response is always the remaining TTL,
so
> it will be less if it's sent from a cache.

The time is relative to the record's original TTL, as per the mDNS spec. 
RecordParsed has separate fields for ttl and time_created so that the full
information on a packet's expiration time is maintained (which is another work
item for this CL, I don't actually use time_created, but will fix that).

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
net/dns/mdns_client_impl.cc:518: void MDnsListenerImpl::ScheduleNextRefresh() {
On 2014/01/23 11:24:03, szym wrote:
> This function silently assumes that if active_refresh_ then last_update_ !=
> Time().
> 
> I suggest you either add DCHECK or explicit check and exit if last_update_ ==
> Time().

Done.

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
net/dns/mdns_client_impl.cc:519: if (active_refresh_) {
On 2014/01/23 11:24:03, szym wrote:
> By convention this should be:
> if (!active_refresh_)
>   return;
> ...

Done.

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
net/dns/mdns_client_impl.cc:533: base::Time next_refresh2 = last_update_ +
base::TimeDelta::FromMilliseconds(
On 2014/01/23 11:24:03, szym wrote:
> You need a comment somewhere why two refresh tasks are scheduled at the same
> time.

Done.

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.h#n...
net/dns/mdns_client_impl.h:237: base::CancelableCallback<void()> next_refresh_;
On 2014/01/23 11:24:03, szym wrote:
> base::CancelableClosure

Done.

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_unittest.cc
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_unittest...
net/dns/mdns_client_unittest.cc:1046: .Times(4);
On 2014/01/23 11:24:03, szym wrote:
> Add comment explaining why 4.

Done.

[Noam Samuel, 2014-01-27 19:52:43]

On 2014/01/23 17:50:41, Noam Samuel wrote:
> https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc
> File net/dns/mdns_client_impl.cc (right):
> 
>
https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
> net/dns/mdns_client_impl.cc:29: const double kListenerRefreshPercentage1 =
0.85;
> On 2014/01/23 11:24:03, szym wrote:
> > Percentage suggests that this is divided by 100, i.e., percentage=0.85 means
> > 0.85% or 0.0085. Suggest ListenerRefreshRatio.
> > 
> > Even then the name is unclear so this needs a comment.
> > 
> > // Refresh ratio is the time before the record expiration at which
> > // the listener should refresh it expressed as a fraction of
> > // the record TTL. For example, with refresh ratio of 0.85, a record
> > // with announced TTL of 100s should be refreshed 15s before expiration.
> > 
> > What's the reasoning for using time relative to TTL instead of absolute? I'm
> > concerned that the TTL received in DNS response is always the remaining TTL,
> so
> > it will be less if it's sent from a cache.
> 
> The time is relative to the record's original TTL, as per the mDNS spec. 
> RecordParsed has separate fields for ttl and time_created so that the full
> information on a packet's expiration time is maintained (which is another work
> item for this CL, I don't actually use time_created, but will fix that).
> 
>
https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
> net/dns/mdns_client_impl.cc:518: void MDnsListenerImpl::ScheduleNextRefresh()
{
> On 2014/01/23 11:24:03, szym wrote:
> > This function silently assumes that if active_refresh_ then last_update_ !=
> > Time().
> > 
> > I suggest you either add DCHECK or explicit check and exit if last_update_
==
> > Time().
> 
> Done.
> 
>
https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
> net/dns/mdns_client_impl.cc:519: if (active_refresh_) {
> On 2014/01/23 11:24:03, szym wrote:
> > By convention this should be:
> > if (!active_refresh_)
> >   return;
> > ...
> 
> Done.
> 
>
https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.cc#...
> net/dns/mdns_client_impl.cc:533: base::Time next_refresh2 = last_update_ +
> base::TimeDelta::FromMilliseconds(
> On 2014/01/23 11:24:03, szym wrote:
> > You need a comment somewhere why two refresh tasks are scheduled at the same
> > time.
> 
> Done.
> 
> https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.h
> File net/dns/mdns_client_impl.h (right):
> 
>
https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_impl.h#n...
> net/dns/mdns_client_impl.h:237: base::CancelableCallback<void()>
next_refresh_;
> On 2014/01/23 11:24:03, szym wrote:
> > base::CancelableClosure
> 
> Done.
> 
>
https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_unittest.cc
> File net/dns/mdns_client_unittest.cc (right):
> 
>
https://codereview.chromium.org/132693025/diff/1/net/dns/mdns_client_unittest...
> net/dns/mdns_client_unittest.cc:1046: .Times(4);
> On 2014/01/23 11:24:03, szym wrote:
> > Add comment explaining why 4.
> 
> Done.

Ping

[szym, 2014-01-29 19:08:39]

lgtm, sorry for the delay

https://codereview.chromium.org/132693025/diff/110001/net/dns/mdns_client_uni...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/132693025/diff/110001/net/dns/mdns_client_uni...
net/dns/mdns_client_unittest.cc:1045: // calls at 95% of the TTL.
nit: I'd remove the 85% and 95% numbers from this comment.
// Expecting 2 calls (one for IPv4 and one for IPv6) for each of the 2 scheduled
refresh queries.

[Noam Samuel, 2014-01-29 19:21:36]

https://codereview.chromium.org/132693025/diff/110001/net/dns/mdns_client_uni...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/132693025/diff/110001/net/dns/mdns_client_uni...
net/dns/mdns_client_unittest.cc:1045: // calls at 95% of the TTL.
On 2014/01/29 19:08:39, szym wrote:
> nit: I'd remove the 85% and 95% numbers from this comment.
> // Expecting 2 calls (one for IPv4 and one for IPv6) for each of the 2
scheduled
> refresh queries.

Done.

[commit-bot: I haz the power, 2014-01-29 19:24:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/132693025/170001

[commit-bot: I haz the power, 2014-01-30 03:03:54]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 03:09:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/132693025/170001

[commit-bot: I haz the power, 2014-01-30 08:07:43]

Change committed as 247889