[Background fetching] Configure and report fetching triggers

components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc

 // 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.
 
 #include "components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h"
 
 #include <random>
 #include <string>
 #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"
 #include "components/ntp_snippets/remote/persistent_scheduler.h"
 #include "components/ntp_snippets/status.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/variations/variations_associated_data.h"
 
@@ skipping 36 matching lines @@
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             arraysize(kDefaultFetchingIntervalActiveSuggestionsConsumer) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             arraysize(kFetchingIntervalParamNameRareNtpUser) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             arraysize(kFetchingIntervalParamNameActiveNtpUser) &&
         static_cast<unsigned int>(FetchingInterval::COUNT) ==
             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* kDefaultTriggerTypes =
+    "persistent_scheduler_wake_up,ntp_opened,browser_foregrounded";
+
 base::TimeDelta GetDesiredFetchingInterval(
     FetchingInterval interval,
     UserClassifier::UserClass user_class) {
   double default_value_hours = 0.0;
 
   DCHECK(interval != FetchingInterval::COUNT);
   const unsigned int index = static_cast<unsigned int>(interval);
   DCHECK(index < arraysize(kDefaultFetchingIntervalRareNtpUser));
 
   const char* param_name = nullptr;
@@ skipping 40 matching lines @@
     const FetchingSchedule& other) const {
   return !operator==(other);
 }
 
 bool SchedulingRemoteSuggestionsProvider::FetchingSchedule::is_empty() const {
   return interval_persistent_wifi.is_zero() &&
          interval_persistent_fallback.is_zero() &&
          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,
     PersistentScheduler* persistent_scheduler,
     const UserClassifier* user_classifier,
     PrefService* pref_service,
     std::unique_ptr<base::Clock> clock)
     : RemoteSuggestionsProvider(observer),
       RemoteSuggestionsScheduler(),
       provider_(std::move(provider)),
       persistent_scheduler_(persistent_scheduler),
       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);
 
   LoadLastFetchingSchedule();
 
   provider_->SetProviderStatusCallback(
       base::MakeUnique<RemoteSuggestionsProvider::ProviderStatusCallback>(
           base::BindRepeating(
               &SchedulingRemoteSuggestionsProvider::OnProviderStatusChanged,
               base::Unretained(this))));
@@ skipping 13 matching lines @@
   registry->RegisterInt64Pref(prefs::kSnippetLastFetchAttempt, 0);
 }
 
 void SchedulingRemoteSuggestionsProvider::RescheduleFetching() {
   // Force the reschedule by stopping and starting it again.
   StopScheduling();
   StartScheduling();
 }
 
 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.
