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..ed3c3b6a7c2023a3ad163161859188ecfbf72338 100644
--- a/components/offline_pages/background/request_picker.cc
+++ b/components/offline_pages/background/request_picker.cc
@@ -8,25 +8,37 @@
 #include "base/logging.h"
 #include "components/offline_pages/background/save_page_request.h"
 
+namespace {
+const int kEquiavalentRequest = 0;

[dougarnett, 2016/07/19 20:49:10]

spelling

[dougarnett, 2016/07/19 20:49:11]

Consider defining enum

[Pete Williamson, 2016/07/19 22:35:18]

Changed to enum, renamed it.

[Pete Williamson, 2016/07/19 22:35:18]

Done.

+const int kBetterRequest = 1;
+const int kWorseRequest = 2;
+}  // namespace
+
 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 +49,160 @@ void RequestPicker::GetRequestResultCallback(
   }
 
   // Pick the most deserving request for our conditions.
-  const SavePageRequest& picked_request = requests[0];
+  const SavePageRequest* picked_request = nullptr;
+
+  // Iterate once through the requests, keeping track of best candidate.
+  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_->GetMinimumBatteryPercentageForNonUserRequestOfflining()) {
+      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 which requests to prefer.
+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;
+
+  int preference = kEquiavalentRequest;
+  // First, see if we can decide based on the retry count.
+  if (policy_->RetryCountIsMoreImportantThanRecency()) {

[dougarnett, 2016/07/19 20:49:11]

As we discussed, still more complicated than we'd like. 

One other idea is to first collect comparison values and then write algorithm
using them. eg,

bool betterPriority = ....;
TriState betterRecency = ...;
TriState betterRetries = ...;

if (betterPriority)
  return true;
else if (RetryCountMoreIMportant)
  return (betterRetries != Same)
      ? betterRetries == Better
      : betterRecency == Better;
else return (betterRecency != Same)
      ? betterRecency == Better
      : betterRetries == Better;

[Pete Williamson, 2016/07/19 22:35:18]

Done.

+    // Check retry count first, then recency.
+    preference = IsNewRequestRetryCountBetter(oldRequest, newRequest);
+    if (preference == kBetterRequest) {
+      return true;
+    } else if (preference == kWorseRequest) {
+      return false;
+    } else {
+      preference = IsNewRequestRecencyBetter(oldRequest, newRequest);
+      if (preference == kBetterRequest) {
+        return true;
+      } else if (preference == kWorseRequest) {
+        return false;
+      }
+    }
+  } else {
+    // Check recency first, then retry count.
+    preference = IsNewRequestRecencyBetter(oldRequest, newRequest);
+    if (preference == kBetterRequest) {
+      return true;
+    } else if (preference == kWorseRequest) {
+      return false;
+    } else {
+      preference = IsNewRequestRetryCountBetter(oldRequest, newRequest);
+      if (preference == kBetterRequest) {
+        return true;
+      } else if (preference == kWorseRequest) {
+        return false;
+      }
+    }
+  }
+
+  // If we have no preference, defailt to prefering the old request.
+  return false;
+}
+
+// Is the new request better as regards retry count?
+int RequestPicker::IsNewRequestRetryCountBetter(
+    const SavePageRequest* oldRequest, const SavePageRequest* newRequest) {
+  // If the retry counts are equal, we have no preference.
+    if (newRequest->attempt_count() == oldRequest->attempt_count())
+      return kEquiavalentRequest;
+
+  // Check the policy to see if we should prefer more tried or less tried
+  // requests.
+  if (policy_->ShouldPreferTriedRequests()) {
+    // We prefer more-tried requests.
+    if (newRequest->attempt_count() > oldRequest->attempt_count())
+      return kBetterRequest;
+  } else {
+    // We prefer less-tried requests.
+    if (newRequest->attempt_count() < oldRequest->attempt_count())
+      return kBetterRequest;
+  }
+
+  // If we found that this wasn't as good in the area of request count,
+  // then we prefer the old request, and exit now.
+  return kWorseRequest;
+
+}
+
+// Is the new request better in regard to how long ago it was created?
+int RequestPicker::IsNewRequestRecencyBetter(
+    const SavePageRequest* oldRequest, const SavePageRequest* newRequest) {
+  // In theory requests should not have the same creation time, but if they do,
+  // we call them equivalent.
+  if (newRequest->creation_time() == oldRequest->creation_time())
+    return kEquiavalentRequest;
+
+  // Should we prefer earlier requests or later ones?
+  if (policy_->ShouldPreferEarlierRequests()) {
+    // We prefer requests made earlier.
+    if (newRequest->creation_time() < oldRequest->creation_time()) {
+      return kBetterRequest;
+    }
+  } else {
+    // We prefer requests made more recently.
+    if (newRequest->creation_time() < oldRequest->creation_time())
+      return kBetterRequest;
+  }
 
-  // When we have a best request to try next, get the request coodinator to
-  // start it.
-  picked_callback_.Run(picked_request);
+  return kWorseRequest;
 }
 
 }  // namespace offline_pages