Skips processing more SavePageLater requests in current processing window when one completes unsucc…

components/offline_pages/background/request_coordinator_unittest.cc

Index: components/offline_pages/background/request_coordinator_unittest.cc
diff --git a/components/offline_pages/background/request_coordinator_unittest.cc b/components/offline_pages/background/request_coordinator_unittest.cc
index ca13ae5a8da5ee00e1dd58fb1b74b8943e341a4a..7fffd093643cb7e6ebf81f48a183a2e242938188 100644
--- a/components/offline_pages/background/request_coordinator_unittest.cc
+++ b/components/offline_pages/background/request_coordinator_unittest.cc
@@ -31,6 +31,9 @@ namespace {
 // put test constants here
 const GURL kUrl("http://universe.com/everything");
 const ClientId kClientId("bookmark", "42");
+const int kRequestId2(2);
+const GURL kUrl2("http://universe.com/toinfinityandbeyond");
+const ClientId kClientId2("bookmark", "43");
 const int kRequestId(1);
 const long kTestTimeoutSeconds = 1;
 const long kTestTimeBudgetSeconds = 200;
@@ -358,6 +361,15 @@ TEST_F(RequestCoordinatorTest, OfflinerDoneRequestFailed) {
                  base::Unretained(this)));
   PumpLoop();
 
+  // Add second request to the queue to check handling when first fails.
+  offline_pages::SavePageRequest request2(
+      kRequestId2, kUrl2, kClientId2, base::Time::Now(), kUserRequested);
+  coordinator()->queue()->AddRequest(
+      request2,
+      base::Bind(&RequestCoordinatorTest::AddRequestDone,
+                 base::Unretained(this)));
+  PumpLoop();
+
   // We need to give a callback to the request.
   base::Callback<void(bool)> callback =
       base::Bind(
@@ -375,23 +387,76 @@ TEST_F(RequestCoordinatorTest, OfflinerDoneRequestFailed) {
   EnableOfflinerCallback(true);
   SendOfflinerDoneCallback(request,
                            Offliner::RequestStatus::PRERENDERING_FAILED);
-  // There will be one request left in the queue after the prerender fails, stop
-  // processing now so that it will remain in the queue for us to check.  This
-  // won't affect the offliner done callback other than preventing
-  // TryNextRequest from doing anything.
-  coordinator()->StopProcessing();
+  PumpLoop();
+
+  // TODO(dougarnett): Consider injecting mock RequestPicker for this test
+  // and verifying that there is no attempt to pick another request following
+  // this failure code.
+
+  // Verify neither request is removed from the queue; wait for callbacks.
+  coordinator()->queue()->GetRequests(
+      base::Bind(&RequestCoordinatorTest::GetRequestsDone,
+                 base::Unretained(this)));
+  PumpLoop();
 
+  // Still two requests in the queue.
+  EXPECT_EQ(2UL, last_requests().size());
+  // Verify retry count was incremented for first request.
+  const SavePageRequest& found_request = last_requests().front();
+  EXPECT_EQ(1L, found_request.attempt_count());
+}
+
+TEST_F(RequestCoordinatorTest, OfflinerDoneRequestCanceled) {

[Pete Williamson, 2016/08/02 22:20:00]

Why are we removing this test?  Intentional?

[dougarnett, 2016/08/03 15:33:26]

Intentional - wasn't really testing anything additional and have some merge
conflict coming up with your change that also defines 2nd request values. My
plan is to inject mock RequestPicker (per line 392 TODO) when I split the
PRENDERING_FAILED codes so we can really verify whether TryNextRequest gets
called or not.

+  // Add a request to the queue, wait for callbacks to finish.
+  offline_pages::SavePageRequest request(
+      kRequestId, kUrl, kClientId, base::Time::Now(), kUserRequested);
+  coordinator()->queue()->AddRequest(
+      request,
+      base::Bind(&RequestCoordinatorTest::AddRequestDone,
+                 base::Unretained(this)));
   PumpLoop();
 
-  // Verify the request is not removed from the queue, and wait for callbacks.
+  // Add second request to the queue to check handling when first cancels.
+  offline_pages::SavePageRequest request2(
+      kRequestId2, kUrl2, kClientId2, base::Time::Now(), kUserRequested);
+  coordinator()->queue()->AddRequest(
+      request2,
+      base::Bind(&RequestCoordinatorTest::AddRequestDone,
+                 base::Unretained(this)));
+  PumpLoop();
+
+  // We need to give a callback to the request.
+  base::Callback<void(bool)> callback =
+      base::Bind(
+          &RequestCoordinatorTest::EmptyCallbackFunction,
+          base::Unretained(this));
+  coordinator()->SetProcessingCallbackForTest(callback);
+
+  // Set up device conditions for the test.
+  DeviceConditions device_conditions(
+      false, 75, net::NetworkChangeNotifier::CONNECTION_3G);
+  SetDeviceConditionsForTest(device_conditions);
+
+  // Call the OfflinerDoneCallback to simulate the request failed, wait
+  // for callbacks.
+  EnableOfflinerCallback(true);
+  SendOfflinerDoneCallback(request,
+                           Offliner::RequestStatus::PRERENDERING_CANCELED);
+  PumpLoop();
+
+  // TODO(dougarnett): Consider injecting mock RequestPicker for this test
+  // and verifying that there is no attempt to pick another request following
+  // the first one being canceled.
+
+  // Verify neither request is removed from the queue; wait for callbacks.
   coordinator()->queue()->GetRequests(
       base::Bind(&RequestCoordinatorTest::GetRequestsDone,
                  base::Unretained(this)));
   PumpLoop();
 
-  // Still one request in the queue.
-  EXPECT_EQ(1UL, last_requests().size());
-  // Verify retry count was incremented.
+  // Still two requests in the queue.
+  EXPECT_EQ(2UL, last_requests().size());
+  // Verify retry count was incremented for first request.
   const SavePageRequest& found_request = last_requests().front();
   EXPECT_EQ(1L, found_request.attempt_count());
 }