Request Picker task

components/offline_pages/background/pick_request_task.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_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_store.h"
 #include "components/offline_pages/background/save_page_request.h"
 
 namespace {
 template <typename T>
 int signum(T t) {
   return (T(0) < t) - (t < T(0));
 }
 
 #define CALL_MEMBER_FUNCTION(object, ptrToMember) ((object)->*(ptrToMember))
 }  // namespace
 
 namespace offline_pages {
 
-RequestPicker::RequestPicker(RequestQueue* requestQueue,
-                             OfflinerPolicy* policy,
-                             RequestNotifier* notifier,
-                             RequestCoordinatorEventLogger* event_logger)
-    : queue_(requestQueue),
+PickRequestTask::PickRequestTask(RequestQueueStore* store,
+                                 OfflinerPolicy* policy,
+                                 RequestNotifier* notifier,
+                                 RequestCoordinatorEventLogger* event_logger,
+                                 RequestPickedCallback picked_callback,
+                                 RequestNotPickedCallback not_picked_callback,
+                                 DeviceConditions& device_conditions,
+                                 const std::set<int64_t>& disabled_requests)
+    : store_(store),
       policy_(policy),
       notifier_(notifier),
       event_logger_(event_logger),
-      fewer_retries_better_(false),
-      earlier_requests_better_(false),
-      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));
+      picked_callback_(picked_callback),
+      not_picked_callback_(not_picked_callback),
+      disabled_requests_(disabled_requests),
+      weak_ptr_factory_(this) {
+  device_conditions_.reset(new DeviceConditions(device_conditions));
 }
 
-// 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,
+PickRequestTask::~PickRequestTask() {}
+
+void PickRequestTask::Run() {
+  // Get all the requests from the queue, we will classify them in the callback.
+  store_->GetRequests(base::Bind(&PickRequestTask::ChooseAndPrune,
+                                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void PickRequestTask::ChooseAndPrune(
+    bool success,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
   // If there is nothing to do, return right away.
   if (requests.size() == 0) {
     not_picked_callback_.Run(false);

[fgorski, 2016/11/09 17:58:07]

Early call to TaskComplete() is in order here (before return).

Please check your code for cases like that.

[Pete Williamson, 2016/11/09 23:53:55]

Done, and checked the code for more cases (I did not find any in
pick_request_task.cc)

     return;
   }
 
   // Get the expired requests to be removed from the queue, and the valid ones
   // from which to pick the next request.
   std::vector<std::unique_ptr<SavePageRequest>> valid_requests;
   std::vector<std::unique_ptr<SavePageRequest>> expired_requests;
   SplitRequests(std::move(requests), &valid_requests, &expired_requests);
   std::vector<int64_t> expired_request_ids;

[fgorski, 2016/11/09 17:58:07]

looks like this should be a type of the third parameter of SplitRequests.
WDYT?

[Pete Williamson, 2016/11/09 23:53:55]

Changed (but I actually replaced the second parameter instead of adding a
third).

[fgorski, 2016/11/10 19:15:50]

You are technically correct. :)

   for (const auto& request : expired_requests)
     expired_request_ids.push_back(request->request_id());

[dougarnett, 2016/11/09 18:23:51]

this *_ids vector not actually used?

[Pete Williamson, 2016/11/09 23:53:55]

Good catch, removed (it got copied into the RemoveStaleRequests function, but I
forgot to delete it here).

 
-  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.

[fgorski, 2016/11/09 17:58:07]

Add early return here if expired_request_ids is empty. (And remember to include
TaskComplete()) This prevents one extra cross thread roundtrip.

[Pete Williamson, 2016/11/09 23:53:55]

Done.

+  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;
 
   RequestCompareFunction comparator = nullptr;
 
   // 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()) {

[fgorski, 2016/11/09 17:58:06]

nit: drop {}

[Pete Williamson, 2016/11/09 23:53:55]

Done.

       continue;
     }
     if (!valid_requests[i]->user_requested())
       non_user_requested_tasks_remaining = true;
     if (!RequestConditionsSatisfied(valid_requests[i].get()))

[fgorski, 2016/11/09 17:58:06]

I am missing a comment here explaining why we first check for user requested
tasks and then check if the task matches the conditions.
Could you please add one?

[dougarnett, 2016/11/09 18:39:56]

Btw, that non_user_requested_tasks_remaining bool is really about returning
indication of what TriggerConditions, if any, makes sense based on what is in
the queue. Might be nice to define a more targeted enum in the future for this
instead of this hard-to-name bool.

[Pete Williamson, 2016/11/09 23:53:55]

TODO added.

[Pete Williamson, 2016/11/09 23:53:55]

Done.

       continue;
     if (IsNewRequestBetter(picked_request, valid_requests[i].get(), comparator))
       picked_request = valid_requests[i].get();
   }
 
   // If we have a best request to try next, get the request coodinator to
   // start it.  Otherwise return that we have no candidates.
   if (picked_request != nullptr) {

[fgorski, 2016/11/09 17:58:07]

nit: braces not necessary for both clauses.

[Pete Williamson, 2016/11/09 23:53:55]

Done.

     picked_callback_.Run(*picked_request);
   } else {
     not_picked_callback_.Run(non_user_requested_tasks_remaining);
   }
 }
 
