More detailed implementation of the RequestPicker

BUG=610521

Review-Url: https://codereview.chromium.org/2113383002
Cr-Commit-Position: refs/heads/master@{#407281}

chrome/browser/android/offline_pages/offline_page_bridge.cc

@@ -41,6 +41,7 @@ namespace android {
 namespace {
 
 const char kOfflinePageBridgeKey[] = "offline-page-bridge";
+const bool kUserRequested = true;
 
 void ToJavaOfflinePageList(JNIEnv* env,
                            jobject j_result_obj,
@@ -371,7 +372,7 @@ void OfflinePageBridge::SavePageLater(
           GetForBrowserContext(browser_context_);
 
   coordinator->SavePageLater(
-      GURL(ConvertJavaStringToUTF8(env, j_url)), client_id);
+      GURL(ConvertJavaStringToUTF8(env, j_url)), client_id, kUserRequested);
 }
 
 void OfflinePageBridge::DeletePages(

chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc

@@ -26,6 +26,7 @@ const int64_t kRequestId = 7;
 const GURL kHttpUrl("http://tunafish.com");
 const GURL kFileUrl("file://sailfish.png");
 const ClientId kClientId("AsyncLoading", "88");
+const bool kUserRequested = true;
 
 // Mock Loader for testing the Offliner calls.
 class MockPrerenderingLoader : public PrerenderingLoader {
@@ -199,14 +200,16 @@ void PrerenderingOfflinerTest::OnCompletion(const SavePageRequest& request,
 
 TEST_F(PrerenderingOfflinerTest, LoadAndSaveBadUrl) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kFileUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kFileUrl, kClientId, creation_time, kUserRequested);
   EXPECT_FALSE(offliner()->LoadAndSave(request, callback()));
   EXPECT_TRUE(loader()->IsIdle());
 }
 
 TEST_F(PrerenderingOfflinerTest, LoadAndSavePrerenderingDisabled) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kHttpUrl, kClientId, creation_time, kUserRequested);
   loader()->DisablePrerendering();
   EXPECT_FALSE(offliner()->LoadAndSave(request, callback()));
   EXPECT_TRUE(loader()->IsIdle());
@@ -214,7 +217,8 @@ TEST_F(PrerenderingOfflinerTest, LoadAndSavePrerenderingDisabled) {
 
 TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadStartedButFails) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kHttpUrl, kClientId, creation_time, kUserRequested);
   EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
   EXPECT_FALSE(loader()->IsIdle());
   EXPECT_EQ(Offliner::RequestStatus::UNKNOWN, request_status());
@@ -229,7 +233,8 @@ TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadStartedButFails) {
 
 TEST_F(PrerenderingOfflinerTest, CancelWhenLoading) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kHttpUrl, kClientId, creation_time, kUserRequested);
   EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
   EXPECT_FALSE(loader()->IsIdle());
 
@@ -239,7 +244,8 @@ TEST_F(PrerenderingOfflinerTest, CancelWhenLoading) {
 
 TEST_F(PrerenderingOfflinerTest, CancelWhenLoaded) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kHttpUrl, kClientId, creation_time, kUserRequested);
   EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
   EXPECT_FALSE(loader()->IsIdle());
   EXPECT_EQ(Offliner::RequestStatus::UNKNOWN, request_status());
@@ -267,7 +273,8 @@ TEST_F(PrerenderingOfflinerTest, CancelWhenLoaded) {
 
 TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadedButSaveFails) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kHttpUrl, kClientId, creation_time, kUserRequested);
   EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
   EXPECT_FALSE(loader()->IsIdle());
   EXPECT_EQ(Offliner::RequestStatus::UNKNOWN, request_status());
@@ -288,7 +295,8 @@ TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadedButSaveFails) {
 
 TEST_F(PrerenderingOfflinerTest, LoadAndSaveSuccessful) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kHttpUrl, kClientId, creation_time, kUserRequested);
   EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
   EXPECT_FALSE(loader()->IsIdle());
   EXPECT_EQ(Offliner::RequestStatus::UNKNOWN, request_status());
@@ -309,7 +317,8 @@ TEST_F(PrerenderingOfflinerTest, LoadAndSaveSuccessful) {
 
 TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadedButThenCanceledFromLoader) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kHttpUrl, kClientId, creation_time, kUserRequested);
   EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
   EXPECT_FALSE(loader()->IsIdle());
   EXPECT_EQ(Offliner::RequestStatus::UNKNOWN, request_status());
@@ -333,7 +342,8 @@ TEST_F(PrerenderingOfflinerTest, ForegroundTransitionCancelsOnLowEndDevice) {
   offliner()->SetLowEndDeviceForTesting(true);
 
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kHttpUrl, kClientId, creation_time, kUserRequested);
   EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
   EXPECT_FALSE(loader()->IsIdle());
 
