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

components/offline_pages/background/request_coordinator.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 "components/offline_pages/background/request_coordinator.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 119 matching lines @@
                                  weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void RequestCoordinator::GetQueuedRequestsCallback(
     const GetRequestsCallback& callback,
     RequestQueue::GetRequestsResult result,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
   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;
     active_request_.reset();
   }
 
 }
 
 void RequestCoordinator::GetRequestsForSchedulingCallback(
     RequestQueue::GetRequestsResult result,
@@ skipping 16 matching lines @@
 }
 
 bool RequestCoordinator::CancelActiveRequestIfItMatches(
     const std::vector<int64_t>& request_ids) {
   // If we have a request in progress and need to cancel it, call the
   // pre-renderer to cancel.  TODO Make sure we remove any page created by the
   // prerenderer if it doesn't get the cancel in time.
   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;
     }
   }
 
   return false;
 }
 
 void RequestCoordinator::AbortRequestAttempt(SavePageRequest* request) {
   request->MarkAttemptAborted();
   if (request->started_attempt_count() >= policy_->GetMaxStartedTries()) {
@@ skipping 134 matching lines @@
 }
 
 void RequestCoordinator::ScheduleAsNeeded() {
   // Get all requests from queue (there is no filtering mechanism).
   queue_->GetRequests(
       base::Bind(&RequestCoordinator::GetRequestsForSchedulingCallback,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 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);
 }
 
 // Returns true if the caller should expect a callback, false otherwise. For
 // instance, this would return false if a request is already in progress.
 bool RequestCoordinator::StartProcessing(
     const DeviceConditions& device_conditions,
     const base::Callback<void(bool)>& callback) {
@@ skipping 108 matching lines @@
           updated_request, base::Bind(&RequestCoordinator::OfflinerDoneCallback,
                                       weak_ptr_factory_.GetWeakPtr()))) {
     // Offliner accepted request so update it in the queue.
     queue_->UpdateRequest(updated_request,
                           base::Bind(&RequestCoordinator::UpdateRequestCallback,
                                      weak_ptr_factory_.GetWeakPtr(),
                                      updated_request.client_id()));
 
     // 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";
     StopProcessing();
   }
 }
 
 void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
                                               Offliner::RequestStatus status) {
   DVLOG(2) << "offliner finished, saved: "
@@ skipping 98 matching lines @@
   FOR_EACH_OBSERVER(Observer, observers_, OnChanged(request));
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages