[Remote suggestions] Allow to switch to M58 fetching schedule simply

This CL allows simple experimental comparision of changes vs. M58.

Bug: 728976
Change-Id: Ia977aebafba7e9004caa06fbb77128ab6d7da2ac
Reviewed-on: https://chromium-review.googlesource.com/522124Reviewed-by: Friedrich Horschig <fhorschig@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477929}

components/ntp_snippets/features.cc

@@ -55,6 +55,10 @@ const base::Feature kPublisherFaviconsFromNewServerFeature{
     "ContentSuggestionsFaviconsFromNewServer",
     base::FEATURE_DISABLED_BY_DEFAULT};
 
+const base::Feature kRemoteSuggestionsEmulateM58FetchingSchedule{
+    "RemoteSuggestionsEmulateM58FetchingSchedule",
+    base::FEATURE_DISABLED_BY_DEFAULT};
+
 const char kCategoryRankerParameter[] = "category_ranker";
 const char kCategoryRankerConstantRanker[] = "constant";
 const char kCategoryRankerClickBasedRanker[] = "click_based";

components/ntp_snippets/features.h

@@ -46,6 +46,12 @@ extern const base::Feature kCategoryRanker;
 // Feature to allow the new Google favicon server for fetching publisher icons.
 extern const base::Feature kPublisherFaviconsFromNewServerFeature;
 
+// Feature for simple experimental comparision and validation of changes since
+// M58: enabling this brings back the M58 Stable fetching schedule (which is
+// suitable for Holdback groups).
+// TODO(jkrcal): Remove when the comparision is done (probably after M62).
+extern const base::Feature kRemoteSuggestionsEmulateM58FetchingSchedule;
+
 // Parameter and its values for the kCategoryRanker feature flag.
 extern const char kCategoryRankerParameter[];
 extern const char kCategoryRankerConstantRanker[];

components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc

@@ -4,11 +4,13 @@
 
 #include "components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h"
 
+#include <cfloat>
 #include <random>
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
+#include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
@@ -68,6 +70,18 @@ const double kDefaultFetchingIntervalHoursActiveNtpUser[] = {96.0, 48.0, 48.0,
 const double kDefaultFetchingIntervalHoursActiveSuggestionsConsumer[] = {
     48.0, 24.0, 24.0, 6.0, 2.0, 1.0};
 
+// For a simple comparision: fetching intervals that emulate the state really
+// rolled out to 100% M58 Stable. Used for evaluation of later changes. DBL_MAX
+// values simulate this interval being disabled.
+// TODO(jkrcal): Remove when not needed any more, incl. the feature. Probably
+// after M62 when CH is launched.
+const double kM58FetchingIntervalHoursRareNtpUser[] = {48.0,    24.0, DBL_MAX,
+                                                       DBL_MAX, 4.0,  4.0};
+const double kM58FetchingIntervalHoursActiveNtpUser[] = {24.0,    8.0,  DBL_MAX,
+                                                         DBL_MAX, 10.0, 10.0};
+const double kM58FetchingIntervalHoursActiveSuggestionsConsumer[] = {
+    24.0, 6.0, DBL_MAX, DBL_MAX, 1.0, 1.0};
+
 // Variation parameters than can be used to override the default fetching
 // intervals. For backwards compatibility, we do not rename
 //  - the "fetching_" params (should be "persistent_fetching_");
@@ -101,6 +115,12 @@ static_assert(
             arraysize(kDefaultFetchingIntervalHoursActiveNtpUser) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             arraysize(kDefaultFetchingIntervalHoursActiveSuggestionsConsumer) &&
+        static_cast<unsigned int>(FetchingInterval::COUNT) ==
+            arraysize(kM58FetchingIntervalHoursRareNtpUser) &&
+        static_cast<unsigned int>(FetchingInterval::COUNT) ==
+            arraysize(kM58FetchingIntervalHoursActiveNtpUser) &&
+        static_cast<unsigned int>(FetchingInterval::COUNT) ==
+            arraysize(kM58FetchingIntervalHoursActiveSuggestionsConsumer) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             arraysize(kFetchingIntervalParamNameRareNtpUser) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
@@ -132,20 +152,29 @@ base::TimeDelta GetDesiredFetchingInterval(
   const unsigned int index = static_cast<unsigned int>(interval);
   DCHECK(index < arraysize(kDefaultFetchingIntervalHoursRareNtpUser));
 
+  bool emulateM58 = base::FeatureList::IsEnabled(
+      kRemoteSuggestionsEmulateM58FetchingSchedule);
+
   double default_value_hours = 0.0;
   const char* param_name = nullptr;
   switch (user_class) {
     case UserClassifier::UserClass::RARE_NTP_USER:
-      default_value_hours = kDefaultFetchingIntervalHoursRareNtpUser[index];
+      default_value_hours =
+          emulateM58 ? kM58FetchingIntervalHoursRareNtpUser[index]
+                     : kDefaultFetchingIntervalHoursRareNtpUser[index];
       param_name = kFetchingIntervalParamNameRareNtpUser[index];
       break;
     case UserClassifier::UserClass::ACTIVE_NTP_USER:
-      default_value_hours = kDefaultFetchingIntervalHoursActiveNtpUser[index];
+      default_value_hours =
+          emulateM58 ? kM58FetchingIntervalHoursActiveNtpUser[index]
+                     : kDefaultFetchingIntervalHoursActiveNtpUser[index];
       param_name = kFetchingIntervalParamNameActiveNtpUser[index];
       break;
     case UserClassifier::UserClass::ACTIVE_SUGGESTIONS_CONSUMER:
       default_value_hours =
-          kDefaultFetchingIntervalHoursActiveSuggestionsConsumer[index];
+          emulateM58
+              ? kM58FetchingIntervalHoursActiveSuggestionsConsumer[index]
+              : kDefaultFetchingIntervalHoursActiveSuggestionsConsumer[index];
       param_name = kFetchingIntervalParamNameActiveSuggestionsConsumer[index];
       break;
   }

components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/clock.h"
@@ -867,4 +868,27 @@ TEST_F(RemoteSuggestionsSchedulerImplTest,
   scheduler()->OnPersistentSchedulerWakeUp();
 }
 
+TEST_F(RemoteSuggestionsSchedulerImplTest,
+       ShouldIgnoreSubsequentStartupSignalsForM58) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      kRemoteSuggestionsEmulateM58FetchingSchedule);
+  RemoteSuggestionsProvider::FetchStatusCallback signal_fetch_done;
+
+  // First enable the scheduler -- this will trigger the persistent scheduling.
+  EXPECT_CALL(*persistent_scheduler(), Schedule(_, _));
+  ActivateProvider();
+
+  // The startup triggers are ignored.
+  EXPECT_CALL(*provider(), RefetchInTheBackground(_)).Times(0);
+  scheduler()->OnBrowserForegrounded();
+  scheduler()->OnBrowserColdStart();
+
+  // Foreground the browser again after a very long delay. Again, no fetch is
+  // executed for neither Foregrounded, nor ColdStart.
+  test_clock()->Advance(base::TimeDelta::FromHours(100000));
+  scheduler()->OnBrowserForegrounded();
+  scheduler()->OnBrowserColdStart();
+}
+
 }  // namespace ntp_snippets