Adjust scheduling for non-user requested items

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"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
 #include "base/time/time.h"
 #include "components/offline_pages/background/offliner_factory.h"
 #include "components/offline_pages/background/offliner_policy.h"
 #include "components/offline_pages/background/request_picker.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/offline_page_model.h"
 
 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.
 void RecordOfflinerResultUMA(const ClientId& client_id,
                              Offliner::RequestStatus request_status) {
   // TODO(dougarnett): Consider exposing AddHistogramSuffix from
   // offline_page_model_impl.cc as visible utility method.
   std::string histogram_name("OfflinePages.Background.OfflinerRequestStatus");
   if (!client_id.name_space.empty()) {
     histogram_name += "." + client_id.name_space;
@@ skipping 70 matching lines @@
                                  weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void RequestCoordinator::GetQueuedRequestsCallback(
     const GetRequestsCallback& callback,
     RequestQueue::GetRequestsResult result,
     const std::vector<SavePageRequest>& requests) {
   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,
                          base::Bind(&RequestCoordinator::RemoveRequestsCallback,
                                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RequestCoordinator::PauseRequests(
     const std::vector<int64_t>& request_ids) {
   queue_->ChangeRequestsState(
       request_ids, SavePageRequest::RequestState::PAUSED,
       base::Bind(&RequestCoordinator::UpdateMultipleRequestsCallback,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RequestCoordinator::ResumeRequests(
     const std::vector<int64_t>& request_ids) {
   queue_->ChangeRequestsState(
       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.
 void RequestCoordinator::UpdateRequestCallback(
     const ClientId& client_id,
     RequestQueue::UpdateRequestResult result) {
   // If the request succeeded, nothing to do.  If it failed, we can't really do
   // much, so just log it.
   if (result != RequestQueue::UpdateRequestResult::SUCCESS) {
     DVLOG(1) << "Failed to update request attempt details. "
@@ skipping 10 matching lines @@
     NotifyChanged(request);
 }
 
 void RequestCoordinator::RemoveRequestsCallback(
     const RequestQueue::UpdateMultipleRequestResults& results,
     const std::vector<SavePageRequest>& requests) {
   for (SavePageRequest request : requests)
     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_) {
     // TODO(dougarnett): Find current request and mark attempt aborted.
     offliner_->Cancel();
   }
 
   // Stopping offliner means it will not call callback.
   last_offlining_status_ =
       Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED;
@@ skipping 51 matching lines @@
                  weak_ptr_factory_.GetWeakPtr()),
       current_conditions_.get());
 }
 
 // Called by the request picker when a request has been picked.
 void RequestCoordinator::RequestPicked(const SavePageRequest& request) {
   // Send the request on to the offliner.
   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);
 }
 
 void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
   // Check that offlining didn't get cancelled while performing some async
   // steps.
   if (is_canceled_)
     return;
 
@@ skipping 109 matching lines @@
     case Offliner::RequestStatus::PRERENDERING_FAILED:
       // No further processing in this service window.
       break;
     default:
       // Make explicit choice about new status codes that actually reach here.
       // Their default is no further processing in this service window.
       DCHECK(false);
   }
 }
 
-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;
 }
 
 void RequestCoordinator::AddObserver(Observer* observer) {
   DCHECK(observer);
   observers_.AddObserver(observer);
 }
 
 void RequestCoordinator::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
@@ skipping 12 matching lines @@
   FOR_EACH_OBSERVER(Observer, observers_, OnChanged(request));
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages