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

chrome/browser/android/offline_pages/prerendering_loader.cc

Index: chrome/browser/android/offline_pages/prerendering_loader.cc
diff --git a/chrome/browser/android/offline_pages/prerendering_loader.cc b/chrome/browser/android/offline_pages/prerendering_loader.cc
index bbc1fe0851ac8f1d9753ce70fbdbde02b3ff795a..502260b51b64bc4c70bcc50e57e5d392cc1f1115 100644
--- a/chrome/browser/android/offline_pages/prerendering_loader.cc
+++ b/chrome/browser/android/offline_pages/prerendering_loader.cc
@@ -17,6 +17,33 @@
 
 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),
@@ -161,8 +188,10 @@ void PrerenderingLoader::HandleLoadingStopped() {
   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;
@@ -171,16 +200,17 @@ void PrerenderingLoader::HandleLoadingStopped() {
     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(),