[Offline Pages] Breaks out new TIMED_OUT Offliner status for RequestCoordinator watchdog timeout in…

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 d5d37101dede68d04e3dddb2f6d4e57aee048d84..85c211863e9379a00ce24159e90ef728f937f8d2 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -137,18 +137,20 @@ void RequestCoordinator::GetQueuedRequestsCallback(
   callback.Run(std::move(requests));
 }
 
-void RequestCoordinator::StopPrerendering() {
+void RequestCoordinator::StopPrerendering(Offliner::RequestStatus stop_status) {
   if (offliner_ && is_busy_) {
     DCHECK(active_request_.get());
     offliner_->Cancel();
     AbortRequestAttempt(active_request_.get());
   }
 
-  // Stopping offliner means it will not call callback.
-  last_offlining_status_ =
-      Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED;
+  // Stopping offliner means it will not call callback so set last status.
+  last_offlining_status_ = stop_status;
 
   if (active_request_) {
+    event_logger_.RecordOfflinerResult(active_request_->client_id().name_space,
+                                       last_offlining_status_,
+                                       active_request_->request_id());
     RecordOfflinerResultUMA(active_request_->client_id(),
                             last_offlining_status_);
     is_busy_ = false;
@@ -185,7 +187,7 @@ bool RequestCoordinator::CancelActiveRequestIfItMatches(
   if (active_request_ != nullptr) {
     if (request_ids.end() != std::find(request_ids.begin(), request_ids.end(),
                                        active_request_->request_id())) {
-      StopPrerendering();
+      StopPrerendering(Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED);

[Pete Williamson, 2016/09/21 00:29:09]

This is really the user deciding to cancel, not the request coordinator.  Should
we have a different status for user cancelling?

[dougarnett, 2016/09/21 15:58:56]

We don't actually know at this point whether this is due to a user Pause or user
Remove (the two current callers). We could consider plumbing finer grained
status into this method. But I'm not sure yet we want to expand too much into RC
land causes. This Offliner::RequestStatus was originally about the offline
processing disposition (the notifier status is more about the request
disposition). It does makes sense to me to split out the timeouts though as this
means the offlining took too long vs. a cancel due to some other activity
outside the scope of offlining. I wonder if separately we want UMA for Pause and
Remove and other user driven actions (they would correlate to these cancels for
the subset of cases where the request was active).

       return true;
     }
   }
@@ -340,8 +342,17 @@ void RequestCoordinator::ScheduleAsNeeded() {
 }
 
 void RequestCoordinator::StopProcessing() {
+  StopProcessingWithStatus(Offliner::REQUEST_COORDINATOR_CANCELED);
+}
+
+void RequestCoordinator::HandleWatchdogTimeout() {
+  StopProcessingWithStatus(Offliner::REQUEST_COORDINATOR_TIMED_OUT);
+}
+
+void RequestCoordinator::StopProcessingWithStatus(
+    Offliner::RequestStatus stop_status) {
   is_stopped_ = true;
-  StopPrerendering();
+  StopPrerendering(stop_status);
 
   // Let the scheduler know we are done processing.
   scheduler_callback_.Run(true);
@@ -470,7 +481,7 @@ void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
 
     // Start a watchdog timer to catch pre-renders running too long
     watchdog_timer_.Start(FROM_HERE, offliner_timeout_, this,
-                          &RequestCoordinator::StopProcessing);
+                          &RequestCoordinator::HandleWatchdogTimeout);
   } else {
     is_busy_ = false;
     DVLOG(0) << "Unable to start LoadAndSave";