[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/remote/remote_suggestions_provider_impl.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 "components/ntp_snippets/remote/remote_suggestions_provider_impl.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/default_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/category_rankers/category_ranker.h"
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/pref_names.h"
-#include "components/ntp_snippets/remote/ntp_snippets_request_params.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"
 #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 152 matching lines @@
 }
 
 void CachedImageFetcher::OnSnippetImageFetchedFromDatabase(
     const ImageFetchedCallback& callback,
     const ContentSuggestion::ID& suggestion_id,
     const GURL& url,
     std::string data) {  // SnippetImageCallback requires nonconst reference.
   // |image_decoder_| is null in tests.
   if (image_decoder_ && !data.empty()) {
     image_decoder_->DecodeImage(
-        data, base::Bind(
-                  &CachedImageFetcher::OnSnippetImageDecodedFromDatabase,
-                  base::Unretained(this), callback, suggestion_id, url));
+        data, base::Bind(&CachedImageFetcher::OnSnippetImageDecodedFromDatabase,
+                         base::Unretained(this), callback, suggestion_id, url));
     return;
   }
   // Fetching from the DB failed; start a network fetch.
   FetchSnippetImageFromNetwork(suggestion_id, url, callback);
 }
 
 void CachedImageFetcher::OnSnippetImageDecodedFromDatabase(
     const ImageFetchedCallback& callback,
     const ContentSuggestion::ID& suggestion_id,
     const GURL& url,
@@ skipping 20 matching lines @@
     callback.Run(gfx::Image());
     return;
   }
 
   image_fetcher_->StartOrQueueNetworkRequest(
       suggestion_id.id_within_category(), url,
       base::Bind(&CachedImageFetcher::OnImageDecodingDone,
                  base::Unretained(this), callback));
 }
 
-RemoteSuggestionsProvider::RemoteSuggestionsProvider(
+RemoteSuggestionsProviderImpl::RemoteSuggestionsProviderImpl(
     Observer* observer,
     PrefService* pref_service,
     const std::string& application_language_code,
     CategoryRanker* category_ranker,
-    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,
     std::unique_ptr<RemoteSuggestionsDatabase> database,
     std::unique_ptr<RemoteSuggestionsStatusService> status_service)
-    : ContentSuggestionsProvider(observer),
+    : RemoteSuggestionsProvider(observer),
       state_(State::NOT_INITED),
       pref_service_(pref_service),
       articles_category_(
           Category::FromKnownCategory(KnownCategories::ARTICLES)),
       application_language_code_(application_language_code),
       category_ranker_(category_ranker),
-      user_classifier_(user_classifier),
-      scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
       database_(std::move(database)),
       image_fetcher_(std::move(image_fetcher),
                      std::move(image_decoder),
                      pref_service,
                      database_.get()),
       status_service_(std::move(status_service)),
       fetch_when_ready_(false),
+      fetch_when_ready_interactive_(false),
+      fetch_when_ready_callback_(nullptr),
+      provider_status_callback_(nullptr),
       nuke_when_initialized_(false),
       clock_(base::MakeUnique<base::DefaultClock>()) {
   pref_service_->ClearPref(kDeprecatedSnippetHostsPref);
 
   RestoreCategoriesFromPrefs();
   // The articles category always exists. Add it if we didn't get it from prefs.
   // TODO(treib): Rethink this.
   category_contents_.insert(
       std::make_pair(articles_category_,
                      CategoryContent(BuildArticleCategoryInfo(base::nullopt))));
   // Tell the observer about all the categories.
   for (const auto& entry : category_contents_) {
     observer->OnCategoryStatusChanged(this, entry.first, entry.second.status);
   }
 
   if (database_->IsErrorState()) {
     EnterState(State::ERROR_OCCURRED);
     UpdateAllCategoryStatus(CategoryStatus::LOADING_ERROR);
     return;
   }
 
   database_->SetErrorCallback(base::Bind(
-      &RemoteSuggestionsProvider::OnDatabaseError, base::Unretained(this)));
+      &RemoteSuggestionsProviderImpl::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)));
+  database_->LoadSnippets(
+      base::Bind(&RemoteSuggestionsProviderImpl::OnDatabaseLoaded,
+                 base::Unretained(this)));
 }
 
-RemoteSuggestionsProvider::~RemoteSuggestionsProvider() = default;
+RemoteSuggestionsProviderImpl::~RemoteSuggestionsProviderImpl() = default;
 
 // static
-void RemoteSuggestionsProvider::RegisterProfilePrefs(
+void RemoteSuggestionsProviderImpl::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);
 
   RemoteSuggestionsStatusService::RegisterProfilePrefs(registry);
 }
 
