Fix auto-reload histograms.

chrome/renderer/net/net_error_helper_core.cc

Index: chrome/renderer/net/net_error_helper_core.cc
diff --git a/chrome/renderer/net/net_error_helper_core.cc b/chrome/renderer/net/net_error_helper_core.cc
index fb14be734505dfa833e68d67a1dcd76618d5b9a8..e9c6be528b465e389d63209738e8f1d122e38010 100644
--- a/chrome/renderer/net/net_error_helper_core.cc
+++ b/chrome/renderer/net/net_error_helper_core.cc
@@ -17,6 +17,7 @@
 #include "base/strings/string16.h"
 #include "base/values.h"
 #include "chrome/common/localized_error.h"
+#include "content/public/common/url_constants.h"
 #include "grit/generated_resources.h"
 #include "net/base/escape.h"
 #include "net/base/net_errors.h"
@@ -247,6 +248,29 @@ LocalizedError::ErrorPageParams* ParseAdditionalSuggestions(
   return params.release();
 }
 
+void ReportAutoReloadSuccess(const blink::WebURLError& error, size_t count) {
+  if (error.domain.utf8() != net::kErrorDomain)
+    return;
+  UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtSuccess",
+                                   -error.reason,
+                                   net::GetAllErrorCodesForUma());
+  UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtSuccess", count);
+  if (count == 1) {
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtFirstSuccess",
+                                     -error.reason,
+                                     net::GetAllErrorCodesForUma());
+  }
+}
+
+void ReportAutoReloadFailure(const blink::WebURLError& error, size_t count) {
+  if (error.domain.utf8() != net::kErrorDomain)
+    return;
+  UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtStop",
+                                   -error.reason,
+                                   net::GetAllErrorCodesForUma());
+  UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", count);
+}
+
 }  // namespace
 
 struct NetErrorHelperCore::ErrorPageInfo {
@@ -256,7 +280,8 @@ struct NetErrorHelperCore::ErrorPageInfo {
         needs_dns_updates(false),
         reload_button_in_page(false),
         load_stale_button_in_page(false),
-        is_finished_loading(false) {
+        is_finished_loading(false),
+        auto_reload_triggered(false) {
   }
 
   // Information about the failed page load.
@@ -288,6 +313,10 @@ struct NetErrorHelperCore::ErrorPageInfo {
   // True if a page has completed loading, at which point it can receive
   // updates.
   bool is_finished_loading;
+
+  // True if the auto-reload timer has fired and a reload is or has been in
+  // flight.
+  bool auto_reload_triggered;
 };
 
 bool NetErrorHelperCore::IsReloadableError(
@@ -322,12 +351,6 @@ void NetErrorHelperCore::CancelPendingFetches() {
   // Cancel loading the alternate error page, and prevent any pending error page
   // load from starting a new error page load.  Swapping in the error page when
   // it's finished loading could abort the navigation, otherwise.
-  if (committed_error_page_info_ && can_auto_reload_page_) {
-    UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtStop",
-                                     -committed_error_page_info_->error.reason,
-                                     net::GetAllErrorCodesForUma());
-    UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", auto_reload_count_);
-  }
   if (committed_error_page_info_) {
     committed_error_page_info_->navigation_correction_url = GURL();
     committed_error_page_info_->navigation_correction_request_body.clear();
@@ -342,6 +365,12 @@ void NetErrorHelperCore::CancelPendingFetches() {
 }
 
 void NetErrorHelperCore::OnStop() {
+  if (committed_error_page_info_ &&
+      (committed_error_page_info_->auto_reload_triggered ||
+       auto_reload_timer_->IsRunning())) {
+    ReportAutoReloadFailure(committed_error_page_info_->error,
+                            auto_reload_count_);
+  }
   CancelPendingFetches();
   auto_reload_count_ = 0;
 }
@@ -360,7 +389,7 @@ void NetErrorHelperCore::OnStartLoad(FrameType frame_type, PageType page_type) {
   }
 }
 
-void NetErrorHelperCore::OnCommitLoad(FrameType frame_type) {
+void NetErrorHelperCore::OnCommitLoad(FrameType frame_type, const GURL& url) {
   if (frame_type != MAIN_FRAME)
     return;
 
@@ -383,17 +412,13 @@ void NetErrorHelperCore::OnCommitLoad(FrameType frame_type) {
   navigation_from_button_ = NO_BUTTON;
 
   if (committed_error_page_info_ && !pending_error_page_info_ &&
-      can_auto_reload_page_) {
-    int reason = committed_error_page_info_->error.reason;
-    UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtSuccess",
-                                     -reason,
-                                     net::GetAllErrorCodesForUma());
-    UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtSuccess", auto_reload_count_);
-    if (auto_reload_count_ == 1) {
-      UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtFirstSuccess",
-                                       -reason,
-                                       net::GetAllErrorCodesForUma());
-    }
+      committed_error_page_info_->auto_reload_triggered) {

[mmenke, 2014/04/28 20:21:14]

Right...So whenever we destroy committed_error_page_info_, we update the stats
if auto_reload_triggered is true, unless we're committing another error page. 
Sounds good.

[Randy Smith (Not in Mondays), 2014/04/28 21:05:52]

Why aren't we recording a failure if we're committing another error page?

[Elly Fong-Jones, 2014/04/30 15:34:08]

Done.

[Elly Fong-Jones, 2014/04/30 15:34:08]

Because another error page is just a reload attempt that failed. An actual
auto-reload failure is when the user navigates somewhere else or closes the tab
because auto-reload didn't do its job.

+    const blink::WebURLError& error = committed_error_page_info_->error;
+    const GURL& error_url = error.unreachableURL;

[mmenke, 2014/04/28 20:21:14]

Does this work?  error.unreachableURL is a WebURL, not a GURL....

[Elly Fong-Jones, 2014/04/30 15:34:08]

Yes, because WebURL has a GURL operator() on it.

+    if (url == error_url)
+      ReportAutoReloadSuccess(error, auto_reload_count_);
+    else if (url != GURL(content::kUnreachableWebDataURL))

[Randy Smith (Not in Mondays), 2014/04/28 21:05:52]

What is the purpose of this test?

[Elly Fong-Jones, 2014/04/30 15:34:08]

To avoid logging a failure if we're just loading a new error page.

+      ReportAutoReloadFailure(error, auto_reload_count_);
   }
 
   committed_error_page_info_.reset(pending_error_page_info_.release());
@@ -635,6 +660,8 @@ bool NetErrorHelperCore::MaybeStartAutoReloadTimer() {
 
   DCHECK(IsReloadableError(*committed_error_page_info_));
 
+  committed_error_page_info_->auto_reload_triggered = true;

[mmenke, 2014/04/28 20:21:14]

So if we never reload because we're offline, but the user triggers a reload and
it works (while offline?  Hrm...), then we count it as a failure?  I don't think
that's a problem.

[Elly Fong-Jones, 2014/04/30 15:34:08]

Right.

+
   if (!online_)
     return false;
 
@@ -646,13 +673,18 @@ void NetErrorHelperCore::StartAutoReloadTimer() {
   DCHECK(committed_error_page_info_);
   DCHECK(can_auto_reload_page_);
   base::TimeDelta delay = GetAutoReloadTime(auto_reload_count_);
-  auto_reload_count_++;
   auto_reload_timer_->Stop();
   auto_reload_timer_->Start(FROM_HERE, delay,
-      base::Bind(&NetErrorHelperCore::Reload,
+      base::Bind(&NetErrorHelperCore::AutoReloadTimerFired,
                  base::Unretained(this)));
 }
 
+void NetErrorHelperCore::AutoReloadTimerFired() {
+  auto_reload_count_++;
+  committed_error_page_info_->auto_reload_triggered = true;

[mmenke, 2014/04/28 20:21:14]

This isn't needed any more, is it?

[Elly Fong-Jones, 2014/04/30 15:34:08]

Done.

+  Reload();
+}
+
 void NetErrorHelperCore::NetworkStateChanged(bool online) {
   online_ = online;
   if (auto_reload_timer_->IsRunning()) {