[Offline Pages] Adds classification of some prerender FinalStatus codes as canceled operations or a…

chrome/browser/android/offline_pages/prerendering_loader.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/prerendering_loader.h"
 
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/network_change_notifier.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace offline_pages {
 
+// Classifies whether final status represent a canceled operation vs. a full

[Dmitry Titov, 2016/09/24 00:53:30]

Could you clarify the comment? Both options sound the same (canceled operation
vs failed attempt). Quite cryptic.

[Pete Williamson, 2016/09/27 19:25:07]

This may be better to return an enum CANCELED or FAILED instead of true or
false.

[dougarnett, 2016/09/29 01:11:23]

Done.

+// attempt that failed.
+bool IsCanceledOperation(prerender::FinalStatus final_status) {

[chili, 2016/09/23 00:44:17]

nit: while both spellings are valid, I think request coordinator used
'Cancelled' for most things

On the other hand, offliner final status uses one L, so feel free to ignore

[dougarnett, 2016/09/23 18:20:01]

Yeah, the Offliner statuses all use single L so that is what drove this method
name (ie, look at where it is used below). Also our Archiver uses single L. I
think we can ignore that Prerender FinalStatus uses double L but I do see our
SavePageResult uses double L and maybe we should fix that one to be more
consistent with our other spellings (maybe something to raise with Filip/Dmitry
though).

+  switch (final_status) {
+    case prerender::FINAL_STATUS_CANCELLED:
+    case prerender::FINAL_STATUS_RECENTLY_VISITED:
+    // TODO(dougarnett): Reconsider if/when get better granularity (642768)
+    case prerender::FINAL_STATUS_UNSUPPORTED_SCHEME:
+      return true;
+    default:
+      return false;
+  }
+}
+
+// Classifies whether final status represent a retryable failure vs. one that
+// should not be retried.

[Pete Williamson, 2016/09/27 19:25:07]

Maybe combine this with the function above, and have RETRYABLE as a potentially
returned enum value.

[dougarnett, 2016/09/29 01:11:23]

Done.

+bool IsRetryableFailure(prerender::FinalStatus final_status) {
+  switch (final_status) {
+    case prerender::FINAL_STATUS_SAFE_BROWSING:
+    case prerender::FINAL_STATUS_CREATING_AUDIO_STREAM:
+    case prerender::FINAL_STATUS_JAVASCRIPT_ALERT:
+      return false;
+    default:
+      return true;
+  }
+}
+
 PrerenderingLoader::PrerenderingLoader(content::BrowserContext* browser_context)
     : state_(State::IDLE),
       snapshot_controller_(nullptr),
       browser_context_(browser_context) {
   adapter_.reset(new PrerenderAdapter(this));
 }
 
 PrerenderingLoader::~PrerenderingLoader() {
   CancelPrerender();
 }
@@ skipping 124 matching lines @@
   // Loading has stopped so unless the Loader has already transistioned to the
   // idle state, clean up the previous request state, transition to the idle
   // state, and post callback.
   // Note: it is possible to receive some asynchronous stopped indication after
   // the request has completed/stopped via another path so the Loader may be
   // idle at this point.
 
   if (IsIdle())
     return;
 
-  // Request status depends on whether we are still loading (failed) or
-  // did load and then loading was stopped (cancel - from prerender stack).
+  // If we have already loaded, then the prerender stack is now canceling

[Dmitry Titov, 2016/09/24 00:53:30]

Same, this comment is not clear. loaded means cancel? Also, this 'baseline with
refinement' - can it all become just a single switch or a series of if
statements?

[dougarnett, 2016/09/29 01:11:23]

Done.

+  // the operation (before we may be done with the WebContents). Otherwise,
+  // the baseline case is a prerender failure but we will consider the
+  // prerender final status subsequently to refine the status.
   Offliner::RequestStatus request_status =
       IsLoaded() ? Offliner::RequestStatus::PRERENDERING_CANCELED
                  : Offliner::RequestStatus::PRERENDERING_FAILED;
 
   if (adapter_->IsActive()) {
     prerender::FinalStatus final_status = adapter_->GetFinalStatus();
     DVLOG(1) << "Load failed: " << final_status;
 
+    if (IsCanceledOperation(final_status)) {
+      request_status = Offliner::RequestStatus::PRERENDERING_CANCELED;
+    } else if (!IsRetryableFailure(final_status)) {
+      request_status = Offliner::RequestStatus::PRERENDERING_FAILED_NO_RETRY;
+    }
+
     // Loss of network connection can show up as unsupported scheme per
     // a redirect to a special data URL is used to navigate to error page.
-    // We want to be able to retry these request so for now treat any
-    // unsupported scheme error as a cancel.
-    // TODO(dougarnett): Use new FinalStatus code if/when supported (642768).
-    // TODO(dougarnett): Create whitelist of final status codes that should
-    // not be considered failures (and define new RequestStatus code for them).
-    if (adapter_->GetFinalStatus() ==
-        prerender::FinalStatus::FINAL_STATUS_UNSUPPORTED_SCHEME) {
-      request_status = Offliner::RequestStatus::PRERENDERING_CANCELED;
+    // Capture the current connectivity here in case we can leverage that
+    // to differentiate how to treat it.
+    if (final_status == prerender::FINAL_STATUS_UNSUPPORTED_SCHEME) {

[Pete Williamson, 2016/09/27 19:25:07]

Why stop emitting UMA for cancels?

[dougarnett, 2016/09/29 01:11:23]

There was no UMA for cancels here - just setting the request_status which has
moved.

       UMA_HISTOGRAM_ENUMERATION(
           "OfflinePages.Background.UnsupportedScheme.ConnectionType",
           net::NetworkChangeNotifier::GetConnectionType(),
           net::NetworkChangeNotifier::ConnectionType::CONNECTION_LAST + 1);
     }
     adapter_->DestroyActive();
   }
 
   snapshot_controller_.reset(nullptr);
   session_contents_.reset(nullptr);
   state_ = State::IDLE;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(callback_, request_status, nullptr));
 }
 
 void PrerenderingLoader::CancelPrerender() {
   if (adapter_->IsActive()) {
     adapter_->DestroyActive();
   }
   snapshot_controller_.reset(nullptr);
   session_contents_.reset(nullptr);
   state_ = State::IDLE;
 }
 
 }  // namespace offline_pages