Surface button for loading stale cache copy on net error page.

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"
 #include "base/i18n/rtl.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/location.h"
+#include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string16.h"
 #include "base/values.h"
 #include "chrome/common/localized_error.h"
 #include "grit/generated_resources.h"
 #include "net/base/escape.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/platform/WebURLError.h"
@@ skipping 221 matching lines @@
   return params.release();
 }
 
 }  // 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) {
   }
 
   // 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.
   bool needs_dns_updates;
 
   // Navigation correction service url, which will be used in response to
   // certain types of network errors.  This is also stored by the
   // NetErrorHelperCore itself, but it stored here as well in case its modified
   // in the middle of an error page load.  Empty when no error page should be
   // fetched, or if there's already a fetch in progress.
   GURL navigation_correction_url;
 
   // Request body to use when requesting corrections from a web service.
   // TODO(mmenke):  Investigate loading the error page at the same time as
   //                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;
 };
 
 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)
     : delegate_(delegate),
       last_probe_status_(chrome_common_net::DNS_PROBE_POSSIBLE),
       auto_reload_enabled_(false),
       auto_reload_timer_(new base::Timer(false, false)),
       // TODO(ellyjones): Make online_ accurate at object creation.
       online_(true),
       auto_reload_count_(0),
-      can_auto_reload_page_(false) {
+      can_auto_reload_page_(false),
+      navigation_from_button_(NO_BUTTON) {
 }
 
 NetErrorHelperCore::~NetErrorHelperCore() {
   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_);
   }
 }
@@ skipping 37 matching lines @@
   } 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) {
   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 ?

[jar (doing other things), 2014/04/21 23:48:19]

nit: This will fit on the previous line...and then you can avoid the
questionable double-indent you applied on the next two line (standard 4 space
indent is probably more style compliant).

[mmenke, 2014/04/22 00:24:59]

While the Google style guide doesn't explicitly discuss single arguments that
take up multiple-lines, this does seem to fit more closely what it does say than
what you suggest.

The rules it gives:

"If the arguments do not all fit on one line, they should be broken up onto
multiple lines, with each subsequent line aligned with the first argument. Do
not add spaces after the open paren or before the close paren"

"Arguments may optionally all be placed on subsequent lines, with one line per
argument"

In particular, your suggestion seems to violate the "with each subsequent line
aligned with the first argument".  (This follows it, with the addendum that the
subsequent lines of each argument are indented 4 spaces)

[jar (doing other things), 2014/04/22 17:53:43]

If you really prefer to start a new line... that's fine... 

nit:  ... just don't indent the next two lines "extra" because there is a "?" on
the previous line.  This is all one monster line continuation, and there is no
reason to indent further on additional lines

[Randy Smith (Not in Mondays), 2014/04/22 20:40:02]

I formatted it this way in response to Matt's comment in
https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net...
.  I don't particularly care, so I'm happy to do whatever you and Matt agree on,
but in the absence of an agreement, I'll leave it the way I've already landed
it.

[mmenke, 2014/04/22 21:03:27]

The style guide says nothing about this case, so we can't really rely on it.

I find this a lot clearer because it makes it clear these are continuation
lines, rather than separate parameters.  I don't think jar and I are going to
reach consensus on this, other than perhaps by searching the codebase and
finding out what the most common style is.

One option is just to write out a whole else/if statement, I guess.

[jar (doing other things), 2014/04/23 01:52:01]

As noted, this is a nit.  I couldn't find any reason for adding additional
indentation.

To be honest, I think your indentation makes some sense... I just couldn't find
any justification for doing so... and could see that IF you did this... then you
could keep on indenting more and more "whenever it feels right" ... and I didn't
like where that was going.

When I see extra indents, I try to figure out why, as "I must have missed
something."  

Bottom line: Leave it as is if you feel strongly that it helps more than it
hurts (folks like me that are surprised by it).

+            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_) {
     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_.reset(pending_error_page_info_.release());
 }
 
 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;
 
