More detailed implementation of the RequestPicker

components/offline_pages/background/offliner_policy.h

 // 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.
 
 #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;

[fgorski, 2016/07/20 16:23:31]

nit: having defaults specified in such way, you will pretty much use your budget
on a single failed attempt.

Did you consider other combinations, where you would maybe succeed 2 single page
attempts, or give more time for a single attempt?

[Pete Williamson, 2016/07/20 19:50:43]

 use your budget
Yep.  There is a bug to tune these, and at the moment, these are just guesses.
(crbug/625204)
The 170 comes from the 3 minute Marshmallow maintenance cycle.
The 120 comes from a 95% time for popular websites in India on 2G.
I understand that we only have time for a single failed attempt and to start a
second attempt, but it seems that we should figure out good numbers for these
first before worrying about their relation to eachother.

[fgorski, 2016/07/21 17:01:20]

Acknowledged.

+const int kMinimumBatteryPercentageForNonUserRequestOfflining = 50;
+}  // namespace
+
 namespace offline_pages {
 
 // Policy for the Background Offlining system.  Some policy will belong to the
 // RequestCoordinator, some to the RequestQueue, and some to the Offliner.
 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 already tried requests to
+  // starting new requests to maximize utility of any cached partial results.
+  bool ShouldPreferTriedRequests() { return true; }
+
+  // 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 for the time being.

[fgorski, 2016/07/20 16:23:31]

nit: can you remove "for the time being"?
just "return control to the scheduler" 

[Pete Williamson, 2016/07/20 19:50:43]

Done.

+  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() {

[fgorski, 2016/07/20 16:23:31]

nit: I think it is safe to drop offlining (given the context of policy name).

[Pete Williamson, 2016/07/20 19:50:43]

Not done, I think it is better to be more explict in this case.  I want to be
explict that the non-user request is an offlining request, not some other thing
we decided to do.

+    return kMinimumBatteryPercentageForNonUserRequestOfflining;
+  }
 };
 }
 
 #endif  // COMPONENTS_OFFLINE_PAGES_BACKGROUND_OFFLINER_POLICY_H_