[Offline Pages] Remove expired requests from the queue.

components/offline_pages/background/request_picker_unittest.cc

Index: components/offline_pages/background/request_picker_unittest.cc
diff --git a/components/offline_pages/background/request_picker_unittest.cc b/components/offline_pages/background/request_picker_unittest.cc
index cd74c70db4cc43d4e52c7f8a5e2219bf6e7c12d2..01ae74cee02ff3af6c8354587ebd04b23ff50b3c 100644
--- a/components/offline_pages/background/request_picker_unittest.cc
+++ b/components/offline_pages/background/request_picker_unittest.cc
@@ -9,6 +9,9 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "components/offline_pages/background/device_conditions.h"
+#include "components/offline_pages/background/offliner_factory.h"
+#include "components/offline_pages/background/offliner_policy.h"
+#include "components/offline_pages/background/request_notifier.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"
@@ -28,7 +31,6 @@ const GURL kUrl2("http://nytimes.com");
 const ClientId kClientId2("bookmark", "5678");
 const bool kUserRequested = true;
 const int kAttemptCount = 1;
-const int kTwoWeeksInSeconds = 60 * 60 * 24 * 7 * 2;
 const int kMaxStartedTries = 5;
 const int kMaxCompletedTries = 1;
 
@@ -36,8 +38,44 @@ const int kMaxCompletedTries = 1;
 const bool kPreferUntried = false;
 const bool kPreferEarlier = true;
 const bool kPreferRetryCount = true;
+
+// Default request
+const SavePageRequest kDefaultRequest(0UL,

[Pete Williamson, 2016/08/18 17:14:46]

kEmptyRequest might be a better name than kDefaultRequest.

[romax, 2016/08/18 17:48:38]

Done.

+                                      GURL(""),
+                                      ClientId("", ""),
+                                      base::Time(),
+                                      true);
 }  // namespace
 
