More detailed implementation of the RequestPicker

components/offline_pages/background/offliner_policy.h

Index: components/offline_pages/background/offliner_policy.h
diff --git a/components/offline_pages/background/offliner_policy.h b/components/offline_pages/background/offliner_policy.h
index 04e7f8443d184a4bc864550b7aeabf6044831b5c..7b6b0f4b56fb3f99a17324786b869d5113ca7eb7 100644
--- a/components/offline_pages/background/offliner_policy.h
+++ b/components/offline_pages/background/offliner_policy.h
@@ -13,8 +13,31 @@ 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; }
+
+  // The max number of times we will retry a request.
+  int GetMaxRetries() { return 4; }

[dougarnett, 2016/07/18 21:45:44]

So if retry count instead of try count, this means up to 5 tries. Wonder if we
should start smaller - perhaps 2 retries (so up to 3 tries).

[dougarnett, 2016/07/18 21:45:44]

I wonder if we will want to have different values for user-requested and not. If
so, do we pass in parameter to this method or have separate OfflinerPolicy
instances or something else. [Perhaps not actionable for this CL but something
for us to consider]

[dougarnett, 2016/07/18 21:45:44]

Consider defining these values in constants (eg, DEFAULT_MAX_RETRIES)?

[Pete Williamson, 2016/07/18 22:52:09]

Done (changed to 2)

[Pete Williamson, 2016/07/18 22:52:09]

This is temporary code, we will eventually change to get it from finch, so I
planned to keep it simple, which is why I didn't already use constants, but
since you asked, I changed the file to use constants everywhere.

[Pete Williamson, 2016/07/18 22:52:09]

Considering different values for later

+
+  // How many seconds to keep trying new pages for, before we call it a day

[dougarnett, 2016/07/18 21:45:44]

"call it a day" may be bad metaphor for a time in seconds value :-)

[Pete Williamson, 2016/07/18 22:52:09]

Done.

+  // and return to the scheduler for the time being.
+  int GetBackgroundTimeBudgetInSeconds() { return 170; }

[dougarnett, 2016/07/18 21:45:44]

GetBackgroundProcessingTimeBudgetSeconds?

[Pete Williamson, 2016/07/18 22:52:09]

Done.

+
+  // How long do we allow a page to load before giving up on it
+  int GetSinglePageTimeBudgetInSeconds() { return 120; }

[dougarnett, 2016/07/18 21:45:44]

GetBackgroundLoadTimeBudgetSeconds?

[not sure about these name suggestions but wanted to consider tying them more to
the operation terminology]

+
+  // How much battery must we have before fetching a page not explicitly
+  // requested by the user?
+  int GetBatteryRequiredForSpeculativeOfflining() { return 50; }

[dougarnett, 2016/07/18 21:45:44]

"Speculative" might carry more meaning that just not being user requested (eg,
Zine seems more "speculative" than top search result).

GetMinimumBatteryLevelForUserRequest() { return 0; } and 
GetMinimumBatteryLevelForNonUserRequest() or
GetMinimumBatteryLevel(bool is_user_request) ?

Also, should we hold off on defining Speculative yet?

[Pete Williamson, 2016/07/18 22:52:09]

Went with GetMinimumBatteryLevelForNonUserRequestOfflining();

While I'm happy to hold off, when I made similar suggestions about deferring
parts of the speculative loading path to later, you generally seemed to prefer
doing it optimistically.  Also, the Now/AGSA stuff is starting to land, so I
thought it was maybe time to start adding support.

 };
 }