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..1e5f1a12ef16ff5e30dc78c1998d28747f651142 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 queue contents from the queue, use them to pick the next

[dougarnett, 2016/07/18 21:45:44]

remove one of the "queue" references on this line

[Pete Williamson, 2016/07/18 22:52:09]

Done.

+// request to operate on (if any).
 void RequestPicker::GetRequestResultCallback(
     RequestQueue::GetRequestsResult,
     const std::vector<SavePageRequest>& requests) {
@@ -37,11 +43,110 @@ 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

[dougarnett, 2016/07/18 21:45:44]

Consider if better: Iterate once through the requests, keeping track of best
candidate.

[Pete Williamson, 2016/07/18 22:52:09]

I think that is what we are doing.  We iterate once through, asking two
questions each time, is this request eligible, and is it better.  Is my code
formatting confusing somehow?  If so, let me know and I'll make it more
readable.  Is my comment misleading?

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

Was proposing different comment wording. I found "handle" wording bit confusing
initially but is ok. 

Fix spelling if you keep your wording.

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

Done.

+  // is better.
+  for (unsigned i = 0; i < requests.size(); ++i) {
+    if (!RequestMeetsConditions(requests[i])) {

[dougarnett, 2016/07/18 21:45:44]

ConditionsSatisyRequest() ?

[Pete Williamson, 2016/07/18 22:52:09]

ConditionsSatisfyRequest seems a bit ambiguous to me.
RequestConditionsSatisfied?

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

good

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

Done.

+      DVLOG(0) << "@@@@@@ request did not meet.";

[dougarnett, 2016/07/18 21:45:44]

clean up these before landing?

[Pete Williamson, 2016/07/18 22:52:09]

Oops, though they were already gone.  Removed now.

+        continue;
+    }
+    DVLOG(0) << "@@@@@@ request met.";
+    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::RequestMeetsConditions(const SavePageRequest& request) {

[dougarnett, 2016/07/18 21:45:44]

might be nicer to pass in conditions rather then use member var (could be
simpler to test).

[Pete Williamson, 2016/07/18 22:52:09]

I'm not sure it helps much, we typically make a new picker for each test,
setting conditions in the ctor, and it is easy to add a SetConditionsForTest
method if we end up needing it.

I'm happy to change this if you feel strongly, or if we discover a need for it
later while testing.

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

Just was idea to consider. It seems like pure logic function so if it was also
static, then could have very simple suite of tests for it that don't involve
setting up picker instance. 

+  // 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.

[Pete Williamson, 2016/07/18 17:41:06]

Reviewers: Please think about if we got the right criteria.  Did we check them
in the right order? Are there any other criteria to consider? Should we use
last_attempt_time?

[dougarnett, 2016/07/19 15:49:49]

Down the road, I expect we may want to consider last_attempt_time if we know it
failed previously rather than canceled. But we may need to understand failure
and cancel scenarios better first.

I did not find it immediately obvious that you have the right order of retry
count vs. earlier/later as they have some interaction. If you want earlier
requests, then I think that consideration has higher priority than retry count
but if you don't want earlier requests then I think the retry count has higher
priority. So I think what you have works but is a bit hard to follow the retry
count logic and understanding that you won't be inserting "earlier" requests
that can have a lower retry count than existing entries. Maybe easier to discuss
in person today to keep as is or how we might improve it. 

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

Per our discussion, I added a new policy to pick whether retry count or recency
should come first.

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