+class RequestNotifierStub : public RequestNotifier {
+ public:
+  RequestNotifierStub()
+      : last_expired_request_(kDefaultRequest), total_expired_requests_(0) {}
+
+  void NotifyAdded(const SavePageRequest& request) override {}
+  void NotifyChanged(const SavePageRequest& request) override {}
+
+  void NotifyCompleted(const SavePageRequest& request,
+                       SavePageStatus status) override {
+    last_expired_request_ = request;
+    last_request_expiration_status_ = status;
+    total_expired_requests_++;

[Pete Williamson, 2016/08/18 17:14:46]

I don't see any code to check the number of expired requests, maybe drop this
variable?

[romax, 2016/08/18 17:48:38]

Ah i meant to use that to check how many requests and I'd like to keep it, but
adding a check for it :) 

+  }
+
+  const SavePageRequest& last_expired_request() {
+    return last_expired_request_;
+  }
+
+  RequestCoordinator::SavePageStatus last_request_expiration_status() {
+    return last_request_expiration_status_;
+  }
+
+ private:
+  SavePageStatus last_request_expiration_status_;
+  SavePageRequest last_expired_request_;
+  int32_t total_expired_requests_;
+};
+
 class RequestPickerTest : public testing::Test {
  public:
   RequestPickerTest();
@@ -58,11 +96,14 @@ class RequestPickerTest : public testing::Test {
   void QueueRequestsAndChooseOne(const SavePageRequest& request1,
                                  const SavePageRequest& request2);
 
+  RequestNotifierStub* GetNotifier() { return notifier_.get(); }
+
  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<RequestNotifierStub> notifier_;
   std::unique_ptr<SavePageRequest> last_picked_;
   std::unique_ptr<OfflinerPolicy> policy_;
   bool request_queue_empty_called_;
@@ -83,7 +124,9 @@ void RequestPickerTest::SetUp() {
       new RequestQueueInMemoryStore());
   queue_.reset(new RequestQueue(std::move(store)));
   policy_.reset(new OfflinerPolicy());
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
+  notifier_.reset(new RequestNotifierStub());
+  picker_.reset(
+      new RequestPicker(queue_.get(), policy_.get(), notifier_.get()));
   request_queue_empty_called_ = false;
 }
 
@@ -147,7 +190,8 @@ TEST_F(RequestPickerTest, ChooseRequestWithHigherRetryCount) {
   policy_.reset(new OfflinerPolicy(kPreferUntried, kPreferEarlier,
                                    kPreferRetryCount, kMaxStartedTries,
                                    kMaxCompletedTries + 1));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
+  picker_.reset(
+      new RequestPicker(queue_.get(), policy_.get(), notifier_.get()));
 
   base::Time creation_time = base::Time::Now();
   SavePageRequest request1(
@@ -182,7 +226,8 @@ TEST_F(RequestPickerTest, ChooseEarlierRequest) {
   policy_.reset(new OfflinerPolicy(kPreferUntried, kPreferEarlier,
                                    !kPreferRetryCount, kMaxStartedTries,
                                    kMaxCompletedTries));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
+  picker_.reset(
+      new RequestPicker(queue_.get(), policy_.get(), notifier_.get()));
 
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(10);
@@ -204,7 +249,8 @@ TEST_F(RequestPickerTest, ChooseSameTimeRequestWithHigherRetryCount) {
   policy_.reset(new OfflinerPolicy(kPreferUntried, kPreferEarlier,
                                    !kPreferRetryCount, kMaxStartedTries,
                                    kMaxCompletedTries + 1));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
+  picker_.reset(
+      new RequestPicker(queue_.get(), policy_.get(), notifier_.get()));
 
   base::Time creation_time = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
@@ -224,7 +270,8 @@ TEST_F(RequestPickerTest, ChooseRequestWithLowerRetryCount) {
   policy_.reset(new OfflinerPolicy(!kPreferUntried, kPreferEarlier,
                                    kPreferRetryCount, kMaxStartedTries,
                                    kMaxCompletedTries + 1));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
+  picker_.reset(
+      new RequestPicker(queue_.get(), policy_.get(), notifier_.get()));
 
   base::Time creation_time = base::Time::Now();
   SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
@@ -244,7 +291,8 @@ TEST_F(RequestPickerTest, ChooseLaterRequest) {
   policy_.reset(new OfflinerPolicy(kPreferUntried, !kPreferEarlier,
                                    !kPreferRetryCount, kMaxStartedTries,
                                    kMaxCompletedTries));
-  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
+  picker_.reset(
+      new RequestPicker(queue_.get(), policy_.get(), notifier_.get()));
 
   base::Time creation_time1 =
       base::Time::Now() - base::TimeDelta::FromSeconds(10);
@@ -260,19 +308,25 @@ TEST_F(RequestPickerTest, ChooseLaterRequest) {
   EXPECT_FALSE(request_queue_empty_called_);
 }
 
-TEST_F(RequestPickerTest, ChooseUnexpiredRequest) {
-  base::Time creation_time1 =
-      base::Time::Now() - base::TimeDelta::FromSeconds(kTwoWeeksInSeconds);
-  base::Time creation_time2 = base::Time::Now();
-  SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time1,
+TEST_F(RequestPickerTest, ChooseNonExpiredRequest) {
+  base::Time creation_time = base::Time::Now();
+  base::Time expired_time =
+      creation_time -
+      base::TimeDelta::FromSeconds(kRequestExpirationTimeInSeconds + 60);

[Pete Williamson, 2016/08/18 17:14:46]

How does this compile?  I thought that kReqeustExpriationTimeInSeconds was a
private member of offliner_policy.  Maybe instead use
policy_->GetRequestExpirationTimeInSeconds().

[romax, 2016/08/18 17:48:38]

hmm it's not private but those constants are defined within a namespace... But
changed to the getter :)

+  SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time,
                            kUserRequested);
-  SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time2,
+  SavePageRequest request2(kRequestId2, kUrl2, kClientId2, expired_time,
                            kUserRequested);
 
   QueueRequestsAndChooseOne(request1, request2);
 
-  EXPECT_EQ(kRequestId2, last_picked_->request_id());
+  PumpLoop();
+
+  EXPECT_EQ(kRequestId1, last_picked_->request_id());
   EXPECT_FALSE(request_queue_empty_called_);
+  EXPECT_EQ(kRequestId2, GetNotifier()->last_expired_request().request_id());
+  EXPECT_EQ(RequestNotifier::SavePageStatus::EXPIRED,
+            GetNotifier()->last_request_expiration_status());
 }
 
 TEST_F(RequestPickerTest, ChooseRequestThatHasNotExceededStartLimit) {