[Offline Pages] Add Android version check for timeout in offliner policy.

components/offline_pages/core/background/offliner_policy.h

Index: components/offline_pages/core/background/offliner_policy.h
diff --git a/components/offline_pages/core/background/offliner_policy.h b/components/offline_pages/core/background/offliner_policy.h
index d828d4c6ea6ba5992bad1b020b6c41bc5ddf0509..ef9f8cb6b45c0de14c7e8c4def1a2bd7ad98648f 100644
--- a/components/offline_pages/core/background/offliner_policy.h
+++ b/components/offline_pages/core/background/offliner_policy.h
@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_OFFLINE_PAGES_CORE_BACKGROUND_OFFLINER_POLICY_H_
 #define COMPONENTS_OFFLINE_PAGES_CORE_BACKGROUND_OFFLINER_POLICY_H_
 
+#include "base/sys_info.h"
+
 namespace {
 // The max number of started tries is to guard against pages that make the
 // prerenderer crash.  It should be greater than or equal to the max number of
@@ -17,13 +19,22 @@ const int kMaxCompletedTries = 3;
 // By the time we get to a week, the user has forgotten asking for a page.
 const int kRequestExpirationTimeInSeconds = 60 * 60 * 24 * 7;
 
-// Scheduled background processing time limits.
+// Scheduled background processing time limits for doze mode, which requires
+// Android version >= 6.0 (API level >= 23). Otherwise the scheduled background
+// processing time should be the same as immediate loading (4 min 50 secs), it's
+// capped by the Prerenderer's 5 minute timeout.
 const int kDozeModeBackgroundServiceWindowSeconds = 60 * 3;
 const int kDefaultBackgroundProcessingTimeBudgetSeconds =
     kDozeModeBackgroundServiceWindowSeconds - 10;
 const int kSinglePageTimeLimitWhenBackgroundScheduledSeconds =
     kDozeModeBackgroundServiceWindowSeconds - 10;
 
+const int kNonDozeModeBackgroundServiceWindowSeconds = 60 * 5;
+const int kNonDozeDefaultBackgroundProcessingTimeBudgetSeconds =
+    kNonDozeModeBackgroundServiceWindowSeconds - 10;
+const int kNonDozeSinglePageTimeLimitWhenBackgroundScheduledSeconds =
+    kNonDozeModeBackgroundServiceWindowSeconds - 10;
+
 // Immediate processing time limits.  Note: experiments on GIN-2g-poor show many
 // page requests took 3 or 4 attempts in background scheduled mode with timeout
 // of 2 minutes. So for immediate processing mode, give page requests just under
@@ -45,9 +56,22 @@ class OfflinerPolicy {
         prefer_earlier_requests_(true),
         retry_count_is_more_important_than_recency_(true),
         max_started_tries_(kMaxStartedTries),
-        max_completed_tries_(kMaxCompletedTries),
-        background_scheduled_processing_time_budget_(
-            kDefaultBackgroundProcessingTimeBudgetSeconds) {}
+        max_completed_tries_(kMaxCompletedTries) {
+    int32_t os_major_version = 0;
+    int32_t os_minor_version = 0;
+    int32_t os_bugfix_version = 0;
+    base::SysInfo::OperatingSystemVersionNumbers(
+        &os_major_version, &os_minor_version, &os_bugfix_version);
+    if (os_major_version < 6) {
+      background_scheduled_processing_time_budget_ =
+          kNonDozeDefaultBackgroundProcessingTimeBudgetSeconds;
+      has_doze_mode_ = false;

[romax, 2017/03/17 02:58:34]

not sure if it's worth to have a member variable here, or i can change it to
check it everytime a getter is called.

[Pete Williamson, 2017/03/17 21:05:55]

I think I'd prefer checking this the first time that a getter is called instead
of in the constructor.  I don't mind if you use a member variable to remember
the state, but I'm concerned that we shouldn't do things that might possibly
fail in the constructor, since the constructor can't really return an error.  
Is there any chance the call to get version numbers could fail? 

Also, did you run unit tests?  I'm not sure that all the unit test frameworks
use a sane number for the android version.

[romax, 2017/03/23 21:56:37]

Generally i wouldn't think this would fail. Even if it fails we'll just use the
longer timeout in the current case. And putting it in constructor seems okay
since it's trying to initialize a member variable?
Also instead of putting it in constructor, it requires another boolean to track
if it's the first time that a getter is called, or the has_doze_mode_ needs to
be a tri-state variable (like an enum?)...
Please let me know if it's still not considered safe to do so or you're strongly
against it, i'll make the change again :)
I've also removed a piece of code for setting the default processing time added
for test harness but end up not being used.

[Pete Williamson, 2017/03/23 22:52:53]

OK, I took a look at the function, it doesn't seem like it can fail on android
(if it does, it just guesses we are on the default version of android).  So,
this should be OK in a constructor.

+    } else {
+      background_scheduled_processing_time_budget_ =
+          kDefaultBackgroundProcessingTimeBudgetSeconds;
+      has_doze_mode_ = true;
+    }
+  }
 
   // Constructor for unit tests.
   OfflinerPolicy(bool prefer_untried,
@@ -121,7 +145,9 @@ class OfflinerPolicy {
   // How long do we allow a page to load before giving up on it when
   // background loading was scheduled.
   int GetSinglePageTimeLimitWhenBackgroundScheduledInSeconds() const {
-    return kSinglePageTimeLimitWhenBackgroundScheduledSeconds;
+    if (has_doze_mode_)
+      return kSinglePageTimeLimitWhenBackgroundScheduledSeconds;
+    return kNonDozeSinglePageTimeLimitWhenBackgroundScheduledSeconds;
   }
 
   // How long do we allow a page to load before giving up on it when
@@ -139,6 +165,7 @@ class OfflinerPolicy {
   bool prefer_untried_requests_;
   bool prefer_earlier_requests_;
   bool retry_count_is_more_important_than_recency_;
+  bool has_doze_mode_;
   int max_started_tries_;
   int max_completed_tries_;
   int background_scheduled_processing_time_budget_;