[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 4ff354f6b697750382057030828db4b3d856afab..9ee50ab89144e0c46f52393c1913118800605024 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider.cc
@@ -29,6 +29,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"
@@ -49,22 +50,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";
@@ -74,42 +59,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,7 +176,7 @@ RemoteSuggestionsProvider::RemoteSuggestionsProvider(
     PrefService* pref_service,
     const std::string& application_language_code,
     const UserClassifier* user_classifier,
-    NTPSnippetsScheduler* scheduler,
+    RemoteSuggestionsHardScheduler* hard_scheduler,

[markusheintz_, 2016/12/12 14:36:16]

Should we pass in the scheduler instead of the hard scheduler?

I assume this would also make testing easier. We would need to build the
scheduler externally then.

[jkrcal, 2016/12/14 09:42:09]

Done.

     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
     std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
     std::unique_ptr<image_fetcher::ImageDecoder> image_decoder,
@@ -240,7 +189,9 @@ RemoteSuggestionsProvider::RemoteSuggestionsProvider(
           category_factory->FromKnownCategory(KnownCategories::ARTICLES)),
       application_language_code_(application_language_code),
       user_classifier_(user_classifier),

[markusheintz_, 2016/12/12 15:32:28]

This is not used in this class any more. So we can get rid of it.

[jkrcal, 2016/12/14 09:42:09]

Done.

-      scheduler_(scheduler),
+      scheduler_(hard_scheduler,
+                 user_classifier,
+                 pref_service),

[Marc Treib, 2016/12/09 12:25:26]

I think this fits on one line?

Maybe for a later CL: Would we want to inject the whole scheduler? Might make
testing easier.

[jkrcal, 2016/12/14 09:42:09]

Done.

       snippets_fetcher_(std::move(snippets_fetcher)),
       image_fetcher_(std::move(image_fetcher)),
       image_decoder_(std::move(image_decoder)),
@@ -279,6 +230,8 @@ RemoteSuggestionsProvider::RemoteSuggestionsProvider(
   database_load_start_ = base::TimeTicks::Now();
   database_->LoadSnippets(base::Bind(
       &RemoteSuggestionsProvider::OnDatabaseLoaded, base::Unretained(this)));
+
+  scheduler_.SetUpdater(this);
 }
 
 RemoteSuggestionsProvider::~RemoteSuggestionsProvider() = default;
@@ -289,18 +242,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);
   registry->RegisterInt64Pref(prefs::kLastSuccessfulBackgroundFetchTime, 0);
 
+  RemoteSuggestionsScheduler::RegisterProfilePrefs(registry);
   RemoteSuggestionsStatusService::RegisterProfilePrefs(registry);
 }
 
-void RemoteSuggestionsProvider::FetchSnippetsInTheBackground() {
-  FetchSnippets(/*interactive_request=*/false);
-}
-
 void RemoteSuggestionsProvider::FetchSnippetsForAllCategories() {
   // TODO(markusheintz): Investigate whether we can call the Fetch method
   // instead of the FetchSnippets.
@@ -385,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) {

[Marc Treib, 2016/12/09 12:25:26]

Is this case still handled in the new world?

[jkrcal, 2016/12/14 09:42:09]

It is. 
 - Unschedule() is not called on state change when not inited.
 - forced is handled by calling Unschedule() and Schedule() in a sequence.

-      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());
@@ -581,6 +489,10 @@ void RemoteSuggestionsProvider::OnImageDataFetched(
   database_->SaveImage(id_within_category, image_data);
 }
 
+void RemoteSuggestionsProvider::UpdateRemoteSuggestionsBySchedule() {
+  FetchSnippets(/*interactive_request=*/false);
+}
+
 void RemoteSuggestionsProvider::OnDatabaseLoaded(
     NTPSnippet::PtrVector snippets) {
   if (state_ == State::ERROR_OCCURRED) {
@@ -785,8 +697,8 @@ void RemoteSuggestionsProvider::OnFetchFinished(
   // 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) {
+    scheduler_.OnSuccessfulUpdate();

[tschumann, 2016/12/09 17:27:28]

this has a smell, especially when reading the comment. If I understand the
motivation correctly, the RemoteSuggestionsProvider shouldn't be involved in
scheduling decisions anymore -- that's in contrast to the comment above.
Why not just always signal the result of a fetch to the scheduler? I guess
knowing about failed attempts might also be useful.

For sure, it would make the provider simpler. Rename to OnFetchFinished()?

That said, I think we should make the scheduled fetch properly asynchronous and
thus simplify the scheduler's interface. Instead of requiring a specific call to
the scheduler once the fetch is done, why not simply pass a done callback into
the UpdateRemoteSuggestionsBySchedule() function and require it to be called
when done? That's something we can move in a separate CL, but we should stop
implemetning asynchronous actions with synchronous functions ;-)

[jkrcal, 2016/12/14 09:42:09]

Done.

   }
 }
 
@@ -1147,6 +1059,7 @@ void RemoteSuggestionsProvider::EnterState(State state) {
       DVLOG(1) << "Entering state: READY";
       state_ = State::READY;
       EnterStateReady();
+      scheduler_.Schedule();
       break;
 
     case State::DISABLED:
@@ -1155,21 +1068,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(