[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..c4c86b1a027e7cc1a7b5a3e267bc0055c3289ffe 100644
--- a/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
+++ b/components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc
@@ -4,10 +4,12 @@
 
 #include "components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h"
 
+#include <random>
 #include <string>
 #include <utility>
 
 #include "base/memory/ptr_util.h"
+#include "base/time/default_clock.h"
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/persistent_scheduler.h"
@@ -21,28 +23,57 @@ namespace ntp_snippets {
 
 namespace {
 
+enum class FetchingInterval {
+  PERSISTENT_FALLBACK,
+  PERSISTENT_WIFI,
+  SOFT_ON_USAGE_EVENT,
+  COUNT
+};
+
 // 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"};
-
-base::TimeDelta GetDesiredUpdateInterval(
-    bool is_wifi,
+    "fetching_interval_hours-wifi-active_suggestions_consumer",
+    "soft_fetching_interval_hours-active-active_suggestions_consumer"};
+
+static_assert(
+    static_cast<unsigned int>(FetchingInterval::COUNT) ==
+            arraysize(kDefaultFetchingIntervalRareNtpUser) &&
+        static_cast<unsigned int>(FetchingInterval::COUNT) ==
+            arraysize(kDefaultFetchingIntervalActiveNtpUser) &&
+        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.");
+
+base::TimeDelta GetDesiredFetchingInterval(
+    FetchingInterval interval,
     UserClassifier::UserClass user_class) {
   double default_value_hours = 0.0;
 
-  const int index = is_wifi ? 1 : 0;
+  DCHECK(interval != FetchingInterval::COUNT);
+  const unsigned int index = static_cast<unsigned int>(interval);
+  DCHECK(index < arraysize(kDefaultFetchingIntervalRareNtpUser));
+
   const char* param_name = nullptr;
   switch (user_class) {
     case UserClassifier::UserClass::RARE_NTP_USER:
@@ -69,28 +100,30 @@ base::TimeDelta GetDesiredUpdateInterval(
 
 }  // namespace
 
-struct SchedulingRemoteSuggestionsProvider::FetchingSchedule {
-  base::TimeDelta interval_wifi;
-  base::TimeDelta interval_fallback;
-
-  static FetchingSchedule Empty() {
-    return FetchingSchedule{base::TimeDelta(),
-                            base::TimeDelta()};
-  }
+// static
+SchedulingRemoteSuggestionsProvider::FetchingSchedule
+SchedulingRemoteSuggestionsProvider::FetchingSchedule::Empty() {
+  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;
-  }
+bool SchedulingRemoteSuggestionsProvider::FetchingSchedule::operator==(
+    const FetchingSchedule& other) const {
+  return interval_persistent_wifi == other.interval_persistent_wifi &&
+         interval_persistent_fallback == other.interval_persistent_fallback &&
+         interval_soft_on_usage_event == other.interval_soft_on_usage_event;
+}
 
-  bool operator!=(const FetchingSchedule& other) const {
-    return !operator==(other);
-  }
+bool SchedulingRemoteSuggestionsProvider::FetchingSchedule::operator!=(
+    const FetchingSchedule& other) const {
+  return !operator==(other);
+}
 
-  bool is_empty() const {
-    return interval_wifi.is_zero() && interval_fallback.is_zero();
-  }
-};
+bool SchedulingRemoteSuggestionsProvider::FetchingSchedule::is_empty() const {
+  return interval_persistent_wifi.is_zero() &&
+         interval_persistent_fallback.is_zero() &&
+         interval_soft_on_usage_event.is_zero();
+}
 
 SchedulingRemoteSuggestionsProvider::SchedulingRemoteSuggestionsProvider(
     Observer* observer,
@@ -102,11 +135,15 @@ SchedulingRemoteSuggestionsProvider::SchedulingRemoteSuggestionsProvider(
       RemoteSuggestionsScheduler(),
       provider_(std::move(provider)),
       persistent_scheduler_(persistent_scheduler),
+      background_fetch_in_progress_(false),
       user_classifier_(user_classifier),
-      pref_service_(pref_service) {
+      pref_service_(pref_service),
+      clock_(base::MakeUnique<base::DefaultClock>()) {
   DCHECK(user_classifier);
   DCHECK(pref_service);
 
+  LoadLastFetchingSchedule();
+
   provider_->SetProviderStatusCallback(
       base::MakeUnique<RemoteSuggestionsProvider::ProviderStatusCallback>(
           base::BindRepeating(
@@ -120,9 +157,12 @@ SchedulingRemoteSuggestionsProvider::~SchedulingRemoteSuggestionsProvider() =
 // static
 void SchedulingRemoteSuggestionsProvider::RegisterProfilePrefs(
     PrefRegistrySimple* registry) {
-  registry->RegisterInt64Pref(prefs::kSnippetBackgroundFetchingIntervalWifi, 0);
-  registry->RegisterInt64Pref(prefs::kSnippetBackgroundFetchingIntervalFallback,
+  registry->RegisterInt64Pref(prefs::kSnippetPersistentFetchingIntervalWifi, 0);
+  registry->RegisterInt64Pref(prefs::kSnippetPersistentFetchingIntervalFallback,
                               0);
+  registry->RegisterInt64Pref(prefs::kSnippetSoftFetchingIntervalOnUsageEvent,
+                              0);
+  registry->RegisterInt64Pref(prefs::kSnippetLastFetchAttempt, 0);
 }
 
 void SchedulingRemoteSuggestionsProvider::RescheduleFetching() {

[tschumann, 2017/01/04 10:43:52]

ultimatively, this method should go away, right? (why would somebody tell the
scheduler to reschedule). Maybe add a TODO()?.

[jkrcal, 2017/01/04 14:19:03]

No. This needs to be called when Chrome is updated. At that moment, existing
persistent tasks are deleted while Chrome thinks (based on prefs) they are
running.

[tschumann, 2017/01/04 15:15:24]

I see. Maybe we should rename this function then to tell the scheduler what
happend (chrome-update) instead of what it should do to its internal state
(reset the persistent scheduler).

@@ -132,10 +172,11 @@ void SchedulingRemoteSuggestionsProvider::RescheduleFetching() {
 }
 
 void SchedulingRemoteSuggestionsProvider::OnPersistentSchedulerWakeUp() {
-  provider_->RefetchInTheBackground(
-      base::MakeUnique<RemoteSuggestionsProvider::FetchStatusCallback>(
-          base::Bind(&SchedulingRemoteSuggestionsProvider::OnFetchCompleted,
-                     base::Unretained(this))));
+  if (schedule_.is_empty()) {

[tschumann, 2017/01/04 10:43:52]

can we wrap this in a helper method BackgroundFetchesDisabled()?
This would remove the necessity of the comments below, which is usually a nice
indicator for introducing helper functions.

[jkrcal, 2017/01/04 14:19:03]

Done.

+    return;  // Persistent background fetches are switched off.
+  }
+
+  RefetchInTheBackground(/*callback=*/nullptr);
 }
 
 void SchedulingRemoteSuggestionsProvider::OnBrowserStartup() {
@@ -143,17 +184,38 @@ void SchedulingRemoteSuggestionsProvider::OnBrowserStartup() {
 }
 
 void SchedulingRemoteSuggestionsProvider::OnNTPOpened() {
-  // TODO(jkrcal): Implement.
+  if (schedule_.is_empty()) {
+    return;  // Soft background fetches are switched off.
+  }
+
+  if (!ShouldRefetchInTheBackgroundNow()) {
+    return;  // Not enough time has elapsed since last fetch attempt.

[tschumann, 2017/01/04 10:43:52]

nit: let's remove this comment as it duplicates logic -- at this point we should
only care whether we should fetch or not. The helper function owns the logic
(and the reasons) for why we shouldn't.

[jkrcal, 2017/01/04 14:19:03]

Good point, done.

+  }
+
+  RefetchInTheBackground(/*callback=*/nullptr);
 }
 
 void SchedulingRemoteSuggestionsProvider::SetProviderStatusCallback(
-      std::unique_ptr<ProviderStatusCallback> callback) {
+    std::unique_ptr<ProviderStatusCallback> callback) {
   provider_->SetProviderStatusCallback(std::move(callback));
 }
 
 void SchedulingRemoteSuggestionsProvider::RefetchInTheBackground(
     std::unique_ptr<FetchStatusCallback> callback) {
-  provider_->RefetchInTheBackground(std::move(callback));
+  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,43 +299,40 @@ void SchedulingRemoteSuggestionsProvider::OnProviderStatusChanged(
 }
 
 void SchedulingRemoteSuggestionsProvider::StartScheduling() {
-  // The scheduler only exists on Android so far, it's null on other platforms.
-  if (!persistent_scheduler_) {
+  FetchingSchedule new_schedule = GetDesiredFetchingSchedule();
+
+  if (schedule_ == new_schedule) {
+    // Do not schedule if nothing has changed;
     return;
   }
 
-  FetchingSchedule last_schedule = GetLastFetchingSchedule();
-  FetchingSchedule schedule = GetDesiredFetchingSchedule();
-
-  // Reset the schedule only if the parameters have changed.
-  if (last_schedule != schedule) {
-    ApplyFetchingSchedule(schedule);
-  }
+  schedule_ = new_schedule;
+  StoreFetchingSchedule();
+  ApplyFetchingSchedule(/*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_) {
+  if (schedule_.is_empty()) {
+    // Do not unschedule if already switched off.
     return;
   }
 
-  // Do not unschedule if already switched off
-  FetchingSchedule last_schedule = GetLastFetchingSchedule();
-  if (last_schedule.is_empty()) {
-    return;
-  }
-
-  persistent_scheduler_->Unschedule();
-
-  StoreLastFetchingSchedule(FetchingSchedule::Empty());
+  schedule_ = FetchingSchedule::Empty();
+  StoreFetchingSchedule();
+  ApplyFetchingSchedule(/*also_apply_persistent_schedule=*/true);
 }
 
 void SchedulingRemoteSuggestionsProvider::ApplyFetchingSchedule(

[tschumann, 2017/01/04 10:43:52]

Rename to ApplyPersistentFetchingSchedule and get rid of the parameter?

[jkrcal, 2017/01/04 14:19:03]

Done.

-    const FetchingSchedule& schedule) {
-  persistent_scheduler_->Schedule(schedule.interval_wifi,
-                                  schedule.interval_fallback);
-
-  StoreLastFetchingSchedule(schedule);
+    bool also_apply_persistent_schedule) {
+  // The scheduler only exists on Android so far, it's null on other platforms.
+  if (persistent_scheduler_ && also_apply_persistent_schedule) {
+    if (schedule_.is_empty()) {
+      persistent_scheduler_->Unschedule();
+    } else {
+      persistent_scheduler_->Schedule(schedule_.interval_persistent_wifi,
+                                      schedule_.interval_persistent_fallback);
+    }
+  }
 }
 
 SchedulingRemoteSuggestionsProvider::FetchingSchedule
@@ -281,58 +340,75 @@ SchedulingRemoteSuggestionsProvider::GetDesiredFetchingSchedule() const {
   UserClassifier::UserClass user_class = user_classifier_->GetUserClass();
 
   FetchingSchedule schedule;
-  schedule.interval_wifi =
-      GetDesiredUpdateInterval(/*is_wifi=*/true, user_class);
-  schedule.interval_fallback =
-      GetDesiredUpdateInterval(/*is_wifi=*/false, user_class);
+  schedule.interval_persistent_wifi =
+      GetDesiredFetchingInterval(FetchingInterval::PERSISTENT_WIFI, user_class);
+  schedule.interval_persistent_fallback = GetDesiredFetchingInterval(
+      FetchingInterval::PERSISTENT_FALLBACK, user_class);
+  schedule.interval_soft_on_usage_event = GetDesiredFetchingInterval(
+      FetchingInterval::SOFT_ON_USAGE_EVENT, user_class);
+
   return schedule;
 }
 
-SchedulingRemoteSuggestionsProvider::FetchingSchedule
-SchedulingRemoteSuggestionsProvider::GetLastFetchingSchedule() const {
-  FetchingSchedule schedule;
-  schedule.interval_wifi = base::TimeDelta::FromInternalValue(
-      pref_service_->GetInt64(prefs::kSnippetBackgroundFetchingIntervalWifi));
-  schedule.interval_fallback =
+void SchedulingRemoteSuggestionsProvider::LoadLastFetchingSchedule() {
+  schedule_.interval_persistent_wifi = base::TimeDelta::FromInternalValue(
+      pref_service_->GetInt64(prefs::kSnippetPersistentFetchingIntervalWifi));
+  schedule_.interval_persistent_fallback =
       base::TimeDelta::FromInternalValue(pref_service_->GetInt64(
-          prefs::kSnippetBackgroundFetchingIntervalFallback));
-  return schedule;
+          prefs::kSnippetPersistentFetchingIntervalFallback));
+  schedule_.interval_soft_on_usage_event = base::TimeDelta::FromInternalValue(
+      pref_service_->GetInt64(prefs::kSnippetSoftFetchingIntervalOnUsageEvent));
 }
 
-void SchedulingRemoteSuggestionsProvider::StoreLastFetchingSchedule(
-    const FetchingSchedule& schedule) {
+void SchedulingRemoteSuggestionsProvider::StoreFetchingSchedule() {
+  pref_service_->SetInt64(prefs::kSnippetPersistentFetchingIntervalWifi,
+                          schedule_.interval_persistent_wifi.ToInternalValue());
   pref_service_->SetInt64(
-      prefs::kSnippetBackgroundFetchingIntervalWifi,
-      schedule.interval_wifi.ToInternalValue());
+      prefs::kSnippetPersistentFetchingIntervalFallback,
+      schedule_.interval_persistent_fallback.ToInternalValue());
   pref_service_->SetInt64(
-      prefs::kSnippetBackgroundFetchingIntervalFallback,
-      schedule.interval_fallback.ToInternalValue());
+      prefs::kSnippetSoftFetchingIntervalOnUsageEvent,
+      schedule_.interval_soft_on_usage_event.ToInternalValue());
+}
+
+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();
 }
 
 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));
+  OnFetchCompleted(fetch_status);
+  callback.Run(fetch_status, std::move(suggestions));

[tschumann, 2017/01/04 10:43:52]

should we also check for the empty callback?

[jkrcal, 2017/01/04 14:19:03]

Done.

 }
 
-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;
-  }
+  background_fetch_in_progress_ = false;
+  OnFetchCompleted(fetch_status);
 
-  if (fetch_status.code != StatusCode::SUCCESS) {
-    return;
+  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());
+void SchedulingRemoteSuggestionsProvider::OnFetchCompleted(
+    Status fetch_status) {
+  pref_service_->SetInt64(prefs::kSnippetLastFetchAttempt,
+                          clock_->Now().ToInternalValue());
+
+  // 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(/*also_apply_persistent_schedule=*/fetch_status.code ==
+                        StatusCode::SUCCESS);
 }
 
 }  // namespace ntp_snippets