[Offline pages] Add navigation error handling to background loader.

chrome/browser/android/offline_pages/background_loader_offliner.cc

Index: chrome/browser/android/offline_pages/background_loader_offliner.cc
diff --git a/chrome/browser/android/offline_pages/background_loader_offliner.cc b/chrome/browser/android/offline_pages/background_loader_offliner.cc
index fc4243f26ba82118c2b86cebcb62c63cadc13ddc..d8fc0786426058480efc05c08398fa03b582e7f6 100644
--- a/chrome/browser/android/offline_pages/background_loader_offliner.cc
+++ b/chrome/browser/android/offline_pages/background_loader_offliner.cc
@@ -13,6 +13,7 @@
 #include "components/offline_pages/core/client_namespace_constants.h"
 #include "components/offline_pages/core/offline_page_model.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 
 namespace offline_pages {
@@ -25,6 +26,7 @@ BackgroundLoaderOffliner::BackgroundLoaderOffliner(
       offline_page_model_(offline_page_model),
       is_low_end_device_(base::SysInfo::IsLowEndDevice()),
       save_state_(NONE),
+      is_error_(false),
       weak_ptr_factory_(this) {
   DCHECK(offline_page_model_);
   DCHECK(browser_context_);
@@ -127,8 +129,17 @@ void BackgroundLoaderOffliner::DidStopLoading() {
     return;
   }
 
-  save_state_ = SAVING;
   SavePageRequest request(*pending_request_.get());
+  // If there was an error navigating to page, return loading failed.
+  if (is_error_) {
+    completion_callback_.Run(
+        request,
+        Offliner::RequestStatus::LOADING_FAILED_NO_RETRY);

[Pete Williamson, 2017/01/25 18:34:26]

Why NO_RETRY?  It might be a malformed URL, in which case NO_RETRY is right, but
maybe the net being down also results in an error, in which case retrying is
appropriate.

[chili, 2017/01/31 20:07:37]

This is a good point.  While my gut feeling says there are more NO_RETRY cases
than RETRY cases, it'll probably be more detrimental user experience if we
mis-mark a retriable case as no_retry than the other way around.

I added a case statement for most popular/common non-retry cases. Please review
:)

+    ResetState();
+    return;
+  }
+
+  save_state_ = SAVING;
   content::WebContents* web_contents(
       content::WebContentsObserver::web_contents());
 
@@ -168,12 +179,21 @@ void BackgroundLoaderOffliner::RenderProcessGone(
 void BackgroundLoaderOffliner::WebContentsDestroyed() {
   if (pending_request_) {
     SavePageRequest request(*pending_request_.get());
-    completion_callback_.Run(*pending_request_.get(),
+    completion_callback_.Run(request,
                              Offliner::RequestStatus::LOADING_FAILED);
     ResetState();
   }
 }
 
+void BackgroundLoaderOffliner::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  // If there was an error of any kind (certificate, client, DNS, etc),
+  // Mark as error page. Resetting here causes RecordNavigationMetrics to crash.
+  if (navigation_handle->IsErrorPage()) {

[fgorski, 2017/01/25 17:05:14]

nit: {} not needed

[chili, 2017/01/31 20:07:37]

Added a switch case. Keeping the {} :)

+    is_error_ = true;
+  }
+}
+
 void BackgroundLoaderOffliner::OnPageSaved(SavePageResult save_result,
                                            int64_t offline_id) {
   if (!pending_request_)
@@ -200,6 +220,7 @@ void BackgroundLoaderOffliner::OnPageSaved(SavePageResult save_result,
 
 void BackgroundLoaderOffliner::ResetState() {
   pending_request_.reset();
+  is_error_ = false;
   // TODO(chili): Remove after RequestCoordinator can handle multiple offliners.
   // We reset the loader and observer after completion so loaders
   // will not be re-used across different requests/tries. This is a temporary