[Offline Pages] Defines longer processing budget for immediate bg loads.

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 d3424f39eb7a7b53e568fb8b75789655a990a8dc..7780a5f34190a817d9a0473a8cdb68a63f4a42ab 100644
--- a/components/offline_pages/background/offliner_policy.h
+++ b/components/offline_pages/background/offliner_policy.h
@@ -8,9 +8,10 @@
 namespace {
 const int kMaxStartedTries = 4;
 const int kMaxCompletedTries = 1;
-const int kDefaultBackgroundProcessingTimeBudgetSeconds = 170;
-const int kSinglePageTimeLimitWhenBackgroundScheduledSeconds = 120;
-const int kSinglePageTimeLimitForImmediateLoadSeconds = 300;
+const int kDefaultBackgroundProcessingTimeBudgetSeconds = 60 * 3 - 10;

[Dmitry Titov, 2016/10/19 21:41:26]

Would be good to have some comments reflecting why these specific values are
selected. Are they arbitrary? Or not? "-10" looks like it tries to be 10 seconds
less than some other constant. Which one?

[Pete Williamson, 2016/10/19 23:18:18]

This is based on the Marshmallow Doze mode 3 minute limit.  We want to avoid
being killed by doze mode by finishing up before it is over.  In the future, we
might be smarter about what OS we are running on, and use longer timeouts on
pre-marshmallow OSs.

[dougarnett, 2016/10/21 20:01:19]

Did some doc and naming here to try to communicate a bit about these
values/guesses

+const int kImmediateLoadProcessingTimeBudgetSeconds = 60 * 10;
+const int kSinglePageTimeLimitWhenBackgroundScheduledSeconds = 60 * 2;

[Pete Williamson, 2016/10/19 23:18:18]

Based on our recent Gin2G-poor testing, we might also put this at 170 seconds.

[dougarnett, 2016/10/21 20:01:19]

Done.

+const int kSinglePageTimeLimitForImmediateLoadSeconds = 60 * 5;
 const int kRequestExpirationTimeInSeconds = 60 * 60 * 24 * 7;
 }  // namespace
 
@@ -26,7 +27,7 @@ class OfflinerPolicy {
         retry_count_is_more_important_than_recency_(false),
         max_started_tries_(kMaxStartedTries),
         max_completed_tries_(kMaxCompletedTries),
-        background_processing_time_budget_(
+        background_scheduled_processing_time_budget_(
             kDefaultBackgroundProcessingTimeBudgetSeconds) {}
 
   // Constructor for unit tests.
@@ -41,7 +42,8 @@ class OfflinerPolicy {
         retry_count_is_more_important_than_recency_(prefer_retry_count),
         max_started_tries_(max_started_tries),
         max_completed_tries_(max_completed_tries),
-        background_processing_time_budget_(background_processing_time_budget) {}
+        background_scheduled_processing_time_budget_(
+            background_processing_time_budget) {}
 
   // TODO(petewil): Numbers here are chosen arbitrarily, do the proper studies
   // to get good policy numbers. Eventually this should get data from a finch
@@ -85,10 +87,16 @@ class OfflinerPolicy {
     return 25;
   }
 
-  // How many seconds to keep trying new pages for, before we give up,  and
+  // How many seconds to keep trying new pages for, before we give up, and
   // return to the scheduler.
-  int GetBackgroundProcessingTimeBudgetSeconds() const {
-    return background_processing_time_budget_;
+  int GetProcessingTimeBudgetWhenBackgroundScheduledInSeconds() const {

[Pete Williamson, 2016/10/19 23:18:18]

Maybe better to have one method that takes an enum instead of two.
SCHEDULED_WINDOW or IMMEDIATE_WINDOW might work here.

[dougarnett, 2016/10/21 20:01:19]

Adding TODO for now. Currently this enum is private so let's talk more about
exposing and possible other policy considerations between these modes - eg, I'm
wondering about different TryNextRequest behaviour on failures/timeouts with
immediate mode (but will need design discussion).

+    return background_scheduled_processing_time_budget_;
+  }
+
+  // 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
@@ -114,7 +122,7 @@ class OfflinerPolicy {
   bool retry_count_is_more_important_than_recency_;
   int max_started_tries_;
   int max_completed_tries_;
-  int background_processing_time_budget_;
+  int background_scheduled_processing_time_budget_;
 };
 }  // namespace offline_pages