[Offline Pages] Pick correct request when resuming.

components/offline_pages/core/background/pick_request_task.cc

Index: components/offline_pages/core/background/pick_request_task.cc
diff --git a/components/offline_pages/core/background/pick_request_task.cc b/components/offline_pages/core/background/pick_request_task.cc
index 006d08b47aff2211d377e6869f8b95e49273185f..8f5a888ed724134572ba4abd7006a8d2535543be 100644
--- a/components/offline_pages/core/background/pick_request_task.cc
+++ b/components/offline_pages/core/background/pick_request_task.cc
@@ -35,13 +35,15 @@ PickRequestTask::PickRequestTask(RequestQueueStore* store,
                                  RequestNotPickedCallback not_picked_callback,
                                  RequestCountCallback request_count_callback,
                                  DeviceConditions& device_conditions,
-                                 const std::set<int64_t>& disabled_requests)
+                                 const std::set<int64_t>& disabled_requests,
+                                 std::vector<int64_t>& prioritized_requests)
     : store_(store),
       policy_(policy),
       picked_callback_(picked_callback),
       not_picked_callback_(not_picked_callback),
       request_count_callback_(request_count_callback),
       disabled_requests_(disabled_requests),
+      prioritized_requests_(prioritized_requests),
       weak_ptr_factory_(this) {
   device_conditions_.reset(new DeviceConditions(device_conditions));
 }
@@ -69,7 +71,7 @@ void PickRequestTask::Choose(
     return;
   }
 
-  // Pick the most deserving request for our conditions.
+  // pick the most deserving request for our conditions.

[Pete Williamson, 2017/03/07 19:06:04]

This was better before.  Accidental change?

[romax, 2017/03/07 20:41:45]

Done.

   const SavePageRequest* picked_request = nullptr;
 
   RequestCompareFunction comparator = nullptr;
@@ -84,9 +86,14 @@ void PickRequestTask::Choose(
   bool non_user_requested_tasks_remaining = false;
   bool cleanup_needed = false;
 
+  size_t request_size = requests.size();
   size_t available_request_count = 0;
 
-  // Iterate once through the requests, keeping track of best candidate.
+  std::vector<std::unique_ptr<SavePageRequest>> available_requests;
+
+  // Iterate through the requests, filter out unavailable requests and get other
+  // information (if cleanup is needed and number of non-user-requested
+  // requests).
   for (unsigned i = 0; i < requests.size(); ++i) {
     // If the request is expired or has exceeded the retry count, skip it.
     if (OfflinerPolicyUtils::CheckRequestExpirationStatus(requests[i].get(),
@@ -95,8 +102,7 @@ void PickRequestTask::Choose(
       cleanup_needed = true;
       continue;
     }
-
-    // If the  request is on the disabled list, skip it.
+    // If the request is on the disabled list, skip it.
     auto search = disabled_requests_.find(requests[i]->request_id());
     if (search != disabled_requests_.end())
       continue;
@@ -112,14 +118,36 @@ void PickRequestTask::Choose(
         SavePageRequest::RequestState::AVAILABLE) {
       available_request_count++;
     }
-    if (!RequestConditionsSatisfied(requests[i].get()))
-      continue;
-    if (IsNewRequestBetter(picked_request, requests[i].get(), comparator))
-      picked_request = requests[i].get();
+    available_requests.push_back(std::move(requests[i]));
   }
-
   // Report the request queue counts.
-  request_count_callback_.Run(requests.size(), available_request_count);
+  request_count_callback_.Run(request_size, available_requests.size());
+
+  // Search for and pick the prioritized requests which is not disabled and in

[Pete Williamson, 2017/03/07 19:06:04]

Nit: requests -> request

[romax, 2017/03/07 20:41:45]

Done.

+  // the request queue. An assumption is that requests later in the vector have
+  // higher priority than earlier ones in the vector.
+  while (!picked_request && !prioritized_requests_.empty()) {
+    int64_t id = prioritized_requests_.back();
+    for (unsigned i = 0; i < available_requests.size(); ++i) {
+      if (available_requests[i]->request_id() == id) {
+        picked_request = available_requests[i].get();
+        break;
+      }
+    }
+    // Remove the prioritized requests which are not in request queue.
+    prioritized_requests_.pop_back();

[Pete Williamson, 2017/03/07 19:06:04]

Maybe instead of popping requests off the end of the array, we could just start
at the highest index and iterate down towards 0.

[romax, 2017/03/07 20:41:45]

Done.

+  }
+
+  if (!picked_request) {

[Pete Williamson, 2017/03/07 19:06:04]

add comment
// If no request was found from the priority list, then find the best request
according to current policies.

[romax, 2017/03/07 20:41:45]

Done.

+    for (unsigned i = 0; i < available_requests.size(); ++i) {
+      if (!RequestConditionsSatisfied(available_requests[i].get()))
+        continue;
+      if (IsNewRequestBetter(picked_request, available_requests[i].get(),
+                             comparator)) {
+        picked_request = available_requests[i].get();
+      }
+    }
+  }
 
   // If we have a best request to try next, get the request coodinator to
   // start it.  Otherwise return that we have no candidates.