[Offline Pages] Adds UMA for number of started attempts and for network connection when Unsupported…

components/offline_pages/background/request_coordinator.cc

Index: components/offline_pages/background/request_coordinator.cc
diff --git a/components/offline_pages/background/request_coordinator.cc b/components/offline_pages/background/request_coordinator.cc
index d6fc9858312ded63053eb3844293a68d5ec9bab4..0f032e227776ae1c7d9607b00a62957b8597fad2 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -45,6 +45,21 @@ void RecordOfflinerResultUMA(const ClientId& client_id,
   histogram->Add(static_cast<int>(request_status));
 }
 
+// Records the number of started attempts for completed requests (whether
+// successful or not).
+void RecordAttemptCount(const SavePageRequest& request,

[Pete Williamson, 2016/09/01 00:27:20]

Maybe this should be RecordStartedAttemptCount(), and we should have a
corresponding RecordCompletedAttemptCount().

[dougarnett, 2016/09/01 19:27:37]

Actually, my thought was to just add the completed attempt UMA in this same
method. Adding TODO to capture that intent.

+                        RequestNotifier::SavePageStatus status) {
+  if (status == RequestNotifier::SavePageStatus::SUCCESS) {
+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        "OfflinePages.Background.RequestSuccess.StartedAttemptCount",
+        request.started_attempt_count(), 1, 10, 10);
+  } else {
+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        "OfflinePages.Background.RequestFailure.StartedAttemptCount",
+        request.started_attempt_count(), 1, 10, 10);
+  }
+}
+
 // This should use the same algorithm as we use for OfflinePageItem, so the IDs
 // are similar.
 int64_t GenerateOfflineId() {
@@ -119,13 +134,9 @@ void RequestCoordinator::GetQueuedRequestsCallback(
 
 void RequestCoordinator::StopPrerendering() {
   if (offliner_ && is_busy_) {
+    DCHECK(active_request_.get());
     offliner_->Cancel();
-    // Find current request and mark attempt aborted.
-    active_request_->MarkAttemptAborted();
-    queue_->UpdateRequest(*(active_request_.get()),
-                          base::Bind(&RequestCoordinator::UpdateRequestCallback,
-                                     weak_ptr_factory_.GetWeakPtr(),
-                                     active_request_->client_id()));
+    AbortRequestAttempt(active_request_.get());
   }
 
   // Stopping offliner means it will not call callback.
@@ -157,6 +168,28 @@ bool RequestCoordinator::CancelActiveRequestIfItMatches(
   return false;
 }
 
+void RequestCoordinator::AbortRequestAttempt(SavePageRequest* request) {

[Pete Williamson, 2016/09/01 00:27:20]

This splits the responsibility for garbage collecting obsolete things from the
queue between RC and RequestPicker classes.  I wonder if it would be better to
do it all in the RequestPicker.  What do you think?

[dougarnett, 2016/09/01 19:27:37]

We are deleting the DoneCallback requests in other cases here so there is some
argument that it should be consistently done here as we know about it. Also, we
don't call the Picker for the abort path now, so they would linger in the queue
until next schedule window (and could cause confusion wrt queue size or showing
up in internals page).

I think the Picker garbage collection is a secondary yet important safeguard
against crash loops (as well as needed for expired requests). We might want
different UMA (and possibly different notification status) for the garbage
collected cases on starts to yield insights on potential crashes or being killed
by Android.

+  request->MarkAttemptAborted();
+  if (request->started_attempt_count() >= policy_->GetMaxStartedTries()) {
+    RemoveAttemptedRequest(*request, SavePageStatus::START_COUNT_EXCEEDED);
+  } else {
+    queue_->UpdateRequest(
+        *request,
+        base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                   weak_ptr_factory_.GetWeakPtr(), request->client_id()));
+  }
+}
+
+void RequestCoordinator::RemoveAttemptedRequest(const SavePageRequest& request,
+                                                SavePageStatus status) {
+  std::vector<int64_t> remove_requests;
+  remove_requests.push_back(request.request_id());
+  queue_->RemoveRequests(remove_requests,
+                         base::Bind(&RequestCoordinator::HandleRemovedRequests,
+                                    weak_ptr_factory_.GetWeakPtr(), status));
+  RecordAttemptCount(request, status);
+}
+
 void RequestCoordinator::RemoveRequests(
     const std::vector<int64_t>& request_ids,
     const RemoveRequestsCallback& callback) {
@@ -412,37 +445,22 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
     // TODO(dougarnett): See if we can conclusively identify other attempt
     // aborted cases to treat this way (eg, for Render Process Killed).
     SavePageRequest updated_request(request);
-    updated_request.MarkAttemptAborted();
-    queue_->UpdateRequest(updated_request,
-                          base::Bind(&RequestCoordinator::UpdateRequestCallback,
-                                     weak_ptr_factory_.GetWeakPtr(),
-                                     updated_request.client_id()));
+    AbortRequestAttempt(&updated_request);
     SavePageStatus notify_status =
         (status == Offliner::RequestStatus::FOREGROUND_CANCELED)
             ? SavePageStatus::FOREGROUND_CANCELED
             : SavePageStatus::PRERENDER_CANCELED;
     NotifyCompleted(updated_request, notify_status);
-
   } else if (status == Offliner::RequestStatus::SAVED) {
     // Remove the request from the queue if it succeeded.
-    std::vector<int64_t> remove_requests;
-    remove_requests.push_back(request.request_id());
-    queue_->RemoveRequests(
-        remove_requests,
-        base::Bind(&RequestCoordinator::HandleRemovedRequests,
-                   weak_ptr_factory_.GetWeakPtr(), SavePageStatus::SUCCESS));
+    RemoveAttemptedRequest(request, SavePageStatus::SUCCESS);
   } else if (request.completed_attempt_count() + 1 >=
              policy_->GetMaxCompletedTries()) {
-    // Remove from the request queue if we exceeeded max retries. The +1
+    // Remove from the request queue if we exceeded max retries. The +1
     // represents the request that just completed. Since we call
     // MarkAttemptCompleted within the if branches, the completed_attempt_count
     // has not yet been updated when we are checking the if condition.
-    std::vector<int64_t> remove_requests;
-    remove_requests.push_back(request.request_id());
-    queue_->RemoveRequests(
-        remove_requests, base::Bind(&RequestCoordinator::HandleRemovedRequests,
-                                    weak_ptr_factory_.GetWeakPtr(),
-                                    SavePageStatus::RETRY_COUNT_EXCEEDED));
+    RemoveAttemptedRequest(request, SavePageStatus::RETRY_COUNT_EXCEEDED);
   } else {
     // If we failed, but are not over the limit, update the request in the
     // queue.