+// 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

[fgorski, 2016/11/09 17:58:07]

this comment/todo probably needs revisiting after moving to the store-based
approach.

[Pete Williamson, 2016/11/09 23:53:55]

The comment is current.  It is basically explaining your original suggestion
that it might be better to have a separate task for queue cleanup instead of
piggybacking on the choosing task for efficiency reasons.  LMK if you think I
can improve the wording to make it more obvious.

[fgorski, 2016/11/10 19:15:50]

OK. Reread this and it can stay this way.

+// 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.
+// Consider moving this to a fresh task type to clean the queue.
+void PickRequestTask::RemoveStaleRequests(
+    std::vector<std::unique_ptr<SavePageRequest>> stale_requests) {
+  std::vector<int64_t> stale_request_ids;

[fgorski, 2016/11/09 17:58:07]

and these 3 lines is exactly why my comment about SplitRequests was valid.

[Pete Williamson, 2016/11/09 23:53:55]

Done.

+  for (const auto& request : stale_requests)
+    stale_request_ids.push_back(request->request_id());
+
+  store_->RemoveRequests(stale_request_ids,
+                         base::Bind(&PickRequestTask::OnRequestsExpired,
+                                    weak_ptr_factory_.GetWeakPtr()));
+}
+
+void PickRequestTask::OnRequestsExpired(
+    std::unique_ptr<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);
+  }
+
+  // The task is now done, return control 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) {

[fgorski, 2016/11/09 17:58:07]

this one should be std::vector<int64_t>

[Pete Williamson, 2016/11/09 23:53:55]

Done.

+  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;
   }
 
   // If we have already started this page the max number of times, it is not
   // eligible to try again.
   if (request->started_attempt_count() >= policy_->GetMaxStartedTries())
     return false;
 
   // If we have already completed trying this page the max number of times, it
   // is not eligible to try again.
   if (request->completed_attempt_count() >= policy_->GetMaxCompletedTries())
     return false;
 
   // If the request is paused, do not consider it.
   if (request->request_state() == SavePageRequest::RequestState::PAUSED)
     return false;
 
   // 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.
   // TODO(petewil): If the only reason we return nothing to do is that we have
   // inactive requests, we still want to try again later after their activation
   // time elapses, we shouldn't take ourselves completely off the scheduler.
   if (request->activation_time() > base::Time::Now())
     return false;
 
   return true;
 }
 
 // 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;
 
   // User requested pages get priority.
   if (newRequest->user_requested() && !oldRequest->user_requested())
     return true;
 
   // Otherwise, use the comparison function for the current policy, which
   // returns true if the older request is better.
   return !(CALL_MEMBER_FUNCTION(this, comparator)(oldRequest, newRequest));
 }
 
 // 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);
 
   if (result != 0)
     return (result > 0);
 
   // If we get here, the attempt counts were the same, so check recency.
   result = CompareCreationTime(left, right);
 
   return (result > 0);
 }
 
 // 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);
 
   if (result != 0)
     return (result > 0);
 
   // If we get here, the recency was the same, so check the attempt count.
   result = CompareRetryCount(left, right);
 
   return (result > 0);
 }
 
 // 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;
 }
 
 // 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