Split the RequestPicker task into two separate tasks.

components/offline_pages/background/pick_request_task_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/pick_request_task.h"
 
 #include <memory>
 #include <set>
 
 #include "base/bind.h"
@@ skipping 77 matching lines @@
   PickRequestTaskTest();
 
   ~PickRequestTaskTest() override;
 
   void SetUp() override;
 
   void PumpLoop();
 
   void AddRequestDone(ItemActionStatus status);
 
-  void RequestPicked(const SavePageRequest& request);
+  void RequestPicked(const SavePageRequest& request, bool cleanup_needed);
 
-  void RequestNotPicked(const bool non_user_requested_tasks_remaining);
+  void RequestNotPicked(const bool non_user_requested_tasks_remaining,
+                        bool cleanup_needed);
 
   void RequestCountCallback(size_t total_count, size_t available_count);
 
   void QueueRequests(const SavePageRequest& request1,
                      const SavePageRequest& request2);
 
   // Reset the factory and the task using the current policy.
-  void MakeFactoryAndTask();
+  void MakeTask();

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

MakePickerTask?

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

Changed to MakePickRequestTask ('Picker' isn't actually in the class name).

 
   RequestNotifierStub* GetNotifier() { return notifier_.get(); }
 
   PickRequestTask* task() { return task_.get(); }
 
   void TaskCompletionCallback(Task* completed_task);
 
  protected:
   void InitializeStoreDone(bool success);
 
   std::unique_ptr<RequestQueueStore> store_;
   std::unique_ptr<RequestNotifierStub> notifier_;
   std::unique_ptr<SavePageRequest> last_picked_;
   std::unique_ptr<OfflinerPolicy> policy_;
   RequestCoordinatorEventLogger event_logger_;
   std::set<int64_t> disabled_requests_;
-  std::unique_ptr<PickRequestTaskFactory> factory_;
   std::unique_ptr<PickRequestTask> task_;
   bool request_queue_not_picked_called_;
+  bool cleanup_needed_;
   size_t total_request_count_;
   size_t available_request_count_;
   bool task_complete_called_;
 
  private:
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
 };
 
 PickRequestTaskTest::PickRequestTaskTest()
     : task_runner_(new base::TestSimpleTaskRunner),
       task_runner_handle_(task_runner_) {}
 
 PickRequestTaskTest::~PickRequestTaskTest() {}
 
 void PickRequestTaskTest::SetUp() {
   DeviceConditions conditions;
   store_.reset(new RequestQueueInMemoryStore());
   policy_.reset(new OfflinerPolicy());
   notifier_.reset(new RequestNotifierStub());
-  MakeFactoryAndTask();
+  MakeTask();
   request_queue_not_picked_called_ = false;
   total_request_count_ = 9999;
   available_request_count_ = 9999;
   task_complete_called_ = false;
   last_picked_.reset();
+  cleanup_needed_ = false;
 
   store_->Initialize(base::Bind(&PickRequestTaskTest::InitializeStoreDone,
                                 base::Unretained(this)));
   PumpLoop();
 }
 
 void PickRequestTaskTest::PumpLoop() {
   task_runner_->RunUntilIdle();
 }
 
 void PickRequestTaskTest::TaskCompletionCallback(Task* completed_task) {
   task_complete_called_ = true;
 }
 
 void PickRequestTaskTest::AddRequestDone(ItemActionStatus status) {}
 
