[Offline Pages] Defines longer processing budget for immediate bg loads.

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 <string>
 #include <utility>
 #include <vector>
@@ skipping 244 matching lines @@
   void GetRequestsDone(RequestQueue::GetRequestsResult result,
                        std::vector<std::unique_ptr<SavePageRequest>> requests);
 
   // Callback for removing requests.
   void RemoveRequestsDone(const MultipleItemStatuses& results);
 
   // Callback for getting request statuses.
   void GetQueuedRequestsDone(
       std::vector<std::unique_ptr<SavePageRequest>> requests);
 
+  void StartProcessingImmediately() {
+    // Be sure conditions will allow the immediate start.
+    SetIsLowEndDeviceForTest(false);

[Pete Williamson, 2016/10/19 23:18:18]

I think I'd prefer to see this set-up in the test, these methods are supposed to
be simple pass-throughs to the same named method.

I'd even be happy with putting these values into the SetUp routine (and
overriding them as needed).

[dougarnett, 2016/10/19 23:29:12]

Had this all repeated in many tests. I can go back to that. Since they are
related to making the immediate trigger work, I was trying to collect together.

[dougarnett, 2016/10/21 20:01:19]

Dropped the immediate start and reworked common setup for OfflinerDoneCallback
tests into helper method that includes setting some necessary processing state.

+    SetEffectiveConnectionTypeForTest(
+        net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_3G);
+    coordinator()->StartImmediatelyForTest();
+  }
+
   void SendOfflinerDoneCallback(const SavePageRequest& request,
                                 Offliner::RequestStatus status);
 
   RequestQueue::GetRequestsResult last_get_requests_result() const {
     return last_get_requests_result_;
   }
 
   const std::vector<std::unique_ptr<SavePageRequest>>& last_requests() const {
     return last_requests_;
   }
 
   const MultipleItemStatuses& last_remove_results() const {
     return last_remove_results_;
   }
 
   void DisableLoading() {
     offliner_->disable_loading();
   }
 
   void EnableOfflinerCallback(bool enable) {
     offliner_->enable_callback(enable);
   }
 
   void SetNetworkConditionsForTest(
       net::NetworkChangeNotifier::ConnectionType connection) {
     coordinator()->SetNetworkConditionsForTest(connection);
   }
 
