Add more data to net-internals view of DNS cache

chrome/browser/resources/net_internals/dns_view.js

Index: chrome/browser/resources/net_internals/dns_view.js
diff --git a/chrome/browser/resources/net_internals/dns_view.js b/chrome/browser/resources/net_internals/dns_view.js
index 05bb521f92a24408d95d72919cf0b9fc5a255b2b..d935f4b92e55508a2151fdd7b036a1d026836694 100644
--- a/chrome/browser/resources/net_internals/dns_view.js
+++ b/chrome/browser/resources/net_internals/dns_view.js
@@ -55,6 +55,7 @@ var DnsView = (function() {
 
   DnsView.ACTIVE_SPAN_ID = 'dns-view-cache-active';
   DnsView.EXPIRED_SPAN_ID = 'dns-view-cache-expired';
+  DnsView.NETWORK_SPAN_ID = 'dns-view-network-changes';
   DnsView.CACHE_TBODY_ID = 'dns-view-cache-tbody';
 
   cr.addSingletonGetter(DnsView);
@@ -73,6 +74,7 @@ var DnsView = (function() {
       $(DnsView.CACHE_TBODY_ID).innerHTML = '';
       $(DnsView.ACTIVE_SPAN_ID).innerHTML = '0';
       $(DnsView.EXPIRED_SPAN_ID).innerHTML = '0';
+      $(DnsView.NETWORK_SPAN_ID).innerHTML = '0';
 
       // Update fields containing async DNS configuration information.
       displayAsyncDnsConfig_(hostResolverInfo);
@@ -84,6 +86,7 @@ var DnsView = (function() {
       // Fill in the basic cache information.
       var hostResolverCache = hostResolverInfo.cache;
       $(DnsView.CAPACITY_SPAN_ID).innerText = hostResolverCache.capacity;
+      $(DnsView.NETWORK_SPAN_ID).innerText = hostResolverCache.network_changes;

[eroman, 2017/03/28 22:41:59]

When loading older log files, network_changes will not be defined, in which case
I imagine this will display "undefined".

How about replacing that expression with:

hostResolverCache.network_changes || '';

[mgersh, 2017/03/29 18:15:25]

Ah yeah. I thought to check old Chrome loading new netlog, but not new Chrome
loading old netlog. But your proposed simple fix doesn't work because 0 is
falsy, and properly getting this and the other fields to show empty string
instead of "undefined" seems like more complexity than it's worth in this
context for a case that won't happen much. What do you think?

[eroman, 2017/03/29 18:36:11]

Yes I think it is worth it.

In general not doing this can lead to artifacts like "undefined" or "null"
showing up when loading logs, or in other instances even worse it throws and
exception and then things break. It is often solved by adding checks accompanied
by a TODO comment that it can be removed in milestone YYY (a couple in the
future).

What do you think about something like this:

// TODO(yyy): The |ttl| and |network_changes| properties were introduced in M___
and may not exist when loading older logs. Can remove this in M___.
function valueOrDefault(value, defaultValue) {
  if (value != undefined)
    return value;
  return defaultValue
}

...innerText = valueOrDefault(hostResolverCache.network_changes, '');

and basically wrap each call to .ttl or network_changes in a call to
valueOrDefault(); should just be a couple callsites in this file, but does give
graceful backward compatibility.

[mgersh, 2017/03/29 20:20:53]

I still don't entirely understand why it's important for fields to be empty
instead of having the word "undefined" in them, but sure, done.

 
       var expiredEntries = 0;
       // Date the cache was logged.  This will be either now, when actively
@@ -99,6 +102,7 @@ var DnsView = (function() {
       for (var i = 0; i < hostResolverCache.entries.length; ++i) {
         var e = hostResolverCache.entries[i];
         var tr = addNode($(DnsView.CACHE_TBODY_ID), 'tr');
+        var expired = false;
 
         var hostnameCell = addNode(tr, 'td');
         addTextNode(hostnameCell, e.hostname);
@@ -116,15 +120,31 @@ var DnsView = (function() {
           addListToNode_(addNode(addressesCell, 'div'), e.addresses);
         }
 
+        var ttlCell = addNode(tr, 'td');
+        addTextNode(ttlCell, e.ttl);
+
         var expiresDate = timeutil.convertTimeTicksToDate(e.expiration);
         var expiresCell = addNode(tr, 'td');
         timeutil.addNodeWithDate(expiresCell, expiresDate);
         if (logDate > timeutil.convertTimeTicksToDate(e.expiration)) {
-          ++expiredEntries;
+          expired = true;
           var expiredSpan = addNode(expiresCell, 'span');
           expiredSpan.classList.add('warning-text');
           addTextNode(expiredSpan, ' [Expired]');
         }
+
+        var networkChangesCell = addNode(tr, 'td');
+        addTextNode(networkChangesCell, e.network_changes);
+        if (e.network_changes < hostResolverCache.network_changes) {

[eroman, 2017/03/28 22:41:59]

same comment -- what happens to this comparison when loading older logs that
don't have network_changes properties?


Also can you add a comment explaining this logic?

[mgersh, 2017/03/29 18:15:25]

undefined < undefined evaluates to false, so the comparison works fine as-is.
Done.

+          expired = true;
+          var expiredSpan = addNode(networkChangesCell, 'span');
+          expiredSpan.classList.add('warning-text');
+          addTextNode(expiredSpan, ' [Expired]');
+        }
+
+        if (expired) {
+          expiredEntries++;
+        }
       }
 
       $(DnsView.ACTIVE_SPAN_ID).innerText =