Request Picker task

components/offline_pages/background/request_queue.h

 // 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_REQUEST_QUEUE_H_
 #define COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_QUEUE_H_
 
 #include <stdint.h>
 
 #include <memory>
+#include <set>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "components/offline_pages/background/device_conditions.h"
+#include "components/offline_pages/background/pick_request_task.h"
+#include "components/offline_pages/background/pick_request_task_factory.h"
+#include "components/offline_pages/background/request_queue_results.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/core/task_queue.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/offline_store_types.h"
 
 namespace offline_pages {
 
 class RequestQueueStore;
-typedef StoreUpdateResult<SavePageRequest> UpdateRequestsResult;
+class PickRequestTaskFactory;
 
 // Class responsible for managing save page requests.
 class RequestQueue {
  public:
-  enum class GetRequestsResult {
-    SUCCESS,
-    STORE_FAILURE,
-  };
-
-  enum class AddRequestResult {
-    SUCCESS,
-    STORE_FAILURE,
-    ALREADY_EXISTS,
-    REQUEST_QUOTA_HIT,  // Cannot add a request with this namespace, as it has
-                        // reached a quota of active requests.
-  };
-
-  // GENERATED_JAVA_ENUM_PACKAGE:org.chromium.components.offlinepages.background
-  enum class UpdateRequestResult {
-    SUCCESS,
-    STORE_FAILURE,
-    REQUEST_DOES_NOT_EXIST,  // Failed to delete the request because it does not
-                             // exist.
-  };
 
   // Callback used for |GetRequests|.
   typedef base::Callback<void(GetRequestsResult,
                               std::vector<std::unique_ptr<SavePageRequest>>)>
       GetRequestsCallback;
 
   // Callback used for |AddRequest|.
   typedef base::Callback<void(AddRequestResult, const SavePageRequest& request)>
       AddRequestCallback;
 
@@ skipping 36 matching lines @@
 
   // Marks attempt with |request_id| as aborted. Results are returned through
   // |callback|.
   void MarkAttemptAborted(int64_t request_id, const UpdateCallback& callback);
 
   // Marks attempt with |request_id| as completed. The attempt may have
   // completed with either success or failure (not denoted here). Results
   // are returned through |callback|.
   void MarkAttemptCompleted(int64_t request_id, const UpdateCallback& callback);
 
+  // Make a task to pick the next request, and report our choice to the
+  // callbacks.
+  void PickNextRequest(
+      PickRequestTask::RequestPickedCallback picked_callback,
+      PickRequestTask::RequestNotPickedCallback not_picked_callback,
+      DeviceConditions& conditions,
+      std::set<int64_t>& disabled_requests);
+
+  // Takes ownership of the factory.

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

ownership transfer is implied by the parameter type.

please document how is that used.
Bonus points for explaining why it does not happen on construction.

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

Done.

+  void SetPickerFactory(std::unique_ptr<PickRequestTaskFactory> factory) {

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

why is the factory injected by setter?

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

I made a design choice to not require a PickerFactory to make the RequestQueue
easier to construct since we use it in so many places.  This will ease use of
the RequestQueue in unit tests and everywhere else besides the
RequestCoordinator.

If you disagree, my fallback plan is to just require a PickerFactory to make a
RequestQueue, and make stub picker factories for all the unit tests that need a
RequestQueue.

+    picker_factory_ = std::move(factory);
+  }
+
  private:
   // Callback used by |PurgeRequests|.
   typedef base::Callback<void(UpdateRequestResult,
                               int /* removed requests count */)>
       PurgeRequestsCallback;
 
   // Purges the queue, removing the requests that are no longer relevant, e.g.
   // expired request. Result is returned through |callback| carries the number
   // of removed requests.
   void PurgeRequests(const PurgeRequestsCallback& callback);
 
   std::unique_ptr<RequestQueueStore> store_;
 
   // Task queue to serialize store access.
   TaskQueue task_queue_;
 
+  // Builds PickRequestTask objects

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

nit: please add period at the end.

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

Done.

+  std::unique_ptr<PickRequestTaskFactory> picker_factory_;
+
   // Allows us to pass a weak pointer to callbacks.
   base::WeakPtrFactory<RequestQueue> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RequestQueue);
 };
 
 }  // namespace offline_pages
 
 #endif  // COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_QUEUE_H_