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.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(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() {}
+PickRequestTask::~PickRequestTask() {}
 
-// 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));
+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) {
     not_picked_callback_.Run(false);
     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;
   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.
+  ChooseRequest(std::move(valid_requests));

[dougarnett, 2016/11/03 17:01:22]

Consider ChooseRequestAndCallback() Or mentioning will call callback in comment
above

[Pete Williamson, 2016/11/03 19:26:29]

Done.

 
+  // Continue Async Processing.
+  RemoveExpiredEntries(std::move(expired_requests));

[dougarnett, 2016/11/03 17:01:22]

Generalize to remove other stale requests too? Maybe SplitRequests identifies
additional cases and then can remove from RequestConditionsSatisfied().  

[Pete Williamson, 2016/11/03 19:26:29]

Renamed to "RemoveStaleRequests"

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

Still some more work though to check started_attempt_count and
completed_attempt_count against policy limits in SplitRequests?

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

I'm not sure it is better to clean them here.  We can clean them well when we
detect the condition.  We don't detect the condition of something expiring.

Today we clean these up in AbortRequestAttempt() and OfflinerDoneCallback().

If you do want this to be done here, let me know, and I'll add a TODO and a bug
(but this changelist is complex enough already).

[dougarnett, 2016/11/04 19:28:26]

Yes, I want you to remove them here. If they are detected here it
is because we never got the OfflinerDoneCallback (eg, crash or
killed). This is bug 655178. Sure, you can do as a follow-up but
this leads to bad bug with notification not getting cleared so
we should fix for M-56.

[dougarnett, 2016/11/07 17:09:07]

Btw, I'm happy to do the follow-up here but I am not yet sure you agree this
needs to be done so maybe we can chat in person today or tomorrow. 

[Pete Williamson, 2016/11/08 00:36:45]

I agree that it needs to be done (in the light of your comments).  However, I
hope to do it in a later changelist.

+}
+
+void PickRequestTask::ChooseRequest(
+    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()) {
       continue;
     }
     if (!valid_requests[i]->user_requested())
       non_user_requested_tasks_remaining = true;
     if (!RequestConditionsSatisfied(valid_requests[i].get()))
       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) {
     picked_callback_.Run(*picked_request);
   } else {
     not_picked_callback_.Run(non_user_requested_tasks_remaining);
   }
+
+  // The task is now done, return control to the task queue.
+  TaskComplete();
+}
+
+// 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::RemoveExpiredEntries(
+    std::vector<std::unique_ptr<SavePageRequest>> expired_requests) {
+  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;
   }
 
   // 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