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

BUG=655341

Review-Url: https://codereview.chromium.org/2431193003
Cr-Commit-Position: refs/heads/master@{#427098}

components/offline_pages/background/offliner_policy.h

@@ -8,10 +8,23 @@
 namespace {
 const int kMaxStartedTries = 4;
 const int kMaxCompletedTries = 1;
-const int kDefaultBackgroundProcessingTimeBudgetSeconds = 170;
-const int kSinglePageTimeLimitWhenBackgroundScheduledSeconds = 120;
-const int kSinglePageTimeLimitForImmediateLoadSeconds = 300;
 const int kRequestExpirationTimeInSeconds = 60 * 60 * 24 * 7;
+
+// Scheduled background processing time limits.
+const int kDozeModeBackgroundServiceWindowSeconds = 60 * 3;
+const int kDefaultBackgroundProcessingTimeBudgetSeconds =
+    kDozeModeBackgroundServiceWindowSeconds - 10;
+const int kSinglePageTimeLimitWhenBackgroundScheduledSeconds =
+    kDozeModeBackgroundServiceWindowSeconds - 10;
+
+// Immediate processing time limits.
+// Note: experiments on GIN-2g-poor show many page requests took 3 or 4
+// attempts in background scheduled mode with timeout of 2 minutes. So for
+// immediate processing mode, give page requests 4 times that limit (8 min).
+// Then budget up to 5 of those requests in processing window.
+const int kSinglePageTimeLimitForImmediateLoadSeconds = 60 * 8;
+const int kImmediateLoadProcessingTimeBudgetSeconds =
+    kSinglePageTimeLimitForImmediateLoadSeconds * 5;
 }  // namespace
 
 namespace offline_pages {
@@ -26,7 +39,7 @@ class OfflinerPolicy {
         retry_count_is_more_important_than_recency_(false),
         max_started_tries_(kMaxStartedTries),
         max_completed_tries_(kMaxCompletedTries),
-        background_processing_time_budget_(
+        background_scheduled_processing_time_budget_(
             kDefaultBackgroundProcessingTimeBudgetSeconds) {}
 
   // Constructor for unit tests.
@@ -41,7 +54,8 @@ class OfflinerPolicy {
         retry_count_is_more_important_than_recency_(prefer_retry_count),
         max_started_tries_(max_started_tries),
         max_completed_tries_(max_completed_tries),
-        background_processing_time_budget_(background_processing_time_budget) {}
+        background_scheduled_processing_time_budget_(
+            background_processing_time_budget) {}
 
   // TODO(petewil): Numbers here are chosen arbitrarily, do the proper studies
   // to get good policy numbers. Eventually this should get data from a finch
@@ -87,8 +101,16 @@ class OfflinerPolicy {
 
   // How many seconds to keep trying new pages for, before we give up, and
   // return to the scheduler.
-  int GetBackgroundProcessingTimeBudgetSeconds() const {
-    return background_processing_time_budget_;
+  // TODO(dougarnett): Consider parameterizing these time limit/budget
+  // calls with processing mode.
+  int GetProcessingTimeBudgetWhenBackgroundScheduledInSeconds() const {
+    return background_scheduled_processing_time_budget_;
+  }
+
+  // How many seconds to keep trying new pages for, before we give up, when
+  // processing started immediately (without scheduler).
+  int GetProcessingTimeBudgetForImmediateLoadInSeconds() const {
+    return kImmediateLoadProcessingTimeBudgetSeconds;
   }
 
   // How long do we allow a page to load before giving up on it when
@@ -114,7 +136,7 @@ class OfflinerPolicy {
   bool retry_count_is_more_important_than_recency_;
   int max_started_tries_;
   int max_completed_tries_;
-  int background_processing_time_budget_;
+  int background_scheduled_processing_time_budget_;
 };
 }  // namespace offline_pages
 

components/offline_pages/background/request_coordinator.cc

@@ -122,7 +122,8 @@ RequestCoordinator::RequestCoordinator(
     std::unique_ptr<Scheduler> scheduler,
     net::NetworkQualityEstimator::NetworkQualityProvider*
         network_quality_estimator)
-    : is_busy_(false),
+    : is_low_end_device_(base::SysInfo::IsLowEndDevice()),
+      is_busy_(false),
       is_starting_(false),
       processing_state_(ProcessingWindowState::STOPPED),
       use_test_connection_type_(false),
@@ -468,8 +469,8 @@ RequestCoordinator::TryImmediateStart() {
     return OfflinerImmediateStartStatus::BUSY;
 
   // Make sure we are not on svelte device to start immediately.
+  if (is_low_end_device_) {
     // Let the scheduler know we are done processing and failed due to svelte.
-  if (base::SysInfo::IsLowEndDevice()) {
     immediate_schedule_callback_.Run(false);
     return OfflinerImmediateStartStatus::NOT_STARTED_ON_SVELTE;
   }
@@ -499,13 +500,21 @@ RequestCoordinator::TryImmediateStart() {
 }
 
 void RequestCoordinator::TryNextRequest() {
+  base::TimeDelta processing_time_budget;
+  if (processing_state_ == ProcessingWindowState::SCHEDULED_WINDOW) {
+    processing_time_budget = base::TimeDelta::FromSeconds(
+        policy_->GetProcessingTimeBudgetWhenBackgroundScheduledInSeconds());
+  } else {
+    DCHECK(processing_state_ == ProcessingWindowState::IMMEDIATE_WINDOW);
+    processing_time_budget = base::TimeDelta::FromSeconds(
+        policy_->GetProcessingTimeBudgetForImmediateLoadInSeconds());
+  }
+
   // If there is no time left in the budget, return to the scheduler.
   // We do not remove the pending task that was set up earlier in case
   // we run out of time, so the background scheduler will return to us
   // at the next opportunity to run background tasks.
-  if (base::Time::Now() - operation_start_time_ >
-      base::TimeDelta::FromSeconds(
-          policy_->GetBackgroundProcessingTimeBudgetSeconds())) {
+  if ((base::Time::Now() - operation_start_time_) > processing_time_budget) {
     is_starting_ = false;
 
     // Let the scheduler know we are done processing.

components/offline_pages/background/request_coordinator.h

@@ -145,6 +145,8 @@ class RequestCoordinator : public KeyedService,
     immediate_schedule_callback_ = callback;
   }
 
+  void StartImmediatelyForTest() { StartImmediatelyIfConnected(); }
+
   // Observers implementing the RequestCoordinator::Observer interface can
   // register here to get notifications of changes to request state.  This
   // pointer is not owned, and it is the callers responsibility to remove the
@@ -339,6 +341,9 @@ class RequestCoordinator : public KeyedService,
 
   friend class RequestCoordinatorTest;
 
+  // Cached value of whether low end device. Overwritable for testing.
+  bool is_low_end_device_;
+
   // The offliner can only handle one request at a time - if the offliner is
   // busy, prevent other requests.  This flag marks whether the offliner is in
   // use.

components/offline_pages/background/request_coordinator_unittest.cc

@@ -262,6 +262,9 @@ class RequestCoordinatorTest
   void GetQueuedRequestsDone(
       std::vector<std::unique_ptr<SavePageRequest>> requests);
 
+  void SetupForOfflinerDoneCallbackTest(
+      offline_pages::SavePageRequest* request);
+
   void SendOfflinerDoneCallback(const SavePageRequest& request,
                                 Offliner::RequestStatus status);
 
@@ -290,6 +293,19 @@ class RequestCoordinatorTest
     coordinator()->SetNetworkConditionsForTest(connection);
   }
 
+  void SetIsLowEndDeviceForTest(bool is_low_end_device) {
+    coordinator()->is_low_end_device_ = is_low_end_device;
+  }
+
+  void SetProcessingStateForTest(
+      RequestCoordinator::ProcessingWindowState processing_state) {
+    coordinator()->processing_state_ = processing_state;
+  }
+
+  void SetOperationStartTimeForTest(base::Time start_time) {
+    coordinator()->operation_start_time_ = start_time;
+  }
+
   void SetEffectiveConnectionTypeForTest(net::EffectiveConnectionType type) {
     network_quality_estimator_->SetEffectiveConnectionTypeForTest(type);
   }
@@ -403,6 +419,33 @@ void RequestCoordinatorTest::AddRequestDone(
     RequestQueue::AddRequestResult result,
     const SavePageRequest& request) {}
 
+void RequestCoordinatorTest::SetupForOfflinerDoneCallbackTest(
+    offline_pages::SavePageRequest* request) {
+  // Mark request as started and add it to the queue,
+  // then wait for callback to finish.
+  request->MarkAttemptStarted(base::Time::Now());
+  coordinator()->queue()->AddRequest(
+      *request, base::Bind(&RequestCoordinatorTest::AddRequestDone,
+                           base::Unretained(this)));
+  PumpLoop();
+
+  // Override the processing callback for test visiblity.
+  base::Callback<void(bool)> callback =
+      base::Bind(&RequestCoordinatorTest::ImmediateScheduleCallbackFunction,
+                 base::Unretained(this));
+  coordinator()->SetProcessingCallbackForTest(callback);
+
+  // Mock that coordinator is in actively processing state starting now.
+  SetProcessingStateForTest(
+      RequestCoordinator::ProcessingWindowState::IMMEDIATE_WINDOW);
+  SetOperationStartTimeForTest(base::Time::Now());
+
+  // Set up good device conditions for the test.
+  DeviceConditions device_conditions(false, 75,
+                                     net::NetworkChangeNotifier::CONNECTION_3G);
+  SetDeviceConditionsForTest(device_conditions);
+}
+
 void RequestCoordinatorTest::SendOfflinerDoneCallback(
     const SavePageRequest& request, Offliner::RequestStatus status) {
   // Using the fact that the test class is a friend, call to the callback
@@ -553,23 +596,7 @@ TEST_F(RequestCoordinatorTest, OfflinerDoneRequestSucceeded) {
   // 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.
-  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);
+  SetupForOfflinerDoneCallbackTest(&request);
 
   // Call the OfflinerDoneCallback to simulate the page being completed, wait
   // for callbacks.
@@ -598,12 +625,7 @@ TEST_F(RequestCoordinatorTest, OfflinerDoneRequestFailed) {
   // 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();
+  SetupForOfflinerDoneCallbackTest(&request);
 
   // Add second request to the queue to check handling when first fails.
   offline_pages::SavePageRequest request2(
@@ -614,22 +636,14 @@ TEST_F(RequestCoordinatorTest, OfflinerDoneRequestFailed) {
                  base::Unretained(this)));
   PumpLoop();
 
-  // We need to give a callback to the request.
-  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();
+
+  // For retriable failure, processing should stop and scheduler callback
+  // called (so that request can be retried first next processing window).
   EXPECT_TRUE(immediate_schedule_callback_called());
 
   // TODO(dougarnett): Consider injecting mock RequestPicker for this test
@@ -655,11 +669,7 @@ 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();
+  SetupForOfflinerDoneCallbackTest(&request);
 
   // Add second request to the queue to check handling when first fails.
   offline_pages::SavePageRequest request2(kRequestId2, kUrl2, kClientId2,
@@ -669,23 +679,15 @@ TEST_F(RequestCoordinatorTest, OfflinerDoneRequestFailedNoRetryFailure) {
                            base::Unretained(this)));
   PumpLoop();
 
-  // We need to give a callback to the request.
-  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());
+
+  // For no retry failure, processing should continue to 2nd request so
+  // no scheduler callback yet.
+  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
@@ -708,22 +710,7 @@ 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.
-  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);
+  SetupForOfflinerDoneCallbackTest(&request);
 
   // Call the OfflinerDoneCallback to simulate the request failed, wait
   // for callbacks.
@@ -747,22 +734,7 @@ 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.
-  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);
+  SetupForOfflinerDoneCallbackTest(&request);
 
   // Call the OfflinerDoneCallback to simulate the request failed, wait
   // for callbacks.
@@ -1107,9 +1079,8 @@ TEST_F(RequestCoordinatorTest, WatchdogTimeoutForScheduledProcessing) {
 }
 
 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.