[Background fetching] Background fetching when opening an NTP.

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 7387410d8cc7b624f27103882229b4c152fd2094..b7bb990f849a64ffaf448d79ace972e30d16c6d6 100644
--- a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
+++ b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
@@ -4,6 +4,7 @@
 
 #include "components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h"
 
+#include <random>
 #include <string>
 #include <utility>
 
@@ -21,28 +22,47 @@ namespace ntp_snippets {
 
 namespace {
 
+enum class FetchingInterval {
+  BACKGROUND_FALLBACK,
+  BACKGROUND_WIFI,
+  SOFT_ACTIVE,

[tschumann, 2017/01/03 14:35:04]

hmm... what about: 'SOFT_ON_USAGE_EVENT' or just 'ON_USAGE_EVENT'?
We already have a notion of 'active' (--> active users) so overloading this
might be confusing.

[jkrcal, 2017/01/03 18:36:04]

Done.

+  MAX
+};
+
 // Default values for fetching intervals, fallback and wifi.
-const double kDefaultFetchingIntervalRareNtpUser[] = {48.0, 24.0};
-const double kDefaultFetchingIntervalActiveNtpUser[] = {24.0, 6.0};
-const double kDefaultFetchingIntervalActiveSuggestionsConsumer[] = {24.0, 6.0};
+const double kDefaultFetchingIntervalRareNtpUser[] = {48.0, 24.0, 12.0};
+const double kDefaultFetchingIntervalActiveNtpUser[] = {24.0, 6.0, 2.0};
+const double kDefaultFetchingIntervalActiveSuggestionsConsumer[] = {24.0, 6.0,
+                                                                    2.0};
 
 // Variation parameters than can the default fetching intervals.
 const char* kFetchingIntervalParamNameRareNtpUser[] = {
     "fetching_interval_hours-fallback-rare_ntp_user",
-    "fetching_interval_hours-wifi-rare_ntp_user"};
+    "fetching_interval_hours-wifi-rare_ntp_user",
+    "soft_fetching_interval_hours-active-rare_ntp_user"};
 const char* kFetchingIntervalParamNameActiveNtpUser[] = {
     "fetching_interval_hours-fallback-active_ntp_user",
-    "fetching_interval_hours-wifi-active_ntp_user"};
+    "fetching_interval_hours-wifi-active_ntp_user",
+    "soft_fetching_interval_hours-active-active_ntp_user"};
 const char* kFetchingIntervalParamNameActiveSuggestionsConsumer[] = {
     "fetching_interval_hours-fallback-active_suggestions_consumer",
-    "fetching_interval_hours-wifi-active_suggestions_consumer"};
+    "fetching_interval_hours-wifi-active_suggestions_consumer",
+    "soft_fetching_interval_hours-active-active_suggestions_consumer"};
 
