[Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API.

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"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
 #include "components/offline_pages/background/offliner_factory.h"
 #include "components/offline_pages/background/offliner_policy.h"
 #include "components/offline_pages/background/request_picker.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/offline_page_item.h"
+#include "components/offline_pages/offline_page_model.h"
 
 namespace offline_pages {
 
 namespace {
 
 // Records the final request status UMA for an offlining request. This should
 // only be called once per Offliner::LoadAndSave request.
 void RecordOfflinerResultUMA(Offliner::RequestStatus request_status) {
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.Background.OfflinerRequestStatus",
                             request_status,
@@ skipping 20 matching lines @@
   DCHECK(policy_ != nullptr);
   picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
 }
 
 RequestCoordinator::~RequestCoordinator() {}
 
 bool RequestCoordinator::SavePageLater(
     const GURL& url, const ClientId& client_id, bool was_user_requested) {
   DVLOG(2) << "URL is " << url << " " << __func__;
 
+  if (!OfflinePageModel::CanSaveURL(url)) {
+    DVLOG(1) << "Not able to save page for requested url: " << url;
+    return false;
+  }
+
   // TODO(petewil): We need a robust scheme for allocating new IDs.
   static int64_t id = 0;
 
   // Build a SavePageRequest.
   offline_pages::SavePageRequest request(
       id++, url, client_id, base::Time::Now(), was_user_requested);
 
   // Put the request on the request queue.
   queue_->AddRequest(request,
                      base::Bind(&RequestCoordinator::AddRequestResultCallback,
                                 weak_ptr_factory_.GetWeakPtr()));
   return true;
 }
 
 void RequestCoordinator::RemoveRequests(
     const std::vector<ClientId>& client_ids) {
   queue_->RemoveRequestsByClientId(
-      client_ids, base::Bind(&RequestCoordinator::UpdateRequestCallback,
+      client_ids, base::Bind(&RequestCoordinator::UpdateMultipleRequestCallback,
                              weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RequestCoordinator::AddRequestResultCallback(
     RequestQueue::AddRequestResult result,
     const SavePageRequest& request) {
 
   // Inform the scheduler that we have an outstanding task..
   scheduler_->Schedule(GetTriggerConditionsForUserRequest());
 }
 
 // Called in response to updating a request in the request queue.
 void RequestCoordinator::UpdateRequestCallback(
+    const ClientId& client_id, RequestQueue::UpdateRequestResult result) {

[fgorski, 2016/08/04 06:01:34]

please run git cl format.

[dougarnett, 2016/08/04 16:24:38]

Done.

+  // If the request succeeded, nothing to do.  If it failed, we can't really do
+  // much, so just log it.
+  if (result != RequestQueue::UpdateRequestResult::SUCCESS) {
+    DLOG(WARNING) << "Failed to update request attempt details. "

[fgorski, 2016/08/04 06:01:34]

nit: use DVLOG here too

[dougarnett, 2016/08/04 16:24:38]

Done.

+                  << static_cast<int>(result);
+    event_logger_.RecordUpdateRequestFailed(
+        client_id.name_space,
+        RequestQueue::UpdateRequestResultToString(result));
+  }
+}
+
+// Called in response to updating multiple requests in the request queue.
+void RequestCoordinator::UpdateMultipleRequestCallback(
     RequestQueue::UpdateRequestResult result) {
   // If the request succeeded, nothing to do.  If it failed, we can't really do
   // much, so just log it.
   if (result != RequestQueue::UpdateRequestResult::SUCCESS) {
-    // TODO(petewil): Consider adding UMA or showing on offline-internals page.
-    DLOG(WARNING) << "Failed to update a request retry count. "
+    DLOG(WARNING) << "Failed to update request attempt details. "
                   << static_cast<int>(result);
   }
 }
 
 void RequestCoordinator::StopProcessing() {
   is_canceled_ = true;
-  if (offliner_ && is_busy_)
+  if (offliner_ && is_busy_) {
+    // TODO(dougarnett): Find current request and mark attempt aborted.
     offliner_->Cancel();
+  }
 
   // Stopping offliner means it will not call callback.
   last_offlining_status_ =
       Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED;
   RecordOfflinerResultUMA(last_offlining_status_);
 
   // Let the scheduler know we are done processing.
   scheduler_callback_.Run(true);
 }
 
@@ skipping 63 matching lines @@
   GetOffliner();
   if (!offliner_) {
     DVLOG(0) << "Unable to create Offliner. "
              << "Cannot background offline page.";
     return;
   }
 
   DCHECK(!is_busy_);
   is_busy_ = true;
 
+  // Prepare an updated request to attempt.
+  SavePageRequest updated_request(request);
+  updated_request.MarkAttemptStarted(base::Time::Now());
+
   // Start the load and save process in the offliner (Async).
-  offliner_->LoadAndSave(request,
+  if (offliner_->LoadAndSave(updated_request,
                          base::Bind(&RequestCoordinator::OfflinerDoneCallback,

[fgorski, 2016/08/04 06:01:34]

nit: alignment

[dougarnett, 2016/08/04 16:24:38]

Done.

-                                    weak_ptr_factory_.GetWeakPtr()));
+                                    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);
+    // Start a watchdog timer to catch pre-renders running too long
+    watchdog_timer_.Start(FROM_HERE, offliner_timeout_, this,
+                          &RequestCoordinator::StopProcessing);
+  } 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: "
            << (status == Offliner::RequestStatus::SAVED)
            << ", status: " << static_cast<int>(status) << ", " << __func__;
   DCHECK_NE(status, Offliner::RequestStatus::UNKNOWN);
   DCHECK_NE(status, Offliner::RequestStatus::LOADED);
   event_logger_.RecordSavePageRequestUpdated(
       request.client_id().name_space,
-      "Saved",
+      Offliner::RequestStatusToString(status),
       request.request_id());
   last_offlining_status_ = status;
   RecordOfflinerResultUMA(last_offlining_status_);
   watchdog_timer_.Stop();
 
   is_busy_ = false;
 
-  int64_t new_attempt_count = request.attempt_count() + 1;
+  if (status == Offliner::RequestStatus::FOREGROUND_CANCELED) {
+    // Update the request for the canceled attempt.
+    // 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()));
 
-  // Remove the request from the queue if it either succeeded or exceeded the
-  // max number of retries.
-  if (status == Offliner::RequestStatus::SAVED
-       || new_attempt_count > policy_->GetMaxTries()) {
+  } else if (status == Offliner::RequestStatus::SAVED
+       || request.attempt_count() >= policy_->GetMaxTries()) {
+    // Remove the request from the queue if it either succeeded or exceeded the
+    // max number of retries.
     queue_->RemoveRequest(request.request_id(),
                           base::Bind(&RequestCoordinator::UpdateRequestCallback,
-                                     weak_ptr_factory_.GetWeakPtr()));
+                                     weak_ptr_factory_.GetWeakPtr(),
+                                     request.client_id()));
   } else {
     // If we failed, but are not over the limit, update the request in the
     // queue.
     SavePageRequest updated_request(request);
-    updated_request.set_attempt_count(new_attempt_count);
-    updated_request.set_last_attempt_time(base::Time::Now());
-    RequestQueue::UpdateRequestCallback update_callback =
-        base::Bind(&RequestCoordinator::UpdateRequestCallback,
-                   weak_ptr_factory_.GetWeakPtr());
+    updated_request.MarkAttemptCompleted();
     queue_->UpdateRequest(
         updated_request,
         base::Bind(&RequestCoordinator::UpdateRequestCallback,
-                   weak_ptr_factory_.GetWeakPtr()));
+                   weak_ptr_factory_.GetWeakPtr(),
+                   updated_request.client_id()));
   }
 
   // Determine whether we might try another request in this
   // processing window based on how the previous request completed.
   //
   // TODO(dougarnett): Need to split PRERENDERING_FAILED into separate
   // codes as to whether we should try another request or not.
   switch (status) {
     case Offliner::RequestStatus::SAVED:
     case Offliner::RequestStatus::SAVE_FAILED:
@@ skipping 22 matching lines @@
   return trigger_conditions;
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages