[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/remote/remote_suggestions_provider.cc

Index: components/ntp_snippets/remote/remote_suggestions_provider.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_provider.cc b/components/ntp_snippets/remote/remote_suggestions_provider.cc
index 8201c0f97c5b8b6ad6b7698c7176de9a919b3ad9..8febe7abd544c2bf9642240b52874daa1200b736 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider.cc
@@ -31,7 +31,6 @@
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
 #include "components/ntp_snippets/switches.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"
@@ -50,22 +49,6 @@ const int kMaxSnippetCount = 10;
 // Number of archived snippets we keep around in memory.
 const int kMaxArchivedSnippetCount = 200;
 
-// 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};
-
-// Variation parameters than can override the default fetching intervals.
-const char* kFetchingIntervalParamNameRareNtpUser[] = {
-    "fetching_interval_hours-fallback-rare_ntp_user",
-    "fetching_interval_hours-wifi-rare_ntp_user"};
-const char* kFetchingIntervalParamNameActiveNtpUser[] = {
-    "fetching_interval_hours-fallback-active_ntp_user",
-    "fetching_interval_hours-wifi-active_ntp_user"};
-const char* kFetchingIntervalParamNameActiveSuggestionsConsumer[] = {
-    "fetching_interval_hours-fallback-active_suggestions_consumer",
-    "fetching_interval_hours-wifi-active_suggestions_consumer"};
-
 // Keys for storing CategoryContent info in prefs.
 const char kCategoryContentId[] = "id";
 const char kCategoryContentTitle[] = "title";
@@ -75,42 +58,6 @@ const char kCategoryContentAllowFetchingMore[] = "allow_fetching_more";
 // TODO(treib): Remove after M57.
 const char kDeprecatedSnippetHostsPref[] = "ntp_snippets.hosts";
 
-base::TimeDelta GetFetchingInterval(bool is_wifi,
-                                    UserClassifier::UserClass user_class) {
-  double value_hours = 0.0;
-
-  const int index = is_wifi ? 1 : 0;
-  const char* param_name = "";
-  switch (user_class) {
-    case UserClassifier::UserClass::RARE_NTP_USER:
-      value_hours = kDefaultFetchingIntervalRareNtpUser[index];
-      param_name = kFetchingIntervalParamNameRareNtpUser[index];
-      break;
-    case UserClassifier::UserClass::ACTIVE_NTP_USER:
-      value_hours = kDefaultFetchingIntervalActiveNtpUser[index];
-      param_name = kFetchingIntervalParamNameActiveNtpUser[index];
-      break;
-    case UserClassifier::UserClass::ACTIVE_SUGGESTIONS_CONSUMER:
-      value_hours = kDefaultFetchingIntervalActiveSuggestionsConsumer[index];
-      param_name = kFetchingIntervalParamNameActiveSuggestionsConsumer[index];
-      break;
-  }
-
-  // The default value can be overridden by a variation parameter.
-  std::string param_value_str = variations::GetVariationParamValueByFeature(
-      ntp_snippets::kArticleSuggestionsFeature, param_name);
-  if (!param_value_str.empty()) {
-    double param_value_hours = 0.0;
-    if (base::StringToDouble(param_value_str, &param_value_hours)) {
-      value_hours = param_value_hours;
-    } else {
-      LOG(WARNING) << "Invalid value for variation parameter " << param_name;
-    }
-  }
-
-  return base::TimeDelta::FromSecondsD(value_hours * 3600.0);
-}
-
 std::unique_ptr<std::vector<std::string>> GetSnippetIDVector(
     const NTPSnippet::PtrVector& snippets) {
   auto result = base::MakeUnique<std::vector<std::string>>();
@@ -227,8 +174,6 @@ RemoteSuggestionsProvider::RemoteSuggestionsProvider(
     CategoryFactory* category_factory,
     PrefService* pref_service,
     const std::string& application_language_code,
-    const UserClassifier* user_classifier,
-    NTPSnippetsScheduler* scheduler,
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
     std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
     std::unique_ptr<image_fetcher::ImageDecoder> image_decoder,
@@ -240,14 +185,13 @@ RemoteSuggestionsProvider::RemoteSuggestionsProvider(
       articles_category_(
           category_factory->FromKnownCategory(KnownCategories::ARTICLES)),
       application_language_code_(application_language_code),
-      user_classifier_(user_classifier),
-      scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
       image_fetcher_(std::move(image_fetcher)),
       image_decoder_(std::move(image_decoder)),
       database_(std::move(database)),
       status_service_(std::move(status_service)),
       fetch_when_ready_(false),
+      fetch_when_ready_interactive_(false),
       nuke_when_initialized_(false),
       thumbnail_requests_throttler_(
           pref_service,
@@ -290,29 +234,51 @@ void RemoteSuggestionsProvider::RegisterProfilePrefs(
   // TODO(treib): Remove after M57.
   registry->RegisterListPref(kDeprecatedSnippetHostsPref);
   registry->RegisterListPref(prefs::kRemoteSuggestionCategories);
-  registry->RegisterInt64Pref(prefs::kSnippetBackgroundFetchingIntervalWifi, 0);
-  registry->RegisterInt64Pref(prefs::kSnippetBackgroundFetchingIntervalFallback,
-                              0);
   registry->RegisterInt64Pref(prefs::kLastSuccessfulBackgroundFetchTime, 0);
 
   RemoteSuggestionsStatusService::RegisterProfilePrefs(registry);
 }
 
-void RemoteSuggestionsProvider::FetchSnippetsInTheBackground() {
-  FetchSnippets(/*interactive_request=*/false);
+void RemoteSuggestionsProvider::RegisterActivenessObserver(
+    const ProviderActiveCallback& callback) {
+  provider_active_callback_ = callback;
+
+  // Call the observer right away if we've reached any final state.
+  switch (state_) {
+    case State::NOT_INITED:
+      // Initial state, not sure yet whether active or not.
+      break;
+    case State::READY:
+      provider_active_callback_.Run(/*active=*/true);
+      break;
+    case State::DISABLED:
+      provider_active_callback_.Run(/*active=*/false);
+      break;
+    case State::ERROR_OCCURRED:
+      provider_active_callback_.Run(/*active=*/false);
+      break;
+    case State::COUNT:
+      NOTREACHED();
+      break;
+  }
+}
+
+void RemoteSuggestionsProvider::ReloadSuggestions() {
+  FetchSnippets(/*interactive_request=*/true, FetchStatusCallback());
 }
 
-void RemoteSuggestionsProvider::FetchSnippetsForAllCategories() {
-  // TODO(markusheintz): Investigate whether we can call the Fetch method
-  // instead of the FetchSnippets.
-  FetchSnippets(/*interactive_request=*/true);
+void RemoteSuggestionsProvider::RefetchInTheBackground(
+    const FetchStatusCallback& callback) {
+  FetchSnippets(/*interactive_request=*/false, callback);
 }
 
 void RemoteSuggestionsProvider::FetchSnippets(
-    bool interactive_request) {
+    bool interactive_request,
+    const FetchStatusCallback& callback) {
   if (!ready()) {
     fetch_when_ready_ = true;
-    return;
+    fetch_when_ready_interactive_ = interactive_request;
+    fetch_when_ready_callback_ = callback;
   }
 
   MarkEmptyCategoriesAsLoading();
@@ -320,8 +286,9 @@ void RemoteSuggestionsProvider::FetchSnippets(
   NTPSnippetsFetcher::Params params = BuildFetchParams();
   params.interactive_request = interactive_request;
   snippets_fetcher_->FetchSnippets(
-      params, base::BindOnce(&RemoteSuggestionsProvider::OnFetchFinished,
-                             base::Unretained(this), interactive_request));
+      params,
+      base::BindOnce(&RemoteSuggestionsProvider::OnFetchFinished,
+                     base::Unretained(this), callback, interactive_request));
 }
 
 void RemoteSuggestionsProvider::Fetch(
@@ -372,45 +339,6 @@ void RemoteSuggestionsProvider::MarkEmptyCategoriesAsLoading() {
   }
 }
 
-void RemoteSuggestionsProvider::RescheduleFetching(bool force) {
-  // The scheduler only exists on Android so far, it's null on other platforms.
-  if (!scheduler_) {
-    return;
-  }
-
-  if (ready()) {
-    base::TimeDelta old_interval_wifi = base::TimeDelta::FromInternalValue(
-        pref_service_->GetInt64(prefs::kSnippetBackgroundFetchingIntervalWifi));
-    base::TimeDelta old_interval_fallback =
-        base::TimeDelta::FromInternalValue(pref_service_->GetInt64(
-            prefs::kSnippetBackgroundFetchingIntervalFallback));
-    UserClassifier::UserClass user_class = user_classifier_->GetUserClass();
-    base::TimeDelta interval_wifi =
-        GetFetchingInterval(/*is_wifi=*/true, user_class);
-    base::TimeDelta interval_fallback =
-        GetFetchingInterval(/*is_wifi=*/false, user_class);
-    if (force || interval_wifi != old_interval_wifi ||
-        interval_fallback != old_interval_fallback) {
-      scheduler_->Schedule(interval_wifi, interval_fallback);
-      pref_service_->SetInt64(prefs::kSnippetBackgroundFetchingIntervalWifi,
-                              interval_wifi.ToInternalValue());
-      pref_service_->SetInt64(prefs::kSnippetBackgroundFetchingIntervalFallback,
-                              interval_fallback.ToInternalValue());
-    }
-  } else {
-    // If we're NOT_INITED, we don't know whether to schedule or unschedule.
-    // If |force| is false, all is well: We'll reschedule on the next state
-    // change anyway. If it's true, then unschedule here, to make sure that the
-    // next reschedule actually happens.
-    if (state_ != State::NOT_INITED || force) {
-      scheduler_->Unschedule();
-      pref_service_->ClearPref(prefs::kSnippetBackgroundFetchingIntervalWifi);
-      pref_service_->ClearPref(
-          prefs::kSnippetBackgroundFetchingIntervalFallback);
-    }
-  }
-}
-
 CategoryStatus RemoteSuggestionsProvider::GetCategoryStatus(Category category) {
   auto content_it = category_contents_.find(category);
   DCHECK(content_it != category_contents_.end());
@@ -634,7 +562,8 @@ void RemoteSuggestionsProvider::OnFetchMoreFinished(
     NTPSnippetsFetcher::FetchResult fetch_result,
     NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
   if (!fetched_categories) {
-    // TODO(fhorschig): Disambiguate the kind of error that led here.
+    // TODO(fhorschig): Disambiguate the kind of error that led here. Use a
+    // common function here and in OnFetchFinished().
     CallWithEmptyResults(fetching_callback,
                          Status(StatusCode::PERMANENT_ERROR,
                                 "The NTPSnippetsFetcher did not "
@@ -693,6 +622,7 @@ void RemoteSuggestionsProvider::OnFetchMoreFinished(
 }
 
 void RemoteSuggestionsProvider::OnFetchFinished(
+    const FetchStatusCallback& callback,
     bool interactive_request,
     NTPSnippetsFetcher::FetchResult fetch_result,
     NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
@@ -768,12 +698,14 @@ void RemoteSuggestionsProvider::OnFetchFinished(
                          content.dismissed.size());
   }
 
-  // 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.
-  if (fetched_categories) {
-    RescheduleFetching(true);
+  if (fetch_result == NTPSnippetsFetcher::FetchResult::SUCCESS) {
+    callback.Run(Status(StatusCode::SUCCESS));
+  } else {
+    // TODO(fhorschig): Disambiguate the kind of error that led here. Use a
+    // common function here and in OnFetchMoreFinished().
+    callback.Run(Status(StatusCode::PERMANENT_ERROR,
+                        "The NTPSnippetsFetcher did not "
+                        "complete the fetching successfully."));
   }
 }
 
@@ -1015,7 +947,7 @@ void RemoteSuggestionsProvider::EnterStateReady() {
     // Either add a DCHECK here that we actually are allowed to do network I/O
     // or change the logic so that some explicit call is always needed for the
     // network request.
-    FetchSnippets(/*interactive_request=*/false);
+    FetchSnippets(fetch_when_ready_interactive_, fetch_when_ready_callback_);
     fetch_when_ready_ = false;
   }
 
@@ -1081,7 +1013,8 @@ void RemoteSuggestionsProvider::OnStatusChanged(
         // Clear nonpersonalized suggestions.
         NukeAllSnippets();
         // Fetch personalized ones.
-        FetchSnippets(/*interactive_request=*/true);
+        // TODO(jkrcal): Loop in SchedulingRemoteSuggestionsProvider somehow.
+        FetchSnippets(/*interactive_request=*/true, FetchStatusCallback());
       } else {
         // Do not change the status. That will be done in EnterStateReady().
         EnterState(State::READY);
@@ -1094,7 +1027,8 @@ void RemoteSuggestionsProvider::OnStatusChanged(
         // Clear personalized suggestions.
         NukeAllSnippets();
         // Fetch nonpersonalized ones.
-        FetchSnippets(/*interactive_request=*/true);
+        // TODO(jkrcal): Loop in SchedulingRemoteSuggestionsProvider somehow.
+        FetchSnippets(/*interactive_request=*/true, FetchStatusCallback());
       } else {
         // Do not change the status. That will be done in EnterStateReady().
         EnterState(State::READY);
@@ -1134,6 +1068,7 @@ void RemoteSuggestionsProvider::EnterState(State state) {
       DVLOG(1) << "Entering state: READY";
       state_ = State::READY;
       EnterStateReady();
+      provider_active_callback_.Run(/*active=*/true);
       break;
 
     case State::DISABLED:
@@ -1142,21 +1077,20 @@ void RemoteSuggestionsProvider::EnterState(State state) {
       DVLOG(1) << "Entering state: DISABLED";
       state_ = State::DISABLED;
       EnterStateDisabled();
+      provider_active_callback_.Run(/*active=*/false);
       break;
 
     case State::ERROR_OCCURRED:
       DVLOG(1) << "Entering state: ERROR_OCCURRED";
       state_ = State::ERROR_OCCURRED;
       EnterStateError();
+      provider_active_callback_.Run(/*active=*/false);
       break;
 
     case State::COUNT:
       NOTREACHED();
       break;
   }
-
-  // Schedule or un-schedule background fetching after each state change.
-  RescheduleFetching(false);
 }
 
 void RemoteSuggestionsProvider::NotifyNewSuggestions(