Split the RequestPicker task into two separate tasks.

components/offline_pages/background/pick_request_task.cc

Index: components/offline_pages/background/pick_request_task.cc
diff --git a/components/offline_pages/background/pick_request_task.cc b/components/offline_pages/background/pick_request_task.cc
index dd936f54e416cefecbfdd691009c7f5e9f27ffd9..8a79a1da235f67e9a9f2f88d6d24870a49cfb4b0 100644
--- a/components/offline_pages/background/pick_request_task.cc
+++ b/components/offline_pages/background/pick_request_task.cc
@@ -27,8 +27,6 @@ namespace offline_pages {
 
 PickRequestTask::PickRequestTask(RequestQueueStore* store,
                                  OfflinerPolicy* policy,
-                                 RequestNotifier* notifier,
-                                 RequestCoordinatorEventLogger* event_logger,
                                  RequestPickedCallback picked_callback,
                                  RequestNotPickedCallback not_picked_callback,
                                  RequestCountCallback request_count_callback,
@@ -36,8 +34,6 @@ PickRequestTask::PickRequestTask(RequestQueueStore* store,
                                  const std::set<int64_t>& disabled_requests)
     : store_(store),
       policy_(policy),
-      notifier_(notifier),
-      event_logger_(event_logger),
       picked_callback_(picked_callback),
       not_picked_callback_(not_picked_callback),
       request_count_callback_(request_count_callback),
@@ -49,42 +45,26 @@ PickRequestTask::PickRequestTask(RequestQueueStore* store,
 PickRequestTask::~PickRequestTask() {}
 
 void PickRequestTask::Run() {
+  GetRequests();
+}
+
+void PickRequestTask::GetRequests() {
   // Get all the requests from the queue, we will classify them in the callback.
-  store_->GetRequests(base::Bind(&PickRequestTask::ChooseAndPrune,
-                                 weak_ptr_factory_.GetWeakPtr()));
+  store_->GetRequests(
+      base::Bind(&PickRequestTask::Choose, weak_ptr_factory_.GetWeakPtr()));
 }
 
-void PickRequestTask::ChooseAndPrune(
+void PickRequestTask::Choose(
     bool success,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
   // If there is nothing to do, return right away.
   if (requests.empty()) {
     request_count_callback_.Run(requests.size(), 0);
-    not_picked_callback_.Run(false);
-    TaskComplete();
-    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<int64_t> expired_request_ids;
-  SplitRequests(std::move(requests), &valid_requests, &expired_request_ids);
-
-  // Continue processing by choosing a request.
-  ChooseRequestAndCallback(std::move(valid_requests));
-
-  // Continue processing by handling expired requests, if any.
-  if (expired_request_ids.size() == 0) {
+    not_picked_callback_.Run(false, false);

[dougarnett, 2016/12/02 17:20:27]

...(false /* non user requests */, false /* cleanup needed*/);   ?

[Pete Williamson, 2016/12/05 20:39:45]

I did this, but with enums, not comments, WDYT?

[dougarnett, 2016/12/05 21:27:45]

The constants are fine. I have noticed the parm comment style in some other team
member CLs so it might be good to discuss that style more broadly at some point.
I came to prefer that comment style but I think it takes some getting used to.

[Pete Williamson, 2016/12/06 00:25:38]

I've always preferred named constants to comments.  If nothing else, it is nice
to avoid using the open/close comment operators so that you can use them for
commenting out large swaths of code when doing binary search for problems by
removing code.  However, I don't feel strongly enough to force my opinions on
others, or that we all need to be consistent on this.  I *do* feel strongly that
we should never call with just plain true or false.

     TaskComplete();
     return;
   }
 
-  RemoveStaleRequests(std::move(expired_request_ids));
-}
-
-void PickRequestTask::ChooseRequestAndCallback(
-    std::vector<std::unique_ptr<SavePageRequest>> valid_requests) {
   // Pick the most deserving request for our conditions.
   const SavePageRequest* picked_request = nullptr;
 
@@ -98,84 +78,60 @@ void PickRequestTask::ChooseRequestAndCallback(
 
   // TODO(petewil): Consider replacing this bool with a better named enum.
   bool non_user_requested_tasks_remaining = false;
+  bool cleanup_needed = false;
 
   size_t available_request_count = 0;
 
   // Iterate once through the requests, keeping track of best candidate.
-  for (unsigned i = 0; i < valid_requests.size(); ++i) {
+  for (unsigned i = 0; i < requests.size(); ++i) {
+    // If the request is expired or has exceeded the retry count, skip it.
+    if (base::Time::Now() - requests[i]->creation_time() >=

[fgorski, 2016/12/02 21:44:04]

One more reason to extract a method as suggested earlier and at this point I
don't see it as an optional thing. WDYT?

[Pete Williamson, 2016/12/05 20:39:45]

Done, new class is OfflinerPolicyUtils.

+            base::TimeDelta::FromSeconds(kRequestExpirationTimeInSeconds) ||
+        requests[i]->started_attempt_count() >= policy_->GetMaxStartedTries() ||
+        requests[i]->completed_attempt_count() >=
+            policy_->GetMaxCompletedTries()) {
+      cleanup_needed = true;
+      continue;
+    }
+
     // If the  request is on the disabled list, skip it.
-    auto search = disabled_requests_.find(valid_requests[i]->request_id());
+    auto search = disabled_requests_.find(requests[i]->request_id());
     if (search != disabled_requests_.end())
       continue;
+
     // If there are non-user-requested tasks remaining, we need to make sure
     // that they are scheduled after we run out of user requested tasks. Here we
     // detect if any exist. If we don't find any user-requested tasks, we will
     // inform the "not_picked_callback_" that it needs to schedule a task for
     // non-user-requested items, which have different network and power needs.
-    if (!valid_requests[i]->user_requested())
+    if (!requests[i]->user_requested())
       non_user_requested_tasks_remaining = true;
-    if (valid_requests[i]->request_state() ==
+    if (requests[i]->request_state() ==
         SavePageRequest::RequestState::AVAILABLE) {
       available_request_count++;
     }
-    if (!RequestConditionsSatisfied(valid_requests[i].get()))
+    if (!RequestConditionsSatisfied(requests[i].get()))
       continue;
-    if (IsNewRequestBetter(picked_request, valid_requests[i].get(), comparator))
-      picked_request = valid_requests[i].get();
+    if (IsNewRequestBetter(picked_request, requests[i].get(), comparator))
+      picked_request = requests[i].get();
   }
 
-  // Report the request queue counts.
-  request_count_callback_.Run(valid_requests.size(), available_request_count);
+  // Report the request queue counts.  TODO(reviewers): This used to report only
+  // the count of valid entries, but now we report all entries. Reviewers, is

[dougarnett, 2016/12/02 23:00:11]

Looks fine. Before valid_requests.size() was the total count considering that
the expired ones were going to be deleted. We capture UMA for the difference of
these two counts - so it can now include expired requests that haven't been
deleted yet which seems fine (since they are cleaned up at start up and once
detected).

[Pete Williamson, 2016/12/05 20:39:45]

Acknowledged.

+  // that a problem?
+  request_count_callback_.Run(requests.size(), available_request_count);
 
   // 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/12/02 21:44:04]

add {} to both clauses, because your else spans multiple lines now.

[Pete Williamson, 2016/12/05 20:39:45]

Ah, I must have missed seeing it wrap.  Fixed.

-    picked_callback_.Run(*picked_request);
+    picked_callback_.Run(*picked_request, cleanup_needed);
   else
-    not_picked_callback_.Run(non_user_requested_tasks_remaining);
-}
+    not_picked_callback_.Run(non_user_requested_tasks_remaining,
+                             cleanup_needed);
 
-// 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.
-// Consider moving this to a fresh task type to clean the queue.
-void PickRequestTask::RemoveStaleRequests(
-    std::vector<int64_t> stale_request_ids) {
-  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<int64_t>* expired_request_ids) {
-  for (auto& request : requests) {
-    if (base::Time::Now() - request->creation_time() >=
-        base::TimeDelta::FromSeconds(kRequestExpirationTimeInSeconds)) {
-      expired_request_ids->push_back(request->request_id());
-    } 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.
@@ -200,26 +156,10 @@ bool PickRequestTask::RequestConditionsSatisfied(
     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()))
-    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