More detailed implementation of the RequestPicker

components/offline_pages/background/request_picker.cc

Index: components/offline_pages/background/request_picker.cc
diff --git a/components/offline_pages/background/request_picker.cc b/components/offline_pages/background/request_picker.cc
index 665b39fee53977fd875c138db8e3e7bbe4729b4f..c9cf1018b6baa24a7146abcc2c5b0b4b8ab28f06 100644
--- a/components/offline_pages/background/request_picker.cc
+++ b/components/offline_pages/background/request_picker.cc
@@ -11,22 +11,28 @@
 namespace offline_pages {
 
 RequestPicker::RequestPicker(
-    RequestQueue* requestQueue)
+    RequestQueue* requestQueue, OfflinerPolicy* policy)
     : queue_(requestQueue),
+      policy_(policy),
       weak_ptr_factory_(this) {}
 
 RequestPicker::~RequestPicker() {}
 
+// Entry point for the async operation to choose the next request.
 void RequestPicker::ChooseNextRequest(
     RequestCoordinator::RequestPickedCallback picked_callback,
-    RequestCoordinator::RequestQueueEmptyCallback empty_callback) {
+    RequestCoordinator::RequestQueueEmptyCallback empty_callback,
+    DeviceConditions* device_conditions) {
   picked_callback_ = picked_callback;
   empty_callback_ = empty_callback;
+  current_conditions_.reset(new DeviceConditions(*device_conditions));
   // Get all requests from queue (there is no filtering mechanism).
   queue_->GetRequests(base::Bind(&RequestPicker::GetRequestResultCallback,
                                  weak_ptr_factory_.GetWeakPtr()));
 }
 
+// When we get contents from the queue, use them to pick the next
+// request to operate on (if any).
 void RequestPicker::GetRequestResultCallback(
     RequestQueue::GetRequestsResult,
     const std::vector<SavePageRequest>& requests) {
@@ -37,11 +43,108 @@ void RequestPicker::GetRequestResultCallback(
   }
 
   // Pick the most deserving request for our conditions.
-  const SavePageRequest& picked_request = requests[0];
+  const SavePageRequest* picked_request = nullptr;
 
-  // When we have a best request to try next, get the request coodinator to
-  // start it.
-  picked_callback_.Run(picked_request);
+  // Handle each request only once, replacing the best reqeust candidate if it
+  // is better.
+  for (unsigned i = 0; i < requests.size(); ++i) {
+    if (!RequestConditionsSatisfied(requests[i])) {
+        continue;
+    }
+    if (IsNewRequestBetter(picked_request, &(requests[i])))
+      picked_request = &(requests[i]);
+  }
+
+  // If we have a best request to try next, get the request coodinator to
+  // start it.  Otherwise return that we have no candidates.
+  if (picked_request != nullptr) {
+    picked_callback_.Run(*picked_request);
+  } else {
+    empty_callback_.Run();
+  }
+}
+
+// Filter out requests that don't meet the current conditions.  For instance, if
+// this is a predictive request, and we are not on WiFi, it should be ignored
+// this round.
+bool RequestPicker::RequestConditionsSatisfied(const SavePageRequest& request) {
+  // If the user did not request the page directly, make sure we are connected
+  // to power and have WiFi and sufficient battery remaining before we take this
+  // reqeust.
+  // TODO(petewil): We may later want to configure whether to allow 2G for non
+  // user_requested items, add that to policy.
+  if (!request.user_requested()) {
+    if (!current_conditions_->IsPowerConnected())
+      return false;
+
+    if (current_conditions_->GetNetConnectionType() !=
+        net::NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI) {
+      return false;
+    }
+
+    if (current_conditions_->GetBatteryPercentage() <
+        policy_->GetBatteryRequiredForSpeculativeOfflining()) {
+      return false;
+    }
+  }
+
+  // If we have already tried this page the max number of times, it is not
+  // eligible to try again.
+  // TODO(petewil): Instead, we should have code to remove the page from the
+  // queue after the last retry.
+  if (request.attempt_count() >= policy_->GetMaxRetries())
+    return false;
+
+  // If this request is not active yet, return false.
+  if (request.activation_time() > base::Time::Now())
+    return false;
+
+  return true;
+}
+
+// Look at policies to decide if we prefer more-tried or less tried requests.
+bool RequestPicker::IsNewRequestBetter(
+    const SavePageRequest* oldRequest, const SavePageRequest* newRequest) {
+
+  // If there is no old request, the new one is better.
+  if (oldRequest == nullptr)
+    return true;
+
+  // User requested pages get priority.
+  if (newRequest->user_requested() && !oldRequest->user_requested())
+    return true;
+
+  // First, see if we can decide based on the retry count.
+  if (policy_->ShouldPreferTriedRequests()) {
+    // We prefer more-tried requests.
+    if (newRequest->attempt_count() > oldRequest->attempt_count())
+      return true;
+  } else {
+    // We prefer less-tried requests.
+    if (newRequest->attempt_count() < oldRequest->attempt_count())
+      return true;
+  }
+
+  // If we found that this wasn't as good in the area of request count,

[dougarnett, 2016/07/19 00:20:44]

I'm not sure I understand this initially. Is this really part of the above
check? I wonder if the not equal check should go around the above with
fall-through return of false?

[Pete Williamson, 2016/07/19 19:48:08]

I was trying to make the logic above smaller by collecting the return cases. 
This is all different now, but the original intent was that if the retry count
matched, we needed a tiebreaker, if it didn't match, this was the common "fail"
case where the original request was better.

+  // then we prefer the old request, and exit now.
+  if (newRequest->attempt_count() != oldRequest->attempt_count())
+    return false;
+
+  // Try counts are the same, so look at other criteria.
+
+  // Should we prefer earlier requests or later ones?
+  if (policy_->ShouldPreferEarlierRequests()) {
+    // We prefer requests made earlier.
+    if (newRequest->creation_time() < oldRequest->creation_time()) {

[dougarnett, 2016/07/19 00:20:44]

So we make no assumptions about queue ordering which seems safe but I wonder if
we should discuss with Filip. Eg, if we decided that the queue would guarantee
ordering by creation time or something (since it is named "queue"), then we
could consider returning early in some case. 

[Pete Williamson, 2016/07/19 19:48:08]

The "queue" doesn't do any ordering for us in spite of its name.  I did bring
that up once in a code review, but the answer ws that it fits the logical
description of a queue as a place for pending work.

+      return true;
+    }
+  } else {
+    // We prefer requests made more recently.
+    if (newRequest->creation_time() < oldRequest->creation_time())
+      return true;
+  }
+
+  return false;
 }
 
 }  // namespace offline_pages