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

chrome/browser/android/offline_pages/background_loader_offliner.cc

 // Copyright 2016 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/browser/android/offline_pages/background_loader_offliner.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offliner_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/offline_pages/core/background/save_page_request.h"
 #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 {
 
 BackgroundLoaderOffliner::BackgroundLoaderOffliner(
     content::BrowserContext* browser_context,
     const OfflinerPolicy* policy,
     OfflinePageModel* offline_page_model)
     : browser_context_(browser_context),
       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_);
 }
 
 BackgroundLoaderOffliner::~BackgroundLoaderOffliner() {}
 
 bool BackgroundLoaderOffliner::LoadAndSave(const SavePageRequest& request,
                                            const CompletionCallback& callback) {
   DCHECK(callback);
@@ skipping 82 matching lines @@
 
   ResetState();
 }
 
 void BackgroundLoaderOffliner::DidStopLoading() {
   if (!pending_request_.get()) {
     DVLOG(1) << "DidStopLoading called even though no pending request.";
     return;
   }
 
+  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;
-  SavePageRequest request(*pending_request_.get());
   content::WebContents* web_contents(
       content::WebContentsObserver::web_contents());
 
   std::unique_ptr<OfflinePageArchiver> archiver(
       new OfflinePageMHTMLArchiver(web_contents));
 
   OfflinePageModel::SavePageParams params;
   params.url = web_contents->GetLastCommittedURL();
   params.client_id = request.client_id();
   params.proposed_offline_id = request.request_id();
@@ skipping 19 matching lines @@
         completion_callback_.Run(request,
                                  Offliner::RequestStatus::LOADING_FAILED);
     }
     ResetState();
   }
 }
 
 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_)
     return;
 
   SavePageRequest request(*pending_request_.get());
   ResetState();
 
   if (save_state_ == DELETE_AFTER_SAVE) {
     save_state_ = NONE;
     return;
   }
 
   save_state_ = NONE;
 
   Offliner::RequestStatus save_status;
   if (save_result == SavePageResult::SUCCESS)
     save_status = RequestStatus::SAVED;
   else
     save_status = RequestStatus::SAVE_FAILED;
 
   completion_callback_.Run(request, save_status);
 }
 
 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
   // solution while there exists assumptions about the number of offliners
   // there are.
   loader_.reset(
       new background_loader::BackgroundLoaderContents(browser_context_));
   content::WebContentsObserver::Observe(loader_.get()->web_contents());
 }
 
 void BackgroundLoaderOffliner::OnApplicationStateChange(
     base::android::ApplicationState application_state) {
   if (pending_request_ && is_low_end_device_ &&
       application_state ==
           base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) {
     DVLOG(1) << "App became active, canceling current offlining request";
     SavePageRequest* request = pending_request_.get();
     Cancel();
     completion_callback_.Run(*request, RequestStatus::FOREGROUND_CANCELED);
   }
 }
 
 }  // namespace offline_pages