Request Picker task

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 120c08db9b92122170187545e8727f4c00faff31..c2fb6f69547c6504bdfed3ea70c437497dcb5153 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -16,7 +16,6 @@
 #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/client_policy_controller.h"
 #include "components/offline_pages/offline_page_item.h"
@@ -129,7 +128,7 @@ int64_t GenerateOfflineId() {
 void EmptySchedulerCallback(bool started) {}
 
 // Returns whether |result| is a successful result for a single request.
-bool IsSingleSuccessResult(const UpdateRequestsResult* result) {
+bool IsSingleSuccessResult(const QueueResults::UpdateRequestsResult* result) {
   return result->store_state == StoreState::LOADED &&
          result->item_statuses.size() == 1 &&
          result->item_statuses.at(0).second == ItemActionStatus::SUCCESS;
@@ -163,8 +162,8 @@ RequestCoordinator::RequestCoordinator(
       immediate_schedule_callback_(base::Bind(&EmptySchedulerCallback)),
       weak_ptr_factory_(this) {
   DCHECK(policy_ != nullptr);
-  picker_.reset(
-      new RequestPicker(queue_.get(), policy_.get(), this, &event_logger_));
+  queue_->SetPickerBuilder(
+      new PickRequestTaskBuilder(policy_.get(), this, &event_logger_));
 }
 
 RequestCoordinator::~RequestCoordinator() {}
@@ -207,7 +206,7 @@ void RequestCoordinator::GetAllRequests(const GetRequestsCallback& callback) {
 
 void RequestCoordinator::GetQueuedRequestsCallback(
     const GetRequestsCallback& callback,
-    RequestQueue::GetRequestsResult result,
+    QueueResults::GetRequestsResult result,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
   callback.Run(std::move(requests));
 }
@@ -243,7 +242,7 @@ void RequestCoordinator::StopPrerendering(Offliner::RequestStatus stop_status) {
 }
 
 void RequestCoordinator::GetRequestsForSchedulingCallback(
-    RequestQueue::GetRequestsResult result,
+    QueueResults::GetRequestsResult result,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
   bool user_requested = false;
 
@@ -281,8 +280,8 @@ bool RequestCoordinator::CancelActiveRequestIfItMatches(
 
 void RequestCoordinator::AbortRequestAttempt(SavePageRequest* request) {
   if (request->started_attempt_count() >= policy_->GetMaxStartedTries()) {
-    const BackgroundSavePageResult result(
-        BackgroundSavePageResult::START_COUNT_EXCEEDED);
+    const RequestNotifier::BackgroundSavePageResult result(
+        RequestNotifier::BackgroundSavePageResult::START_COUNT_EXCEEDED);
     event_logger_.RecordDroppedSavePageRequest(request->client_id().name_space,
                                                result, request->request_id());
     RemoveAttemptedRequest(*request, result);
@@ -297,7 +296,7 @@ void RequestCoordinator::AbortRequestAttempt(SavePageRequest* request) {
 
 void RequestCoordinator::RemoveAttemptedRequest(
     const SavePageRequest& request,
-    BackgroundSavePageResult result) {
+    RequestNotifier::BackgroundSavePageResult result) {
   std::vector<int64_t> remove_requests;
   remove_requests.push_back(request.request_id());
   queue_->RemoveRequests(remove_requests,
@@ -309,15 +308,15 @@ void RequestCoordinator::RemoveAttemptedRequest(
 void RequestCoordinator::MarkAttemptAbortedDone(
     int64_t request_id,
     const ClientId& client_id,
-    std::unique_ptr<UpdateRequestsResult> result) {
+    std::unique_ptr<QueueResults::UpdateRequestsResult> result) {
   // If the request succeeded, nothing to do.  If it failed, we can't really do
   // much, so just log it.
   if (!IsSingleSuccessResult(result.get())) {
     DVLOG(1) << "Failed to mark request aborted: " << request_id;
-    RequestQueue::UpdateRequestResult request_result =
+    QueueResults::UpdateRequestResult request_result =
         result->store_state != StoreState::LOADED
-            ? RequestQueue::UpdateRequestResult::STORE_FAILURE
-            : RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
+            ? QueueResults::UpdateRequestResult::STORE_FAILURE
+            : QueueResults::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
     event_logger_.RecordUpdateRequestFailed(client_id.name_space,
                                             request_result);
   }
@@ -331,7 +330,7 @@ void RequestCoordinator::RemoveRequests(
       request_ids,
       base::Bind(&RequestCoordinator::HandleRemovedRequestsAndCallback,
                  weak_ptr_factory_.GetWeakPtr(), callback,
-                 BackgroundSavePageResult::REMOVED));
+                 RequestNotifier::BackgroundSavePageResult::REMOVED));
   if (canceled)
     TryNextRequest();
 }
@@ -368,7 +367,7 @@ RequestCoordinator::GetConnectionType() {
 }
 
 void RequestCoordinator::AddRequestResultCallback(
-    RequestQueue::AddRequestResult result,
+    QueueResults::AddRequestResult result,
     const SavePageRequest& request) {
   NotifyAdded(request);
   // Inform the scheduler that we have an outstanding task.
@@ -381,22 +380,22 @@ void RequestCoordinator::AddRequestResultCallback(
 void RequestCoordinator::MarkAttemptCompletedDoneCallback(
     int64_t request_id,
     const ClientId& client_id,
-    std::unique_ptr<UpdateRequestsResult> result) {
+    std::unique_ptr<QueueResults::UpdateRequestsResult> result) {
   if (IsSingleSuccessResult(result.get())) {
     NotifyChanged(result->updated_items.at(0));
   } else {
     DVLOG(1) << "Failed to mark request completed " << request_id;
-    RequestQueue::UpdateRequestResult request_result =
+    QueueResults::UpdateRequestResult request_result =
         result->store_state != StoreState::LOADED
-            ? RequestQueue::UpdateRequestResult::STORE_FAILURE
-            : RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
+            ? QueueResults::UpdateRequestResult::STORE_FAILURE
+            : QueueResults::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
     event_logger_.RecordUpdateRequestFailed(client_id.name_space,
                                             request_result);
   }
 }
 
 void RequestCoordinator::UpdateMultipleRequestsCallback(
-    std::unique_ptr<UpdateRequestsResult> result) {
+    std::unique_ptr<QueueResults::UpdateRequestsResult> result) {
   for (const auto& request : result->updated_items)
     NotifyChanged(request);
 
@@ -421,8 +420,8 @@ void RequestCoordinator::CompletedRequestCallback(
 
 void RequestCoordinator::HandleRemovedRequestsAndCallback(
     const RemoveRequestsCallback& callback,
-    BackgroundSavePageResult status,
-    std::unique_ptr<UpdateRequestsResult> result) {
+    RequestNotifier::BackgroundSavePageResult status,
+    std::unique_ptr<QueueResults::UpdateRequestsResult> result) {
   // TODO(dougarnett): Define status code for user/api cancel and use here
   // to determine whether to record cancel time UMA.
   for (const auto& request : result->updated_items)
@@ -432,8 +431,8 @@ void RequestCoordinator::HandleRemovedRequestsAndCallback(
 }
 
 void RequestCoordinator::HandleRemovedRequests(
-    BackgroundSavePageResult status,
-    std::unique_ptr<UpdateRequestsResult> result) {
+    RequestNotifier::BackgroundSavePageResult status,
+    std::unique_ptr<QueueResults::UpdateRequestsResult> result) {
   for (const auto& request : result->updated_items)
     NotifyCompleted(request, status);
 }
@@ -463,6 +462,7 @@ void RequestCoordinator::HandleWatchdogTimeout() {
 bool RequestCoordinator::StartProcessing(
     const DeviceConditions& device_conditions,
     const base::Callback<void(bool)>& callback) {
+  DVLOG(2) << "Scheduled " << __func__;
   return StartProcessingInternal(ProcessingWindowState::SCHEDULED_WINDOW,
                                  device_conditions, callback);
 }
@@ -496,6 +496,7 @@ void RequestCoordinator::StartImmediatelyIfConnected() {
 
 RequestCoordinator::OfflinerImmediateStartStatus
 RequestCoordinator::TryImmediateStart() {
+  DVLOG(2) << "Immediate " << __func__;
   // Make sure not already busy processing.
   if (is_busy_)
     return OfflinerImmediateStartStatus::BUSY;
@@ -558,14 +559,17 @@ void RequestCoordinator::TryNextRequest() {
     return;
   }
 
-  // Choose a request to process that meets the available conditions.
-  // This is an async call, and returns right away.
-  picker_->ChooseNextRequest(base::Bind(&RequestCoordinator::RequestPicked,
-                                        weak_ptr_factory_.GetWeakPtr()),
-                             base::Bind(&RequestCoordinator::RequestNotPicked,
-                                        weak_ptr_factory_.GetWeakPtr()),
-                             current_conditions_.get(),
-                             disabled_requests_);
+  // Ask request queue to make a new PickRequestTask object, then put it on the
+  // task queue.
+  queue_->PickNextRequest(base::Bind(&RequestCoordinator::RequestPicked,
+                                     weak_ptr_factory_.GetWeakPtr()),
+                          base::Bind(&RequestCoordinator::RequestNotPicked,
+                                     weak_ptr_factory_.GetWeakPtr()),
+                          current_conditions_.get(), disabled_requests_);
+  // TODO(petewil): Verify current_conditions has a good value on all calling
+  // paths.  It is really more of a "last known conditions" than "current
+  // conditions".  Consider having a call to Java to check the current
+  // conditions.
 }
 
 // Called by the request picker when a request has been picked.
@@ -633,7 +637,7 @@ void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
 void RequestCoordinator::StartOffliner(
     int64_t request_id,
     const std::string& client_namespace,
-    std::unique_ptr<UpdateRequestsResult> update_result) {
+    std::unique_ptr<QueueResults::UpdateRequestsResult> update_result) {
   if (update_result->store_state != StoreState::LOADED ||
       update_result->item_statuses.size() != 1 ||
       update_result->item_statuses.at(0).first != request_id ||
@@ -642,10 +646,10 @@ void RequestCoordinator::StartOffliner(
     // TODO(fgorski): what is the best result? Do we create a new status?
     StopProcessing(Offliner::PRERENDERING_NOT_STARTED);
     DVLOG(1) << "Failed to mark attempt started: " << request_id;
-    RequestQueue::UpdateRequestResult request_result =
+    QueueResults::UpdateRequestResult request_result =
         update_result->store_state != StoreState::LOADED
-            ? RequestQueue::UpdateRequestResult::STORE_FAILURE
-            : RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
+            ? QueueResults::UpdateRequestResult::STORE_FAILURE
+            : QueueResults::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
     event_logger_.RecordUpdateRequestFailed(client_namespace, request_result);
     return;
   }
@@ -707,18 +711,19 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
     NotifyChanged(updated_request);
   } else if (status == Offliner::RequestStatus::SAVED) {
     // Remove the request from the queue if it succeeded.
-    RemoveAttemptedRequest(request, BackgroundSavePageResult::SUCCESS);
-  } else if (status == Offliner::RequestStatus::PRERENDERING_FAILED_NO_RETRY) {
     RemoveAttemptedRequest(request,
-                           BackgroundSavePageResult::PRERENDER_FAILURE);
+                           RequestNotifier::BackgroundSavePageResult::SUCCESS);
+  } else if (status == Offliner::RequestStatus::PRERENDERING_FAILED_NO_RETRY) {
+    RemoveAttemptedRequest(
+        request, RequestNotifier::BackgroundSavePageResult::PRERENDER_FAILURE);
   } else if (request.completed_attempt_count() + 1 >=
              policy_->GetMaxCompletedTries()) {
     // 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.
-    const BackgroundSavePageResult result(
-        BackgroundSavePageResult::RETRY_COUNT_EXCEEDED);
+    const RequestNotifier::BackgroundSavePageResult result(
+        RequestNotifier::BackgroundSavePageResult::RETRY_COUNT_EXCEEDED);
     event_logger_.RecordDroppedSavePageRequest(request.client_id().name_space,
                                                result, request.request_id());
     RemoveAttemptedRequest(request, result);
@@ -776,13 +781,13 @@ void RequestCoordinator::MarkRequestCompleted(int64_t request_id) {
 
   // Remove the request, but send out SUCCEEDED instead of removed.
   std::vector<int64_t> request_ids { request_id };
-    queue_->RemoveRequests(
+  queue_->RemoveRequests(
       request_ids,
       base::Bind(&RequestCoordinator::HandleRemovedRequestsAndCallback,
                  weak_ptr_factory_.GetWeakPtr(),
                  base::Bind(&RequestCoordinator::CompletedRequestCallback,
                             weak_ptr_factory_.GetWeakPtr()),
-                 BackgroundSavePageResult::SUCCESS));
+                 RequestNotifier::BackgroundSavePageResult::SUCCESS));
 }
 
 const Scheduler::TriggerConditions RequestCoordinator::GetTriggerConditions(
@@ -807,8 +812,9 @@ void RequestCoordinator::NotifyAdded(const SavePageRequest& request) {
     observer.OnAdded(request);
 }
 
-void RequestCoordinator::NotifyCompleted(const SavePageRequest& request,
-                                         BackgroundSavePageResult status) {
+void RequestCoordinator::NotifyCompleted(
+    const SavePageRequest& request,
+    RequestNotifier::BackgroundSavePageResult status) {
   for (Observer& observer : observers_)
     observer.OnCompleted(request, status);
 }