+  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.
+  RefetchInTheBackgroundIfEnabled(TriggerType::BROWSER_COLD_START);
 }
 
 void SchedulingRemoteSuggestionsProvider::OnNTPOpened() {
-  if (BackgroundFetchesDisabled() || !ShouldRefetchInTheBackgroundNow()) {
+  if (!ShouldRefetchInTheBackgroundNow()) {

[tschumann, 2017/01/10 14:28:16]

this is asymmetric to the other triggers. Shouldn't they check this as well?

[jkrcal, 2017/01/10 18:52:27]

Oops, sure. Thanks!

     return;
   }
 
-  RefetchInTheBackground(/*callback=*/nullptr);
+  RefetchInTheBackgroundIfEnabled(TriggerType::NTP_OPENED);
 }
 
 void SchedulingRemoteSuggestionsProvider::SetProviderStatusCallback(
     std::unique_ptr<ProviderStatusCallback> callback) {
   provider_->SetProviderStatusCallback(std::move(callback));
 }
 
 void SchedulingRemoteSuggestionsProvider::RefetchInTheBackground(
     std::unique_ptr<FetchStatusCallback> callback) {
   if (background_fetch_in_progress_) {
@@ skipping 158 matching lines @@
   pref_service_->SetInt64(prefs::kSnippetPersistentFetchingIntervalWifi,
                           schedule_.interval_persistent_wifi.ToInternalValue());
   pref_service_->SetInt64(
       prefs::kSnippetPersistentFetchingIntervalFallback,
       schedule_.interval_persistent_fallback.ToInternalValue());
   pref_service_->SetInt64(
       prefs::kSnippetSoftFetchingIntervalOnUsageEvent,
       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() {
   base::Time first_allowed_fetch_time =
       base::Time::FromInternalValue(
           pref_service_->GetInt64(prefs::kSnippetLastFetchAttempt)) +
       schedule_.interval_soft_on_usage_event;
   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,
     std::vector<ContentSuggestion> suggestions) {
   OnFetchCompleted(fetch_status);
   if (callback) {
     callback.Run(fetch_status, std::move(suggestions));
   }
 }
 
@@ skipping 15 matching lines @@
   // Reschedule after a fetch. The persistent schedule is applied only after a
   // successful fetch. After a failed fetch, we want to keep the previous
   // persistent schedule intact so that we eventually get a persistent
   // fallback fetch (if the wifi persistent fetches keep failing).
   if (fetch_status.code != StatusCode::SUCCESS) {
     return;
   }
   ApplyPersistentFetchingSchedule();
 }
 
+std::set<SchedulingRemoteSuggestionsProvider::TriggerType>
+SchedulingRemoteSuggestionsProvider::GetEnabledTriggerTypes() {
+  std::string param_value = variations::GetVariationParamValueByFeature(
+      ntp_snippets::kArticleSuggestionsFeature, kTriggerTypesParamName);
+
+  std::set<TriggerType> enabled_types;
+  if (ParseEnabledTriggerTypes(param_value, &enabled_types)) {
+    return enabled_types;
+  }
+
+  DLOG(WARNING) << "Failed to parse variation param " << kTriggerTypesParamName
+                << " with string value " << param_value
+                << " into a comma-separated list of keywords. "
+                   "Falling back to default value of "
+                << kDefaultTriggerTypes;
+
+  if (ParseEnabledTriggerTypes(kDefaultTriggerTypes, &enabled_types)) {
+    return enabled_types;
+  }
+
+  DLOG(WARNING) << "Invalid default value for param " << kTriggerTypesParamName;

[tschumann, 2017/01/10 14:28:16]

this should be a DCHECK(), no?
Actually, if you're going to hard-code a default, why not just hard-code the
enum values?

[jkrcal, 2017/01/10 18:52:27]

Done.

+  return std::set<TriggerType>();
+}
+
+bool SchedulingRemoteSuggestionsProvider::ParseEnabledTriggerTypes(
+    const std::string& param_value,
+    std::set<TriggerType>* out) {
+  static_assert(static_cast<unsigned int>(TriggerType::COUNT) ==
+                    arraysize(kTriggerTypeNames),
+                "Fill in names for trigger types.");

[tschumann, 2017/01/10 14:28:16]

hmm... i wonder if it wouldn't be cleaner to have a function:
TriggerTypeToConfigString() (or the like). That way there wouldn't be a chance
for getting out of sync as you could have a nice switch.

you could put the tokens in a set and simply go through the enum values and look
up their string-representations.

[jkrcal, 2017/01/10 18:52:27]

To me, it seems that I would then need to iterate over enum values. Is there a
clean way to do so? Otherwise, did you mean it differently?

[tschumann, 2017/01/12 12:44:20]

no, there's no way I know of to iterate over them.
the nice thing about an explicit mapping function is that mix-ups in values get
very visible. On the other hand, unit test coverage should also get this
ensured.

[jkrcal, 2017/01/12 15:35:28]

ok, I adapted the unit-tests as you proposed.

+
+  std::vector<std::string> tokens = base::SplitString(
+      param_value, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+  for (const auto& token : tokens) {
+    auto it = std::find(std::begin(kTriggerTypeNames),
+                        std::end(kTriggerTypeNames), token);
+    if (it == std::end(kTriggerTypeNames)) {
+      DLOG(WARNING) << "Unknown token " << token << " found in the parameter "
+                    << kTriggerTypesParamName << ".";
+      out->clear();

[tschumann, 2017/01/10 14:28:16]

this seems a bit brittle. Honestly, I wouldn't worry about the state of 'out' in
case the function returns 'false'. It's a common contract to leave output
parameters unspecified in case of errors.

If you insist, then I'd vote for clearing it at the beginning of the function.

[jkrcal, 2017/01/10 18:52:27]

Done.

+      return false;
+    }
+
+    // Add the enabled type represented by |token| into the result set.
+    out->insert(static_cast<TriggerType>(it - std::begin(kTriggerTypeNames)));
+  }
+  return true;
+}
+
 }  // namespace ntp_snippets