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..7d87ad7bbbc54a0c9faa68e365981742b6549865 100644
--- a/components/offline_pages/background/request_picker.cc
+++ b/components/offline_pages/background/request_picker.cc
@@ -8,25 +8,44 @@
 #include "base/logging.h"
 #include "components/offline_pages/background/save_page_request.h"
 
+namespace {
+template <typename T>
+int signum(T t) {
+  return (T(0) < t) - (t < T(0));
+}
+
+#define CALL_MEMBER_FUNCTION(object,ptrToMember)  ((object)->*(ptrToMember))
+}  // namespace
+
 namespace offline_pages {
 
 RequestPicker::RequestPicker(
-    RequestQueue* requestQueue)
+    RequestQueue* requestQueue, OfflinerPolicy* policy)
     : queue_(requestQueue),
+      policy_(policy),
+      fewer_retries_better_(false),
+      earlier_requests_better_(false),
       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;
+  fewer_retries_better_ = policy_->ShouldPreferUntriedRequests();
+  earlier_requests_better_ = policy_->ShouldPreferEarlierRequests();
+  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 +56,148 @@ void RequestPicker::GetRequestResultCallback(
   }
 
   // Pick the most deserving request for our conditions.
-  const SavePageRequest& picked_request = requests[0];
+  const SavePageRequest* picked_request = nullptr;
+
+  RequestCompareFunction comparator = nullptr;
+
+  // Choose which comparison function to use based on policy.
+  if (policy_->RetryCountIsMoreImportantThanRecency()) {
+    comparator = &RequestPicker::RetryCountFirstCompareFunction;
+  }

[fgorski, 2016/07/21 17:01:20]

I don't think you need either pair of {}

[Pete Williamson, 2016/07/21 19:48:45]

Done.  I misinterpreted our guidelines as anything other than a single line "if"
needing braces.  This is one line of if and one line of else.

+  else {
+    comparator = &RequestPicker::RecencyFirstCompareFunction;
+  }
+
+  // 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]), comparator))
+      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.
+  // TODO(petewil): If the only reason we return nothing to do is that we have
+  // inactive requests, we still want to try again later after their activation
+  // time elapses, we shouldn't take ourselves completely off the scheduler.
+  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,
+                                       RequestCompareFunction comparator) {
+
+  // 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;
+
+  // Otherwise, use the comparison function for the current policy, which
+  // returns true if hte older request is better.

[fgorski, 2016/07/21 17:01:20]

s/hte/the/

[Pete Williamson, 2016/07/21 19:48:45]

Done.

+  return !(CALL_MEMBER_FUNCTION(this, comparator)(oldRequest, newRequest));
+}
+
+// Compare the results, checking request count before recency.  Returns true if
+// left hand side is better, false otherwise.
+bool RequestPicker::RetryCountFirstCompareFunction(
+    const SavePageRequest* left, const SavePageRequest* right) {
+  // Check the attempt count.

[fgorski, 2016/07/21 17:01:20]

Consider extracting the block from here to the first return as
CompareAttemptCount. This will cover sign revert (and its explanation in one
place.

[Pete Williamson, 2016/07/21 19:48:45]

After I thought about it, I prefer leaving the code as is for several reasons.
First, I can't think of a good name for the new function that will indicate what
it really does.
Second, subtracting times gives me a time difference that I can't just do a
signum on, I need the additional step of converting to milliseconds, which would
mean subtracting at the call site something like I have below.
Third, the new function would need to be a template instead of a normal function
since it would take int64_t for the time difference, and int for the retry
count.  While there is nothing wrong with a Template per-se, it makes the code
just a bit more complidated, and my aim here is to make it more readable.

// new function declaration:
int GetSignAndMaybeFlip(T difference, boolean maybeFlip);

// Function at call site
result = GetSignAndMaybeFlip(
    (left->creation_time() - right->creation_time()).InMilliseconds),
    earlier_requests_better));

I think this would lead to code that is harder to understand, though a bit
better factored.

[fgorski, 2016/07/22 16:02:11]

I was thinking of 2 functions that are specific to what they do:
CompareAttemptCount
CompareCreationTime

You don't have to make the change if you don't want to.

[Pete Williamson, 2016/07/22 16:25:53]

Oh, that's different than what I thought you were suggesting, and totally makes
sense.  Done.

+  int result = signum(left->attempt_count() - right->attempt_count());
+
+  // Flip the direction of comparison if policy prefers fewer retries.
+  if (fewer_retries_better_)
+    result *= -1;
+
+  if (result != 0)
+    return (result > 0);
+
+  // If we get here, the attempt counts were the same, so check recency.
+  base::TimeDelta difference = left->creation_time() - right->creation_time();

[fgorski, 2016/07/21 17:01:20]

Same for this section. (Extract)

And both extracted pieces are reused in the other compare function

+  result = signum(difference.InMilliseconds());
+
+  // Flip the direction of the check if policy prefers later requests.
+  if (earlier_requests_better_)
+    result *= -1;
+
+  if (result == 1)

[fgorski, 2016/07/21 17:01:20]

this whole block to the end is equivalent to:

return result > 0;

[Pete Williamson, 2016/07/21 19:48:45]

Done.

+    return true;
+
+  return false;
+}
+
+// Compare the results, checking recency before request count. Returns true if
+// left hand side is better, false otherwise.
+bool RequestPicker::RecencyFirstCompareFunction(
+    const SavePageRequest* left, const SavePageRequest* right) {
+  // Check the recency.
+    base::TimeDelta difference = left->creation_time() - right->creation_time();

[fgorski, 2016/07/21 17:01:20]

nit: indentation

[Pete Williamson, 2016/07/21 19:48:45]

Done.

+    int result = signum(difference.InMilliseconds());
+
+  // Flip the direction of comparison if policy prefers fewer retries.
+  if (earlier_requests_better_)
+    result *= -1;
+
+  if (result != 0)
+    return (result > 0);
+
+  // If we get here, the recency was the same, so check the attempt count.
+  result = signum(left->attempt_count() - right->attempt_count());
+
+  // Flip the direction of the check if policy prefers later requests.
+  if (fewer_retries_better_)
+    result *= -1;
+
+  if (result == 1)

[fgorski, 2016/07/21 17:01:20]

same here: return result > 0

[Pete Williamson, 2016/07/21 19:48:45]

Done.

+    return true;
 
-  // When we have a best request to try next, get the request coodinator to
-  // start it.
-  picked_callback_.Run(picked_request);
+  return false;
 }
 
 }  // namespace offline_pages