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..2409dee428257c4dd2298c5c3b1acf9313cda3c6 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,149 @@ 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);
+  // Iterate once through the requests, keeping track of best candidate.
+  for (unsigned i = 0; i < requests.size(); ++i) {
+    if (!RequestConditionsSatisfied(requests[i])) {

[fgorski, 2016/07/20 16:23:31]

nit: {} not needed

[Pete Williamson, 2016/07/20 19:50:44]

Done.

+        continue;

[fgorski, 2016/07/20 16:23:31]

nit: indentation

[Pete Williamson, 2016/07/20 19:50:44]

Done.

+    }
+    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;
+
+  ComparisonResult retryCountPreference =
+      IsNewRequestRetryCountBetter(oldRequest, newRequest);
+  ComparisonResult recencyPreference =
+      IsNewRequestRecencyBetter(oldRequest, newRequest);
+
+  // Check to see if we should prefer RetryCount or Recency as the primary
+  // deciding factor.
+  if (policy_->RetryCountIsMoreImportantThanRecency()) {
+    // Check retry count first, then recency.
+    if (retryCountPreference != Same) {

[fgorski, 2016/07/20 16:23:31]

This use of result both here and below (specifically treating same as false
through return false), as well as the naming of methods including
IsNewRequest...Better tells me that these methods should be returning bool, as
they are predicates.

(Treat this as an alternative solution to the approach presented below, using
STL.)

[Pete Williamson, 2016/07/20 19:50:44]

OK, per our discussion, I will try an alternate approach with the next version
of the patch, stay tuned...

+      return (retryCountPreference == Better);
+    } else {
+      return (recencyPreference == Better);
+    }
+  } else {
+    // Check recency first, then retry count.
+    if (recencyPreference != Same) {
+      return (recencyPreference == Better);
+    } else {
+      return (retryCountPreference == Better);
+    }
+  }
+
+  // If we have no preference, defailt to prefering the old request.
+  return false;
+}
+
+// Is the new request better as regards retry count?
+ComparisonResult RequestPicker::IsNewRequestRetryCountBetter(

[fgorski, 2016/07/20 16:23:31]

I have 2 concerns with these implementations.
1) They don't conform to Compare as defined by STL, therefor you cannot use
simple std::sort or std::min (which would be much simpler)

2) They are mixing policy and comparison in a way where policy has to be
consulted in every comparison. I see an alternative: a method that produces a
Compare object suitable to a given policy/device conditions or anything else.
And then throwing that Compare object on std::min.

RequestConditionsSatisfied could be applied in a same way, as UnaryPredicate
using std::remove_if.

A set of simple predicates also removes the need for defining Better/Worse/...
enum.

+    const SavePageRequest* oldRequest, const SavePageRequest* newRequest) {
+  // If the retry counts are equal, we have no preference.

[fgorski, 2016/07/20 16:23:31]

This is self evident in the code. (A lot of your comments below are not adding
to the understanding. )

[Pete Williamson, 2016/07/20 19:50:44]

Done.  (My thinking was that this is a bit convoluted, and having comments will
help the readability of the code as a whole.)

+    if (newRequest->attempt_count() == oldRequest->attempt_count())

[fgorski, 2016/07/20 16:23:31]

nit: aligment

[Pete Williamson, 2016/07/20 19:50:44]

Done.

+      return Same;
+
+  // 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 Better;
+  } else {
+    // We prefer less-tried requests.
+    if (newRequest->attempt_count() < oldRequest->attempt_count())
+      return Better;
+  }
+
+  // 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 Worse;
+

[fgorski, 2016/07/20 16:23:31]

nit: empty line is not needed here.

[Pete Williamson, 2016/07/20 19:50:44]

Done.

+}
+
+// Is the new request better in regard to how long ago it was created?
+ComparisonResult 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 Same;
+
+  // Should we prefer earlier requests or later ones?
+  if (policy_->ShouldPreferEarlierRequests()) {
+    // We prefer requests made earlier.
+    if (newRequest->creation_time() < oldRequest->creation_time()) {
+      return Better;
+    }
+  } else {
+    // We prefer requests made more recently.
+    if (newRequest->creation_time() < oldRequest->creation_time())
+      return Better;
+  }
+
+  return Worse;
 }
 
 }  // namespace offline_pages