[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/remote/remote_suggestions_provider.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/remote/remote_suggestions_provider.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 11 matching lines @@
 #include "base/time/default_tick_clock.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/image_fetcher/image_decoder.h"
 #include "components/image_fetcher/image_fetcher.h"
 #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"
 #include "components/prefs/pref_service.h"
 #include "components/variations/variations_associated_data.h"
 #include "grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/image/image.h"
 
 namespace ntp_snippets {
 
 namespace {
 
 // Number of snippets requested to the server. Consider replacing sparse UMA
 // histograms with COUNTS() if this number increases beyond 50.
 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";
 const char kCategoryContentProvidedByServer[] = "provided_by_server";
 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>>();
   for (const auto& snippet : snippets) {
     result->push_back(snippet->id());
   }
   return result;
 }
 
 bool HasIntersection(const std::vector<std::string>& a,
@@ skipping 97 matching lines @@
 }
 
 }  // namespace
 
 RemoteSuggestionsProvider::RemoteSuggestionsProvider(
     Observer* observer,
     CategoryFactory* category_factory,
     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,
     std::unique_ptr<RemoteSuggestionsDatabase> database,
     std::unique_ptr<RemoteSuggestionsStatusService> status_service)
     : ContentSuggestionsProvider(observer, category_factory),
       state_(State::NOT_INITED),
       pref_service_(pref_service),
       articles_category_(
           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)),
       database_(std::move(database)),
       status_service_(std::move(status_service)),
       fetch_when_ready_(false),
       nuke_when_initialized_(false),
       thumbnail_requests_throttler_(
           pref_service,
           RequestThrottler::RequestType::CONTENT_SUGGESTION_THUMBNAIL),
@@ skipping 18 matching lines @@
   }
 
   database_->SetErrorCallback(base::Bind(
       &RemoteSuggestionsProvider::OnDatabaseError, base::Unretained(this)));
 
   // We transition to other states while finalizing the initialization, when the
   // database is done loading.
   database_load_start_ = base::TimeTicks::Now();
   database_->LoadSnippets(base::Bind(
       &RemoteSuggestionsProvider::OnDatabaseLoaded, base::Unretained(this)));
+
+  scheduler_.SetUpdater(this);
 }
 
 RemoteSuggestionsProvider::~RemoteSuggestionsProvider() = default;
 
 // static
 void RemoteSuggestionsProvider::RegisterProfilePrefs(
     PrefRegistrySimple* registry) {
   // 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.
   FetchSnippets(/*interactive_request=*/true);
 }
 
 void RemoteSuggestionsProvider::FetchSnippets(bool interactive_request) {
   // TODO(markusheintz): Merge the FetchSnippets into the
   // FetchSnippetsInTheBackground method.
   if (ready()) {
@@ skipping 64 matching lines @@
 void RemoteSuggestionsProvider::MarkEmptyCategoriesAsLoading() {
   for (const auto& item : category_contents_) {
     Category category = item.first;
     const CategoryContent& content = item.second;
     if (content.snippets.empty()) {
       UpdateCategoryStatus(category, CategoryStatus::AVAILABLE_LOADING);
     }
   }
 }
 
-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());
   return content_it->second.status;
 }
 
 CategoryInfo RemoteSuggestionsProvider::GetCategoryInfo(Category category) {
   auto content_it = category_contents_.find(category);
   DCHECK(content_it != category_contents_.end());
   return content_it->second.info;
@@ skipping 137 matching lines @@
   }
   if (!found) {
     return;
   }
 
   // Only cache the data in the DB, the actual serving is done in the callback
   // provided to |image_fetcher_| (OnSnippetImageDecodedFromNetwork()).
   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) {
     return;
   }
   DCHECK(state_ == State::NOT_INITED);
   DCHECK(base::ContainsKey(category_contents_, articles_category_));
 
   base::TimeDelta database_load_time =
       base::TimeTicks::Now() - database_load_start_;
@@ skipping 184 matching lines @@
                               content.snippets.size());
   if (content.snippets.empty() && !content.dismissed.empty()) {
     UMA_HISTOGRAM_COUNTS("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded",
                          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) {
+    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.

   }
 }
 
 void RemoteSuggestionsProvider::ArchiveSnippets(
     CategoryContent* content,
     NTPSnippet::PtrVector* to_archive) {
   // Archive previous snippets - move them at the beginning of the list.
   content->archived.insert(content->archived.begin(),
                            std::make_move_iterator(to_archive->begin()),
                            std::make_move_iterator(to_archive->end()));
@@ skipping 340 matching lines @@
       // Initial state, it should not be possible to get back there.
       NOTREACHED();
       break;
 
     case State::READY:
       DCHECK(state_ == State::NOT_INITED || state_ == State::DISABLED);
 
       DVLOG(1) << "Entering state: READY";
       state_ = State::READY;
       EnterStateReady();
+      scheduler_.Schedule();
       break;
 
     case State::DISABLED:
       DCHECK(state_ == State::NOT_INITED || state_ == State::READY);
 
       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(
     Category category,
     const CategoryContent& content) {
   DCHECK(IsCategoryStatusAvailable(content.status));
 
   std::vector<ContentSuggestion> result =
       ConvertToContentSuggestions(category, content.snippets);
 
@@ skipping 162 matching lines @@
 RemoteSuggestionsProvider::CategoryContent::CategoryContent(CategoryContent&&) =
     default;
 
 RemoteSuggestionsProvider::CategoryContent::~CategoryContent() = default;
 
 RemoteSuggestionsProvider::CategoryContent&
 RemoteSuggestionsProvider::CategoryContent::operator=(CategoryContent&&) =
     default;
 
 }  // namespace ntp_snippets