+  void SetIsLowEndDeviceForTest(bool is_low_end_device) {
+    coordinator()->is_low_end_device_ = is_low_end_device;
+  }
+
   void SetEffectiveConnectionTypeForTest(net::EffectiveConnectionType type) {
     network_quality_estimator_->SetEffectiveConnectionTypeForTest(type);
   }
 
   void ScheduleForTest() { coordinator_->ScheduleAsNeeded(); }
 
   void CallRequestNotPicked(bool non_user_requested_tasks_remaining,
                             bool disabled_tasks_remaining) {
     if (disabled_tasks_remaining)
       coordinator_->disabled_requests_.insert(kRequestId1);
@@ skipping 244 matching lines @@
   // 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();
 
-  // We need to give a callback to the request.
+  // Trigger immediate processing of queued request without automatic callback.
+  EnableOfflinerCallback(false);
+  StartProcessingImmediately();

[Pete Williamson, 2016/10/19 23:18:18]

Why add this?  The more of the chain that we bring into the test, the more
complicated to debug the test becomes. I'm trying to isolate the method under
test as much as possible.  If we need it, that's fine, but I wouldn't want to
add it just to make the tests look similar.

[dougarnett, 2016/10/19 23:29:12]

The motivation was that we now have a processing_state that was not being set up
by this test approach where it is never actually started. This was hitting a
DCHECK that I would like to keep. An alternative might be to provide test access
to directly set/mock the processing_state but seemed fragile path.

+  PumpLoop();
+
+  // Override the processing callback for test visiblity.
   base::Callback<void(bool)> callback =
       base::Bind(&RequestCoordinatorTest::ImmediateScheduleCallbackFunction,
                  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 page being completed, wait
   // for callbacks.
   SendOfflinerDoneCallback(request, Offliner::RequestStatus::SAVED);
   PumpLoop();
   EXPECT_TRUE(immediate_schedule_callback_called());
 
   // Verify the request gets removed from the queue, and wait for callbacks.
   coordinator()->queue()->GetRequests(
       base::Bind(&RequestCoordinatorTest::GetRequestsDone,
                  base::Unretained(this)));
@@ skipping 23 matching lines @@
 
   // 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.
+  // Trigger immediate processing of queued request without automatic callback.
+  EnableOfflinerCallback(false);
+  StartProcessingImmediately();

[Pete Williamson, 2016/10/19 23:18:18]

Same question here, and the other instances below.

[dougarnett, 2016/10/21 20:01:19]

Done.

+  PumpLoop();
+
+  // Override the processing callback for test visiblity.
   base::Callback<void(bool)> callback =
       base::Bind(&RequestCoordinatorTest::ImmediateScheduleCallbackFunction,
                  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.
   SendOfflinerDoneCallback(request,
                            Offliner::RequestStatus::PRERENDERING_FAILED);
   PumpLoop();
   EXPECT_TRUE(immediate_schedule_callback_called());
 
   // TODO(dougarnett): Consider injecting mock RequestPicker for this test
   // and verifying that there is no attempt to pick another request following
   // this failure code.
@@ skipping 24 matching lines @@
   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.
+  // Trigger immediate processing of queued request without automatic callback.
+  EnableOfflinerCallback(false);
+  StartProcessingImmediately();
+  PumpLoop();
+
+  // Override the processing callback for test visiblity.
   base::Callback<void(bool)> callback =
       base::Bind(&RequestCoordinatorTest::ImmediateScheduleCallbackFunction,
                  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.
   SendOfflinerDoneCallback(
       request, Offliner::RequestStatus::PRERENDERING_FAILED_NO_RETRY);
   PumpLoop();
-  EXPECT_TRUE(immediate_schedule_callback_called());
+  EXPECT_FALSE(immediate_schedule_callback_called());
 
   // 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.
 
   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();
 
-  // We need to give a callback to the request.
+  // Trigger immediate processing of queued request without automatic callback.
+  EnableOfflinerCallback(false);
+  StartProcessingImmediately();
+  PumpLoop();
+
+  // Override the processing callback for test visiblity.
   base::Callback<void(bool)> callback =
       base::Bind(&RequestCoordinatorTest::ImmediateScheduleCallbackFunction,
                  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.
   SendOfflinerDoneCallback(request,
                            Offliner::RequestStatus::FOREGROUND_CANCELED);
   PumpLoop();
   EXPECT_TRUE(immediate_schedule_callback_called());
 
   // Verify the request is not removed from the queue, and wait for callbacks.
   coordinator()->queue()->GetRequests(base::Bind(
       &RequestCoordinatorTest::GetRequestsDone, base::Unretained(this)));
   PumpLoop();
 
   // Request no longer in the queue (for single attempt policy).
   EXPECT_EQ(1UL, last_requests().size());
   // Verify foreground cancel not counted as an attempt after all.
   EXPECT_EQ(0L, last_requests().at(0)->completed_attempt_count());
 }
 
 TEST_F(RequestCoordinatorTest, OfflinerDonePrerenderingCancel) {
   // 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();
 
-  // We need to give a callback to the request.
+  // Trigger immediate processing of queued request without automatic callback.
+  EnableOfflinerCallback(false);
+  StartProcessingImmediately();
+  PumpLoop();
+
+  // Override the processing callback for test visiblity.
   base::Callback<void(bool)> callback =
       base::Bind(&RequestCoordinatorTest::ImmediateScheduleCallbackFunction,
                  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.
   SendOfflinerDoneCallback(request,
                            Offliner::RequestStatus::PRERENDERING_CANCELED);
   PumpLoop();
   EXPECT_TRUE(immediate_schedule_callback_called());
 
   // Verify the request is not removed from the queue, and wait for callbacks.
   coordinator()->queue()->GetRequests(base::Bind(
       &RequestCoordinatorTest::GetRequestsDone, base::Unretained(this)));
@@ skipping 323 matching lines @@
   WaitForCallback();
   PumpLoop();
 
   EXPECT_FALSE(is_starting());
   EXPECT_TRUE(OfflinerWasCanceled());
   EXPECT_EQ(Offliner::RequestStatus::REQUEST_COORDINATOR_TIMED_OUT,
             last_offlining_status());
 }
 
 TEST_F(RequestCoordinatorTest, WatchdogTimeoutForImmediateProcessing) {
-  // Test only applies on non-svelte device.
-  if (base::SysInfo::IsLowEndDevice())
-    return;
+  // If low end device, pretend it is not so that immediate start happens.
+  SetIsLowEndDeviceForTest(false);
 
   // Set good network connection so that adding request will trigger
   // immediate processing.
   SetEffectiveConnectionTypeForTest(
       net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_3G);
 
   // Ensure that the new request does not finish - we simulate it being
   // in progress by asking it to skip making the completion callback.
   EnableOfflinerCallback(false);
 
@@ skipping 237 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