Fix auto-reload histograms.

chrome/renderer/net/net_error_helper_core.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/net/net_error_helper_core.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 238 matching lines @@
 
 }  // namespace
 
 struct NetErrorHelperCore::ErrorPageInfo {
   ErrorPageInfo(blink::WebURLError error, bool was_failed_post)
       : error(error),
         was_failed_post(was_failed_post),
         needs_dns_updates(false),
         reload_button_in_page(false),
         load_stale_button_in_page(false),
-        is_finished_loading(false) {
+        is_finished_loading(false),
+        is_auto_reloading(false) {
   }
 
   // Information about the failed page load.
   blink::WebURLError error;
   bool was_failed_post;
 
   // Information about the status of the error page.
 
   // True if a page is a DNS error page and has not yet received a final DNS
   // probe status.
@@ skipping 11 matching lines @@
   //                the blank page is loading, to get rid of these.
   std::string navigation_correction_request_body;
 
   // Track if specific buttons are included in an error page, for statistics.
   bool reload_button_in_page;
   bool load_stale_button_in_page;
 
   // True if a page has completed loading, at which point it can receive
   // updates.
   bool is_finished_loading;
+
+  // True if the page is being auto-reloaded.
+  bool is_auto_reloading;

[mmenke, 2014/04/25 14:57:42]

Think triggered_auto_reload would be clearer (Makes it clearer its set on the
old ErrorPageInfo, and not the one for the reloaded page, if the reload results
in another error page).

[Elly Fong-Jones, 2014/04/25 20:01:26]

Done.

 };
 
 bool NetErrorHelperCore::IsReloadableError(
     const NetErrorHelperCore::ErrorPageInfo& info) {
   return info.error.domain.utf8() == net::kErrorDomain &&
          info.error.reason != net::ERR_ABORTED &&
          !info.was_failed_post;
 }
 
 NetErrorHelperCore::NetErrorHelperCore(Delegate* delegate)
@@ skipping 14 matching lines @@
                                      -committed_error_page_info_->error.reason,
                                      net::GetAllErrorCodesForUma());
     UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", auto_reload_count_);
   }
 }
 
 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();
   }
   if (pending_error_page_info_) {
     pending_error_page_info_->navigation_correction_url = GURL();
     pending_error_page_info_->navigation_correction_request_body.clear();
   }
   delegate_->CancelFetchNavigationCorrections();
   auto_reload_timer_->Stop();
   can_auto_reload_page_ = false;
 }
 
 void NetErrorHelperCore::OnStop() {
+  if (committed_error_page_info_ &&
+      (committed_error_page_info_->is_auto_reloading ||
+       auto_reload_timer_->IsRunning())) {

[mmenke, 2014/04/25 14:57:42]

So if a user presses stop while the autoreload timer is running, we count it as
an error, but if they trigger a new navigation while it's running, we don't,
unless it's a cross-process navigation?

[Elly Fong-Jones, 2014/04/25 20:01:26]

If they trigger a new navigation while it's running, I think we just get another
OnStart, right? and then when the different URL commits, we'll record that as an
error.

[mmenke, 2014/04/25 20:08:03]

But autoreload won't have been triggered.  We record a failure if:

* Page errors out, error page commits, we start the autoreload timer, stop is
pressed (Record failure here).

But we don't if:

* Page errors out, error page commits, we start the autoreload timer, new
navigation started by used (stopping the timer), new page commits.

auto_reload_triggered is false in the second case (first, too), so we don't
record anything.

+    ReportAutoReloadFailure(committed_error_page_info_->error.reason,
+                            auto_reload_count_);
+  }
   CancelPendingFetches();
   auto_reload_count_ = 0;
 }
 
 void NetErrorHelperCore::OnStartLoad(FrameType frame_type, PageType page_type) {
   if (frame_type != MAIN_FRAME)
     return;
 
   // If there's no pending error page information associated with the page load,
   // or the new page is not an error page, then reset pending error page state.
   if (!pending_error_page_info_ || page_type != ERROR_PAGE) {
     CancelPendingFetches();
   } else if (auto_reload_enabled_) {
     // If an error load is starting, the resulting error page is autoreloadable.
     can_auto_reload_page_ = IsReloadableError(*pending_error_page_info_);
   }
 }
 