@@ -349,7 +359,8 @@ TEST_F(PrerenderingOfflinerTest, ForegroundTransitionIgnoredOnHighEndDevice) {
   offliner()->SetLowEndDeviceForTesting(false);
 
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kHttpUrl, kClientId, creation_time, kUserRequested);
   EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
   EXPECT_FALSE(loader()->IsIdle());
 

components/offline_pages/background/device_conditions.h

@@ -20,6 +20,10 @@ class DeviceConditions {
         battery_percentage_(battery_percentage),
         net_connection_type_(net_connection_type) {}
 
+  DeviceConditions()
+      : power_connected_(true), battery_percentage_(75),
+        net_connection_type_(net::NetworkChangeNotifier::CONNECTION_WIFI) {}
+
   // Returns whether power is connected.
   bool IsPowerConnected() const { return power_connected_; }
 
@@ -35,6 +39,8 @@ class DeviceConditions {
   const bool power_connected_;
   const int battery_percentage_;
   const net::NetworkChangeNotifier::ConnectionType net_connection_type_;
+
+  // NOTE: We intentionally allow the default copy constructor and assignment.
 };
 
 }  // namespace offline_pages

components/offline_pages/background/offliner_policy.h

@@ -5,6 +5,13 @@
 #ifndef COMPONENTS_OFFLINE_PAGES_BACKGROUND_OFFLINER_POLICY_H_
 #define COMPONENTS_OFFLINE_PAGES_BACKGROUND_OFFLINER_POLICY_H_
 
+namespace {
+const int kMaxRetries = 2;
+const int kBackgroundTimeBudgetSeconds = 170;
+const int kSinglePageTimeBudgetSeconds = 120;
+const int kMinimumBatteryPercentageForNonUserRequestOfflining = 50;
+}  // namespace
+
 namespace offline_pages {
 
 // Policy for the Background Offlining system.  Some policy will belong to the
@@ -13,8 +20,40 @@ class OfflinerPolicy {
  public:
   OfflinerPolicy(){};
 
-  // TODO(petewil): Implement and add a .cc file.
-  // Eventually this should get data from a finch experiment.
+  // TODO(petewil): Numbers here are chosen arbitrarily, do the proper studies
+  // to get good policy numbers.
+
+  // TODO(petewil): Eventually this should get data from a finch experiment.
+
+  // Returns true if we should prefer retrying lesser tried requests.
+  bool ShouldPreferUntriedRequests() { return false; }
+
+  // Returns true if we should prefer older requests of equal number of tries.
+  bool ShouldPreferEarlierRequests() { return true; }
+
+  // Returns true if retry count is considered more important than recency in
+  // picking which request to try next.
+  bool RetryCountIsMoreImportantThanRecency() { return true; }
+
+  // The max number of times we will retry a request.
+  int GetMaxRetries() { return kMaxRetries; }
+
+  // How many seconds to keep trying new pages for, before we give up,  and
+  // return to the scheduler.
+  int GetBackgroundProcessingTimeBudgetSeconds() {
+    return kBackgroundTimeBudgetSeconds;
+  }
+
+  // How long do we allow a page to load before giving up on it
+  int GetSinglePageTimeBudgetInSeconds() {
+    return kSinglePageTimeBudgetSeconds;
+  }
+
+  // How much battery must we have before fetching a page not explicitly
+  // requested by the user?
+  int GetMinimumBatteryPercentageForNonUserRequestOfflining() {
+    return kMinimumBatteryPercentageForNonUserRequestOfflining;
+  }
 };
 }
 

components/offline_pages/background/request_coordinator.cc

