Split the RequestPicker task into two separate tasks.

components/offline_pages/background/cleanup_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/cleanup_task.h"
+
+#include "base/bind.h"
+#include "base/logging.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 offline_pages {
+
+CleanupTask::CleanupTask(RequestQueueStore* store,
+                         OfflinerPolicy* policy,
+                         RequestNotifier* notifier,
+                         RequestCoordinatorEventLogger* event_logger)
+    : store_(store),
+      policy_(policy),
+      notifier_(notifier),
+      event_logger_(event_logger),
+      weak_ptr_factory_(this) {}
+
+CleanupTask::~CleanupTask() {}
+
+void CleanupTask::GetExpiredRequestIds(

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

should this be positioned later in file per ordering in .h file?

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

Actually this can be defined as a function in anonymous namespace if you pass in
policy.

This is implementation detail that does not need to be part of the class.

Alternatively I can think of a set of utils that can be tested independently.
(e.g. Policy utils/helper for this particular function). We can test it
separately from this class then.

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

Moved.

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

Created a new class, OfflinerPolicyUtils for the boolean operation.  It actually
got a bit more complex because we need to track why it is being deleted for
metrics further down the async call chain. 

+    std::vector<std::unique_ptr<SavePageRequest>> requests,
+    std::vector<int64_t>* expired_request_ids) {
+  for (auto& request : requests) {
+    // Check for requests past their expiration time or with too many tries.
+    if (base::Time::Now() - request->creation_time() >=
+            base::TimeDelta::FromSeconds(
+                policy_->GetRequestExpirationTimeInSeconds()) ||
+        request->started_attempt_count() >= policy_->GetMaxStartedTries() ||
+        request->completed_attempt_count() >= policy_->GetMaxCompletedTries()) {
+      expired_request_ids->push_back(request->request_id());
+    }
+  }
+}
+
+void CleanupTask::Run() {
+  GetRequests();
+}
+
+void CleanupTask::GetRequests() {
+  // Get all the requests from the queue, we will classify them in the callback.
+  store_->GetRequests(
+      base::Bind(&CleanupTask::Prune, weak_ptr_factory_.GetWeakPtr()));
+}
+
+void CleanupTask::Prune(

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

what is the meaning of prune?
Why does it deserve a separate step from RemoveStaleRequests?

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

No reason to keep RemoveStaleRequests, so I got rid of the second function.  I
think this might be a leftover vestige of the structure we used to have when we
had a combined pick and prune task.

+    bool success,
+    std::vector<std::unique_ptr<SavePageRequest>> requests) {
+  // If there is nothing to do, return right away.
+  if (requests.empty()) {
+    TaskComplete();
+    return;
+  }
+
+  // Get the expired requests to be removed from the queue.
+  std::vector<int64_t> expired_request_ids;
+  GetExpiredRequestIds(std::move(requests), &expired_request_ids);
+
+  // Continue processing by handling expired requests, if any.
+  if (expired_request_ids.size() == 0) {
+    TaskComplete();
+    return;
+  }
+
+  RemoveStaleRequests(std::move(expired_request_ids));
+}
+
+void CleanupTask::RemoveStaleRequests(std::vector<int64_t> stale_request_ids) {
+  // TODO: Add UMA when we remove them, saying why

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

TODO(petewil): 

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

Done.

+  store_->RemoveRequests(stale_request_ids,
+                         base::Bind(&CleanupTask::OnRequestsExpired,
+                                    weak_ptr_factory_.GetWeakPtr()));
+}
+
+void CleanupTask::OnRequestsExpired(
+    std::unique_ptr<UpdateRequestsResult> result) {
+  RequestNotifier::BackgroundSavePageResult save_page_result(
+      RequestNotifier::BackgroundSavePageResult::EXPIRED);

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

I think we want to figure out which of the 3 BackgroundSavePageResult codes to
use here - to pass to RecordDroppedSavePageRequest and NotifyCompleted

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

Can we do that in a later changelist?  It involves a lot more engineering to
pair the reasons with the list of request IDs, and round trip them through the
request queue.  It was also that way before this change. TODO Added.  I also
captured the exact reason for expiration when we build our list, so we just need
to get that reason from there to here.

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

Sure, we can TODO and follow-up. This is new behavior though in that now
COUNT_EXCEEDED cases get deleted and newly get event logging and notification
updates albeit as EXPIRED. I don't think we have any UMA affected by this though
so seems benign for event log to be a bit wrong in the interim. Or one might
argue that "expired" is a more general classification than expired by time.

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

Acknowledged.

+  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();
+}
+};