Request Picker task

components/offline_pages/background/pick_request_task.h

Index: components/offline_pages/background/pick_request_task.h
diff --git a/components/offline_pages/background/pick_request_task.h b/components/offline_pages/background/pick_request_task.h
new file mode 100644
index 0000000000000000000000000000000000000000..13feb0274e71e0989c7e0b444a8f9e8fbef3a58e
--- /dev/null
+++ b/components/offline_pages/background/pick_request_task.h
@@ -0,0 +1,117 @@
+// 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.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_BACKGROUND_PICK_REQUEST_TASK_H_
+#define COMPONENTS_OFFLINE_PAGES_BACKGROUND_PICK_REQUEST_TASK_H_
+
+#include <set>
+
+#include "base/memory/weak_ptr.h"
+#include "components/offline_pages/background/queue_results.h"
+#include "components/offline_pages/background/save_page_request.h"
+#include "components/offline_pages/core/task.h"
+
+namespace offline_pages {
+
+class DeviceConditions;
+class OfflinerPolicy;
+class PickRequestTask;
+class RequestCoordinatorEventLogger;
+class RequestNotifier;
+class RequestQueue;
+
+typedef bool (PickRequestTask::*RequestCompareFunction)(
+    const SavePageRequest* left,
+    const SavePageRequest* right);
+
+class PickRequestTask : public Task {
+ public:
+  // Callback to report when a request was available.
+  typedef base::Callback<void(const SavePageRequest& request)>
+      RequestPickedCallback;
+
+  // Callback to report when no request was available.
+  typedef base::Callback<void(bool)> RequestNotPickedCallback;
+
+  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);
+
+  ~PickRequestTask() override;
+
+  // TaskQueue::Task implementation, starts the async chain
+  void Run() override;
+
+ private:
+  // Step 1, handle getting the results from the store.
+  void HandleGetResults(QueueResults::GetRequestsResult,

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

Can you focus on the outcome of the step when naming the method?

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

How about "ChooseAndPrune"?

+                        std::vector<std::unique_ptr<SavePageRequest>> requests);
+
+  // Step 2a. Handle choosing an entry, and calling the right callback.
+  void ChooseRequestAndCallback(
+      std::vector<std::unique_ptr<SavePageRequest>> valid_requests);
+
+  // Step 2b. Handle deleting expired entries and notifying observers.

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

expired => stale

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

I am happy with Expired, as long as we are consistent.

[dougarnett, 2016/11/03 22:33:15]

In case my point is not clear, we should be culling at least 3 different types
of request situations here (this really being the cleanup task): over expiry
time, over start count, and over completed count. I can accept Expired as long
as it is a term we understand applies to all the types.

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

I changed it to stale everywhere.  I'm not sure that this is the right place to
remove the other cleanup items, though.

+  void RemoveStaleRequests(
+      std::vector<std::unique_ptr<SavePageRequest>> expired_requests);
+
+  // Step 3. Send delete notifications for the expired requests.
+  void OnRequestsExpired(
+      std::unique_ptr<QueueResults::UpdateRequestsResult> result);
+
+  // Helper functions.
+
+  // Split requests into valid and expired categories.
+  void SplitRequests(
+      std::vector<std::unique_ptr<SavePageRequest>> requests,
+      std::vector<std::unique_ptr<SavePageRequest>>* valid_requests,
+      std::vector<std::unique_ptr<SavePageRequest>>* expired_requests);
+
+  // Determine if this request has device conditions appropriate for running it.
+  bool RequestConditionsSatisfied(const SavePageRequest* request);
+
+  // Determine if the new request is preferred under current policies.
+  bool IsNewRequestBetter(const SavePageRequest* oldRequest,
+                          const SavePageRequest* newRequest,
+                          RequestCompareFunction comparator);
+
+  // Returns true if the left hand side is better.
+  bool RetryCountFirstCompareFunction(const SavePageRequest* left,
+                                      const SavePageRequest* right);
+
+  // Returns true if the left hand side is better.
+  bool RecencyFirstCompareFunction(const SavePageRequest* left,
+                                   const SavePageRequest* right);
+
+  // 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 CompareRetryCount(const SavePageRequest* left,
+                        const SavePageRequest* right);
+
+  // 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 CompareCreationTime(const SavePageRequest* left,
+                          const SavePageRequest* right);
+
+  // Member variables, all pointers unowned.
+  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_;
+  // Allows us to pass a weak pointer to callbacks.
+  base::WeakPtrFactory<PickRequestTask> weak_ptr_factory_;
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_BACKGROUND_PICK_REQUEST_TASK_H_