Add the request picker and a unit test

The RequestPicker is responsible for choosing which request from the
request queue to process next.  It will apply policies from the policy
class to the data returned by the request queue, pick a likely
candidate, and call back to the request coordinator so it can start
offlining the candidate.  If there is no candidate, it will call back
to the request coordinator to let it know processing is done for this
cycle.

BUG=610521

Review-Url: https://codereview.chromium.org/2020833002
Cr-Commit-Position: refs/heads/master@{#396967}

components/components_tests.gyp

@@ -423,6 +423,7 @@
     ],
     'offline_pages_background_unittest_sources': [
       'offline_pages/background/request_coordinator_unittest.cc',
+      'offline_pages/background/request_picker_unittest.cc',
       'offline_pages/background/request_queue_in_memory_store_unittest.cc',
       'offline_pages/background/request_queue_unittest.cc',
       'offline_pages/background/save_page_request_unittest.cc',

components/offline_pages.gypi

@@ -64,6 +64,8 @@
         'offline_pages/background/offliner_policy.h',
         'offline_pages/background/request_coordinator.cc',
         'offline_pages/background/request_coordinator.h',
+        'offline_pages/background/request_picker.cc',
+        'offline_pages/background/request_picker.h',
         'offline_pages/background/request_queue.cc',
         'offline_pages/background/request_queue.h',
         'offline_pages/background/request_queue_in_memory_store.cc',

components/offline_pages/background/BUILD.gn

@@ -14,6 +14,8 @@ static_library("background_offliner") {
     "offliner_policy.h",
     "request_coordinator.cc",
     "request_coordinator.h",
+    "request_picker.cc",
+    "request_picker.h",
     "request_queue.cc",
     "request_queue.h",
     "request_queue_in_memory_store.cc",
@@ -37,6 +39,7 @@ source_set("unit_tests") {
   testonly = true
   sources = [
     "request_coordinator_unittest.cc",
+    "request_picker_unittest.cc",
     "request_queue_in_memory_store_unittest.cc",
     "request_queue_unittest.cc",
     "save_page_request_unittest.cc",

components/offline_pages/background/request_coordinator.cc

@@ -7,8 +7,10 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/logging.h"
 #include "components/offline_pages/background/offliner_factory.h"
 #include "components/offline_pages/background/offliner_policy.h"
+#include "components/offline_pages/background/request_picker.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/background/scheduler.h"
 #include "components/offline_pages/offline_page_item.h"
@@ -23,8 +25,10 @@ RequestCoordinator::RequestCoordinator(std::unique_ptr<OfflinerPolicy> policy,
       factory_(std::move(factory)),
       queue_(std::move(queue)),
       scheduler_(std::move(scheduler)),
-      last_offlining_status_(Offliner::RequestStatus::UNKNOWN) {
+      last_offlining_status_(Offliner::RequestStatus::UNKNOWN),
+      weak_ptr_factory_(this) {
   DCHECK(policy_ != nullptr);
+  picker_.reset(new RequestPicker(queue_.get()));
 }
 
 RequestCoordinator::~RequestCoordinator() {}
@@ -44,12 +48,11 @@ bool RequestCoordinator::SavePageLater(
   // Put the request on the request queue.
   queue_->AddRequest(request,
                      base::Bind(&RequestCoordinator::AddRequestResultCallback,
-                                AsWeakPtr()));
+                                weak_ptr_factory_.GetWeakPtr()));
   // TODO(petewil): Do I need to persist the request in case the add fails?
 
-  // TODO(petewil): Eventually we will wait for the StartProcessing callback,
-  // but for now just kick start the request so we can test the wiring.
-  SendRequestToOffliner(request);
+  // TODO(petewil): Make a new chromium command line switch to send the request
+  // immediately for testing.  It should call SendRequestToOffliner()
 
   return true;
 }
@@ -57,7 +60,6 @@ bool RequestCoordinator::SavePageLater(
 void RequestCoordinator::AddRequestResultCallback(
     RequestQueue::AddRequestResult result,
     const SavePageRequest& request) {
-  DVLOG(2) << __FUNCTION__;
 
   // Inform the scheduler that we have an outstanding task.
   // TODO(petewil): Define proper TriggerConditions and set them.
@@ -65,8 +67,28 @@ void RequestCoordinator::AddRequestResultCallback(
   scheduler_->Schedule(conditions);
 }
 
+void RequestCoordinator::RequestPicked(const SavePageRequest& request) {
+  // Send the request on to the offliner.
+  SendRequestToOffliner(request);
+}
+
+void RequestCoordinator::RequestQueueEmpty() {
+  // TODO(petewil): return to the BackgroundScheduler by calling
+  // ProcessingDoneCallback
+}
+
 bool RequestCoordinator::StartProcessing(
     const ProcessingDoneCallback& callback) {
+  // TODO(petewil): Check existing conditions (should be passed down from
+  // BackgroundTask)
+
+  // Choose a request to process that meets the available conditions.
+  // This is an async call, and returns right away.
+  picker_->ChooseNextRequest(
+      base::Bind(&RequestCoordinator::RequestPicked,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&RequestCoordinator::RequestQueueEmpty,
+                 weak_ptr_factory_.GetWeakPtr()));
   return false;
 }
 
@@ -83,9 +105,9 @@ void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
   }
 
   // Start the load and save process in the offliner (Async).
-  offliner->LoadAndSave(
-      request,
-      base::Bind(&RequestCoordinator::OfflinerDoneCallback, AsWeakPtr()));
+  offliner->LoadAndSave(request,
+                        base::Bind(&RequestCoordinator::OfflinerDoneCallback,
+                                   weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
@@ -94,6 +116,11 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
            << (status == Offliner::RequestStatus::SAVED) << ", "
            << __FUNCTION__;
   last_offlining_status_ = status;
+
+  // TODO(petewil): Check time budget.  Start a request if we have time, return
+  // to the scheduler if we are out of time.
+
+  // TODO(petewil): If the request succeeded, remove it from the Queue.
 }
 
 }  // namespace offline_pages

components/offline_pages/background/request_coordinator.h

@@ -21,16 +21,18 @@ struct ClientId;
 class OfflinerPolicy;
 class OfflinerFactory;
 class Offliner;
+class RequestPicker;
 class SavePageRequest;
 class Scheduler;
 
 // Coordinates queueing and processing save page later requests.
-class RequestCoordinator :
-    public KeyedService,
-    public base::SupportsWeakPtr<RequestCoordinator>  {
+class RequestCoordinator : public KeyedService {
  public:
   // Callback to report when the processing of a triggered task is complete.
   typedef base::Callback<void()> ProcessingDoneCallback;
+  typedef base::Callback<void(const SavePageRequest& request)>
+      RequestPickedCallback;
+  typedef base::Callback<void()> RequestQueueEmptyCallback;
 
   RequestCoordinator(std::unique_ptr<OfflinerPolicy> policy,
                      std::unique_ptr<OfflinerFactory> factory,
@@ -69,6 +71,12 @@ class RequestCoordinator :
   void AddRequestResultCallback(RequestQueue::AddRequestResult result,
                                 const SavePageRequest& request);
 
+  // Callback from the request picker when it has chosen our next request.
+  void RequestPicked(const SavePageRequest& request);
+
+  // Callback from the request picker when no more requests are in the queue.
+  void RequestQueueEmpty();
+
   void SendRequestToOffliner(const SavePageRequest& request);
 
   void OfflinerDoneCallback(const SavePageRequest& request,
@@ -84,6 +92,10 @@ class RequestCoordinator :
   std::unique_ptr<Scheduler> scheduler_;
   // Status of the most recent offlining.
   Offliner::RequestStatus last_offlining_status_;
+  // Class to choose which request to schedule next
+  std::unique_ptr<RequestPicker> picker_;
+  // Allows us to pass a weak pointer to callbacks.
+  base::WeakPtrFactory<RequestCoordinator> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RequestCoordinator);
 };

components/offline_pages/background/request_coordinator_unittest.cc

@@ -172,12 +172,6 @@ TEST_F(RequestCoordinatorTest, SavePageLater) {
   SchedulerStub* scheduler_stub = reinterpret_cast<SchedulerStub*>(
       coordinator()->scheduler());
   EXPECT_TRUE(scheduler_stub->schedule_called());
-
-  // Check that the offliner callback got a response.
-  EXPECT_EQ(Offliner::RequestStatus::SAVED,
-            coordinator()->last_offlining_status());
-
-  // TODO(petewil): Expect that the scheduler got notified.
 }
 
 }  // namespace offline_pages

components/offline_pages/background/request_picker.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 "base/bind.h"
+#include "base/logging.h"
+#include "components/offline_pages/background/save_page_request.h"
+
+namespace offline_pages {
+
+RequestPicker::RequestPicker(
+    RequestQueue* requestQueue)
+    : queue_(requestQueue),
+      weak_ptr_factory_(this) {}
+
+RequestPicker::~RequestPicker() {}
+
+void RequestPicker::ChooseNextRequest(
+    RequestCoordinator::RequestPickedCallback picked_callback,
+    RequestCoordinator::RequestQueueEmptyCallback empty_callback) {
+  picked_callback_ = picked_callback;
+  empty_callback_ = empty_callback;
+  // Get all requests from queue (there is no filtering mechanism).
+  queue_->GetRequests(base::Bind(&RequestPicker::GetRequestResultCallback,
+                                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void RequestPicker::GetRequestResultCallback(
+    RequestQueue::GetRequestsResult,
+    const std::vector<SavePageRequest>& requests) {
+  // If there is nothing to do, return right away.
+  if (requests.size() == 0) {
+    empty_callback_.Run();
+    return;
+  }
+
+  // Pick the most deserving request for our conditions.
+  const SavePageRequest& picked_request = requests[0];
+
+  // When we have a best request to try next, get the request coodinator to
+  // start it.
+  picked_callback_.Run(picked_request);
+}
+
+}  // namespace offline_pages

components/offline_pages/background/request_picker.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_PICKER_H_
+#define COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_PICKER_H_
+
+#include "base/memory/weak_ptr.h"
+#include "components/offline_pages/background/request_coordinator.h"
+#include "components/offline_pages/background/request_queue.h"
+
+namespace offline_pages {
+
+class RequestPicker {
+ public:
+  RequestPicker(RequestQueue* requestQueue);
+
+  ~RequestPicker();
+
+  // Choose which request we should process next based on the current
+  // conditions, and call back to the RequestCoordinator when we have one.
+  void ChooseNextRequest(
+      RequestCoordinator::RequestPickedCallback picked_callback,
+      RequestCoordinator::RequestQueueEmptyCallback empty_callback);
+
+ private:
+  // Callback for the GetRequest results to be delivered.
+  void GetRequestResultCallback(RequestQueue::GetRequestsResult result,
+                                const std::vector<SavePageRequest>& results);
+
+  // unowned pointer to the request queue.
+  RequestQueue* queue_;
+  // Callback for when we are done picking a request to do next.
+  RequestCoordinator::RequestPickedCallback picked_callback_;
+  // Callback for when there are no more reqeusts to pick.
+  RequestCoordinator::RequestQueueEmptyCallback empty_callback_;
+  // Allows us to pass a weak pointer to callbacks.
+  base::WeakPtrFactory<RequestPicker> weak_ptr_factory_;
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_PICKER_H_

components/offline_pages/background/request_picker_unittest.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 "base/bind.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "components/offline_pages/background/request_queue.h"
+#include "components/offline_pages/background/request_queue_in_memory_store.h"
+#include "components/offline_pages/background/save_page_request.h"
+#include "components/offline_pages/offline_page_item.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace offline_pages {
+
+namespace {
+// Data for request 1.
+const int64_t kRequestId1 = 17;
+const GURL kUrl1("https://google.com");
+const ClientId kClientId1("bookmark", "1234");
+// Data for request 2.
+const int64_t kRequestId2 = 42;
+const GURL kUrl2("http://nytimes.com");
+const ClientId kClientId2("bookmark", "5678");
+}  // namespace
+
+class RequestPickerTest : public testing::Test {
+ public:
+  RequestPickerTest();
+
+  ~RequestPickerTest() override;
+
+  void SetUp() override;
+
+  void PumpLoop();
+
+  void AddRequestDone(RequestQueue::AddRequestResult result,
+                      const SavePageRequest& request);
+
+  void RequestPicked(const SavePageRequest& request);
+
+  void RequestQueueEmpty();
+
+ protected:
+  // The request queue is simple enough we will use a real queue with a memory
+  // store instead of a stub.
+  std::unique_ptr<RequestQueue> queue_;
+  std::unique_ptr<RequestPicker> picker_;
+  std::unique_ptr<SavePageRequest> last_picked_;
+  bool request_queue_empty_called_;
+
+ private:
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  base::ThreadTaskRunnerHandle task_runner_handle_;
+};
+
+RequestPickerTest::RequestPickerTest()
+    : task_runner_(new base::TestSimpleTaskRunner),
+      task_runner_handle_(task_runner_) {}
+
+RequestPickerTest::~RequestPickerTest() {}
+
+void RequestPickerTest::SetUp() {
+  std::unique_ptr<RequestQueueInMemoryStore> store(
+      new RequestQueueInMemoryStore());
+  queue_.reset(new RequestQueue(std::move(store)));
+  picker_.reset(new RequestPicker(queue_.get()));
+  request_queue_empty_called_ = false;
+}
+
+void RequestPickerTest::PumpLoop() {
+  task_runner_->RunUntilIdle();
+}
+
+void RequestPickerTest::AddRequestDone(RequestQueue::AddRequestResult result,
+                                       const SavePageRequest& request) {}
+
+void RequestPickerTest::RequestPicked(const SavePageRequest& request) {
+  last_picked_.reset(new SavePageRequest(request));
+}
+
+void RequestPickerTest::RequestQueueEmpty() {
+  request_queue_empty_called_ = true;
+}
+
+TEST_F(RequestPickerTest, ChooseNextRequest) {
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time);
+  SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time);
+  // Put some test requests on the Queue.
+  queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+  queue_->AddRequest(request2, base::Bind(&RequestPickerTest::AddRequestDone,
+                                          base::Unretained(this)));
+
+  // Pump the loop to give the async queue the opportunity to do the adds.
+  PumpLoop();
+
+  picker_->ChooseNextRequest(
+      base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
+      base::Bind(&RequestPickerTest::RequestQueueEmpty,
+                 base::Unretained(this)));
+
+  // Pump the loop again to give the async queue the opportunity to return
+  // results from the Get operation, and for the picker to call the "picked"
+  // callback.
+  PumpLoop();
+
+  EXPECT_EQ(kRequestId1, last_picked_->request_id());
+  EXPECT_FALSE(request_queue_empty_called_);
+}
+
+TEST_F(RequestPickerTest, PickFromEmptyQueue) {
+  picker_->ChooseNextRequest(
+      base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
+      base::Bind(&RequestPickerTest::RequestQueueEmpty,
+                 base::Unretained(this)));
+
+  // Pump the loop again to give the async queue the opportunity to return
+  // results from the Get operation, and for the picker to call the "QueueEmpty"
+  // callback.
+  PumpLoop();
+
+  EXPECT_TRUE(request_queue_empty_called_);
+}
+
+}  // namespace offline_pages