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

components/offline_pages/core/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_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
 // completed tries.
 const int kMaxStartedTries = 5;
 // The number of max completed tries is based on Gin2G-poor testing showing that
 // we often need about 4 tries with a 2 minute window, or 3 retries with a 3
 // minute window. Also, we count one try now for foreground/disabled requests.
 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
 // 5 minutes, which is the timeout limit for the prerender itself. Then budget
 // up to 3 of those requests in processing window.
 const int kSinglePageTimeLimitForImmediateLoadSeconds = 60 * 4 + 50;
 const int kImmediateLoadProcessingTimeBudgetSeconds =
     kSinglePageTimeLimitForImmediateLoadSeconds * 5;
 }  // 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()
       : prefer_untried_requests_(true),
         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,
                  bool prefer_earlier,
                  bool prefer_retry_count,
                  int max_started_tries,
                  int max_completed_tries,
                  int background_processing_time_budget)
       : prefer_untried_requests_(prefer_untried),
         prefer_earlier_requests_(prefer_earlier),
@@ skipping 53 matching lines @@
 
   // How many seconds to keep trying new pages for, before we give up, when
   // processing started immediately (without scheduler).
   int GetProcessingTimeBudgetForImmediateLoadInSeconds() const {
     return kImmediateLoadProcessingTimeBudgetSeconds;
   }
 
   // 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
   // immediately background loading.
   int GetSinglePageTimeLimitForImmediateLoadInSeconds() const {
     return kSinglePageTimeLimitForImmediateLoadSeconds;
   }
 
   // How long we allow requests to remain in the system before giving up.
   int GetRequestExpirationTimeInSeconds() const {
     return kRequestExpirationTimeInSeconds;
   }
 
  private:
   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_;
 };
 }  // namespace offline_pages
 
 #endif  // COMPONENTS_OFFLINE_PAGES_CORE_BACKGROUND_OFFLINER_POLICY_H_