-void PickRequestTaskTest::RequestPicked(const SavePageRequest& request) {
+void PickRequestTaskTest::RequestPicked(const SavePageRequest& request,
+                                        const bool cleanup_needed) {
   last_picked_.reset(new SavePageRequest(request));
+  cleanup_needed_ = cleanup_needed;
 }
 
 void PickRequestTaskTest::RequestNotPicked(
-    const bool non_user_requested_tasks_remaining) {
+    const bool non_user_requested_tasks_remaining,
+    const bool cleanup_needed) {
   request_queue_not_picked_called_ = true;
 }
 
 void PickRequestTaskTest::RequestCountCallback(size_t total_count,
                                                size_t available_count) {
   total_request_count_ = total_count;
   available_request_count_ = available_count;
 }
 
 // Test helper to queue the two given requests.
 void PickRequestTaskTest::QueueRequests(const SavePageRequest& request1,
                                         const SavePageRequest& request2) {
   DeviceConditions conditions;
   std::set<int64_t> disabled_requests;
   // Add test requests on the Queue.
   store_->AddRequest(request1, base::Bind(&PickRequestTaskTest::AddRequestDone,
                                           base::Unretained(this)));
   store_->AddRequest(request2, base::Bind(&PickRequestTaskTest::AddRequestDone,
                                           base::Unretained(this)));
 
   // Pump the loop to give the async queue the opportunity to do the adds.
   PumpLoop();
 }
 
-void PickRequestTaskTest::MakeFactoryAndTask() {
-  factory_.reset(new PickRequestTaskFactory(policy_.get(), notifier_.get(),
-                                            &event_logger_));
+void PickRequestTaskTest::MakeTask() {
   DeviceConditions conditions;
-  task_ = factory_->CreatePickerTask(
-      store_.get(),
+  task_.reset(new PickRequestTask(
+      store_.get(), policy_.get(),
       base::Bind(&PickRequestTaskTest::RequestPicked, base::Unretained(this)),
       base::Bind(&PickRequestTaskTest::RequestNotPicked,
                  base::Unretained(this)),
       base::Bind(&PickRequestTaskTest::RequestCountCallback,
                  base::Unretained(this)),
-      conditions, disabled_requests_);
+      conditions, disabled_requests_));
   task_->SetTaskCompletionCallbackForTesting(
       task_runner_.get(),
       base::Bind(&PickRequestTaskTest::TaskCompletionCallback,
                  base::Unretained(this)));
 }
 
 void PickRequestTaskTest::InitializeStoreDone(bool success) {
   ASSERT_TRUE(success);
 }
 
@@ skipping 10 matching lines @@
   EXPECT_EQ((size_t) 0, total_request_count_);
   EXPECT_EQ((size_t) 0, available_request_count_);
   EXPECT_TRUE(task_complete_called_);
 }
 
 TEST_F(PickRequestTaskTest, ChooseRequestWithHigherRetryCount) {
   // Set up policy to prefer higher retry count.
   policy_.reset(new OfflinerPolicy(
       kPreferUntried, kPreferEarlier, kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries + 1, kBackgroundProcessingTimeBudgetSeconds));
-  MakeFactoryAndTask();
+  MakeTask();
 
   base::Time creation_time = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
                            kUserRequested);
   request2.set_completed_attempt_count(kAttemptCount);
 
   QueueRequests(request1, request2);
 
@@ skipping 24 matching lines @@
   EXPECT_EQ(kRequestId1, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
   EXPECT_TRUE(task_complete_called_);
 }
 
 TEST_F(PickRequestTaskTest, ChooseEarlierRequest) {
   // We need a custom policy object prefering recency to retry count.
   policy_.reset(new OfflinerPolicy(
       kPreferUntried, kPreferEarlier, !kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries, kBackgroundProcessingTimeBudgetSeconds));
-  MakeFactoryAndTask();
+  MakeTask();
 
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(10);
   base::Time creation_time2 = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
                            kUserRequested);
   request2.set_completed_attempt_count(kAttemptCount);
 
   QueueRequests(request1, request2);
 
   task()->Run();
   PumpLoop();
 
   EXPECT_EQ(kRequestId1, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
   EXPECT_TRUE(task_complete_called_);
 }
 
 TEST_F(PickRequestTaskTest, ChooseSameTimeRequestWithHigherRetryCount) {
   // We need a custom policy object preferring recency to retry count.
   policy_.reset(new OfflinerPolicy(
       kPreferUntried, kPreferEarlier, !kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries + 1, kBackgroundProcessingTimeBudgetSeconds));
-  MakeFactoryAndTask();
+  MakeTask();
 
   base::Time creation_time = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
                            kUserRequested);
   request2.set_completed_attempt_count(kAttemptCount);
 
   QueueRequests(request1, request2);
 
   task()->Run();
   PumpLoop();
 
   EXPECT_EQ(kRequestId2, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
   EXPECT_TRUE(task_complete_called_);
 }
 
 TEST_F(PickRequestTaskTest, ChooseRequestWithLowerRetryCount) {
   // We need a custom policy object preferring lower retry count.
   policy_.reset(new OfflinerPolicy(
       !kPreferUntried, kPreferEarlier, kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries + 1, kBackgroundProcessingTimeBudgetSeconds));
-  MakeFactoryAndTask();
+  MakeTask();
 
   base::Time creation_time = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
                            kUserRequested);
   request2.set_completed_attempt_count(kAttemptCount);
 
   QueueRequests(request1, request2);
 
   task()->Run();
   PumpLoop();
 
   EXPECT_EQ(kRequestId1, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
   EXPECT_TRUE(task_complete_called_);
 }
 
 TEST_F(PickRequestTaskTest, ChooseLaterRequest) {
   // We need a custom policy preferring recency over retry, and later requests.
   policy_.reset(new OfflinerPolicy(
       kPreferUntried, !kPreferEarlier, !kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries, kBackgroundProcessingTimeBudgetSeconds));
-  MakeFactoryAndTask();
+  MakeTask();
 
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(10);
   base::Time creation_time2 = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
                            kUserRequested);
 
   QueueRequests(request1, request2);
@@ skipping 16 matching lines @@
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, expired_time,
                            kUserRequested);
 
   QueueRequests(request1, request2);
 
   task()->Run();
   PumpLoop();
 
   EXPECT_EQ(kRequestId1, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
-  EXPECT_EQ(kRequestId2, GetNotifier()->last_expired_request().request_id());
-  EXPECT_EQ(RequestNotifier::BackgroundSavePageResult::EXPIRED,
-            GetNotifier()->last_request_expiration_status());
-  EXPECT_EQ(1, GetNotifier()->total_expired_requests());
-  EXPECT_EQ((size_t) 1, total_request_count_);
+  EXPECT_EQ((size_t)2, total_request_count_);

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

wouldn't 2ul simply work?

applies to every size_t cast in this file.

Otherwise, please fix the spacing. (Also multiple lines)

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

Fixed the cast (that was supposed to get fixed before I first checked in the
file, I must have missed it).  

Not sure what you mean by fixing the spacing.  If you mean being inconsistent
about whether there is a space after the cast, that should be fixed by virtue of
removing the casts.

   EXPECT_EQ((size_t) 1, available_request_count_);
   EXPECT_TRUE(task_complete_called_);
+  EXPECT_TRUE(cleanup_needed_);
 }
 
 TEST_F(PickRequestTaskTest, ChooseRequestThatHasNotExceededStartLimit) {
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(1);
   base::Time creation_time2 = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
                            kUserRequested);
 
   // With default policy settings, we should choose the earlier request.
   // However, we will make the earlier reqeust exceed the limit.
   request1.set_started_attempt_count(policy_->GetMaxStartedTries());
 
   QueueRequests(request1, request2);
 
   task()->Run();
   PumpLoop();
 
   EXPECT_EQ(kRequestId2, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
-  // TODO(dougarnett): Counts should be 1 here once requests exceeding start
-  // count get cleaned up from the queue.
   EXPECT_EQ((size_t) 2, total_request_count_);
-  EXPECT_EQ((size_t) 2, available_request_count_);
+  EXPECT_EQ((size_t)1, available_request_count_);
   EXPECT_TRUE(task_complete_called_);
+  EXPECT_TRUE(cleanup_needed_);
 }
 
 TEST_F(PickRequestTaskTest, ChooseRequestThatHasNotExceededCompletionLimit) {
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(1);
   base::Time creation_time2 = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
                            kUserRequested);
 
   // With default policy settings, we should choose the earlier request.
   // However, we will make the earlier reqeust exceed the limit.
   request1.set_completed_attempt_count(policy_->GetMaxCompletedTries());
 
   QueueRequests(request1, request2);
 
   task()->Run();
   PumpLoop();
 
   EXPECT_EQ(kRequestId2, last_picked_->request_id());
   EXPECT_FALSE(request_queue_not_picked_called_);
   EXPECT_TRUE(task_complete_called_);
+  EXPECT_TRUE(cleanup_needed_);
 }
 
 TEST_F(PickRequestTaskTest, ChooseRequestThatIsNotDisabled) {
   policy_.reset(new OfflinerPolicy(
       kPreferUntried, kPreferEarlier, kPreferRetryCount, kMaxStartedTries,
       kMaxCompletedTries + 1, kBackgroundProcessingTimeBudgetSeconds));
 
   // put request 2 on disabled list, ensure request1 picked instead,
   // even though policy would prefer 2.
   disabled_requests_.insert(kRequestId2);
-  MakeFactoryAndTask();
+  MakeTask();
 
   base::Time creation_time = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
                            kUserRequested);
   SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time,
                            kUserRequested);
   request2.set_completed_attempt_count(kAttemptCount);
 
   // Add test requests on the Queue.
   QueueRequests(request1, request2);
 
   task()->Run();
   PumpLoop();
 
   // 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_not_picked_called_);
   EXPECT_EQ((size_t) 2, total_request_count_);
   EXPECT_EQ((size_t) 1, available_request_count_);
   EXPECT_TRUE(task_complete_called_);
 }
 
 }  // namespace offline_pages