-base::TimeDelta GetDesiredUpdateInterval(
-    bool is_wifi,
+// Default value for the maximum value of the random jitter that is added to the
+// soft fetching interval (the minimum value is 0.0).
+const double kDefaultSoftFetchingJitterInHours = 0.5;
+const char* kSoftFetchingJitterParamName =
+    "soft_fetching_interval_jitter_hours";
+
+base::TimeDelta GetDesiredFetchingInterval(
+    FetchingInterval interval,
     UserClassifier::UserClass user_class) {
   double default_value_hours = 0.0;
 
-  const int index = is_wifi ? 1 : 0;
+  const int index = static_cast<int>(interval);
+  DCHECK(index >=0 && index < static_cast<int>(FetchingInterval::MAX));

[tschumann, 2017/01/03 14:35:04]

hmm... i guess what you really want to check is:
DCHECK(interval != FetchingInterval::MAX);
and
DCHECK(index >=0 && index < arraysize(kDefaultFetchingIntervalRareNtpUser));

(and if you really care, compare against the other arrays as well).

[jkrcal, 2017/01/03 18:36:04]

Done.

+
   const char* param_name = nullptr;
   switch (user_class) {
     case UserClassifier::UserClass::RARE_NTP_USER:
@@ -67,20 +87,30 @@ base::TimeDelta GetDesiredUpdateInterval(
   return base::TimeDelta::FromSecondsD(value_hours * 3600.0);
 }
 
+base::TimeDelta GetDesiredFetchingJitter() {
+  double value_hours = variations::GetVariationParamByFeatureAsDouble(
+      ntp_snippets::kArticleSuggestionsFeature,
+      kSoftFetchingJitterParamName, kDefaultSoftFetchingJitterInHours);
+
+  return base::TimeDelta::FromSecondsD(value_hours * 3600.0);
+}
+
 }  // namespace
 
 struct SchedulingRemoteSuggestionsProvider::FetchingSchedule {
   base::TimeDelta interval_wifi;
   base::TimeDelta interval_fallback;
+  base::TimeDelta soft_interval_active;
 
   static FetchingSchedule Empty() {
-    return FetchingSchedule{base::TimeDelta(),
+    return FetchingSchedule{base::TimeDelta(), base::TimeDelta(),
                             base::TimeDelta()};
   }
 
   bool operator==(const FetchingSchedule& other) const {
     return interval_wifi == other.interval_wifi &&
-           interval_fallback == other.interval_fallback;
+           interval_fallback == other.interval_fallback &&
+           soft_interval_active == other.soft_interval_active;
   }
 
   bool operator!=(const FetchingSchedule& other) const {
@@ -88,7 +118,8 @@ struct SchedulingRemoteSuggestionsProvider::FetchingSchedule {
   }
 
   bool is_empty() const {
-    return interval_wifi.is_zero() && interval_fallback.is_zero();
+    return interval_wifi.is_zero() && interval_fallback.is_zero() &&
+           soft_interval_active.is_zero();
   }
 };
 
@@ -132,10 +163,7 @@ void SchedulingRemoteSuggestionsProvider::RescheduleFetching() {
 }
 
 void SchedulingRemoteSuggestionsProvider::OnPersistentSchedulerWakeUp() {
-  provider_->RefetchInTheBackground(
-      base::MakeUnique<RemoteSuggestionsProvider::FetchStatusCallback>(
-          base::Bind(&SchedulingRemoteSuggestionsProvider::OnFetchCompleted,
-                     base::Unretained(this))));
+  RefetchInTheBackground(nullptr);

[tschumann, 2017/01/03 15:16:13]

please add /*callback=*/nullptr
to document the parameter.
Same in other places below.

[jkrcal, 2017/01/03 18:36:04]

Done.

 }
 
 void SchedulingRemoteSuggestionsProvider::OnBrowserStartup() {
@@ -143,7 +171,10 @@ void SchedulingRemoteSuggestionsProvider::OnBrowserStartup() {
 }
 
 void SchedulingRemoteSuggestionsProvider::OnNTPOpened() {
-  // TODO(jkrcal): Implement.
+  if (!ShouldRefetchInTheBackgroundNow()) {
+    return;
+  }
+  RefetchInTheBackground(nullptr);
 }
 
 void SchedulingRemoteSuggestionsProvider::SetProviderStatusCallback(
@@ -153,7 +184,20 @@ void SchedulingRemoteSuggestionsProvider::SetProviderStatusCallback(
 
 void SchedulingRemoteSuggestionsProvider::RefetchInTheBackground(
     std::unique_ptr<FetchStatusCallback> callback) {
-  provider_->RefetchInTheBackground(std::move(callback));

[jkrcal, 2017/01/03 14:02:09]

This was a previous bug that OnFetchCompleted() (now
RefetchInTheBackgroundFinished()) was not called after this call.

+  if (background_fetch_in_progress_) {
+    if (callback) {
+      callback->Run(
+          Status(StatusCode::TEMPORARY_ERROR, "Background fetch in progress"));
+    }
+    return;
+  }
+
+  RemoteSuggestionsProvider::FetchStatusCallback wrapper_callback = base::Bind(
+          &SchedulingRemoteSuggestionsProvider::RefetchInTheBackgroundFinished,
+          base::Unretained(this), base::Passed(&callback));
+  provider_->RefetchInTheBackground(
+      base::MakeUnique<RemoteSuggestionsProvider::FetchStatusCallback>(
+          std::move(wrapper_callback)));
 }
 
 const NTPSnippetsFetcher* SchedulingRemoteSuggestionsProvider::
@@ -237,41 +281,52 @@ void SchedulingRemoteSuggestionsProvider::OnProviderStatusChanged(
 }
 
 void SchedulingRemoteSuggestionsProvider::StartScheduling() {
-  // The scheduler only exists on Android so far, it's null on other platforms.
-  if (!persistent_scheduler_) {
-    return;
-  }
-
   FetchingSchedule last_schedule = GetLastFetchingSchedule();
   FetchingSchedule schedule = GetDesiredFetchingSchedule();
 
   // Reset the schedule only if the parameters have changed.
   if (last_schedule != schedule) {
-    ApplyFetchingSchedule(schedule);
+    ApplyFetchingSchedule(schedule, /*also_apply_persistent_schedule=*/true);
   }
 }
 
 void SchedulingRemoteSuggestionsProvider::StopScheduling() {
-  // The scheduler only exists on Android so far, it's null on other platforms.
-  if (!persistent_scheduler_) {
-    return;
-  }
-
   // Do not unschedule if already switched off
   FetchingSchedule last_schedule = GetLastFetchingSchedule();
   if (last_schedule.is_empty()) {
     return;
   }
 
-  persistent_scheduler_->Unschedule();
+  pref_service_->ClearPref(prefs::kSnippetNextActiveSoftFetch);
+
+  // The scheduler only exists on Android so far, it's null on other platforms.
+  if (persistent_scheduler_) {
+    persistent_scheduler_->Unschedule();
+  }
 
   StoreLastFetchingSchedule(FetchingSchedule::Empty());
 }
 
 void SchedulingRemoteSuggestionsProvider::ApplyFetchingSchedule(
-    const FetchingSchedule& schedule) {
-  persistent_scheduler_->Schedule(schedule.interval_wifi,
-                                  schedule.interval_fallback);
+    const FetchingSchedule& schedule,
+    bool also_apply_persistent_schedule) {
+
+  std::default_random_engine generator;
+  std::uniform_real_distribution<double> distribution(0.0,1.0);
+  // generates a random double in the range 0.0 .. 1.0
+  double random_factor = distribution(generator);
+
+  base::Time next_active_soft_fetch =
+      base::Time::Now() + schedule.soft_interval_active +
+      (GetDesiredFetchingJitter() * random_factor);
+  pref_service_->SetInt64(prefs::kSnippetNextActiveSoftFetch,
+                          next_active_soft_fetch.ToInternalValue());
+
+  // The scheduler only exists on Android so far, it's null on other platforms.
+  if (persistent_scheduler_ && also_apply_persistent_schedule) {
+    persistent_scheduler_->Schedule(schedule.interval_wifi,
+                                    schedule.interval_fallback);
+  }
 
   StoreLastFetchingSchedule(schedule);
 }
@@ -282,9 +337,12 @@ SchedulingRemoteSuggestionsProvider::GetDesiredFetchingSchedule() const {
 
   FetchingSchedule schedule;
   schedule.interval_wifi =
-      GetDesiredUpdateInterval(/*is_wifi=*/true, user_class);
-  schedule.interval_fallback =
-      GetDesiredUpdateInterval(/*is_wifi=*/false, user_class);
+      GetDesiredFetchingInterval(FetchingInterval::BACKGROUND_WIFI, user_class);
+  schedule.interval_fallback = GetDesiredFetchingInterval(
+      FetchingInterval::BACKGROUND_FALLBACK, user_class);
+  schedule.soft_interval_active =
+      GetDesiredFetchingInterval(FetchingInterval::SOFT_ACTIVE, user_class);
+
   return schedule;
 }
 
@@ -296,6 +354,9 @@ SchedulingRemoteSuggestionsProvider::GetLastFetchingSchedule() const {
   schedule.interval_fallback =
       base::TimeDelta::FromInternalValue(pref_service_->GetInt64(
           prefs::kSnippetBackgroundFetchingIntervalFallback));
+  schedule.soft_interval_active = base::TimeDelta::FromInternalValue(
+      pref_service_->GetInt64(prefs::kSnippetSoftFetchingIntervalActive));
+
   return schedule;
 }
 
@@ -307,32 +368,53 @@ void SchedulingRemoteSuggestionsProvider::StoreLastFetchingSchedule(
   pref_service_->SetInt64(
       prefs::kSnippetBackgroundFetchingIntervalFallback,
       schedule.interval_fallback.ToInternalValue());
+  pref_service_->SetInt64(
+      prefs::kSnippetSoftFetchingIntervalActive,
+      schedule.soft_interval_active.ToInternalValue());
+}
+
+bool SchedulingRemoteSuggestionsProvider::ShouldRefetchInTheBackgroundNow() {
+  if (!pref_service_->HasPrefPath(prefs::kSnippetNextActiveSoftFetch)) {

[tschumann, 2017/01/03 15:16:13]

i guess this checks whether the path has been registered. Do we do this
somewhere?

[jkrcal, 2017/01/03 18:36:04]

No, a path should always be registered (I've forgotten to do so, thanks for
pointing out). Here, I check that something has been stored there. I use it as a
signal that background fetching is switched on / off.

[tschumann, 2017/01/03 18:54:24]

Ah, got it -- that's what ClearPref() is doing :-) It might be worth adding a
comment to line 299 explaining that this is not only cleaning up but also
signalling that soft fetches are stopped.

[jkrcal, 2017/01/04 10:06:30]

I fixed it, in the end.

+    // Soft background fetches are switched off.
+    return false;
+  }
+
+  base::Time next_active_soft_fetch = base::Time::FromInternalValue(
+      pref_service_->GetInt64(prefs::kSnippetNextActiveSoftFetch));
+  return next_active_soft_fetch <= base::Time::Now();
 }
 
 void SchedulingRemoteSuggestionsProvider::FetchFinished(
     const FetchDoneCallback& callback,
-    Status status_code,
+    Status fetch_status,
     std::vector<ContentSuggestion> suggestions) {
-  OnFetchCompleted(status_code);
-  callback.Run(status_code, std::move(suggestions));
+  // 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).
+  ApplyFetchingSchedule(GetLastFetchingSchedule(),
+                        /*also_apply_persistent_schedule=*/fetch_status.code ==
+                            StatusCode::SUCCESS);
+
+  callback.Run(fetch_status, std::move(suggestions));
 }
 
-void SchedulingRemoteSuggestionsProvider::OnFetchCompleted(
+void SchedulingRemoteSuggestionsProvider::RefetchInTheBackgroundFinished(
+    std::unique_ptr<FetchStatusCallback> callback,
     Status fetch_status) {
-  // The scheduler only exists on Android so far, it's null on other platforms.
-  if (!persistent_scheduler_) {
-    return;
-  }
-
-  if (fetch_status.code != StatusCode::SUCCESS) {
-    return;
+  background_fetch_in_progress_ = false;
+
+  // 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).
+  ApplyFetchingSchedule(GetLastFetchingSchedule(),
+                        /*also_apply_persistent_schedule=*/fetch_status.code ==
+                            StatusCode::SUCCESS);
+
+  if (callback) {
+    callback->Run(fetch_status);
   }
-
-  // Reschedule after a successful fetch. This resets all currently scheduled
-  // fetches, to make sure the fallback interval triggers only if no wifi fetch
-  // succeeded, and also that we don't do a background fetch immediately after
-  // a user-initiated one.
-  ApplyFetchingSchedule(GetLastFetchingSchedule());
 }
 
 }  // namespace ntp_snippets