[Offline Pages] Add evaluation test support in RequestCoordinator.

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 kMaxStartedTries = 4;
 const int kMaxCompletedTries = 1;
 const int kBackgroundProcessingTimeBudgetSeconds = 170;
 const int kSinglePageTimeLimitWhenBackgroundScheduledSeconds = 120;
 const int kSinglePageTimeLimitForImmediateLoadSeconds = 300;
 const int kRequestExpirationTimeInSeconds = 60 * 60 * 24 * 7;
 }  // 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_(false),
-        prefer_earlier_requests_(true),
-        retry_count_is_more_important_than_recency_(false),
-        max_started_tries_(kMaxStartedTries),
-        max_completed_tries_(kMaxCompletedTries) {}
+      : OfflinerPolicy(false,
+                       true,

[dougarnett, 2016/10/18 17:26:32]

Hard to read what these bools apply to. I would typically add comments on them
but Pete may suggest another way to document.

[Pete Williamson, 2016/10/18 19:24:32]

boolean arguments are always hard to understand at the point of call, like here.
 Normally I suggest using a const kPreferUntried = true;
and at the point of call, setting either kPreferUntried or !kPreferUntried as
the argument supplied.
Another solution that works is to use an enum with values such as
PREFER_UNTRIED_REQUESTS and PREFER_TRIED_REQUESTS, but that change will touch a
lot more places than here.

Why not just add background_processing_time_budget to the existing second
constructor?  It is already for tests, and your code uses the test hooks.

Alternatively, you could add a method to set the background processing time
budget.

[romax, 2016/10/18 20:31:51]

I forgot why I used this but seems like simply adding the time_budget to the
second c-tor is better. I'll do this.

+                       false,
+                       kMaxStartedTries,
+                       kMaxCompletedTries,
+                       kBackgroundProcessingTimeBudgetSeconds) {}

[dougarnett, 2016/10/18 17:26:32]

We might consider renaming to identify this constant now as the default

[romax, 2016/10/18 20:31:51]

Done.

 
   // Constructor for unit tests.
   OfflinerPolicy(bool prefer_untried,
                  bool prefer_earlier,
                  bool prefer_retry_count,
                  int max_started_tries,
                  int max_completed_tries)
+      : OfflinerPolicy(prefer_untried,
+                       prefer_earlier,
+                       prefer_retry_count,
+                       max_started_tries,
+                       max_completed_tries,
+                       kBackgroundProcessingTimeBudgetSeconds) {}
+
+  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),
         retry_count_is_more_important_than_recency_(prefer_retry_count),
         max_started_tries_(max_started_tries),
-        max_completed_tries_(max_completed_tries) {}
+        max_completed_tries_(max_completed_tries),
+        background_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
   // experiment.
 
   // Returns true if we should prefer retrying lesser tried requests.
   bool ShouldPreferUntriedRequests() const { return prefer_untried_requests_; }
 
   // Returns true if we should prefer older requests of equal number of tries.
   bool ShouldPreferEarlierRequests() const { return prefer_earlier_requests_; }
@@ skipping 26 matching lines @@
       return 0;
     // This is so low because we require the device to be plugged in and
     // charging.  If we decide to allow non-user requested pages when not
     // plugged in, we should raise this somewhat higher.
     return 25;
   }
 
   // How many seconds to keep trying new pages for, before we give up,  and
   // return to the scheduler.
   int GetBackgroundProcessingTimeBudgetSeconds() const {
-    return kBackgroundProcessingTimeBudgetSeconds;
+    return background_processing_time_budget_;
   }
 
   // How long do we allow a page to load before giving up on it when
   // background loading was scheduled.
   int GetSinglePageTimeLimitWhenBackgroundScheduledInSeconds() const {
     return kSinglePageTimeLimitWhenBackgroundScheduledSeconds;
   }
 
   // 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_;
   int max_started_tries_;
   int max_completed_tries_;
+  int background_processing_time_budget_;
 };
 }  // namespace offline_pages
 
 
 #endif  // COMPONENTS_OFFLINE_PAGES_BACKGROUND_OFFLINER_POLICY_H_