[Background fetching] Configure and report fetching triggers

This CL enables configuring which trigger types result in a background
fetch. The default configuration can be overridden by a variation
parameter. It also reports the trigger type of each fetch to UMA.

Additionally, this CL enables two additional trigger types that were
previously inactive (Chrome cold start / Chrome foregrounding).

BUG=672479

Review-Url: https://codereview.chromium.org/2626433005
Cr-Commit-Position: refs/heads/master@{#443272}

components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc

@@ -9,6 +9,8 @@
 #include <utility>
 
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/strings/string_split.h"
 #include "base/time/clock.h"
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/pref_names.h"
@@ -65,6 +67,13 @@ static_assert(
             arraysize(kFetchingIntervalParamNameActiveSuggestionsConsumer),
     "Fill in all the info for fetching intervals.");
 
+const char* kTriggerTypeNames[] = {"persistent_scheduler_wake_up", "ntp_opened",
+                                   "browser_foregrounded",
+                                   "browser_cold_start"};
+
+const char* kTriggerTypesParamName = "scheduler_trigger_types";
+const char* kTriggerTypesParamValueForEmptyList = "-";
+
 base::TimeDelta GetDesiredFetchingInterval(
     FetchingInterval interval,
     UserClassifier::UserClass user_class) {
@@ -125,6 +134,17 @@ bool SchedulingRemoteSuggestionsProvider::FetchingSchedule::is_empty() const {
          interval_soft_on_usage_event.is_zero();
 }
 
+// These values are written to logs. New enum values can be added, but existing
+// enums must never be renumbered or deleted and reused. When adding new
+// entries, also update the array |kTriggerTypeNames| above.
+enum class SchedulingRemoteSuggestionsProvider::TriggerType {
+  PERSISTENT_SCHEDULER_WAKE_UP = 0,
+  NTP_OPENED = 1,
+  BROWSER_FOREGROUNDED = 2,
+  BROWSER_COLD_START = 3,
+  COUNT
+};
+
 SchedulingRemoteSuggestionsProvider::SchedulingRemoteSuggestionsProvider(
     Observer* observer,
     std::unique_ptr<RemoteSuggestionsProvider> provider,
@@ -139,7 +159,8 @@ SchedulingRemoteSuggestionsProvider::SchedulingRemoteSuggestionsProvider(
       background_fetch_in_progress_(false),
       user_classifier_(user_classifier),
       pref_service_(pref_service),
-      clock_(std::move(clock)) {
+      clock_(std::move(clock)),
+      enabled_triggers_(GetEnabledTriggerTypes()) {
   DCHECK(user_classifier);
   DCHECK(pref_service);
 
@@ -173,31 +194,35 @@ void SchedulingRemoteSuggestionsProvider::RescheduleFetching() {
 }
 
 void SchedulingRemoteSuggestionsProvider::OnPersistentSchedulerWakeUp() {
-  if (BackgroundFetchesDisabled()) {
-    return;
-  }
-
-  RefetchInTheBackground(/*callback=*/nullptr);
+  RefetchInTheBackgroundIfEnabled(TriggerType::PERSISTENT_SCHEDULER_WAKE_UP);
 }
 
 void SchedulingRemoteSuggestionsProvider::OnBrowserForegrounded() {
   // TODO(jkrcal): Consider that this is called whenever we open or return to an
   // Activity. Therefore, keep work light for fast start up calls.
-  // TODO(jkrcal): Implement.
+  if (!ShouldRefetchInTheBackgroundNow()) {
+    return;
+  }
+
+  RefetchInTheBackgroundIfEnabled(TriggerType::BROWSER_FOREGROUNDED);
 }
 
 void SchedulingRemoteSuggestionsProvider::OnBrowserColdStart() {
   // TODO(fhorschig|jkrcal): Consider that work here must be kept light for fast
   // cold start ups.
-  // TODO(jkrcal): Implement.
+  if (!ShouldRefetchInTheBackgroundNow()) {
+    return;
+  }
+
+  RefetchInTheBackgroundIfEnabled(TriggerType::BROWSER_COLD_START);
 }
 
 void SchedulingRemoteSuggestionsProvider::OnNTPOpened() {
-  if (BackgroundFetchesDisabled() || !ShouldRefetchInTheBackgroundNow()) {
+  if (!ShouldRefetchInTheBackgroundNow()) {
     return;
   }
 
-  RefetchInTheBackground(/*callback=*/nullptr);
+  RefetchInTheBackgroundIfEnabled(TriggerType::NTP_OPENED);
 }
 
 void SchedulingRemoteSuggestionsProvider::SetProviderStatusCallback(
@@ -376,8 +401,17 @@ void SchedulingRemoteSuggestionsProvider::StoreFetchingSchedule() {
       schedule_.interval_soft_on_usage_event.ToInternalValue());
 }
 
-bool SchedulingRemoteSuggestionsProvider::BackgroundFetchesDisabled() const {
-  return schedule_.is_empty();
+void SchedulingRemoteSuggestionsProvider::RefetchInTheBackgroundIfEnabled(
+    SchedulingRemoteSuggestionsProvider::TriggerType trigger) {
+  if (BackgroundFetchesDisabled(trigger)) {
+    return;
+  }
+
+  UMA_HISTOGRAM_ENUMERATION(
+      "NewTabPage.ContentSuggestions.BackgroundFetchTrigger",
+      static_cast<int>(trigger), static_cast<int>(TriggerType::COUNT));
+
+  RefetchInTheBackground(/*callback=*/nullptr);
 }
 
 bool SchedulingRemoteSuggestionsProvider::ShouldRefetchInTheBackgroundNow() {
@@ -388,6 +422,18 @@ bool SchedulingRemoteSuggestionsProvider::ShouldRefetchInTheBackgroundNow() {
   return first_allowed_fetch_time <= clock_->Now();
 }
 
+bool SchedulingRemoteSuggestionsProvider::BackgroundFetchesDisabled(
+    SchedulingRemoteSuggestionsProvider::TriggerType trigger) const {
+  if (schedule_.is_empty()) {
+    return true;  // Background fetches are disabled in general.
+  }
+
+  if (enabled_triggers_.count(trigger) == 0) {
+    return true;  // Background fetches for |trigger| are not enabled.
+  }
+  return false;
+}
+
 void SchedulingRemoteSuggestionsProvider::FetchFinished(
     const FetchDoneCallback& callback,
     Status fetch_status,
@@ -423,4 +469,49 @@ void SchedulingRemoteSuggestionsProvider::OnFetchCompleted(
   ApplyPersistentFetchingSchedule();
 }
 
+std::set<SchedulingRemoteSuggestionsProvider::TriggerType>
+SchedulingRemoteSuggestionsProvider::GetEnabledTriggerTypes() {
+  static_assert(static_cast<unsigned int>(TriggerType::COUNT) ==
+                    arraysize(kTriggerTypeNames),
+                "Fill in names for trigger types.");
+
+  std::string param_value = variations::GetVariationParamValueByFeature(
+      ntp_snippets::kArticleSuggestionsFeature, kTriggerTypesParamName);
+  if (param_value == kTriggerTypesParamValueForEmptyList) {
+    return std::set<TriggerType>();
+  }
+
+  std::vector<std::string> tokens = base::SplitString(
+      param_value, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+  if (tokens.empty()) {
+    return GetDefaultEnabledTriggerTypes();
+  }
+
+  std::set<TriggerType> enabled_types;
+  for (const auto& token : tokens) {
+    auto it = std::find(std::begin(kTriggerTypeNames),
+                        std::end(kTriggerTypeNames), token);
+    if (it == std::end(kTriggerTypeNames)) {
+      DLOG(WARNING) << "Failed to parse variation param "
+                    << kTriggerTypesParamName << " with string value "
+                    << param_value
+                    << " into a comma-separated list of keywords. "
+                    << "Unknown token " << token
+                    << " found. Falling back to default value.";
+      return GetDefaultEnabledTriggerTypes();
+    }
+
+    // Add the enabled type represented by |token| into the result set.
+    enabled_types.insert(
+        static_cast<TriggerType>(it - std::begin(kTriggerTypeNames)));
+  }
+  return enabled_types;
+}
+
+std::set<SchedulingRemoteSuggestionsProvider::TriggerType>
+SchedulingRemoteSuggestionsProvider::GetDefaultEnabledTriggerTypes() {
+  return {TriggerType::PERSISTENT_SCHEDULER_WAKE_UP, TriggerType::NTP_OPENED,
+          TriggerType::BROWSER_FOREGROUNDED};
+}
+
 }  // namespace ntp_snippets

components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h

@@ -116,6 +116,8 @@ class SchedulingRemoteSuggestionsProvider final
       base::TimeDelta interval_soft_on_usage_event;
   };
 
+  enum class TriggerType;
+
   // Callback that is notified whenever the status of |provider_| changes.
   void OnProviderStatusChanged(
       RemoteSuggestionsProvider::ProviderStatus status);
@@ -130,10 +132,16 @@ class SchedulingRemoteSuggestionsProvider final
   // schedule.
   void StopScheduling();
 
+  // Trigger a background refetch for the given |trigger| if enabled.
+  void RefetchInTheBackgroundIfEnabled(TriggerType trigger);
+
   // Checks whether it is time to perform a soft background fetch, according to
   // |schedule|.
   bool ShouldRefetchInTheBackgroundNow();
 
+  // Returns whether background fetching (for the given |trigger|) is disabled.
+  bool BackgroundFetchesDisabled(TriggerType trigger) const;
+
   // Callback after Fetch is completed.
   void FetchFinished(const FetchDoneCallback& callback,
                      Status fetch_status,
@@ -152,11 +160,16 @@ class SchedulingRemoteSuggestionsProvider final
   // Load and store |schedule_|.
   void LoadLastFetchingSchedule();
   void StoreFetchingSchedule();
-  bool BackgroundFetchesDisabled() const;
 
   // Applies the persistent schedule given by |schedule_|.
   void ApplyPersistentFetchingSchedule();
 
+  // Gets enabled trigger types from the variation parameter.
+  std::set<TriggerType> GetEnabledTriggerTypes();
+
+  // Gets trigger types enabled by default.
+  std::set<TriggerType> GetDefaultEnabledTriggerTypes();
+
   // Interface for doing all the actual work (apart from scheduling).
   std::unique_ptr<RemoteSuggestionsProvider> provider_;
 
@@ -172,6 +185,7 @@ class SchedulingRemoteSuggestionsProvider final
 
   PrefService* pref_service_;
   std::unique_ptr<base::Clock> clock_;
+  std::set<SchedulingRemoteSuggestionsProvider::TriggerType> enabled_triggers_;
 
   DISALLOW_COPY_AND_ASSIGN(SchedulingRemoteSuggestionsProvider);
 };

components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc

@@ -120,12 +120,22 @@ class SchedulingRemoteSuggestionsProviderTest
     : public ::testing::Test {
  public:
   SchedulingRemoteSuggestionsProviderTest()
-      : underlying_provider_(nullptr),
+      :  // For the test we enabled all trigger types.
+        default_variation_params_{{"scheduler_trigger_types",
+                                   "persistent_scheduler_wake_up,ntp_opened,"
+                                   "browser_foregrounded,browser_cold_start"}},
+        params_manager_(ntp_snippets::kStudyName,
+                        default_variation_params_,
+                        {kArticleSuggestionsFeature.name}),
+        underlying_provider_(nullptr),
         scheduling_provider_(nullptr),
         user_classifier_(/*pref_service=*/nullptr) {
     SchedulingRemoteSuggestionsProvider::RegisterProfilePrefs(
         utils_.pref_service()->registry());
+    ResetProvider();
+  }
 
+  void ResetProvider() {
     auto underlying_provider =
         base::MakeUnique<StrictMock<MockRemoteSuggestionsProvider>>(
             /*observer=*/nullptr);
@@ -147,7 +157,20 @@ class SchedulingRemoteSuggestionsProviderTest
             std::move(test_clock));
   }
 
+  void SetVariationParameter(const std::string& param_name,
+                             const std::string& param_value) {
+    std::map<std::string, std::string> params = default_variation_params_;
+    params[param_name] = param_value;
+
+    params_manager_.ClearAllVariationParams();
+    params_manager_.SetVariationParamsWithFeatureAssociations(
+        ntp_snippets::kStudyName, params,
+        {ntp_snippets::kArticleSuggestionsFeature.name});
+  }
+
  protected:
+  std::map<std::string, std::string> default_variation_params_;
+  variations::testing::VariationParamsManager params_manager_;
   StrictMock<MockPersistentScheduler> persistent_scheduler_;
   StrictMock<MockRemoteSuggestionsProvider>* underlying_provider_;
   std::unique_ptr<SchedulingRemoteSuggestionsProvider> scheduling_provider_;
@@ -170,11 +193,68 @@ TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ShouldIgnoreSignalsWhenNotEnabled) {
   scheduling_provider_->OnPersistentSchedulerWakeUp();
   scheduling_provider_->OnNTPOpened();
+  scheduling_provider_->OnBrowserForegrounded();
+  scheduling_provider_->OnBrowserColdStart();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldIgnoreSignalsWhenDisabledByParam) {
+  // First set an empty list of allowed trigger types.
+  SetVariationParameter("scheduler_trigger_types", "-");
+  ResetProvider();
+
+  // Then enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+  scheduling_provider_->OnNTPOpened();
+  scheduling_provider_->OnBrowserForegrounded();
+  scheduling_provider_->OnBrowserColdStart();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldHandleEmptyParamForTriggerTypes) {
+  // First set an empty param for allowed trigger types -> should result in the
+  // default list.
+  SetVariationParameter("scheduler_trigger_types", "");
+  ResetProvider();
+
+  // Then enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  // For instance, persistent scheduler wake up should be enabled by default.
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldHandleIncorrentParamForTriggerTypes) {
+  // First set an invalid list of allowed trigger types.
+  SetVariationParameter("scheduler_trigger_types", "ntp_opened,foo;");
+  ResetProvider();
+
+  // Then enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  // For instance, persistent scheduler wake up should be enabled by default.
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
 }
 
 TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ShouldFetchOnPersistentSchedulerWakeUp) {
-  // First enable the scheduler.
+  // First set only this type to be allowed.
+  SetVariationParameter("scheduler_trigger_types",
+                        "persistent_scheduler_wake_up");
+  ResetProvider();
+
+  // Then enable the scheduler.
   EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
   ChangeStatusOfUnderlyingProvider(
       RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
@@ -223,7 +303,11 @@ TEST_F(SchedulingRemoteSuggestionsProviderTest,
 
 TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ShouldFetchOnNTPOpenedForTheFirstTime) {
-  // First enable the scheduler.
+  // First set only this type to be allowed.
+  SetVariationParameter("scheduler_trigger_types", "ntp_opened");
+  ResetProvider();
+
+  // Then enable the scheduler.
   EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
   ChangeStatusOfUnderlyingProvider(
       RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
@@ -232,6 +316,36 @@ TEST_F(SchedulingRemoteSuggestionsProviderTest,
   scheduling_provider_->OnNTPOpened();
 }
 
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldFetchOnBrowserForegroundedForTheFirstTime) {
+  // First set only this type to be allowed.
+  SetVariationParameter("scheduler_trigger_types", "browser_foregrounded");
+  ResetProvider();
+
+  // Then enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  scheduling_provider_->OnBrowserForegrounded();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldFetchOnBrowserColdStartForTheFirstTime) {
+  // First set only this type to be allowed.
+  SetVariationParameter("scheduler_trigger_types", "browser_cold_start");
+  ResetProvider();
+
+  // Then enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ChangeStatusOfUnderlyingProvider(
+      RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
+
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  scheduling_provider_->OnBrowserColdStart();
+}
+
 TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ShouldNotFetchOnNTPOpenedAfterSuccessfulSoftFetch) {
   // First enable the scheduler; the second Schedule is called after the
@@ -433,10 +547,7 @@ TEST_F(SchedulingRemoteSuggestionsProviderTest,
 
   // UserClassifier defaults to UserClass::ACTIVE_NTP_USER if PrefService is
   // null. Change the wifi interval for this class.
-  variations::testing::VariationParamsManager params_manager(
-      ntp_snippets::kStudyName,
-      {{"fetching_interval_hours-wifi-active_ntp_user", "1.5"}},
-      {kArticleSuggestionsFeature.name});
+  SetVariationParameter("fetching_interval_hours-wifi-active_ntp_user", "1.5");
 
   // Schedule() should get called for the second time after params have changed.
   ChangeStatusOfUnderlyingProvider(
@@ -450,11 +561,9 @@ TEST_F(SchedulingRemoteSuggestionsProviderTest,
       RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
 
   // UserClassifier defaults to UserClass::ACTIVE_NTP_USER if PrefService is
-  // null. Change the wifi interval for this class.
-  variations::testing::VariationParamsManager params_manager(
-      ntp_snippets::kStudyName,
-      {{"fetching_interval_hours-fallback-active_ntp_user", "1.5"}},
-      {kArticleSuggestionsFeature.name});
+  // null. Change the fallback interval for this class.
+  SetVariationParameter("fetching_interval_hours-fallback-active_ntp_user",
+                        "1.5");
 
   // Schedule() should get called for the second time after params have changed.
   ChangeStatusOfUnderlyingProvider(
@@ -468,11 +577,9 @@ TEST_F(SchedulingRemoteSuggestionsProviderTest,
       RemoteSuggestionsProvider::ProviderStatus::ACTIVE);
 
   // UserClassifier defaults to UserClass::ACTIVE_NTP_USER if PrefService is
-  // null. Change the wifi interval for this class.
-  variations::testing::VariationParamsManager params_manager(
-      ntp_snippets::kStudyName,
-      {{"soft_fetching_interval_hours-active-active_ntp_user", "1.5"}},
-      {kArticleSuggestionsFeature.name});
+  // null. Change the on usage interval for this class.
+  SetVariationParameter("soft_fetching_interval_hours-active-active_ntp_user",
+                        "1.5");
 
   // Schedule() should get called for the second time after params have changed.
   ChangeStatusOfUnderlyingProvider(

tools/metrics/histograms/histograms.xml

@@ -38797,6 +38797,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="NewTabPage.ContentSuggestions.BackgroundFetchTrigger"
+    enum="BackgroundFetchTrigger">
+  <owner>jkrcal@chromium.org</owner>
+  <summary>
+    Android: The type of trigger that caused a background fetch of NTP content
+    suggestions from a suggestion server. Every background fetch is recorded.
+  </summary>
+</histogram>
+
 <histogram name="NewTabPage.ContentSuggestions.CategoryDismissed"
     enum="ContentSuggestionsCategory">
   <owner>treib@chromium.org</owner>
@@ -77800,6 +77809,13 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   <int value="2" label="Ok, got it"/>
 </enum>
 
+<enum name="BackgroundFetchTrigger" type="int">
+  <int value="0" label="Wake-up of the persistent scheduler"/>
+  <int value="1" label="NTP opened"/>
+  <int value="2" label="Browser foregrounded"/>
+  <int value="3" label="Cold start of the browser"/>
+</enum>
+
 <enum name="BackgroundModeMenuItem" type="int">
   <int value="0" label="About"/>
   <int value="1" label="Task manager"/>