-  // Only enable stale cache JS bindings if this wasn't a post.
-  if (!committed_error_page_info_->was_failed_post) {
-    delegate_->EnableStaleLoadBindings(
-        committed_error_page_info_->error.unreachableURL);
+  chrome_common_net::RecordEvent(chrome_common_net::NETWORK_ERROR_PAGE_SHOWN);
+  if (committed_error_page_info_->reload_button_in_page) {
+    chrome_common_net::RecordEvent(
+        chrome_common_net::NETWORK_ERROR_PAGE_RELOAD_BUTTON_SHOWN);
   }
+  if (committed_error_page_info_->load_stale_button_in_page) {
+    chrome_common_net::RecordEvent(
+        chrome_common_net::NETWORK_ERROR_PAGE_LOAD_STALE_BUTTON_SHOWN);
+  }
+
+  delegate_->EnablePageHelperFunctions();
 
   if (committed_error_page_info_->navigation_correction_url.is_valid()) {
     // If there is another pending error page load, |fix_url| should have been
     // cleared.
     DCHECK(!pending_error_page_info_);
     DCHECK(!committed_error_page_info_->needs_dns_updates);
     delegate_->FetchNavigationCorrections(
         committed_error_page_info_->navigation_correction_url,
         committed_error_page_info_->navigation_correction_request_body);
   } else if (auto_reload_enabled_ &&
@@ skipping 57 matching lines @@
     pending_error_page_info_.reset(new ErrorPageInfo(error, is_failed_post));
     // Skip DNS logic if suggestions were received from a remote server.
     if (IsDnsError(error) && !params) {
       // This is not strictly necessary, but waiting for a new status to be
       // sent as a result of the DidFinishLoading call keeps the histograms
       // consistent with older versions of the code, at no real cost.
       last_probe_status_ = chrome_common_net::DNS_PROBE_POSSIBLE;
 
       delegate_->GenerateLocalizedErrorPage(
           GetUpdatedError(error), is_failed_post, params.Pass(),
+          &pending_error_page_info_->reload_button_in_page,
+          &pending_error_page_info_->load_stale_button_in_page,
           error_html);
       pending_error_page_info_->needs_dns_updates = true;
       return;
     }
   }
 
-  delegate_->GenerateLocalizedErrorPage(error, is_failed_post,
-                                        params.Pass(), error_html);
+  bool reload_button_in_page = false;
+  bool load_stale_button_in_page = false;
+  delegate_->GenerateLocalizedErrorPage(
+      error, is_failed_post, params.Pass(),
+      &reload_button_in_page, &load_stale_button_in_page, error_html);
+  if (pending_error_page_info_ && frame_type == MAIN_FRAME) {
+    pending_error_page_info_->reload_button_in_page = reload_button_in_page;
+    pending_error_page_info_->load_stale_button_in_page =
+        load_stale_button_in_page;
+  }
 }
 
 void NetErrorHelperCore::OnNetErrorInfo(
     chrome_common_net::DnsProbeStatus status) {
   DCHECK_NE(chrome_common_net::DNS_PROBE_POSSIBLE, status);
 
   last_probe_status_ = status;
 
   if (!committed_error_page_info_ ||
       !committed_error_page_info_->needs_dns_updates ||
@@ skipping 24 matching lines @@
 
   UMA_HISTOGRAM_ENUMERATION("DnsProbe.ErrorPageUpdateStatus",
                             last_probe_status_,
                             chrome_common_net::DNS_PROBE_MAX);
   // Every status other than DNS_PROBE_POSSIBLE and DNS_PROBE_STARTED is a
   // final status code.  Once one is reached, the page does not need further
   // updates.
   if (last_probe_status_ != chrome_common_net::DNS_PROBE_STARTED)
     committed_error_page_info_->needs_dns_updates = false;
 
+  // There is no need to worry about the button display statistics here because
+  // the presentation of the reload and load stale buttons can't be changed
+  // by a DNS error update.
   delegate_->UpdateErrorPage(
       GetUpdatedError(committed_error_page_info_->error),
       committed_error_page_info_->was_failed_post);
 }
 
 void NetErrorHelperCore::OnNavigationCorrectionsFetched(
     const std::string& corrections,
     const std::string& accept_languages,
     bool is_rtl) {
   // Loading suggestions only starts when a blank error page finishes loading,
@@ skipping 112 matching lines @@
   if (error_url != url)
     return false;
 
   // The first iteration of the timer is started by OnFinishLoad calling
   // MaybeStartAutoReloadTimer, but since error pages for subsequent loads are
   // suppressed in this function, subsequent iterations of the timer have to be
   // started here.
   MaybeStartAutoReloadTimer();
   return true;
 }
+
+void NetErrorHelperCore::ExecuteButtonPress(Button button) {
+  switch (button) {
+    case RELOAD_BUTTON:
+      chrome_common_net::RecordEvent(
+          chrome_common_net::NETWORK_ERROR_PAGE_RELOAD_BUTTON_CLICKED);
+      navigation_from_button_ = RELOAD_BUTTON;
+      Reload();
+      return;
+    case LOAD_STALE_BUTTON:
+      chrome_common_net::RecordEvent(
+          chrome_common_net::NETWORK_ERROR_PAGE_LOAD_STALE_BUTTON_CLICKED);
+      navigation_from_button_ = LOAD_STALE_BUTTON;
+      delegate_->LoadPageFromCache(
+          committed_error_page_info_->error.unreachableURL);
+      return;
+    case MORE_BUTTON:
+      // 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;
+  }
+}
+