Adjust scheduling for non-user requested items

components/offline_pages/background/request_coordinator.cc

Index: components/offline_pages/background/request_coordinator.cc
diff --git a/components/offline_pages/background/request_coordinator.cc b/components/offline_pages/background/request_coordinator.cc
index 88ebb6092d655506a1fd9a701bb813f1639ce229..aba77be16e0a3d6524ad5ce15948215e093e8aad 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -22,6 +22,7 @@
 namespace offline_pages {
 
 namespace {
+const bool kNonUserRequest = false;
 
 // Records the final request status UMA for an offlining request. This should
 // only be called once per Offliner::LoadAndSave request.
@@ -112,6 +113,26 @@ void RequestCoordinator::GetQueuedRequestsCallback(
   callback.Run(requests);
 }
 
+void RequestCoordinator::GetRequestsForSchedulingCallback(
+    RequestQueue::GetRequestsResult result,
+    const std::vector<SavePageRequest>& requests) {
+  bool user_requested = false;
+
+  // Examine all requests, if we find a user requested one, we will use the less
+  // restrictive conditions for user_requested requests.  Otherwise we will use
+  // the more restrictive non-user-requested conditions.
+  for (const SavePageRequest& request : requests) {
+    if (request.user_requested()) {
+      user_requested = true;
+      break;
+    }
+  }
+
+  // In the get callback, determine the least restrictive, and call
+  // GetTriggerConditions based on that.
+  scheduler_->Schedule(GetTriggerConditions(user_requested));
+}
+
 void RequestCoordinator::RemoveRequests(
     const std::vector<int64_t>& request_ids) {
   queue_->RemoveRequests(request_ids,
@@ -133,14 +154,15 @@ void RequestCoordinator::ResumeRequests(
       request_ids, SavePageRequest::RequestState::AVAILABLE,
       base::Bind(&RequestCoordinator::UpdateMultipleRequestsCallback,
                  weak_ptr_factory_.GetWeakPtr()));
-  // TODO: Should we also schedule a task, in case there is not one scheduled?
+  // Schedule a task, in case there is not one scheduled.
+  ScheduleAsNeeded();
 }
 
 void RequestCoordinator::AddRequestResultCallback(
     RequestQueue::AddRequestResult result,
     const SavePageRequest& request) {
   // Inform the scheduler that we have an outstanding task..
-  scheduler_->Schedule(GetTriggerConditionsForUserRequest());
+  ScheduleAsNeeded();
 }
 
 // Called in response to updating a request in the request queue.
@@ -171,6 +193,13 @@ void RequestCoordinator::RemoveRequestsCallback(
     NotifyCompleted(request, SavePageStatus::REMOVED);
 }
 
+void RequestCoordinator::ScheduleAsNeeded() {
+  // Get all requests from queue (there is no filtering mechanism).
+  queue_->GetRequests(
+      base::Bind(&RequestCoordinator::GetRequestsForSchedulingCallback,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
 void RequestCoordinator::StopProcessing() {
   is_canceled_ = true;
   if (offliner_ && is_busy_) {
@@ -242,9 +271,14 @@ void RequestCoordinator::RequestPicked(const SavePageRequest& request) {
   SendRequestToOffliner(request);
 }
 
-void RequestCoordinator::RequestQueueEmpty() {
+void RequestCoordinator::RequestQueueEmpty(

[dougarnett, 2016/08/24 17:32:01]

RequestNotPicked?

[Pete Williamson, 2016/08/25 00:00:58]

Done.

+    bool non_user_requested_tasks_remaining) {
   // Clear the outstanding "safety" task in the scheduler.
   scheduler_->Unschedule();
+
+  if (non_user_requested_tasks_remaining) {

[fgorski, 2016/08/24 16:07:04]

nit: drop {}

[Pete Williamson, 2016/08/25 00:00:58]

Done.

+    scheduler_->Schedule(GetTriggerConditions(kNonUserRequest));

[dougarnett, 2016/08/24 17:32:01]

nit - harder to read than !kUserRequested

[Pete Williamson, 2016/08/25 00:00:58]

Done.

+  }
   // Let the scheduler know we are done processing.
   scheduler_callback_.Run(true);
 }
@@ -374,12 +408,20 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
   }
 }
 
-const Scheduler::TriggerConditions
-RequestCoordinator::GetTriggerConditionsForUserRequest() {
-  Scheduler::TriggerConditions trigger_conditions(
-      policy_->PowerRequiredForUserRequestedPage(),
-      policy_->BatteryPercentageRequiredForUserRequestedPage(),
-      policy_->UnmeteredNetworkRequiredForUserRequestedPage());
+const Scheduler::TriggerConditions RequestCoordinator::GetTriggerConditions(
+    const bool user_requested) {
+  bool power = policy_->PowerRequiredForUserRequestedPage();

[fgorski, 2016/08/24 16:07:04]

Here is an idea: how about policy_->PowerRequired(user_requested)?

This applies to all 6 calls.
WDYT?

[Pete Williamson, 2016/08/25 00:00:58]

Done.

+  int battery = policy_->BatteryPercentageRequiredForUserRequestedPage();
+  bool unmetered = policy_->UnmeteredNetworkRequiredForUserRequestedPage();
+
+  if (!user_requested) {
+    power = policy_->PowerRequiredForNonUserRequestedPage();
+    battery = policy_->BatteryPercentageRequiredForNonUserRequestedPage();
+    unmetered = policy_->UnmeteredNetworkRequiredForNonUserRequestedPage();
+  }
+
+  Scheduler::TriggerConditions trigger_conditions(power, battery, unmetered);

[fgorski, 2016/08/24 16:07:04]

Can you return from here?

return Scheduler::TriggerConditions(power, ...);

[Pete Williamson, 2016/08/25 00:00:58]

Done.

+
   return trigger_conditions;
 }