[Offline Pages] Adds classification of some prerender FinalStatus codes as canceled operations or a…

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 526 matching lines @@
   // Now just one request in the queue since failed request removed
   // (for single attempt policy).
   EXPECT_EQ(1UL, last_requests().size());
   // Check that the observer got the notification that we failed (and the
   // subsequent notification that the request was removed).
   EXPECT_TRUE(observer().completed_called());
   EXPECT_EQ(RequestCoordinator::BackgroundSavePageResult::RETRY_COUNT_EXCEEDED,
             observer().last_status());
 }
 
+TEST_F(RequestCoordinatorTest, OfflinerDoneRequestFailedNoRetryFailure) {
+  // Add a request to the queue, wait for callbacks to finish.
+  offline_pages::SavePageRequest request(kRequestId1, kUrl1, kClientId1,
+                                         base::Time::Now(), kUserRequested);
+  request.MarkAttemptStarted(base::Time::Now());
+  coordinator()->queue()->AddRequest(
+      request, base::Bind(&RequestCoordinatorTest::AddRequestDone,
+                          base::Unretained(this)));
+  PumpLoop();

[chili, 2016/09/23 00:44:18]

Just for my information... what does this do?

[dougarnett, 2016/09/23 18:20:01]

Drives posted tasks on the task runner - eg, to get callbacks called.

[Pete Williamson, 2016/09/27 19:25:07]

To clarify a bit more, many tasks that the request coordinator does use a task
queue.  PumpLoop() will take tasks from the task queue, and run until the task
queue is empty (so if a request in a task queue puts something else in the task
queue before it finishes, PumpLoop will run that too before returning.)
In this particular case, this is making sure that the RequestQueue has a chance
to complete the Add operation, and call the callback before the test proceeds to
the next step.

+
+  // 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);

[Pete Williamson, 2016/09/27 19:25:08]

I don't think we want this line.  As I recall, it enables the code to call the
OfflinerDone callback itself after you start processing.  What we want is to
turn off the normal callback since we are calling it below.  Since we don't call
StartProcessing, it never comes into play here, and you should be able to remove
it safely.

[dougarnett, 2016/09/29 01:11:23]

Good catch - cleaning up the other identical cases.

+  SendOfflinerDoneCallback(
+      request, Offliner::RequestStatus::PRERENDERING_FAILED_NO_RETRY);
+  PumpLoop();
+
+  // TODO(dougarnett): Consider injecting mock RequestPicker for this test
+  // and verifying that there is as attempt to pick another request following
+  // this non-retryable failure code.
+
+  // Verify neither request is removed from the queue; wait for callbacks.

[Pete Williamson, 2016/09/27 19:25:07]

Comment looks old and may need updating (since there should now only be one
request in the queue).

[dougarnett, 2016/09/29 01:11:23]

Done.

+  coordinator()->queue()->GetRequests(base::Bind(
+      &RequestCoordinatorTest::GetRequestsDone, base::Unretained(this)));
+  PumpLoop();
+
+  // Now just one request in the queue since non-retryable failure.
+  EXPECT_EQ(1UL, last_requests().size());
+  // Check that the observer got the notification that we failed (and the
+  // subsequent notification that the request was removed).
+  EXPECT_TRUE(observer().completed_called());
+  EXPECT_EQ(RequestCoordinator::BackgroundSavePageResult::PRERENDER_FAILURE,
+            observer().last_status());
+}
+
 TEST_F(RequestCoordinatorTest, OfflinerDoneForegroundCancel) {
   // Add a request to the queue, wait for callbacks to finish.
   offline_pages::SavePageRequest request(
       kRequestId1, kUrl1, kClientId1, base::Time::Now(), kUserRequested);
   request.MarkAttemptStarted(base::Time::Now());
   coordinator()->queue()->AddRequest(
       request, base::Bind(&RequestCoordinatorTest::AddRequestDone,
                           base::Unretained(this)));
   PumpLoop();
 
@@ skipping 500 matching lines @@
   // Now whether processing triggered immediately depends on whether test
   // is run on svelte device or not.
   if (base::SysInfo::IsLowEndDevice()) {
     EXPECT_FALSE(is_busy());
   } else {
     EXPECT_TRUE(is_busy());
   }
 }
 
 }  // namespace offline_pages