Request Picker task

components/offline_pages/background/pick_request_task.cc

Index: components/offline_pages/background/pick_request_task.cc
diff --git a/components/offline_pages/background/request_picker.cc b/components/offline_pages/background/pick_request_task.cc
similarity index 60%
rename from components/offline_pages/background/request_picker.cc
rename to components/offline_pages/background/pick_request_task.cc
index c1d27838165d837bb70bc28bf3768e63a597441d..fa8eb15bb2867caddff6350eb5897c0635115d46 100644
--- a/components/offline_pages/background/request_picker.cc
+++ b/components/offline_pages/background/pick_request_task.cc
@@ -2,10 +2,16 @@
 // 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_picker.h"
+#include "components/offline_pages/background/pick_request_task.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/time/time.h"
+#include "components/offline_pages/background/device_conditions.h"
+#include "components/offline_pages/background/offliner_policy.h"
+#include "components/offline_pages/background/request_coordinator_event_logger.h"
+#include "components/offline_pages/background/request_notifier.h"
+#include "components/offline_pages/background/request_queue.h"
 #include "components/offline_pages/background/save_page_request.h"
 
 namespace {
@@ -19,42 +25,34 @@ int signum(T t) {
 
 namespace offline_pages {
 
-RequestPicker::RequestPicker(RequestQueue* requestQueue,
-                             OfflinerPolicy* policy,
-                             RequestNotifier* notifier,
-                             RequestCoordinatorEventLogger* event_logger)
-    : queue_(requestQueue),
+PickRequestTask::PickRequestTask(RequestQueue* request_queue,
+                                 OfflinerPolicy* policy,
+                                 RequestNotifier* notifier,
+                                 RequestCoordinatorEventLogger* event_logger,
+                                 RequestPickedCallback picked_callback,
+                                 RequestNotPickedCallback not_picked_callback,
+                                 DeviceConditions* device_conditions,
+                                 const std::set<int64_t>& disabled_requests)
+    : request_queue_(request_queue),
       policy_(policy),
       notifier_(notifier),
       event_logger_(event_logger),
-      fewer_retries_better_(false),
-      earlier_requests_better_(false),
+      picked_callback_(picked_callback),
+      not_picked_callback_(not_picked_callback),
+      device_conditions_(device_conditions),
+      disabled_requests_(disabled_requests),
       weak_ptr_factory_(this) {}
 
-RequestPicker::~RequestPicker() {}
-
-// Entry point for the async operation to choose the next request.
-void RequestPicker::ChooseNextRequest(
-    RequestCoordinator::RequestPickedCallback picked_callback,
-    RequestCoordinator::RequestNotPickedCallback not_picked_callback,
-    DeviceConditions* device_conditions,
-    const std::set<int64_t>& disabled_requests) {
-  picked_callback_ = picked_callback;
-  not_picked_callback_ = not_picked_callback;
-  fewer_retries_better_ = policy_->ShouldPreferUntriedRequests();
-  earlier_requests_better_ = policy_->ShouldPreferEarlierRequests();
-  current_conditions_.reset(new DeviceConditions(*device_conditions));
-  // Get all requests from queue (there is no filtering mechanism).
-  queue_->GetRequests(base::Bind(&RequestPicker::GetRequestResultCallback,
-                                 weak_ptr_factory_.GetWeakPtr(),
-                                 disabled_requests));
+PickRequestTask::~PickRequestTask() {}
+
+void PickRequestTask::Run() {
+  // Get all the requests from the queue, we will classify them in the callback.
+  request_queue_->GetRequests(base::Bind(&PickRequestTask::HandleGetResults,
+                                         weak_ptr_factory_.GetWeakPtr()));
 }
 
-// When we get contents from the queue, use them to pick the next
-// request to operate on (if any).
-void RequestPicker::GetRequestResultCallback(
-    const std::set<int64_t>& disabled_requests,
-    RequestQueue::GetRequestsResult,
+void PickRequestTask::HandleGetResults(
+    QueueResults::GetRequestsResult,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
   // If there is nothing to do, return right away.
   if (requests.size() == 0) {
@@ -71,10 +69,15 @@ void RequestPicker::GetRequestResultCallback(
   for (const auto& request : expired_requests)
     expired_request_ids.push_back(request->request_id());
 
-  queue_->RemoveRequests(expired_request_ids,
-                         base::Bind(&RequestPicker::OnRequestExpired,
-                                    weak_ptr_factory_.GetWeakPtr()));
+  // Continue processing by choosing a request.
+  ChooseRequestAndCallback(std::move(valid_requests));
+
+  // Continue Async Processing.
+  RemoveStaleRequests(std::move(expired_requests));
+}
 
+void PickRequestTask::ChooseRequestAndCallback(
+    std::vector<std::unique_ptr<SavePageRequest>> valid_requests) {
   // Pick the most deserving request for our conditions.
   const SavePageRequest* picked_request = nullptr;
 
@@ -82,16 +85,16 @@ void RequestPicker::GetRequestResultCallback(
 
   // Choose which comparison function to use based on policy.
   if (policy_->RetryCountIsMoreImportantThanRecency())
-    comparator = &RequestPicker::RetryCountFirstCompareFunction;
+    comparator = &PickRequestTask::RetryCountFirstCompareFunction;
   else
-    comparator = &RequestPicker::RecencyFirstCompareFunction;
+    comparator = &PickRequestTask::RecencyFirstCompareFunction;
 
   // Iterate once through the requests, keeping track of best candidate.
   bool non_user_requested_tasks_remaining = false;
   for (unsigned i = 0; i < valid_requests.size(); ++i) {
     // If the  request is on the disabled list, skip it.
-    auto search = disabled_requests.find(valid_requests[i]->request_id());
-    if (search != disabled_requests.end()) {
+    auto search = disabled_requests_.find(valid_requests[i]->request_id());
+    if (search != disabled_requests_.end()) {
       continue;
     }
     if (!valid_requests[i]->user_requested())
@@ -109,28 +112,77 @@ void RequestPicker::GetRequestResultCallback(
   } else {
     not_picked_callback_.Run(non_user_requested_tasks_remaining);
   }
+
+  // The task is now done, return control to the task queue.
+  TaskComplete();

[fgorski, 2016/11/03 22:00:29]

seems like you are completing the task twice. That does not seem right.

[Pete Williamson, 2016/11/04 18:53:37]

Here we have two tasks - The first task is choosing, and this TaskComplete ends
the choosing task.

The second task is started by calling request_queue_->RemoveRequests(), which
creates a new task under the hood.  If we don't complete the first task, the
second task will never start, and the callback will never get called, so we
really do need to call TaskComplete here.

I removed the second call to TaskComplete in OnRequestsExpired(), since the
RemoveRequests() call will have its own TaskComplete for the Remove task it
builds.

Now that I see that we need to use a second task (It was hidden under the
request_queue call), I want to refactor the pruning out as you (FGorski)
suggest, but I'd prefer to delay that to a different changelist, this one is big
enough already.

+}
+
+// Continue the async part of the processing by deleting the expired requests.
+// TODO(petewil): Does that really need to be done on the task queue? Hard to
+// see how we need to wait for it before starting the next task. OTOH, we'd hate
+// to do a second slow DB operation to get entries a second time, and waiting
+// until this is done will make sure other gets don't see these old entries.
+void PickRequestTask::RemoveStaleRequests(
+    std::vector<std::unique_ptr<SavePageRequest>> expired_requests) {

[dougarnett, 2016/11/03 19:56:51]

expired_requests => stale_requests ?

[Pete Williamson, 2016/11/04 18:53:37]

Done.

+  std::vector<int64_t> expired_request_ids;
+  for (const auto& request : expired_requests)
+    expired_request_ids.push_back(request->request_id());
+
+  request_queue_->RemoveRequests(expired_request_ids,
+                                 base::Bind(&PickRequestTask::OnRequestsExpired,
+                                            weak_ptr_factory_.GetWeakPtr()));
+}
+
+void PickRequestTask::OnRequestsExpired(
+    std::unique_ptr<QueueResults::UpdateRequestsResult> result) {
+  RequestNotifier::BackgroundSavePageResult save_page_result(
+      RequestNotifier::BackgroundSavePageResult::EXPIRED);
+  for (const auto& request : result->updated_items) {
+    event_logger_->RecordDroppedSavePageRequest(
+        request.client_id().name_space, save_page_result, request.request_id());
+    notifier_->NotifyCompleted(request, save_page_result);
+  }
+
+  // Now all our async operations are complete.  Return processing to the task
+  // queue.
+  TaskComplete();
+}
+
+void PickRequestTask::SplitRequests(
+    std::vector<std::unique_ptr<SavePageRequest>> requests,
+    std::vector<std::unique_ptr<SavePageRequest>>* valid_requests,
+    std::vector<std::unique_ptr<SavePageRequest>>* expired_requests) {
+  for (auto& request : requests) {
+    if (base::Time::Now() - request->creation_time() >=
+        base::TimeDelta::FromSeconds(kRequestExpirationTimeInSeconds)) {
+      expired_requests->push_back(std::move(request));
+    } else {
+      valid_requests->push_back(std::move(request));
+    }
+  }
 }
 
 // Filter out requests that don't meet the current conditions.  For instance, if
 // this is a predictive request, and we are not on WiFi, it should be ignored
 // this round.
-bool RequestPicker::RequestConditionsSatisfied(const SavePageRequest* request) {
+bool PickRequestTask::RequestConditionsSatisfied(
+    const SavePageRequest* request) {
   // If the user did not request the page directly, make sure we are connected
   // to power and have WiFi and sufficient battery remaining before we take this
   // request.
 
-  if (!current_conditions_->IsPowerConnected() &&
+  if (!device_conditions_->IsPowerConnected() &&
       policy_->PowerRequired(request->user_requested())) {
     return false;
   }
 
-  if (current_conditions_->GetNetConnectionType() !=
+  if (device_conditions_->GetNetConnectionType() !=
           net::NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI &&
       policy_->UnmeteredNetworkRequired(request->user_requested())) {
     return false;
   }
 
-  if (current_conditions_->GetBatteryPercentage() <
+  if (device_conditions_->GetBatteryPercentage() <
       policy_->BatteryPercentageRequired(request->user_requested())) {
     return false;
   }
@@ -151,9 +203,8 @@ bool RequestPicker::RequestConditionsSatisfied(const SavePageRequest* request) {
 
   // If the request is expired, do not consider it.
   base::TimeDelta requestAge = base::Time::Now() - request->creation_time();
-  if (requestAge >
-      base::TimeDelta::FromSeconds(
-          policy_->GetRequestExpirationTimeInSeconds()))
+  if (requestAge > base::TimeDelta::FromSeconds(
+                       policy_->GetRequestExpirationTimeInSeconds()))
     return false;
 
   // If this request is not active yet, return false.
@@ -167,10 +218,9 @@ bool RequestPicker::RequestConditionsSatisfied(const SavePageRequest* request) {
 }
 
 // Look at policies to decide which requests to prefer.
-bool RequestPicker::IsNewRequestBetter(const SavePageRequest* oldRequest,
-                                       const SavePageRequest* newRequest,
-                                       RequestCompareFunction comparator) {
-
+bool PickRequestTask::IsNewRequestBetter(const SavePageRequest* oldRequest,
+                                         const SavePageRequest* newRequest,
+                                         RequestCompareFunction comparator) {
   // If there is no old request, the new one is better.
   if (oldRequest == nullptr)
     return true;
@@ -186,8 +236,9 @@ bool RequestPicker::IsNewRequestBetter(const SavePageRequest* oldRequest,
 
 // Compare the results, checking request count before recency.  Returns true if
 // left hand side is better, false otherwise.
-bool RequestPicker::RetryCountFirstCompareFunction(
-    const SavePageRequest* left, const SavePageRequest* right) {
+bool PickRequestTask::RetryCountFirstCompareFunction(
+    const SavePageRequest* left,
+    const SavePageRequest* right) {
   // Check the attempt count.
   int result = CompareRetryCount(left, right);
 
@@ -202,8 +253,9 @@ bool RequestPicker::RetryCountFirstCompareFunction(
 
 // Compare the results, checking recency before request count. Returns true if
 // left hand side is better, false otherwise.
-bool RequestPicker::RecencyFirstCompareFunction(
-    const SavePageRequest* left, const SavePageRequest* right) {
+bool PickRequestTask::RecencyFirstCompareFunction(
+    const SavePageRequest* left,
+    const SavePageRequest* right) {
   // Check the recency.
   int result = CompareCreationTime(left, right);
 
@@ -218,14 +270,14 @@ bool RequestPicker::RecencyFirstCompareFunction(
 
 // Compare left and right side, returning 1 if the left side is better
 // (preferred by policy), 0 if the same, and -1 if the right side is better.
-int RequestPicker::CompareRetryCount(
-    const SavePageRequest* left, const SavePageRequest* right) {
+int PickRequestTask::CompareRetryCount(const SavePageRequest* left,
+                                       const SavePageRequest* right) {
   // Check the attempt count.
   int result = signum(left->completed_attempt_count() -
                       right->completed_attempt_count());
 
   // Flip the direction of comparison if policy prefers fewer retries.
-  if (fewer_retries_better_)
+  if (policy_->ShouldPreferUntriedRequests())
     result *= -1;
 
   return result;
@@ -233,44 +285,17 @@ int RequestPicker::CompareRetryCount(
 
 // Compare left and right side, returning 1 if the left side is better
 // (preferred by policy), 0 if the same, and -1 if the right side is better.
-int RequestPicker::CompareCreationTime(
-    const SavePageRequest* left, const SavePageRequest* right) {
+int PickRequestTask::CompareCreationTime(const SavePageRequest* left,
+                                         const SavePageRequest* right) {
   // Check the recency.
   base::TimeDelta difference = left->creation_time() - right->creation_time();
   int result = signum(difference.InMilliseconds());
 
   // Flip the direction of comparison if policy prefers fewer retries.
-  if (earlier_requests_better_)
+  if (policy_->ShouldPreferEarlierRequests())
     result *= -1;
 
   return result;
 }
 
-void RequestPicker::SplitRequests(
-    std::vector<std::unique_ptr<SavePageRequest>> requests,
-    std::vector<std::unique_ptr<SavePageRequest>>* valid_requests,
-    std::vector<std::unique_ptr<SavePageRequest>>* expired_requests) {
-  for (auto& request : requests) {
-    if (base::Time::Now() - request->creation_time() >=
-        base::TimeDelta::FromSeconds(kRequestExpirationTimeInSeconds)) {
-      expired_requests->push_back(std::move(request));
-    } else {
-      valid_requests->push_back(std::move(request));
-    }
-  }
-}
-
-// Callback used after expired requests are deleted from the queue and notifies
-// the coordinator.
-void RequestPicker::OnRequestExpired(
-    std::unique_ptr<UpdateRequestsResult> result) {
-  const RequestCoordinator::BackgroundSavePageResult save_page_result(
-      RequestCoordinator::BackgroundSavePageResult::EXPIRED);
-  for (const auto& request : result->updated_items) {
-    event_logger_->RecordDroppedSavePageRequest(
-        request.client_id().name_space, save_page_result, request.request_id());
-    notifier_->NotifyCompleted(request, save_page_result);
-  }
-}
-
 }  // namespace offline_pages