Display DNS probe results.

chrome/browser/net/net_error_tab_helper.cc

Index: chrome/browser/net/net_error_tab_helper.cc
diff --git a/chrome/browser/net/net_error_tab_helper.cc b/chrome/browser/net/net_error_tab_helper.cc
index a5d124e4fcb00824beb930921b44f64424bd2539..3dc94a1d9ce78db0eb21fae8e5af602304c442a8 100644
--- a/chrome/browser/net/net_error_tab_helper.cc
+++ b/chrome/browser/net/net_error_tab_helper.cc
@@ -18,7 +18,7 @@
 #include "net/base/net_errors.h"
 
 using base::FieldTrialList;
-using chrome_common_net::DnsProbeResult;
+using chrome_common_net::DnsProbeStatus;
 using content::BrowserContext;
 using content::BrowserThread;
 using content::PageTransition;
@@ -32,9 +32,6 @@ namespace chrome_browser_net {
 
 namespace {
 
-const char kDnsProbeFieldTrialName[] = "DnsProbe-Enable";
-const char kDnsProbeFieldTrialEnableGroupName[] = "enable";
-
 static NetErrorTabHelper::TestingState testing_state_ =
     NetErrorTabHelper::TESTING_DEFAULT;
 
@@ -45,11 +42,6 @@ bool IsDnsError(int net_error) {
          net_error == net::ERR_NAME_RESOLUTION_FAILED;
 }
 
-bool GetEnabledByTrial() {
-  return (FieldTrialList::FindFullName(kDnsProbeFieldTrialName)
-          == kDnsProbeFieldTrialEnableGroupName);
-}
-
 NetErrorTracker::FrameType GetFrameType(bool is_main_frame) {
   return is_main_frame ? NetErrorTracker::FRAME_MAIN
                        : NetErrorTracker::FRAME_SUB;
@@ -66,12 +58,10 @@ NetErrorTracker::ErrorType GetErrorType(int net_error) {
 }
 
 void OnDnsProbeFinishedOnIOThread(
-    const base::Callback<void(DnsProbeResult)>& callback,
-    DnsProbeResult result) {
+    const base::Callback<void(DnsProbeStatus)>& callback,
+    DnsProbeStatus result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
-  DVLOG(1) << "DNS probe finished with result " << result;
-
   BrowserThread::PostTask(
       BrowserThread::UI,
       FROM_HERE,
@@ -81,12 +71,10 @@ void OnDnsProbeFinishedOnIOThread(
 // We can only access g_browser_process->io_thread() from the browser thread,
 // so we have to pass it in to the callback instead of dereferencing it here.
 void StartDnsProbeOnIOThread(
-    const base::Callback<void(DnsProbeResult)>& callback,
+    const base::Callback<void(DnsProbeStatus)>& callback,
     IOThread* io_thread) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
-  DVLOG(1) << "Starting DNS probe";
-
   DnsProbeService* probe_service =
       io_thread->globals()->dns_probe_service.get();
 
@@ -159,56 +147,86 @@ NetErrorTabHelper::NetErrorTabHelper(WebContents* contents)
                      weak_factory_.GetWeakPtr()))),
       dns_error_page_state_(NetErrorTracker::DNS_ERROR_PAGE_NONE),
       dns_probe_state_(DNS_PROBE_NONE),
-      enabled_by_trial_(GetEnabledByTrial()) {
+      enabled_by_trial_(chrome_common_net::DnsProbesEnabledByFieldTrial()) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   InitializePref(contents);
 }
 
 void NetErrorTabHelper::TrackerCallback(
-    NetErrorTracker::DnsErrorPageState state) {
-  dns_error_page_state_ = state;
+    NetErrorTracker::DnsErrorPageState tracker_state) {
+  DVLOG(1) << "Browser in tracker state " << tracker_state << ".";
+
+  dns_error_page_state_ = tracker_state;
 
   MaybePostStartDnsProbeTask();
-  MaybeSendInfo();
+  MaybeSendStatus();
 }
 
 void NetErrorTabHelper::MaybePostStartDnsProbeTask() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (dns_error_page_state_ != NetErrorTracker::DNS_ERROR_PAGE_NONE &&
-      dns_probe_state_ != DNS_PROBE_STARTED &&
-      ProbesAllowed()) {
-    BrowserThread::PostTask(
-        BrowserThread::IO,
-        FROM_HERE,
-        base::Bind(&StartDnsProbeOnIOThread,
-                   base::Bind(&NetErrorTabHelper::OnDnsProbeFinished,
-                              weak_factory_.GetWeakPtr()),
-                   g_browser_process->io_thread()));
-    dns_probe_state_ = DNS_PROBE_STARTED;
+  if (dns_error_page_state_ == NetErrorTracker::DNS_ERROR_PAGE_NONE ||
+      dns_probe_state_ != DNS_PROBE_NONE ||
+      !ProbesAllowed()) {
+    return;
   }
+
+  DVLOG(1) << "Starting DNS probe.";
+
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&StartDnsProbeOnIOThread,
+                 base::Bind(&NetErrorTabHelper::OnDnsProbeFinished,
+                            weak_factory_.GetWeakPtr()),
+                 g_browser_process->io_thread()));
+  dns_probe_state_ = DNS_PROBE_STARTED;
 }
 
-void NetErrorTabHelper::OnDnsProbeFinished(DnsProbeResult result) {
+void NetErrorTabHelper::OnDnsProbeFinished(DnsProbeStatus result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(DNS_PROBE_STARTED, dns_probe_state_);
 
+  DVLOG(1) << "Finished DNS probe with result " << result << ".";
+
   dns_probe_result_ = result;

[mmenke, 2013/04/10 16:56:58]

I don't think we gain anything from having |dns_probe_result_|.  This is now a
sufficiently simple state machine that I think we're adding complexity with no
real benefit.

My suggestion:  Get rid of it and MaybeSendStatus as well.

Put the logic for when to send the other two statuses in
MaybePostStartDnsProbeTask.  Can also get rid of DNS_PROBE_FINISHED.

[Deprecated (see juliatuttle), 2013/04/10 23:42:32]

The state machine is still complicated.  We're simultaneously waiting for the
page to load and maybe waiting for a probe to finish.

   dns_probe_state_ = DNS_PROBE_FINISHED;
 
-  MaybeSendInfo();
+  MaybeSendStatus();
 }
 
-void NetErrorTabHelper::MaybeSendInfo() {
-  if (dns_error_page_state_ == NetErrorTracker::DNS_ERROR_PAGE_LOADED &&
-      dns_probe_state_ == DNS_PROBE_FINISHED) {
-    DVLOG(1) << "Sending result " << dns_probe_result_ << " to renderer";
-    Send(new ChromeViewMsg_NetErrorInfo(routing_id(), dns_probe_result_));
+void NetErrorTabHelper::MaybeSendStatus() {
+  if (dns_error_page_state_ != NetErrorTracker::DNS_ERROR_PAGE_LOADED)
+    return;
+
+  switch (dns_probe_state_) {
+  case DNS_PROBE_STARTED:
+    DVLOG(1) << "Browser sending STARTED.";
+    SendInfo(chrome_common_net::DNS_PROBE_STARTED);
+    break;
+
+  case DNS_PROBE_FINISHED:
+    DVLOG(1) << "Browser sending result " << dns_probe_result_;
+    SendInfo(dns_probe_result_);
     dns_probe_state_ = DNS_PROBE_NONE;
+    break;
+
+  case DNS_PROBE_NONE:
+    DVLOG(1) << "Browser sending NOT_RUN.";
+    SendInfo(chrome_common_net::DNS_PROBE_NOT_RUN);

[mmenke, 2013/04/10 16:56:58]

Think sending this on all tracker events is a bit too aggressive.  Really only
matters when we'd other send a message, except for the fact that ProbesAllowed()
is false.

[Deprecated (see juliatuttle), 2013/04/10 23:42:32]

We don't.  We only send it if the page has just loaded.

+    break;
+
+  default:
+    NOTREACHED();
   }
 }
 
+void NetErrorTabHelper::SendInfo(
+    DnsProbeStatus status) {
+  Send(new ChromeViewMsg_NetErrorInfo(routing_id(), status));
+}
+
 void NetErrorTabHelper::InitializePref(WebContents* contents) {
   DCHECK(contents);