[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..307b5517d3858877e0b9526df6b2e5c4d38b558c 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::list<int64_t>& priortized_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),
+      priortized_requests_(priortized_requests),
       weak_ptr_factory_(this) {
   device_conditions_.reset(new DeviceConditions(device_conditions));
 }
@@ -83,10 +85,26 @@ void PickRequestTask::Choose(
   // TODO(petewil): Consider replacing this bool with a better named enum.
   bool non_user_requested_tasks_remaining = false;
   bool cleanup_needed = false;
+  bool is_priortized_request_picked = false;

[Pete Williamson, 2017/03/06 18:30:57]

nit: was_prioritized_request_picked might be better

[romax, 2017/03/07 18:40:44]

Done.

 
   size_t available_request_count = 0;
 
-  // Iterate once through the requests, keeping track of best candidate.
+  // Search for the priortized requests before iterating all the requests.

[Pete Williamson, 2017/03/06 18:30:57]

There is an implicit assumption here that we should make explicit with a
comment.  Requests earlier in the list have higher priority than requests older
in the list.

[romax, 2017/03/07 18:40:44]

Done.

+  while (!picked_request && !priortized_requests_.empty()) {
+    int64_t id = priortized_requests_.front();
+    for (unsigned i = 0; i < requests.size(); ++i) {
+      if (requests[i]->request_id() == id) {
+        picked_request = requests[i].get();
+        is_priortized_request_picked = true;
+        break;
+      }
+    }
+    // Remove the priortized requests which are not in request queue.
+    priortized_requests_.pop_front();
+  }
+
+  // Iterate once through the requests, keeping track of best candidate if no
+  // priortized request has been chosen.

[fgorski, 2017/03/06 18:12:45]

if statement should probably happen outside of the for look to not make a check
multiple times. Unless there is a chance to get a better fit of course.

[Pete Williamson, 2017/03/06 18:30:57]

Agreed, if a prioritized request is found, we should probably not run this loop
at all.  

Today the loop does two things, it finds a better candidate if one exists, and
detects if there are non-user-requested tasks remaining.  We are going to be
removing the checks for non-user-requested tasks.  If we were keeping them, we
could pull them into a separate loop, instead, we could just ignore them, and
leave them in the loop for now until we properly remove the feature.

[romax, 2017/03/07 18:40:44]

Refactored a little bit. Not sure if it's necessary to have a list of filtered
requests, but otherwise I think there will be some duplicate code for checking
disabled etc. Please share your input :)

   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(),
@@ -96,7 +114,7 @@ void PickRequestTask::Choose(
       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())

[fgorski, 2017/03/06 18:12:45]

Is there a chance that prioritized request is on the disabled list?

[romax, 2017/03/07 18:40:44]

yeah there might be. added a check

       continue;
@@ -112,10 +130,13 @@ 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();
+
+    if (!is_priortized_request_picked) {
+      if (!RequestConditionsSatisfied(requests[i].get()))
+        continue;
+      if (IsNewRequestBetter(picked_request, requests[i].get(), comparator))
+        picked_request = requests[i].get();
+    }
   }
 
   // Report the request queue counts.