Check time budget before starting new requests, and unittest.

components/offline_pages/background/request_coordinator.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 <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 17 matching lines @@
                             request_status,
                             Offliner::RequestStatus::STATUS_COUNT);
 }
 
 // TODO(dougarnett/petewil): Move to Policy object. Also consider lower minimum
 // battery percentage once there is some processing time limits in place.
 const Scheduler::TriggerConditions kUserRequestTriggerConditions(
     false /* require_power_connected */,
     50 /* minimum_battery_percentage */,
     false /* require_unmetered_network */);
-
-// Timeout is 2.5 minutes based on the size of Marshmallow doze mode
-// maintenance window (3 minutes)
-// TODO(petewil): Find the optimal timeout based on data for 2G connections and
-// common EM websites. crbug.com/625204
-// TODO(petewil): Move this value into the policy object.
-long OFFLINER_TIMEOUT_SECONDS = 150;
-
 }  // namespace
 
 RequestCoordinator::RequestCoordinator(std::unique_ptr<OfflinerPolicy> policy,
                                        std::unique_ptr<OfflinerFactory> factory,
                                        std::unique_ptr<RequestQueue> queue,
                                        std::unique_ptr<Scheduler> scheduler)
     : is_busy_(false),
       is_canceled_(false),
-      offliner_timeout_(base::TimeDelta::FromSeconds(OFFLINER_TIMEOUT_SECONDS)),
       offliner_(nullptr),
+      operation_start_time_(base::Time::Now()),

[jianli, 2016/07/25 21:50:38]

It seems to me that this initialization was not needed.

[Pete Williamson, 2016/07/25 22:01:33]

Done.

       policy_(std::move(policy)),
       factory_(std::move(factory)),
       queue_(std::move(queue)),
       scheduler_(std::move(scheduler)),
       last_offlining_status_(Offliner::RequestStatus::UNKNOWN),
+      offliner_timeout_(base::TimeDelta::FromSeconds(
+          policy_->GetSinglePageTimeBudgetInSeconds())),
       weak_ptr_factory_(this) {
   DCHECK(policy_ != nullptr);
   picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
 }
 
 RequestCoordinator::~RequestCoordinator() {}
 
 bool RequestCoordinator::SavePageLater(
     const GURL& url, const ClientId& client_id, bool was_user_requested) {
   DVLOG(2) << "URL is " << url << " " << __FUNCTION__;
@@ skipping 50 matching lines @@
 }
 
 // Returns true if the caller should expect a callback, false otherwise. For
 // instance, this would return false if a request is already in progress.
 bool RequestCoordinator::StartProcessing(
     const DeviceConditions& device_conditions,
     const base::Callback<void(bool)>& callback) {
   current_conditions_.reset(new DeviceConditions(device_conditions));
   if (is_busy_) return false;
 
+  // Mark the time at which we started processing so we can check our time
+  // budget.
+  operation_start_time_ = base::Time::Now();
+
   is_canceled_ = false;
   scheduler_callback_ = callback;
   // TODO(petewil): Check existing conditions (should be passed down from
   // BackgroundTask)
 
   TryNextRequest();
 
   return true;
 }
 
 void RequestCoordinator::TryNextRequest() {
+  // If there is no time left in the budget, return to the scheduler, but leave
+  // the "safety" task, so we get called again next time.

[jianli, 2016/07/25 21:50:38]

I found it hard to understand what the "safety" task was meant for. Could you
please rephrase it?

[Pete Williamson, 2016/07/25 22:01:33]

Done.  I was referring to something in the design docs, but it is better if the
code is standalone.

+  if (base::Time::Now() - operation_start_time_ >
+      base::TimeDelta::FromSeconds(
+          policy_->GetBackgroundProcessingTimeBudgetSeconds())) {
+    // Let the scheduler know we are done processing.
+    scheduler_callback_.Run(true);

[jianli, 2016/07/25 21:50:38]

Do we need to return here?

[Pete Williamson, 2016/07/25 22:01:33]

Oh my goodness, you're right!  Added.

+  }
+
   // Choose a request to process that meets the available conditions.
   // This is an async call, and returns right away.
   picker_->ChooseNextRequest(
       base::Bind(&RequestCoordinator::RequestPicked,
                  weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&RequestCoordinator::RequestQueueEmpty,
                  weak_ptr_factory_.GetWeakPtr()),
       current_conditions_.get());
 }
 
@@ skipping 70 matching lines @@
     updated_request.set_last_attempt_time(base::Time::Now());
     RequestQueue::UpdateRequestCallback update_callback =
         base::Bind(&RequestCoordinator::UpdateRequestCallback,
                    weak_ptr_factory_.GetWeakPtr());
     queue_->UpdateRequest(
         updated_request,
         base::Bind(&RequestCoordinator::UpdateRequestCallback,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 
-  // TODO(petewil): Check time budget. Return to the scheduler if we are out.
-  // Start another request if we have time.
   TryNextRequest();
 }
 
 const Scheduler::TriggerConditions&
 RequestCoordinator::GetTriggerConditionsForUserRequest() {
   return kUserRequestTriggerConditions;
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages