[Offline Pages] Pick correct request when resuming.

components/offline_pages/core/background/pick_request_task.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/offline_pages/core/background/pick_request_task.h"
 
+#include <set>
+
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/time/time.h"
 #include "components/offline_pages/core/background/device_conditions.h"
 #include "components/offline_pages/core/background/offliner_policy.h"
 #include "components/offline_pages/core/background/offliner_policy_utils.h"
 #include "components/offline_pages/core/background/request_coordinator_event_logger.h"
 #include "components/offline_pages/core/background/request_notifier.h"
 #include "components/offline_pages/core/background/request_queue_store.h"
 #include "components/offline_pages/core/background/save_page_request.h"
@@ skipping 11 matching lines @@
 }  // namespace
 
 namespace offline_pages {
 
 PickRequestTask::PickRequestTask(RequestQueueStore* store,
                                  OfflinerPolicy* policy,
                                  RequestPickedCallback picked_callback,
                                  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::deque<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));
 }
 
 PickRequestTask::~PickRequestTask() {}
 
 void PickRequestTask::Run() {
   GetRequests();
 }
 
@@ skipping 23 matching lines @@
   if (policy_->RetryCountIsMoreImportantThanRecency())
     comparator = &PickRequestTask::RetryCountFirstCompareFunction;
   else
     comparator = &PickRequestTask::RecencyFirstCompareFunction;
 
   // TODO(petewil): Consider replacing this bool with a better named enum.
   bool non_user_requested_tasks_remaining = false;
   bool cleanup_needed = false;
 
   size_t available_request_count = 0;
+  std::set<int64_t> available_request_ids;

[fgorski, 2017/03/08 17:33:23]

Did you consider unordered_set?

[romax, 2017/03/08 21:43:58]

Done.
No i didn't...and seems like a hashtable is better.

 
-  // Iterate once through the requests, keeping track of best candidate.
+  // 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(),
                                                           policy_) !=
         OfflinerPolicyUtils::RequestExpirationStatus::VALID) {
       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;
 
     // If there are non-user-requested tasks remaining, we need to make sure
     // that they are scheduled after we run out of user requested tasks. Here we
     // detect if any exist. If we don't find any user-requested tasks, we will
     // inform the "not_picked_callback_" that it needs to schedule a task for
     // non-user-requested items, which have different network and power needs.
     if (!requests[i]->user_requested())
       non_user_requested_tasks_remaining = true;
     if (requests[i]->request_state() ==
         SavePageRequest::RequestState::AVAILABLE) {
       available_request_count++;

[fgorski, 2017/03/08 17:33:24]

since you have one more if after this, the request might not be available.

[romax, 2017/03/08 21:43:58]

i thought about the same thing but ended up keeping as it used to be. After
asking Pete I think this number is for UMA only and it's working as intended and
counting the requests which have state:AVAILABLE only.

     }
     if (!RequestConditionsSatisfied(requests[i].get()))
       continue;
-    if (IsNewRequestBetter(picked_request, requests[i].get(), comparator))
-      picked_request = requests[i].get();
+    available_request_ids.insert(requests[i]->request_id());

[fgorski, 2017/03/08 17:33:24]

would it make sense to put this inside of a check:
if (request available) {

}

[romax, 2017/03/08 21:43:58]

it makes sense as a break-proof thing but not needed IMO.

+  }
+  // Report the request queue counts.
+  request_count_callback_.Run(requests.size(), available_request_count);

[fgorski, 2017/03/08 17:33:24]

available_request_ids.size()

and remove available_requests_count

[romax, 2017/03/08 21:43:58]

See above comment.

+
+  // Search for and pick the prioritized request which is not disabled and in

[fgorski, 2017/03/08 17:33:24]

Update comment to reflect what we discussed.

[romax, 2017/03/08 21:43:58]

Done.

+  // the request queue. An assumption is that requests later in the vector have
+  // higher priority than earlier ones in the vector. Also remove the picked
+  // request from the prioritized list.
+  while (!picked_request && !prioritized_requests_.empty()) {
+    if (available_request_ids.find(prioritized_requests_.back()) !=

[fgorski, 2017/03/08 17:33:24]

if (available_requests_ids.count(prioritized_requests_.back()) > 0)

I think it is easier to read then

[romax, 2017/03/08 21:43:58]

Done.

+        available_request_ids.end()) {
+      for (unsigned i = 0; i < requests.size(); ++i) {

[fgorski, 2017/03/08 17:33:24]

please add a comment above why this is not an infinite loop:
// A request is present in requests for every ID in available_request_ids...

perhaps a NOTREACHED() after for loop would be helpful.

[romax, 2017/03/08 21:43:58]

Done.

+        if (requests[i]->request_id() == prioritized_requests_.back()) {
+          picked_request = requests[i].get();
+          break;
+        }
+      }
+    } else {
+      prioritized_requests_.pop_back();
+    }
   }
 
-  // Report the request queue counts.
-  request_count_callback_.Run(requests.size(), available_request_count);
+  // If no request was found from the priority list, find the best request
+  // according to current policies.
+  if (!picked_request) {
+    for (unsigned i = 0; i < requests.size(); ++i) {

[fgorski, 2017/03/08 17:33:24]

Perhaps an auto iter?

[romax, 2017/03/08 21:43:58]

Done.

+      if ((available_request_ids.find(requests[i]->request_id()) !=

[fgorski, 2017/03/08 17:33:24]

same count() comment.

[romax, 2017/03/08 21:43:58]

Done.

+           available_request_ids.end()) &&
+          (IsNewRequestBetter(picked_request, requests[i].get(), comparator))) {
+        picked_request = 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.
   if (picked_request != nullptr) {
     picked_callback_.Run(*picked_request, cleanup_needed);
   } else {
     not_picked_callback_.Run(non_user_requested_tasks_remaining,
                              cleanup_needed);
   }
 
@@ skipping 113 matching lines @@
   int result = signum(difference.InMilliseconds());
 
   // Flip the direction of comparison if policy prefers fewer retries.
   if (policy_->ShouldPreferEarlierRequests())
     result *= -1;
 
   return result;
 }
 
 }  // namespace offline_pages