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

components/offline_pages/background/request_coordinator_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_coordinator.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 13 matching lines @@
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/background/scheduler.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
 
 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;
 const int kBatteryPercentageHigh = 75;
 const bool kPowerRequired = true;
 const bool kUserRequested = true;
 const int kAttemptCount = 1;
 }  // namespace
 
 class SchedulerStub : public Scheduler {
@@ skipping 307 matching lines @@
 TEST_F(RequestCoordinatorTest, OfflinerDoneRequestFailed) {
   // 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();
 
+  // 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(
           &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_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();
 
-  // Verify the request is not removed from the queue, and wait for callbacks.
+  // 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 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());
 }
+
+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();
+
+  // 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 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());
+}
 
 // This tests a StopProcessing call before we have actually started the
 // prerenderer.
 TEST_F(RequestCoordinatorTest, StartProcessingThenStopProcessingImmediately) {
   // 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,
@@ skipping 149 matching lines @@
   coordinator()->queue()->GetRequests(
       base::Bind(&RequestCoordinatorTest::GetRequestsDone,
                  base::Unretained(this)));
   PumpLoop();
 
   // We should find one request in the queue.
   EXPECT_EQ(1UL, last_requests().size());
 }
 
 }  // namespace offline_pages