@@ -60,22 +60,23 @@ RequestCoordinator::RequestCoordinator(std::unique_ptr<OfflinerPolicy> policy,
       last_offlining_status_(Offliner::RequestStatus::UNKNOWN),
       weak_ptr_factory_(this) {
   DCHECK(policy_ != nullptr);
-  picker_.reset(new RequestPicker(queue_.get()));
+  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
 }
 
 RequestCoordinator::~RequestCoordinator() {}
 
 bool RequestCoordinator::SavePageLater(
-    const GURL& url, const ClientId& client_id) {
+    const GURL& url, const ClientId& client_id, bool was_user_requested) {
   DVLOG(2) << "URL is " << url << " " << __FUNCTION__;
 
-  // TODO(petewil): We need a robust scheme for allocating new IDs.
+  // TODO(petewil): We need a robust scheme for allocating new IDs.  We may need
+  // GUIDS for the downloads home design.
   static int64_t id = 0;
 
   // Build a SavePageRequest.
   // TODO(petewil): Use something like base::Clock to help in testing.
   offline_pages::SavePageRequest request(
-      id++, url, client_id, base::Time::Now());
+      id++, url, client_id, base::Time::Now(), was_user_requested);
 
   // Put the request on the request queue.
   queue_->AddRequest(request,
@@ -116,6 +117,7 @@ void RequestCoordinator::StopProcessing() {
 bool RequestCoordinator::StartProcessing(
     const DeviceConditions& device_conditions,
     const base::Callback<void(bool)>& callback) {
+  current_conditions_.reset(new DeviceConditions(device_conditions));
   if (is_busy_) return false;
 
   is_canceled_ = false;
@@ -135,7 +137,8 @@ void RequestCoordinator::TryNextRequest() {
       base::Bind(&RequestCoordinator::RequestPicked,
                  weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&RequestCoordinator::RequestQueueEmpty,
-                 weak_ptr_factory_.GetWeakPtr()));
+                 weak_ptr_factory_.GetWeakPtr()),
+      current_conditions_.get());
 }
 
 // Called by the request picker when a request has been picked.

components/offline_pages/background/request_coordinator.h

@@ -47,7 +47,8 @@ class RequestCoordinator : public KeyedService {
 
   // Queues |request| to later load and save when system conditions allow.
   // Returns true if the page could be queued successfully.
-  bool SavePageLater(const GURL& url, const ClientId& client_id);
+  bool SavePageLater(
+      const GURL& url, const ClientId& client_id, bool user_reqeusted);
 
   // Starts processing of one or more queued save page later requests.
   // Returns whether processing was started and that caller should expect
@@ -123,6 +124,10 @@ class RequestCoordinator : public KeyedService {
     offliner_timeout_ = timeout;
   }
 
+  void SetDeviceConditionsForTest(DeviceConditions& current_conditions) {
+    current_conditions_.reset(new DeviceConditions(current_conditions));
+  }
+
   friend class RequestCoordinatorTest;
 
   // The offliner can only handle one request at a time - if the offliner is
@@ -135,6 +140,8 @@ class RequestCoordinator : public KeyedService {
   base::TimeDelta offliner_timeout_;
   // Unowned pointer to the current offliner, if any.
   Offliner* offliner_;
+  // Last known conditions for network, battery
+  std::unique_ptr<DeviceConditions> current_conditions_;
   // RequestCoordinator takes over ownership of the policy
   std::unique_ptr<OfflinerPolicy> policy_;
   // OfflinerFactory.  Used to create offline pages. Owned.

components/offline_pages/background/request_coordinator_unittest.cc

@@ -35,6 +35,7 @@ const int kRequestId(1);
 const long kTestTimeoutSeconds = 1;
 const int kBatteryPercentageHigh = 75;
 const bool kPowerRequired = true;
+const bool kUserRequested = true;
 }  // namespace
 
 class SchedulerStub : public Scheduler {
@@ -69,7 +70,8 @@ class SchedulerStub : public Scheduler {
 class OfflinerStub : public Offliner {
  public:
   OfflinerStub()
-      : request_(kRequestId, kUrl, kClientId, base::Time::Now()),
+      : request_(kRequestId, kUrl, kClientId, base::Time::Now(),
+                 kUserRequested),
         enable_callback_(false),
         cancel_called_(false) {}
 
@@ -170,6 +172,10 @@ class RequestCoordinatorTest
     coordinator_->SetOfflinerTimeoutForTest(timeout);
   }
 
+  void SetDeviceConditionsForTest(DeviceConditions device_conditions) {
+    coordinator_->SetDeviceConditionsForTest(device_conditions);
+  }
+
   void WaitForCallback() {
     waiter_.Wait();
   }
@@ -252,7 +258,7 @@ TEST_F(RequestCoordinatorTest, StartProcessingWithNoRequests) {
 
 TEST_F(RequestCoordinatorTest, StartProcessingWithRequestInProgress) {
   // Put the request on the queue.
-  EXPECT_TRUE(coordinator()->SavePageLater(kUrl, kClientId));
+  EXPECT_TRUE(coordinator()->SavePageLater(kUrl, kClientId, kUserRequested));
 
   // Set up for the call to StartProcessing by building arguments.
   DeviceConditions device_conditions(false, 75,
@@ -275,7 +281,7 @@ TEST_F(RequestCoordinatorTest, StartProcessingWithRequestInProgress) {
 }
 
 TEST_F(RequestCoordinatorTest, SavePageLater) {
-  EXPECT_TRUE(coordinator()->SavePageLater(kUrl, kClientId));
+  EXPECT_TRUE(coordinator()->SavePageLater(kUrl, kClientId, kUserRequested));
 
   // Expect that a request got placed on the queue.
   coordinator()->queue()->GetRequests(
@@ -303,7 +309,7 @@ TEST_F(RequestCoordinatorTest, SavePageLater) {
 TEST_F(RequestCoordinatorTest, OfflinerDoneRequestSucceeded) {
   // Add a request to the queue, wait for callbacks to finish.
   offline_pages::SavePageRequest request(
-      kRequestId, kUrl, kClientId, base::Time::Now());
+      kRequestId, kUrl, kClientId, base::Time::Now(), kUserRequested);
   coordinator()->queue()->AddRequest(
       request,
       base::Bind(&RequestCoordinatorTest::AddRequestDone,
@@ -317,6 +323,11 @@ TEST_F(RequestCoordinatorTest, OfflinerDoneRequestSucceeded) {
           base::Unretained(this));
   coordinator()->SetProcessingCallbackForTest(callback);
 
+  // Set up device conditions for the test.
+  DeviceConditions device_conditions(
+      false, 75, net::NetworkChangeNotifier::CONNECTION_3G);
+  SetDeviceConditionsForTest(device_conditions);
+
   // Call the OfflinerDoneCallback to simulate the page being completed, wait
   // for callbacks.
   EnableOfflinerCallback(true);
@@ -338,7 +349,7 @@ TEST_F(RequestCoordinatorTest, OfflinerDoneRequestSucceeded) {
 TEST_F(RequestCoordinatorTest, OfflinerDoneRequestFailed) {
   // Add a request to the queue, wait for callbacks to finish.
   offline_pages::SavePageRequest request(
-      kRequestId, kUrl, kClientId, base::Time::Now());
+      kRequestId, kUrl, kClientId, base::Time::Now(), kUserRequested);
   coordinator()->queue()->AddRequest(
       request,
       base::Bind(&RequestCoordinatorTest::AddRequestDone,
@@ -375,7 +386,7 @@ TEST_F(RequestCoordinatorTest, OfflinerDoneRequestFailed) {
 TEST_F(RequestCoordinatorTest, StartProcessingThenStopProcessingImmediately) {
   // Add a request to the queue, wait for callbacks to finish.
   offline_pages::SavePageRequest request(
-      kRequestId, kUrl, kClientId, base::Time::Now());
+      kRequestId, kUrl, kClientId, base::Time::Now(), kUserRequested);
   coordinator()->queue()->AddRequest(
       request,
       base::Bind(&RequestCoordinatorTest::AddRequestDone,
@@ -409,7 +420,7 @@ TEST_F(RequestCoordinatorTest, StartProcessingThenStopProcessingImmediately) {
 TEST_F(RequestCoordinatorTest, StartProcessingThenStopProcessingLater) {
   // Add a request to the queue, wait for callbacks to finish.
   offline_pages::SavePageRequest request(
-      kRequestId, kUrl, kClientId, base::Time::Now());
+      kRequestId, kUrl, kClientId, base::Time::Now(), kUserRequested);
   coordinator()->queue()->AddRequest(
       request,
       base::Bind(&RequestCoordinatorTest::AddRequestDone,
@@ -448,7 +459,7 @@ TEST_F(RequestCoordinatorTest, StartProcessingThenStopProcessingLater) {
 TEST_F(RequestCoordinatorTest, PrerendererTimeout) {
   // Build a request to use with the pre-renderer, and put it on the queue.
   offline_pages::SavePageRequest request(
-      kRequestId, kUrl, kClientId, base::Time::Now());
+      kRequestId, kUrl, kClientId, base::Time::Now(), kUserRequested);
   coordinator()->queue()->AddRequest(
       request,
       base::Bind(&RequestCoordinatorTest::AddRequestDone,

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,151 @@ 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;
+  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 the older request is better.
+  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.
+  int result = CompareRetryCount(left, right);
+
+  if (result != 0)
+    return (result > 0);
+
+  // If we get here, the attempt counts were the same, so check recency.
+  result = CompareCreationTime(left, right);
+
+  return (result > 0);
+}
+
+// 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.
+  int result = CompareCreationTime(left, right);
+
+  if (result != 0)
+    return (result > 0);
+
+  // If we get here, the recency was the same, so check the attempt count.
+  result = CompareRetryCount(left, right);
+
+  return (result > 0);
+}
+
+// Compare left and right side, returning 1 if the left side is better
+// (preferred by policy), 0 if the same, and -1 if the right side is better.
+int RequestPicker::CompareRetryCount(
+    const SavePageRequest* left, const SavePageRequest* right) {
+  // Check the attempt count.
+  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;
+
+  return result;
+}
+
+// Compare left and right side, returning 1 if the left side is better
+// (preferred by policy), 0 if the same, and -1 if the right side is better.
+int RequestPicker::CompareCreationTime(
+    const SavePageRequest* left, const SavePageRequest* right) {
+  // Check the recency.
+  base::TimeDelta difference = left->creation_time() - right->creation_time();
+  int result = signum(difference.InMilliseconds());
+
+  // Flip the direction of comparison if policy prefers fewer retries.
+  if (earlier_requests_better_)
+    result *= -1;
 
-  // When we have a best request to try next, get the request coodinator to
-  // start it.
-  picked_callback_.Run(picked_request);
+  return result;
 }
 
 }  // namespace offline_pages

components/offline_pages/background/request_picker.h

@@ -5,14 +5,22 @@
 #ifndef COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_PICKER_H_
 #define COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_PICKER_H_
 
+#include <memory>
+
 #include "base/memory/weak_ptr.h"
+#include "components/offline_pages/background/device_conditions.h"
+#include "components/offline_pages/background/offliner_policy.h"
 #include "components/offline_pages/background/request_coordinator.h"
 #include "components/offline_pages/background/request_queue.h"
 
 namespace offline_pages {
+
+typedef bool (RequestPicker::*RequestCompareFunction)(
+    const SavePageRequest* left, const SavePageRequest* right);
+
 class RequestPicker {
  public:
-  RequestPicker(RequestQueue* requestQueue);
+  RequestPicker(RequestQueue* requestQueue, OfflinerPolicy* policy);
 
   ~RequestPicker();
 
@@ -20,15 +28,51 @@ class RequestPicker {
   // conditions, and call back to the RequestCoordinator when we have one.
   void ChooseNextRequest(
       RequestCoordinator::RequestPickedCallback picked_callback,
-      RequestCoordinator::RequestQueueEmptyCallback empty_callback);
+      RequestCoordinator::RequestQueueEmptyCallback empty_callback,
+      DeviceConditions* device_conditions);
 
  private:
   // Callback for the GetRequest results to be delivered.
   void GetRequestResultCallback(RequestQueue::GetRequestsResult result,
                                 const std::vector<SavePageRequest>& results);
 
+  // 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 RequestConditionsSatisfied(const SavePageRequest& request);
+
+  // Using policies, decide if the new request is preferable to the best we have
+  // so far.
+  bool IsNewRequestBetter(const SavePageRequest* oldRequest,
+                          const SavePageRequest* newRequest,
+                          RequestCompareFunction comparator);
+
+  // Is the new request preferable from the retry count first standpoint?
+  bool RetryCountFirstCompareFunction(const SavePageRequest* left,
+                                      const SavePageRequest* right);
+
+  // Is the new request better from the recency first standpoint?
+  bool RecencyFirstCompareFunction(const SavePageRequest* left,
+                                   const SavePageRequest* right);
+
+  // Does the new request have better retry count?
+  int CompareRetryCount(const SavePageRequest* left,
+                        const SavePageRequest* right);
+
+  // Does the new request have better creation time?
+  int CompareCreationTime(const SavePageRequest* left,
+                          const SavePageRequest* right);
+
   // unowned pointer to the request queue.
   RequestQueue* queue_;
+  // unowned pointer to the policy object.
+  OfflinerPolicy* policy_;
+  // Current conditions on the device
+  std::unique_ptr<DeviceConditions> current_conditions_;
+  // True if we prefer less-tried requests
+  bool fewer_retries_better_;
+  // True if we prefer requests submitted more recently
+  bool earlier_requests_better_;
   // Callback for when we are done picking a request to do next.
   RequestCoordinator::RequestPickedCallback picked_callback_;
   // Callback for when there are no more reqeusts to pick.

components/offline_pages/background/request_picker_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
+#include "components/offline_pages/background/device_conditions.h"
 #include "components/offline_pages/background/request_queue.h"
 #include "components/offline_pages/background/request_queue_in_memory_store.h"
 #include "components/offline_pages/background/save_page_request.h"
@@ -25,6 +26,7 @@ const ClientId kClientId1("bookmark", "1234");
 const int64_t kRequestId2 = 42;
 const GURL kUrl2("http://nytimes.com");
 const ClientId kClientId2("bookmark", "5678");
+const bool kUserRequested = true;
 }  // namespace
 
 class RequestPickerTest : public testing::Test {
@@ -50,6 +52,7 @@ class RequestPickerTest : public testing::Test {
   std::unique_ptr<RequestQueue> queue_;
   std::unique_ptr<RequestPicker> picker_;
   std::unique_ptr<SavePageRequest> last_picked_;
+  std::unique_ptr<OfflinerPolicy> policy_;
   bool request_queue_empty_called_;
 
  private:
@@ -67,7 +70,8 @@ void RequestPickerTest::SetUp() {
   std::unique_ptr<RequestQueueInMemoryStore> store(
       new RequestQueueInMemoryStore());
   queue_.reset(new RequestQueue(std::move(store)));
-  picker_.reset(new RequestPicker(queue_.get()));
+  policy_.reset(new OfflinerPolicy());
+  picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
   request_queue_empty_called_ = false;
 }
 
@@ -88,8 +92,11 @@ void RequestPickerTest::RequestQueueEmpty() {
 
 TEST_F(RequestPickerTest, ChooseNextRequest) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request1(kRequestId1, kUrl1, kClientId1, creation_time);
-  SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time);
+  DeviceConditions conditions;
+  SavePageRequest request1(
+      kRequestId1, kUrl1, kClientId1, creation_time, kUserRequested);
+  SavePageRequest request2(
+      kRequestId2, kUrl2, kClientId2, creation_time, kUserRequested);
   // Put some test requests on the Queue.
   queue_->AddRequest(request1, base::Bind(&RequestPickerTest::AddRequestDone,
                                           base::Unretained(this)));
@@ -101,23 +108,24 @@ TEST_F(RequestPickerTest, ChooseNextRequest) {
 
   picker_->ChooseNextRequest(
       base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
-      base::Bind(&RequestPickerTest::RequestQueueEmpty,
-                 base::Unretained(this)));
+      base::Bind(&RequestPickerTest::RequestQueueEmpty, base::Unretained(this)),
+      &conditions);
 
   // Pump the loop again to give the async queue the opportunity to return
   // results from the Get operation, and for the picker to call the "picked"
   // callback.
   PumpLoop();
 
-  EXPECT_EQ(kRequestId1, last_picked_->request_id());
+  EXPECT_EQ(kRequestId2, last_picked_->request_id());
   EXPECT_FALSE(request_queue_empty_called_);
 }
 
 TEST_F(RequestPickerTest, PickFromEmptyQueue) {
+  DeviceConditions conditions;
   picker_->ChooseNextRequest(
       base::Bind(&RequestPickerTest::RequestPicked, base::Unretained(this)),
-      base::Bind(&RequestPickerTest::RequestQueueEmpty,
-                 base::Unretained(this)));
+      base::Bind(&RequestPickerTest::RequestQueueEmpty, base::Unretained(this)),
+      &conditions);
 
   // Pump the loop again to give the async queue the opportunity to return
   // results from the Get operation, and for the picker to call the "QueueEmpty"

components/offline_pages/background/request_queue_store_sql.cc

@@ -22,6 +22,7 @@ namespace {
 // This is a macro instead of a const so that
 // it can be used inline in other SQL statements below.
 #define REQUEST_QUEUE_TABLE_NAME "request_queue_v1"
+const bool kUserRequested = true;
 
 bool CreateRequestQueueTable(sql::Connection* db) {
   const char kSql[] = "CREATE TABLE IF NOT EXISTS " REQUEST_QUEUE_TABLE_NAME
@@ -93,7 +94,8 @@ SavePageRequest MakeSavePageRequest(const sql::Statement& statement) {
   const ClientId client_id(statement.ColumnString(6),
                            statement.ColumnString(7));
 
-  SavePageRequest request(id, url, client_id, creation_time, activation_time);
+  SavePageRequest request(
+      id, url, client_id, creation_time, activation_time, kUserRequested);
   request.set_last_attempt_time(last_attempt_time);
   request.set_attempt_count(last_attempt_count);
   return request;

components/offline_pages/background/request_queue_store_unittest.cc

@@ -24,6 +24,7 @@ namespace {
 const int64_t kRequestId = 42;
 const GURL kUrl("http://example.com");
 const ClientId kClientId("bookmark", "1234");
+const bool kUserRequested = true;
 
 enum class LastResult {
   kNone,
@@ -195,7 +196,8 @@ TYPED_TEST(RequestQueueStoreTest, GetRequestsEmpty) {
 TYPED_TEST(RequestQueueStoreTest, AddRequest) {
   std::unique_ptr<RequestQueueStore> store(this->BuildStore());
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kUrl, kClientId, creation_time, kUserRequested);
 
   store->AddOrUpdateRequest(
       request, base::Bind(&RequestQueueStoreTestBase::AddOrUpdateDone,
@@ -218,7 +220,8 @@ TYPED_TEST(RequestQueueStoreTest, AddRequest) {
 TYPED_TEST(RequestQueueStoreTest, UpdateRequest) {
   std::unique_ptr<RequestQueueStore> store(this->BuildStore());
   base::Time creation_time = base::Time::Now();
-  SavePageRequest original_request(kRequestId, kUrl, kClientId, creation_time);
+  SavePageRequest original_request(
+      kRequestId, kUrl, kClientId, creation_time, kUserRequested);
   store->AddOrUpdateRequest(
       original_request, base::Bind(&RequestQueueStoreTestBase::AddOrUpdateDone,
                                    base::Unretained(this)));
@@ -229,7 +232,8 @@ TYPED_TEST(RequestQueueStoreTest, UpdateRequest) {
       creation_time + base::TimeDelta::FromMinutes(1);
   base::Time activation_time = creation_time + base::TimeDelta::FromHours(6);
   SavePageRequest updated_request(kRequestId, kUrl, kClientId,
-                                  new_creation_time, activation_time);
+                                  new_creation_time, activation_time,
+                                  kUserRequested);
   store->AddOrUpdateRequest(
       updated_request, base::Bind(&RequestQueueStoreTestBase::AddOrUpdateDone,
                                   base::Unretained(this)));
@@ -251,7 +255,8 @@ TYPED_TEST(RequestQueueStoreTest, UpdateRequest) {
 TYPED_TEST(RequestQueueStoreTest, RemoveRequest) {
   std::unique_ptr<RequestQueueStore> store(this->BuildStore());
   base::Time creation_time = base::Time::Now();
-  SavePageRequest original_request(kRequestId, kUrl, kClientId, creation_time);
+  SavePageRequest original_request(
+      kRequestId, kUrl, kClientId, creation_time, kUserRequested);
   store->AddOrUpdateRequest(
       original_request, base::Bind(&RequestQueueStoreTestBase::AddOrUpdateDone,
                                    base::Unretained(this)));
@@ -290,7 +295,8 @@ TYPED_TEST(RequestQueueStoreTest, RemoveRequest) {
 TYPED_TEST(RequestQueueStoreTest, ResetStore) {
   std::unique_ptr<RequestQueueStore> store(this->BuildStore());
   base::Time creation_time = base::Time::Now();
-  SavePageRequest original_request(kRequestId, kUrl, kClientId, creation_time);
+  SavePageRequest original_request(
+      kRequestId, kUrl, kClientId, creation_time, kUserRequested);
   store->AddOrUpdateRequest(
       original_request, base::Bind(&RequestQueueStoreTestBase::AddOrUpdateDone,
                                    base::Unretained(this)));
@@ -319,7 +325,8 @@ class RequestQueueStoreSQLTest
 TEST_F(RequestQueueStoreSQLTest, SaveCloseReopenRead) {
   std::unique_ptr<RequestQueueStore> store(BuildStore());
   base::Time creation_time = base::Time::Now();
-  SavePageRequest original_request(kRequestId, kUrl, kClientId, creation_time);
+  SavePageRequest original_request(
+      kRequestId, kUrl, kClientId, creation_time, kUserRequested);
   store->AddOrUpdateRequest(
       original_request, base::Bind(&RequestQueueStoreTestBase::AddOrUpdateDone,
                                    base::Unretained(this)));

components/offline_pages/background/request_queue_unittest.cc

@@ -28,6 +28,7 @@ const ClientId kClientId("bookmark", "1234");
 const int64_t kRequestId2 = 77;
 const GURL kUrl2("http://test.com");
 const ClientId kClientId2("bookmark", "567");
+const bool kUserRequested = true;
 }  // namespace
 
 // TODO(fgorski): Add tests for store failures in add/remove/get.
@@ -125,7 +126,8 @@ TEST_F(RequestQueueTest, GetRequestsEmpty) {
 
 TEST_F(RequestQueueTest, AddRequest) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kUrl, kClientId, creation_time, kUserRequested);
   queue()->AddRequest(request, base::Bind(&RequestQueueTest::AddRequestDone,
                                           base::Unretained(this)));
   PumpLoop();
@@ -142,7 +144,8 @@ TEST_F(RequestQueueTest, AddRequest) {
 
 TEST_F(RequestQueueTest, RemoveRequest) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kUrl, kClientId, creation_time, kUserRequested);
   queue()->AddRequest(request, base::Bind(&RequestQueueTest::AddRequestDone,
                                           base::Unretained(this)));
   PumpLoop();
@@ -165,12 +168,14 @@ TEST_F(RequestQueueTest, RemoveRequest) {
 // listing multiple items and removing the right item.
 TEST_F(RequestQueueTest, MultipleRequestsAddGetRemove) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request1(kRequestId, kUrl, kClientId, creation_time);
+  SavePageRequest request1(
+      kRequestId, kUrl, kClientId, creation_time, kUserRequested);
   queue()->AddRequest(request1, base::Bind(&RequestQueueTest::AddRequestDone,
                                            base::Unretained(this)));
   PumpLoop();
   ASSERT_EQ(request1.request_id(), last_added_request()->request_id());
-  SavePageRequest request2(kRequestId2, kUrl2, kClientId2, creation_time);
+  SavePageRequest request2(
+      kRequestId2, kUrl2, kClientId2, creation_time, kUserRequested);
   queue()->AddRequest(request2, base::Bind(&RequestQueueTest::AddRequestDone,
                                            base::Unretained(this)));
   PumpLoop();

components/offline_pages/background/save_page_request.cc

@@ -9,25 +9,29 @@ namespace offline_pages {
 SavePageRequest::SavePageRequest(int64_t request_id,
                                  const GURL& url,
                                  const ClientId& client_id,
-                                 const base::Time& creation_time)
+                                 const base::Time& creation_time,
+                                 const bool was_user_requested)
     : request_id_(request_id),
       url_(url),
       client_id_(client_id),
       creation_time_(creation_time),
       activation_time_(creation_time),
-      attempt_count_(0) {}
+      attempt_count_(0),
+      user_requested_(was_user_requested) {}
 
 SavePageRequest::SavePageRequest(int64_t request_id,
                                  const GURL& url,
                                  const ClientId& client_id,
                                  const base::Time& creation_time,
-                                 const base::Time& activation_time)
+                                 const base::Time& activation_time,
+                                 const bool user_requested)
     : request_id_(request_id),
       url_(url),
       client_id_(client_id),
       creation_time_(creation_time),
       activation_time_(activation_time),
-      attempt_count_(0) {}
+      attempt_count_(0),
+      user_requested_(user_requested) {}
 
 SavePageRequest::SavePageRequest(const SavePageRequest& other)
     : request_id_(other.request_id_),
@@ -36,7 +40,8 @@ SavePageRequest::SavePageRequest(const SavePageRequest& other)
       creation_time_(other.creation_time_),
       activation_time_(other.activation_time_),
       attempt_count_(other.attempt_count_),
-      last_attempt_time_(other.last_attempt_time_) {}
+      last_attempt_time_(other.last_attempt_time_),
+      user_requested_(other.user_requested_) {}
 
 SavePageRequest::~SavePageRequest() {}
 

components/offline_pages/background/save_page_request.h

@@ -30,12 +30,14 @@ class SavePageRequest {
   SavePageRequest(int64_t request_id,
                   const GURL& url,
                   const ClientId& client_id,
-                  const base::Time& creation_time);
+                  const base::Time& creation_time,
+                  const bool user_requested);
   SavePageRequest(int64_t request_id,
                   const GURL& url,
                   const ClientId& client_id,
                   const base::Time& creation_time,
-                  const base::Time& activation_time);
+                  const base::Time& activation_time,
+                  const bool user_requested);
   SavePageRequest(const SavePageRequest& other);
   ~SavePageRequest();
 
@@ -68,6 +70,12 @@ class SavePageRequest {
     last_attempt_time_ = last_attempt_time;
   }
 
+  bool user_requested() const { return user_requested_; }
+
+  void set_user_requested(bool user_requested) {
+    user_requested_ = user_requested;
+  }
+
  private:
   // ID of this request.
   int64_t request_id_;
@@ -90,6 +98,10 @@ class SavePageRequest {
 
   // Timestamp of the last request starting.
   base::Time last_attempt_time_;
+
+  // Whether the user specifically requested this page (as opposed to a client
+  // like AGSA or Now.)
+  bool user_requested_;
 };
 
 }  // namespace offline_pages

components/offline_pages/background/save_page_request_unittest.cc

@@ -12,6 +12,7 @@ namespace {
 const int64_t kRequestId = 42;
 const GURL kUrl("http://example.com");
 const ClientId kClientId("bookmark", "1234");
+const bool kUserRequested = true;
 }  // namespace
 
 class SavePageRequestTest : public testing::Test {
@@ -23,7 +24,8 @@ SavePageRequestTest::~SavePageRequestTest() {}
 
 TEST_F(SavePageRequestTest, CreatePendingReqeust) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kUrl, kClientId, creation_time);
+  SavePageRequest request(
+      kRequestId, kUrl, kClientId, creation_time, kUserRequested);
   ASSERT_EQ(kRequestId, request.request_id());
   ASSERT_EQ(kUrl, request.url());
   ASSERT_EQ(kClientId, request.client_id());
@@ -41,7 +43,7 @@ TEST_F(SavePageRequestTest, CreateNotReadyRequest) {
   base::Time creation_time = base::Time::Now();
   base::Time activation_time = creation_time + base::TimeDelta::FromHours(6);
   SavePageRequest request(kRequestId, kUrl, kClientId, creation_time,
-                          activation_time);
+                          activation_time, kUserRequested);
 
   ASSERT_EQ(kRequestId, request.request_id());
   ASSERT_EQ(kUrl, request.url());
@@ -63,7 +65,7 @@ TEST_F(SavePageRequestTest, StartAndCompleteRequest) {
   base::Time creation_time = base::Time::Now();
   base::Time activation_time = creation_time + base::TimeDelta::FromHours(6);
   SavePageRequest request(kRequestId, kUrl, kClientId, creation_time,
-                          activation_time);
+                          activation_time, kUserRequested);
 
   base::Time start_time = activation_time + base::TimeDelta::FromHours(3);
   request.MarkAttemptStarted(start_time);