-void RemoteSuggestionsProvider::FetchSnippetsInTheBackground() {
-  FetchSnippets(/*interactive_request=*/false);
+void RemoteSuggestionsProviderImpl::SetProviderStatusCallback(
+    std::unique_ptr<ProviderStatusCallback> callback) {
+  provider_status_callback_ = std::move(callback);
+  // Call the observer right away if we've reached any final state.
+  NotifyStateChanged();
 }
 
-void RemoteSuggestionsProvider::FetchSnippetsForAllCategories() {
-  // TODO(markusheintz): Investigate whether we can call the Fetch method
-  // instead of the FetchSnippets.
-  FetchSnippets(/*interactive_request=*/true);
+void RemoteSuggestionsProviderImpl::ReloadSuggestions() {
+  FetchSnippets(/*interactive_request=*/true,
+                std::unique_ptr<FetchStatusCallback>(nullptr));

[Bernhard Bauer, 2016/12/21 10:38:42]

You should also be able to just use nullptr here (and in other places in this
CL).

[jkrcal, 2016/12/21 11:54:40]

Done.

 }
 
-void RemoteSuggestionsProvider::FetchSnippets(
-    bool interactive_request) {
+void RemoteSuggestionsProviderImpl::RefetchInTheBackground(
+    std::unique_ptr<FetchStatusCallback> callback) {
+  FetchSnippets(/*interactive_request=*/false, std::move(callback));
+}
+
+const NTPSnippetsFetcher*
+RemoteSuggestionsProviderImpl::snippets_fetcher_for_testing_and_debugging()
+    const {
+  return snippets_fetcher_.get();
+}
+
+void RemoteSuggestionsProviderImpl::FetchSnippets(
+    bool interactive_request,
+    std::unique_ptr<FetchStatusCallback> callback) {
   if (!ready()) {
     fetch_when_ready_ = true;
+    fetch_when_ready_interactive_ = interactive_request;
+    fetch_when_ready_callback_ = std::move(callback);
     return;
   }
 
   MarkEmptyCategoriesAsLoading();
 
   NTPSnippetsRequestParams params = BuildFetchParams();
   params.interactive_request = interactive_request;
   snippets_fetcher_->FetchSnippets(
-      params, base::BindOnce(&RemoteSuggestionsProvider::OnFetchFinished,
-                             base::Unretained(this), interactive_request));
+      params, base::BindOnce(&RemoteSuggestionsProviderImpl::OnFetchFinished,
+                             base::Unretained(this), std::move(callback),
+                             interactive_request));
 }
 
-void RemoteSuggestionsProvider::Fetch(
+void RemoteSuggestionsProviderImpl::Fetch(
     const Category& category,
     const std::set<std::string>& known_suggestion_ids,
     const FetchDoneCallback& callback) {
   if (!ready()) {
     CallWithEmptyResults(callback,
                          Status(StatusCode::TEMPORARY_ERROR,
                                 "RemoteSuggestionsProvider is not ready!"));
     return;
   }
   NTPSnippetsRequestParams params = BuildFetchParams();
   params.excluded_ids.insert(known_suggestion_ids.begin(),
                              known_suggestion_ids.end());
   params.interactive_request = true;
   params.exclusive_category = category;
 
   snippets_fetcher_->FetchSnippets(
-      params, base::BindOnce(&RemoteSuggestionsProvider::OnFetchMoreFinished,
-                             base::Unretained(this), callback));
+      params,
+      base::BindOnce(&RemoteSuggestionsProviderImpl::OnFetchMoreFinished,
+                     base::Unretained(this), callback));
 }
 
 // Builds default fetcher params.
-NTPSnippetsRequestParams RemoteSuggestionsProvider::BuildFetchParams() const {
+NTPSnippetsRequestParams RemoteSuggestionsProviderImpl::BuildFetchParams()
+    const {
   NTPSnippetsRequestParams result;
   result.language_code = application_language_code_;
   result.count_to_fetch = kMaxSnippetCount;
   for (const auto& map_entry : category_contents_) {
     const CategoryContent& content = map_entry.second;
     for (const auto& dismissed_snippet : content.dismissed) {
       result.excluded_ids.insert(dismissed_snippet->id());
     }
   }
   return result;
 }
 
-void RemoteSuggestionsProvider::MarkEmptyCategoriesAsLoading() {
+void RemoteSuggestionsProviderImpl::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) {
-      scheduler_->Unschedule();
-      pref_service_->ClearPref(prefs::kSnippetBackgroundFetchingIntervalWifi);
-      pref_service_->ClearPref(
-          prefs::kSnippetBackgroundFetchingIntervalFallback);
-    }
-  }
-}
-
-CategoryStatus RemoteSuggestionsProvider::GetCategoryStatus(Category category) {
+CategoryStatus RemoteSuggestionsProviderImpl::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) {
+CategoryInfo RemoteSuggestionsProviderImpl::GetCategoryInfo(Category category) {
   auto content_it = category_contents_.find(category);
   DCHECK(content_it != category_contents_.end());
   return content_it->second.info;
 }
 
-void RemoteSuggestionsProvider::DismissSuggestion(
+void RemoteSuggestionsProviderImpl::DismissSuggestion(
     const ContentSuggestion::ID& suggestion_id) {
   if (!ready()) {
     return;
   }
 
   auto content_it = category_contents_.find(suggestion_id.category());
   DCHECK(content_it != category_contents_.end());
   CategoryContent* content = &content_it->second;
   DismissSuggestionFromCategoryContent(content,
                                        suggestion_id.id_within_category());
 }
 
-void RemoteSuggestionsProvider::ClearHistory(
+void RemoteSuggestionsProviderImpl::ClearHistory(
     base::Time begin,
     base::Time end,
     const base::Callback<bool(const GURL& url)>& filter) {
   // Both time range and the filter are ignored and all suggestions are removed,
   // because it is not known which history entries were used for the suggestions
   // personalization.
   if (!ready()) {
     nuke_when_initialized_ = true;
   } else {
     NukeAllSnippets();
   }
 }
 
-void RemoteSuggestionsProvider::ClearCachedSuggestions(Category category) {
+void RemoteSuggestionsProviderImpl::ClearCachedSuggestions(Category category) {
   if (!initialized()) {
     return;
   }
 
   auto content_it = category_contents_.find(category);
   if (content_it == category_contents_.end()) {
     return;
   }
   CategoryContent* content = &content_it->second;
   if (content->snippets.empty()) {
     return;
   }
 
   database_->DeleteSnippets(GetSnippetIDVector(content->snippets));
   database_->DeleteImages(GetSnippetIDVector(content->snippets));
   content->snippets.clear();
 
   if (IsCategoryStatusAvailable(content->status)) {
     NotifyNewSuggestions(category, *content);
   }
 }
 
-void RemoteSuggestionsProvider::OnSignInStateChanged() {
+void RemoteSuggestionsProviderImpl::OnSignInStateChanged() {
   // Make sure the status service is registered and we already initialised its
   // start state.
   if (!initialized()) {
     return;
   }
 
   status_service_->OnSignInStateChanged();
 }
 
-void RemoteSuggestionsProvider::GetDismissedSuggestionsForDebugging(
+void RemoteSuggestionsProviderImpl::GetDismissedSuggestionsForDebugging(
     Category category,
     const DismissedSuggestionsCallback& callback) {
   auto content_it = category_contents_.find(category);
   DCHECK(content_it != category_contents_.end());
   callback.Run(
       ConvertToContentSuggestions(category, content_it->second.dismissed));
 }
 
-void RemoteSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
+void RemoteSuggestionsProviderImpl::ClearDismissedSuggestionsForDebugging(
     Category category) {
   auto content_it = category_contents_.find(category);
   DCHECK(content_it != category_contents_.end());
   CategoryContent* content = &content_it->second;
 
   if (!initialized()) {
     return;
   }
 
   if (content->dismissed.empty()) {
     return;
   }
 
   database_->DeleteSnippets(GetSnippetIDVector(content->dismissed));
   // The image got already deleted when the suggestion was dismissed.
 
   content->dismissed.clear();
 }
 
 // static
-int RemoteSuggestionsProvider::GetMaxSnippetCountForTesting() {
+int RemoteSuggestionsProviderImpl::GetMaxSnippetCountForTesting() {
   return kMaxSnippetCount;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
-GURL RemoteSuggestionsProvider::FindSnippetImageUrl(
+GURL RemoteSuggestionsProviderImpl::FindSnippetImageUrl(
     const ContentSuggestion::ID& suggestion_id) const {
   DCHECK(base::ContainsKey(category_contents_, suggestion_id.category()));
 
   const CategoryContent& content =
       category_contents_.at(suggestion_id.category());
   const NTPSnippet* snippet =
       content.FindSnippet(suggestion_id.id_within_category());
   if (!snippet) {
     return GURL();
   }
   return snippet->salient_image_url();
 }
 
-void RemoteSuggestionsProvider::OnDatabaseLoaded(
+void RemoteSuggestionsProviderImpl::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_;
   UMA_HISTOGRAM_MEDIUM_TIMES("NewTabPage.Snippets.DatabaseLoadTime",
@@ skipping 35 matching lines @@
   }
 
   // TODO(tschumann): If I move ClearExpiredDismissedSnippets() to the beginning
   // of the function, it essentially does nothing but tests are still green. Fix
   // this!
   ClearExpiredDismissedSnippets();
   ClearOrphanedImages();
   FinishInitialization();
 }
 
-void RemoteSuggestionsProvider::OnDatabaseError() {
+void RemoteSuggestionsProviderImpl::OnDatabaseError() {
   EnterState(State::ERROR_OCCURRED);
   UpdateAllCategoryStatus(CategoryStatus::LOADING_ERROR);
 }
 
-void RemoteSuggestionsProvider::OnFetchMoreFinished(
+void RemoteSuggestionsProviderImpl::OnFetchMoreFinished(
     const FetchDoneCallback& fetching_callback,
     Status status,
     NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
   if (!fetched_categories) {
     DCHECK(!status.IsSuccess());
     CallWithEmptyResults(fetching_callback, status);
     return;
   }
   if (fetched_categories->size() != 1u) {
     LOG(DFATAL) << "Requested one exclusive category but received "
@@ skipping 39 matching lines @@
   // e.g. to make sure we don't serve results after the sign-out. Revisit this
   // once the snippets fetcher supports concurrent requests. We can then see if
   // Nuke should also cancel outstanding requests or we want to check the
   // status.
   UpdateCategoryStatus(category, CategoryStatus::AVAILABLE);
   // Notify callers and observers.
   fetching_callback.Run(Status::Success(), std::move(result));
   NotifyNewSuggestions(category, *existing_content);
 }
 
-void RemoteSuggestionsProvider::OnFetchFinished(
+void RemoteSuggestionsProviderImpl::OnFetchFinished(
+    std::unique_ptr<FetchStatusCallback> callback,
     bool interactive_request,
     Status status,
     NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
   if (!ready()) {
     // TODO(tschumann): What happens if this was a user-triggered, interactive
     // request? Is the UI waiting indefinitely now?
     return;
   }
 
   // Record the fetch time of a successfull background fetch.
@@ skipping 55 matching lines @@
   auto content_it = category_contents_.find(articles_category_);
   DCHECK(content_it != category_contents_.end());
   const CategoryContent& content = content_it->second;
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticles",
                               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 (callback) {
+    callback->Run(status);
   }
 }
 
-void RemoteSuggestionsProvider::ArchiveSnippets(
+void RemoteSuggestionsProviderImpl::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()));
   to_archive->clear();
 
   // If there are more archived snippets than we want to keep, delete the
   // oldest ones by their fetch time (which are always in the back).
   if (content->archived.size() > kMaxArchivedSnippetCount) {
     NTPSnippet::PtrVector to_delete(
         std::make_move_iterator(content->archived.begin() +
                                 kMaxArchivedSnippetCount),
         std::make_move_iterator(content->archived.end()));
     content->archived.resize(kMaxArchivedSnippetCount);
     database_->DeleteImages(GetSnippetIDVector(to_delete));
   }
 }
 
-void RemoteSuggestionsProvider::SanitizeReceivedSnippets(
+void RemoteSuggestionsProviderImpl::SanitizeReceivedSnippets(
     const NTPSnippet::PtrVector& dismissed,
     NTPSnippet::PtrVector* snippets) {
   DCHECK(ready());
   EraseMatchingSnippets(snippets, dismissed);
   RemoveIncompleteSnippets(snippets);
 }
 
-void RemoteSuggestionsProvider::IntegrateSnippets(
+void RemoteSuggestionsProviderImpl::IntegrateSnippets(
     CategoryContent* content,
     NTPSnippet::PtrVector new_snippets) {
   DCHECK(ready());
 
   // Do not touch the current set of snippets if the newly fetched one is empty.
   // TODO(tschumann): This should go. If we get empty results we should update
   // accordingly and remove the old one (only of course if this was not received
   // through a fetch-more).
   if (new_snippets.empty()) {
     return;
   }
 
   // It's entirely possible that the newly fetched snippets contain articles
   // that have been present before.
   // We need to make sure to only delete and archive snippets that don't
   // appear with the same ID in the new suggestions (it's fine for additional
   // IDs though).
   EraseByPrimaryID(&content->snippets, *GetSnippetIDVector(new_snippets));
   // Do not delete the thumbnail images as they are still handy on open NTPs.
   database_->DeleteSnippets(GetSnippetIDVector(content->snippets));
   // Note, that ArchiveSnippets will clear |content->snippets|.
   ArchiveSnippets(content, &content->snippets);
 
   database_->SaveSnippets(new_snippets);
 
   content->snippets = std::move(new_snippets);
 }
 
-void RemoteSuggestionsProvider::DismissSuggestionFromCategoryContent(
+void RemoteSuggestionsProviderImpl::DismissSuggestionFromCategoryContent(
     CategoryContent* content,
     const std::string& id_within_category) {
   auto it = std::find_if(
       content->snippets.begin(), content->snippets.end(),
       [&id_within_category](const std::unique_ptr<NTPSnippet>& snippet) {
         return snippet->id() == id_within_category;
       });
   if (it == content->snippets.end()) {
     return;
   }
 
   (*it)->set_dismissed(true);
 
   database_->SaveSnippet(**it);
 
   content->dismissed.push_back(std::move(*it));
   content->snippets.erase(it);
 }
 
-void RemoteSuggestionsProvider::ClearExpiredDismissedSnippets() {
+void RemoteSuggestionsProviderImpl::ClearExpiredDismissedSnippets() {
   std::vector<Category> categories_to_erase;
 
   const base::Time now = base::Time::Now();
 
   for (auto& item : category_contents_) {
     Category category = item.first;
     CategoryContent* content = &item.second;
 
     NTPSnippet::PtrVector to_delete;
     // Move expired dismissed snippets over into |to_delete|.
@@ skipping 17 matching lines @@
   }
 
   for (Category category : categories_to_erase) {
     UpdateCategoryStatus(category, CategoryStatus::NOT_PROVIDED);
     category_contents_.erase(category);
   }
 
   StoreCategoriesToPrefs();
 }
 
-void RemoteSuggestionsProvider::ClearOrphanedImages() {
+void RemoteSuggestionsProviderImpl::ClearOrphanedImages() {
   auto alive_snippets = base::MakeUnique<std::set<std::string>>();
   for (const auto& entry : category_contents_) {
     const CategoryContent& content = entry.second;
     for (const auto& snippet_ptr : content.snippets) {
       alive_snippets->insert(snippet_ptr->id());
     }
     for (const auto& snippet_ptr : content.dismissed) {
       alive_snippets->insert(snippet_ptr->id());
     }
   }
   database_->GarbageCollectImages(std::move(alive_snippets));
 }
 
-void RemoteSuggestionsProvider::NukeAllSnippets() {
+void RemoteSuggestionsProviderImpl::NukeAllSnippets() {
   std::vector<Category> categories_to_erase;
 
   // Empty the ARTICLES category and remove all others, since they may or may
   // not be personalized.
   for (const auto& item : category_contents_) {
     Category category = item.first;
 
     ClearCachedSuggestions(category);
     ClearDismissedSuggestionsForDebugging(category);
 
     UpdateCategoryStatus(category, CategoryStatus::NOT_PROVIDED);
 
     // Remove the category entirely; it may or may not reappear.
     if (category != articles_category_) {
       categories_to_erase.push_back(category);
     }
   }
 
   for (Category category : categories_to_erase) {
     category_contents_.erase(category);
   }
 
   StoreCategoriesToPrefs();
 }
 
-void RemoteSuggestionsProvider::FetchSuggestionImage(
+void RemoteSuggestionsProviderImpl::FetchSuggestionImage(
     const ContentSuggestion::ID& suggestion_id,
     const ImageFetchedCallback& callback) {
   if (!base::ContainsKey(category_contents_, suggestion_id.category())) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback, gfx::Image()));
     return;
   }
   GURL image_url = FindSnippetImageUrl(suggestion_id);
   if (image_url.is_empty()) {
     // As we don't know the corresponding snippet anymore, we don't expect to
     // find it in the database (and also can't fetch it remotely). Cut the
     // lookup short and return directly.
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback, gfx::Image()));
     return;
   }
   image_fetcher_.FetchSuggestionImage(suggestion_id, image_url, callback);
 }
 
-void RemoteSuggestionsProvider::EnterStateReady() {
+void RemoteSuggestionsProviderImpl::EnterStateReady() {
   if (nuke_when_initialized_) {
     NukeAllSnippets();
     nuke_when_initialized_ = false;
   }
 
   auto article_category_it = category_contents_.find(articles_category_);
   DCHECK(article_category_it != category_contents_.end());
   if (article_category_it->second.snippets.empty() || fetch_when_ready_) {
     // TODO(jkrcal): Fetching snippets automatically upon creation of this
     // lazily created service can cause troubles, e.g. in unit tests where
     // network I/O is not allowed.
     // 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_,
+                  std::move(fetch_when_ready_callback_));
     fetch_when_ready_ = false;
+    fetch_when_ready_callback_.reset();

[Bernhard Bauer, 2016/12/21 10:38:42]

Is this necessary if you move the callback anyway?

[jkrcal, 2016/12/21 11:54:40]

Not needed, you are right.

I just remembered the general semantics rule that std::moving something leaves
it in an unspecified state. std::unique_ptr luckily gives a more concrete
contract :)

[Bernhard Bauer, 2016/12/21 12:24:36]

Oh hey, actually now that you mention it: We could store the callbacks directly
instead of in unique_ptr<>s (and also pass them around by value as per
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...).
I think Callback also resets cleanly when it's moved (its only member is a
scoped_refptr to the state, which explicitly resets, see
https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?sq=package:chr...).

[jkrcal, 2016/12/21 12:41:20]

I had it like that before but changed to unique_ptr because I needed to support
empty callbacks. Maybe I did not try hard (I find documentation / the right
source code for callbacks hard to find). Does it behave like a pointer?

Can I write 
   auto callback = Callback<type>() 
to create an empty callback and test its non-emptyness by
   if (callback) {
     callback.Run();
   }
?

If so, the last question: can I leave it for a later CL, please? ;)
 (the time is pressing and I would like to actually start implementing the soft
fetches)

[tschumann, 2016/12/21 12:42:51]

let's not rely on undocumented properties here. Unless it's explicitly stated in
the callback's documentation that the moved-out state is as we expect it, we
shouldn't rely on it. It's tricky enough.
IMO, the unique_ptr<> doesn't add a lot of noise but makes it very clear to the
reader what the possible states are. Another option would be to use Optional,
but IMO that's a bit clumsy to use. The nice thing about the unique_ptr is that
callers can also unregister themselves cleanly.

[Bernhard Bauer, 2016/12/21 13:16:33]

Yup. (There is an explicit is_null() method, but also a bool() operator, so
using it directly works.)
Yeah, sure.

[Bernhard Bauer, 2016/12/21 13:16:33]

The explicit recommendation is to pass callbacks by value, and for OnceCallbacks
std::move()ing them is the only way to get an rvalue, so, yes, I would assume
that my colleagues are in fact not incompetent and have made moving callbacks
possible without undefined behavior. If you're concerned about the lack of
documentation, I'm more than happy to clarify the behavior with the authors and
add documentation.

[tschumann, 2016/12/21 13:33:47]

hehehe... not questioning competence; but usually people tend to not think too
much of the moved-out state as most best practices are about not touching it
then.
However, I'm not sure we have the problem of the moved-out state anyways. Happy
to have it changed back to value semantics given the is_null() check we missed
before :-)

[jkrcal, 2016/12/21 14:22:56]

I file a bug for this.

   }
 
   for (const auto& item : category_contents_) {
     Category category = item.first;
     const CategoryContent& content = item.second;
     // FetchSnippets has set the status to |AVAILABLE_LOADING| if relevant,
     // otherwise we transition to |AVAILABLE| here.
     if (content.status != CategoryStatus::AVAILABLE_LOADING) {
       UpdateCategoryStatus(category, CategoryStatus::AVAILABLE);
     }
   }
 }
 
-void RemoteSuggestionsProvider::EnterStateDisabled() {
+void RemoteSuggestionsProviderImpl::EnterStateDisabled() {
   NukeAllSnippets();
 }
 
-void RemoteSuggestionsProvider::EnterStateError() {
+void RemoteSuggestionsProviderImpl::EnterStateError() {
   status_service_.reset();
 }
 
-void RemoteSuggestionsProvider::FinishInitialization() {
+void RemoteSuggestionsProviderImpl::FinishInitialization() {
   if (nuke_when_initialized_) {
     // We nuke here in addition to EnterStateReady, so that it happens even if
     // we enter the DISABLED state below.
     NukeAllSnippets();
     nuke_when_initialized_ = false;
   }
 
   // Note: Initializing the status service will run the callback right away with
   // the current state.
-  status_service_->Init(base::Bind(&RemoteSuggestionsProvider::OnStatusChanged,
-                                   base::Unretained(this)));
+  status_service_->Init(base::Bind(
+      &RemoteSuggestionsProviderImpl::OnStatusChanged, base::Unretained(this)));
 
   // Always notify here even if we got nothing from the database, because we
   // don't know how long the fetch will take or if it will even complete.
   for (const auto& item : category_contents_) {
     Category category = item.first;
     const CategoryContent& content = item.second;
     // Note: We might be in a non-available status here, e.g. DISABLED due to
     // enterprise policy.
     if (IsCategoryStatusAvailable(content.status)) {
       NotifyNewSuggestions(category, content);
     }
   }
 }
 
-void RemoteSuggestionsProvider::OnStatusChanged(
+void RemoteSuggestionsProviderImpl::OnStatusChanged(
     RemoteSuggestionsStatus old_status,
     RemoteSuggestionsStatus new_status) {
   switch (new_status) {
     case RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN:
       if (old_status == RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT) {
         DCHECK(state_ == State::READY);
         // Clear nonpersonalized suggestions.
         NukeAllSnippets();
         // Fetch personalized ones.
-        FetchSnippets(/*interactive_request=*/true);
+        // TODO(jkrcal): Loop in SchedulingRemoteSuggestionsProvider somehow.
+        FetchSnippets(/*interactive_request=*/true,
+                      std::unique_ptr<FetchStatusCallback>(nullptr));
       } else {
         // Do not change the status. That will be done in EnterStateReady().
         EnterState(State::READY);
       }
       break;
 
     case RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT:
       if (old_status == RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN) {
         DCHECK(state_ == State::READY);
         // Clear personalized suggestions.
         NukeAllSnippets();
         // Fetch nonpersonalized ones.
-        FetchSnippets(/*interactive_request=*/true);
+        // TODO(jkrcal): Loop in SchedulingRemoteSuggestionsProvider somehow.
+        FetchSnippets(/*interactive_request=*/true,
+                      std::unique_ptr<FetchStatusCallback>(nullptr));
       } else {
         // Do not change the status. That will be done in EnterStateReady().
         EnterState(State::READY);
       }
       break;
 
     case RemoteSuggestionsStatus::EXPLICITLY_DISABLED:
       EnterState(State::DISABLED);
       UpdateAllCategoryStatus(CategoryStatus::CATEGORY_EXPLICITLY_DISABLED);
       break;
 
     case RemoteSuggestionsStatus::SIGNED_OUT_AND_DISABLED:
       EnterState(State::DISABLED);
       UpdateAllCategoryStatus(CategoryStatus::SIGNED_OUT);
       break;
   }
 }
 
-void RemoteSuggestionsProvider::EnterState(State state) {
+void RemoteSuggestionsProviderImpl::EnterState(State state) {
   if (state == state_) {
     return;
   }
 
   UMA_HISTOGRAM_ENUMERATION("NewTabPage.Snippets.EnteredState",
                             static_cast<int>(state),
                             static_cast<int>(State::COUNT));
 
   switch (state) {
     case State::NOT_INITED:
@@ skipping 21 matching lines @@
       DVLOG(1) << "Entering state: ERROR_OCCURRED";
       state_ = State::ERROR_OCCURRED;
       EnterStateError();
       break;
 
     case State::COUNT:
       NOTREACHED();
       break;
   }
 
-  // Schedule or un-schedule background fetching after each state change.
-  RescheduleFetching(false);
+  NotifyStateChanged();
 }
 
-void RemoteSuggestionsProvider::NotifyNewSuggestions(
+void RemoteSuggestionsProviderImpl::NotifyStateChanged() {
+  if (!provider_status_callback_) {
+    return;
+  }
+
+  switch (state_) {
+    case State::NOT_INITED:
+      // Initial state, not sure yet whether active or not.
+      break;
+    case State::READY:
+      provider_status_callback_->Run(ProviderStatus::ACTIVE);
+      break;
+    case State::DISABLED:
+      provider_status_callback_->Run(ProviderStatus::INACTIVE);
+      break;
+    case State::ERROR_OCCURRED:
+      provider_status_callback_->Run(ProviderStatus::INACTIVE);
+      break;
+    case State::COUNT:
+      NOTREACHED();
+      break;
+  }
+}
+
+void RemoteSuggestionsProviderImpl::NotifyNewSuggestions(
     Category category,
     const CategoryContent& content) {
   DCHECK(IsCategoryStatusAvailable(content.status));
 
   std::vector<ContentSuggestion> result =
       ConvertToContentSuggestions(category, content.snippets);
 
   DVLOG(1) << "NotifyNewSuggestions(): " << result.size()
            << " items in category " << category;
   observer()->OnNewSuggestions(this, category, std::move(result));
 }
 
-void RemoteSuggestionsProvider::UpdateCategoryStatus(Category category,
-                                                     CategoryStatus status) {
+void RemoteSuggestionsProviderImpl::UpdateCategoryStatus(
+    Category category,
+    CategoryStatus status) {
   auto content_it = category_contents_.find(category);
   DCHECK(content_it != category_contents_.end());
   CategoryContent& content = content_it->second;
 
   if (status == content.status) {
     return;
   }
 
   DVLOG(1) << "UpdateCategoryStatus(): " << category.id() << ": "
            << static_cast<int>(content.status) << " -> "
            << static_cast<int>(status);
   content.status = status;
   observer()->OnCategoryStatusChanged(this, category, content.status);
 }
 
-void RemoteSuggestionsProvider::UpdateAllCategoryStatus(CategoryStatus status) {
+void RemoteSuggestionsProviderImpl::UpdateAllCategoryStatus(
+    CategoryStatus status) {
   for (const auto& category : category_contents_) {
     UpdateCategoryStatus(category.first, status);
   }
 }
 
 namespace {
 
 template <typename T>
 typename T::const_iterator FindSnippetInContainer(
     const T& container,
     const std::string& id_within_category) {
   return std::find_if(
       container.begin(), container.end(),
       [&id_within_category](const std::unique_ptr<NTPSnippet>& snippet) {
         return snippet->id() == id_within_category;
       });
 }
 
 }  // namespace
 
-const NTPSnippet* RemoteSuggestionsProvider::CategoryContent::FindSnippet(
+const NTPSnippet* RemoteSuggestionsProviderImpl::CategoryContent::FindSnippet(
     const std::string& id_within_category) const {
   // Search for the snippet in current and archived snippets.
   auto it = FindSnippetInContainer(snippets, id_within_category);
   if (it != snippets.end()) {
     return it->get();
   }
   auto archived_it = FindSnippetInContainer(archived, id_within_category);
   if (archived_it != archived.end()) {
     return archived_it->get();
   }
   auto dismissed_it = FindSnippetInContainer(dismissed, id_within_category);
   if (dismissed_it != dismissed.end()) {
     return dismissed_it->get();
   }
   return nullptr;
 }
 
-RemoteSuggestionsProvider::CategoryContent*
-RemoteSuggestionsProvider::UpdateCategoryInfo(Category category,
-                                              const CategoryInfo& info) {
+RemoteSuggestionsProviderImpl::CategoryContent*
+RemoteSuggestionsProviderImpl::UpdateCategoryInfo(Category category,
+                                                  const CategoryInfo& info) {
   auto content_it = category_contents_.find(category);
   if (content_it == category_contents_.end()) {
     content_it = category_contents_
                      .insert(std::make_pair(category, CategoryContent(info)))
                      .first;
   } else {
     content_it->second.info = info;
   }
   return &content_it->second;
 }
 
-void RemoteSuggestionsProvider::RestoreCategoriesFromPrefs() {
+void RemoteSuggestionsProviderImpl::RestoreCategoriesFromPrefs() {
   // This must only be called at startup, before there are any categories.
   DCHECK(category_contents_.empty());
 
   const base::ListValue* list =
       pref_service_->GetList(prefs::kRemoteSuggestionCategories);
   for (const std::unique_ptr<base::Value>& entry : *list) {
     const base::DictionaryValue* dict = nullptr;
     if (!entry->GetAsDictionary(&dict)) {
       DLOG(WARNING) << "Invalid category pref value: " << *entry;
       continue;
@@ skipping 32 matching lines @@
     CategoryInfo info =
         category == articles_category_
             ? BuildArticleCategoryInfo(title)
             : BuildRemoteCategoryInfo(title, allow_fetching_more_results);
     CategoryContent* content = UpdateCategoryInfo(category, info);
     content->included_in_last_server_response =
         included_in_last_server_response;
   }
 }
 
-void RemoteSuggestionsProvider::StoreCategoriesToPrefs() {
+void RemoteSuggestionsProviderImpl::StoreCategoriesToPrefs() {
   // Collect all the CategoryContents.
   std::vector<std::pair<Category, const CategoryContent*>> to_store;
   for (const auto& entry : category_contents_) {
     to_store.emplace_back(entry.first, &entry.second);
   }
   // The ranker may not persist the order, thus, it is stored by the provider.
   std::sort(to_store.begin(), to_store.end(),
             [this](const std::pair<Category, const CategoryContent*>& left,
                    const std::pair<Category, const CategoryContent*>& right) {
               return category_ranker_->Compare(left.first, right.first);
@@ skipping 10 matching lines @@
     dict->SetBoolean(kCategoryContentProvidedByServer,
                      content.included_in_last_server_response);
     dict->SetBoolean(kCategoryContentAllowFetchingMore,
                      content.info.has_more_action());
     list.Append(std::move(dict));
   }
   // Finally, store the result in the pref service.
   pref_service_->Set(prefs::kRemoteSuggestionCategories, list);
 }
 
-RemoteSuggestionsProvider::CategoryContent::CategoryContent(
+RemoteSuggestionsProviderImpl::CategoryContent::CategoryContent(
     const CategoryInfo& info)
     : info(info) {}
 
-RemoteSuggestionsProvider::CategoryContent::CategoryContent(CategoryContent&&) =
-    default;
+RemoteSuggestionsProviderImpl::CategoryContent::CategoryContent(
+    CategoryContent&&) = default;
 
-RemoteSuggestionsProvider::CategoryContent::~CategoryContent() = default;
+RemoteSuggestionsProviderImpl::CategoryContent::~CategoryContent() = default;
 
-RemoteSuggestionsProvider::CategoryContent&
-RemoteSuggestionsProvider::CategoryContent::operator=(CategoryContent&&) =
+RemoteSuggestionsProviderImpl::CategoryContent&
+RemoteSuggestionsProviderImpl::CategoryContent::operator=(CategoryContent&&) =
     default;
 
 }  // namespace ntp_snippets