-void NetErrorHelperCore::OnCommitLoad(FrameType frame_type) {
+void NetErrorHelperCore::OnCommitLoad(FrameType frame_type, const GURL& url) {
   if (frame_type != MAIN_FRAME)
     return;
 
   // Track if an error occurred due to a page button press.
   // This isn't perfect; if (for instance), the server is slow responding
   // to a request generated from the page reload button, and the user hits
   // the browser reload button, this code will still believe the
   // result is from the page reload button.
   if (committed_error_page_info_ && pending_error_page_info_ &&
       navigation_from_button_ != NO_BUTTON &&
       committed_error_page_info_->error.unreachableURL ==
           pending_error_page_info_->error.unreachableURL) {
     DCHECK(navigation_from_button_ == RELOAD_BUTTON ||
            navigation_from_button_ == LOAD_STALE_BUTTON);
     chrome_common_net::RecordEvent(
         navigation_from_button_ == RELOAD_BUTTON ?
             chrome_common_net::NETWORK_ERROR_PAGE_RELOAD_BUTTON_ERROR :
             chrome_common_net::NETWORK_ERROR_PAGE_LOAD_STALE_BUTTON_ERROR);
   }
   navigation_from_button_ = NO_BUTTON;
 
   if (committed_error_page_info_ && !pending_error_page_info_ &&
-      can_auto_reload_page_) {
+      committed_error_page_info_->is_auto_reloading) {
+    const GURL& error_url = committed_error_page_info_->error.unreachableURL;
     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());
-    }
+    if (url == error_url)
+      ReportAutoReloadSuccess(reason, auto_reload_count_);
+    else
+      ReportAutoReloadFailure(reason, auto_reload_count_);
   }
 
   committed_error_page_info_.reset(pending_error_page_info_.release());
 }
 
+void NetErrorHelperCore::ReportAutoReloadSuccess(int error, size_t count) {

[mmenke, 2014/04/25 14:57:42]

These should either be in a private anonymous namespace or static.  Also, they
should ignore the case when committed_error_page_info_->error.domain !=
net::kErrorDomain.  I'd suggest passing in the full WebURLError and having them
check it themselves, since it's easy to forget about that when mucking with
callsites.

[Elly Fong-Jones, 2014/04/25 20:01:26]

Done.

+  UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtSuccess",
+                                   -error,
+                                   net::GetAllErrorCodesForUma());
+  UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtSuccess", count);
+  if (count == 1) {
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtFirstSuccess",
+                                     -error,
+                                     net::GetAllErrorCodesForUma());
+  }
+}
+
+void NetErrorHelperCore::ReportAutoReloadFailure(int error, size_t count) {
+  UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtStop",
+                                   -error,
+                                   net::GetAllErrorCodesForUma());
+  UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", count);
+}
+
 void NetErrorHelperCore::OnFinishLoad(FrameType frame_type) {
   if (frame_type != MAIN_FRAME)
     return;
 
   if (!committed_error_page_info_) {
     auto_reload_count_ = 0;
     return;
   }
 
   committed_error_page_info_->is_finished_loading = true;
@@ skipping 227 matching lines @@
     return false;
 
   StartAutoReloadTimer();
   return true;
 }
 
 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_->is_auto_reloading = true;
+  Reload();
+}
+
 void NetErrorHelperCore::NetworkStateChanged(bool online) {
   online_ = online;
   if (auto_reload_timer_->IsRunning()) {
     DCHECK(committed_error_page_info_);
     // If there's an existing timer running, stop it and reset the retry count.
     auto_reload_timer_->Stop();
     auto_reload_count_ = 0;
   }
 
   // If the network state changed to online, maybe start auto-reloading again.
@@ skipping 49 matching lines @@
       // Visual effects on page are handled in Javascript code.
       chrome_common_net::RecordEvent(
           chrome_common_net::NETWORK_ERROR_PAGE_MORE_BUTTON_CLICKED);
       return;
     case NO_BUTTON:
       NOTREACHED();
       return;
   }
 }