[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 653c82b401289a641bde225d90324a52c4239aa9..788345cf2b064f453eea439c899fdedc79dedaef 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider.cc
@@ -28,6 +28,7 @@
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
+#include "components/ntp_snippets/remote/remote_suggestions_scheduler.h"
 #include "components/ntp_snippets/switches.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -48,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";
@@ -73,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>>();
@@ -226,7 +175,7 @@ RemoteSuggestionsProvider::RemoteSuggestionsProvider(
     PrefService* pref_service,
     const std::string& application_language_code,
     const UserClassifier* user_classifier,
-    NTPSnippetsScheduler* scheduler,
+    RemoteSuggestionsHardScheduler* hard_scheduler,
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
     std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
     std::unique_ptr<image_fetcher::ImageDecoder> image_decoder,
@@ -239,7 +188,10 @@ RemoteSuggestionsProvider::RemoteSuggestionsProvider(
           category_factory->FromKnownCategory(KnownCategories::ARTICLES)),
       application_language_code_(application_language_code),
       user_classifier_(user_classifier),
-      scheduler_(scheduler),
+      scheduler_(hard_scheduler,
+                 this /* scheduler_updater */,

[Marc Treib, 2016/12/08 16:14:58]

/*scheduler_updater=*/this

That said, it's a bit dangerous to pass out the |this| pointer here - if the
scheduler tries to do anything with it, things will probably fail in interesting
ways, because this object isn't initialized yet.

[jkrcal, 2016/12/09 09:20:38]

Fair enough. Removed it from the constructor and added another method to pass it
in later.

+                 user_classifier,
+                 pref_service),
       snippets_fetcher_(std::move(snippets_fetcher)),
       image_fetcher_(std::move(image_fetcher)),
       image_decoder_(std::move(image_decoder)),
@@ -287,14 +239,12 @@ 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);
 
+  RemoteSuggestionsScheduler::RegisterProfilePrefs(registry);
   RemoteSuggestionsStatusService::RegisterProfilePrefs(registry);
 }
 
-void RemoteSuggestionsProvider::FetchSnippetsInTheBackground() {
+void RemoteSuggestionsProvider::UpdateRemoteSuggestionsBySchedule() {
   FetchSnippets(/*interactive_request=*/false);
 }
 
@@ -382,45 +332,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());
@@ -773,7 +684,7 @@ void RemoteSuggestionsProvider::OnFetchFinished(
   // succeeded, and also that we don't do a background fetch immediately after
   // a user-initiated one.
   if (fetched_categories) {
-    RescheduleFetching(true);
+    scheduler_.OnSuccessfulUpdate();
   }
 }
 
@@ -1134,6 +1045,7 @@ void RemoteSuggestionsProvider::EnterState(State state) {
       DVLOG(1) << "Entering state: READY";
       state_ = State::READY;
       EnterStateReady();
+      scheduler_.Schedule();
       break;
 
     case State::DISABLED:
@@ -1142,21 +1054,20 @@ void RemoteSuggestionsProvider::EnterState(State state) {
       DVLOG(1) << "Entering state: DISABLED";
       state_ = State::DISABLED;
       EnterStateDisabled();
+      scheduler_.Unschedule();
       break;
 
     case State::ERROR_OCCURRED:
       DVLOG(1) << "Entering state: ERROR_OCCURRED";
       state_ = State::ERROR_OCCURRED;
       EnterStateError();
+      scheduler_.Unschedule();
       break;
 
     case State::COUNT:
       NOTREACHED();
       break;
   }
-
-  // Schedule or un-schedule background fetching after each state change.
-  RescheduleFetching(false);
 }
 
 void RemoteSuggestionsProvider::NotifyNewSuggestions(