[Background fetching] Configure and report fetching triggers

components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc

Index: components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
diff --git a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
index da41c57c2008cba6287bb1f4b89684c8a79e956e..23a75b4f699855b648a1b7b4c51c81b9470dd9c8 100644
--- a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
+++ b/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,14 @@ 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* kDefaultTriggerTypes =
+    "persistent_scheduler_wake_up,ntp_opened,browser_foregrounded";
+
 base::TimeDelta GetDesiredFetchingInterval(
     FetchingInterval interval,
     UserClassifier::UserClass user_class) {
@@ -125,6 +135,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 +160,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 +195,27 @@ 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.
+  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(
@@ -376,8 +394,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 +415,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 +462,53 @@ void SchedulingRemoteSuggestionsProvider::OnFetchCompleted(
   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