[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/remote/remote_suggestions_provider_impl.cc

Index: components/ntp_snippets/remote/remote_suggestions_provider_impl.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_provider.cc b/components/ntp_snippets/remote/remote_suggestions_provider_impl.cc
similarity index 81%
copy from components/ntp_snippets/remote/remote_suggestions_provider.cc
copy to components/ntp_snippets/remote/remote_suggestions_provider_impl.cc
index 3b64ebb7363effab68ec09ac823420e91d184816..68302cbded62cfb494ce77f212544e71682e963e 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider_impl.cc
@@ -2,7 +2,7 @@
 // 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>
@@ -28,10 +28,8 @@
 #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"
@@ -50,22 +48,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 +57,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>>();
@@ -283,9 +229,8 @@ void CachedImageFetcher::OnSnippetImageFetchedFromDatabase(
   // |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.
@@ -326,27 +271,23 @@ void CachedImageFetcher::FetchSnippetImageFromNetwork(
                  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),
@@ -355,6 +296,9 @@ RemoteSuggestionsProvider::RemoteSuggestionsProvider(
                      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);
@@ -377,45 +321,59 @@ RemoteSuggestionsProvider::RemoteSuggestionsProvider(
   }
 
   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 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::FetchSnippetsForAllCategories() {
-  // TODO(markusheintz): Investigate whether we can call the Fetch method
-  // instead of the FetchSnippets.
-  FetchSnippets(/*interactive_request=*/true);
+void RemoteSuggestionsProviderImpl::RefetchInTheBackground(
+    std::unique_ptr<FetchStatusCallback> callback) {
+  FetchSnippets(/*interactive_request=*/false, std::move(callback));
 }
 
-void RemoteSuggestionsProvider::FetchSnippets(
-    bool interactive_request) {
+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;
   }
 
@@ -424,11 +382,12 @@ void RemoteSuggestionsProvider::FetchSnippets(
   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) {
@@ -445,12 +404,14 @@ void RemoteSuggestionsProvider::Fetch(
   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;
@@ -463,7 +424,7 @@ NTPSnippetsRequestParams RemoteSuggestionsProvider::BuildFetchParams() const {
   return result;
 }
 
-void RemoteSuggestionsProvider::MarkEmptyCategoriesAsLoading() {
+void RemoteSuggestionsProviderImpl::MarkEmptyCategoriesAsLoading() {
   for (const auto& item : category_contents_) {
     Category category = item.first;
     const CategoryContent& content = item.second;
@@ -473,58 +434,20 @@ 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) {
+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;
@@ -537,7 +460,7 @@ void RemoteSuggestionsProvider::DismissSuggestion(
                                        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) {
@@ -551,7 +474,7 @@ void RemoteSuggestionsProvider::ClearHistory(
   }
 }
 
-void RemoteSuggestionsProvider::ClearCachedSuggestions(Category category) {
+void RemoteSuggestionsProviderImpl::ClearCachedSuggestions(Category category) {
   if (!initialized()) {
     return;
   }
@@ -574,7 +497,7 @@ void RemoteSuggestionsProvider::ClearCachedSuggestions(Category category) {
   }
 }
 
-void RemoteSuggestionsProvider::OnSignInStateChanged() {
+void RemoteSuggestionsProviderImpl::OnSignInStateChanged() {
   // Make sure the status service is registered and we already initialised its
   // start state.
   if (!initialized()) {
@@ -584,7 +507,7 @@ void RemoteSuggestionsProvider::OnSignInStateChanged() {
   status_service_->OnSignInStateChanged();
 }
 
-void RemoteSuggestionsProvider::GetDismissedSuggestionsForDebugging(
+void RemoteSuggestionsProviderImpl::GetDismissedSuggestionsForDebugging(
     Category category,
     const DismissedSuggestionsCallback& callback) {
   auto content_it = category_contents_.find(category);
@@ -593,7 +516,7 @@ void RemoteSuggestionsProvider::GetDismissedSuggestionsForDebugging(
       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());
@@ -614,14 +537,14 @@ void RemoteSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
 }
 
 // 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()));
 
@@ -635,7 +558,7 @@ GURL RemoteSuggestionsProvider::FindSnippetImageUrl(
   return snippet->salient_image_url();
 }
 
-void RemoteSuggestionsProvider::OnDatabaseLoaded(
+void RemoteSuggestionsProviderImpl::OnDatabaseLoaded(
     NTPSnippet::PtrVector snippets) {
   if (state_ == State::ERROR_OCCURRED) {
     return;
@@ -691,12 +614,12 @@ void RemoteSuggestionsProvider::OnDatabaseLoaded(
   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) {
@@ -756,7 +679,8 @@ void RemoteSuggestionsProvider::OnFetchMoreFinished(
   NotifyNewSuggestions(category, *existing_content);
 }
 
-void RemoteSuggestionsProvider::OnFetchFinished(
+void RemoteSuggestionsProviderImpl::OnFetchFinished(
+    std::unique_ptr<FetchStatusCallback> callback,
     bool interactive_request,
     Status status,
     NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
@@ -832,16 +756,12 @@ 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 (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.
@@ -862,7 +782,7 @@ void RemoteSuggestionsProvider::ArchiveSnippets(
   }
 }
 
-void RemoteSuggestionsProvider::SanitizeReceivedSnippets(
+void RemoteSuggestionsProviderImpl::SanitizeReceivedSnippets(
     const NTPSnippet::PtrVector& dismissed,
     NTPSnippet::PtrVector* snippets) {
   DCHECK(ready());
@@ -870,7 +790,7 @@ void RemoteSuggestionsProvider::SanitizeReceivedSnippets(
   RemoveIncompleteSnippets(snippets);
 }
 
-void RemoteSuggestionsProvider::IntegrateSnippets(
+void RemoteSuggestionsProviderImpl::IntegrateSnippets(
     CategoryContent* content,
     NTPSnippet::PtrVector new_snippets) {
   DCHECK(ready());
@@ -899,7 +819,7 @@ void RemoteSuggestionsProvider::IntegrateSnippets(
   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(
@@ -919,7 +839,7 @@ void RemoteSuggestionsProvider::DismissSuggestionFromCategoryContent(
   content->snippets.erase(it);
 }
 
-void RemoteSuggestionsProvider::ClearExpiredDismissedSnippets() {
+void RemoteSuggestionsProviderImpl::ClearExpiredDismissedSnippets() {
   std::vector<Category> categories_to_erase;
 
   const base::Time now = base::Time::Now();
@@ -957,7 +877,7 @@ void RemoteSuggestionsProvider::ClearExpiredDismissedSnippets() {
   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;
@@ -971,7 +891,7 @@ void RemoteSuggestionsProvider::ClearOrphanedImages() {
   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
@@ -997,7 +917,7 @@ void RemoteSuggestionsProvider::NukeAllSnippets() {
   StoreCategoriesToPrefs();
 }
 
-void RemoteSuggestionsProvider::FetchSuggestionImage(
+void RemoteSuggestionsProviderImpl::FetchSuggestionImage(
     const ContentSuggestion::ID& suggestion_id,
     const ImageFetchedCallback& callback) {
   if (!base::ContainsKey(category_contents_, suggestion_id.category())) {
@@ -1017,7 +937,7 @@ void RemoteSuggestionsProvider::FetchSuggestionImage(
   image_fetcher_.FetchSuggestionImage(suggestion_id, image_url, callback);
 }
 
-void RemoteSuggestionsProvider::EnterStateReady() {
+void RemoteSuggestionsProviderImpl::EnterStateReady() {
   if (nuke_when_initialized_) {
     NukeAllSnippets();
     nuke_when_initialized_ = false;
@@ -1032,8 +952,10 @@ 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_,
+                  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_) {
@@ -1047,15 +969,15 @@ void RemoteSuggestionsProvider::EnterStateReady() {
   }
 }
 
-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.
@@ -1065,8 +987,8 @@ void RemoteSuggestionsProvider::FinishInitialization() {
 
   // 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.
@@ -1081,7 +1003,7 @@ void RemoteSuggestionsProvider::FinishInitialization() {
   }
 }
 
-void RemoteSuggestionsProvider::OnStatusChanged(
+void RemoteSuggestionsProviderImpl::OnStatusChanged(
     RemoteSuggestionsStatus old_status,
     RemoteSuggestionsStatus new_status) {
   switch (new_status) {
@@ -1091,7 +1013,9 @@ void RemoteSuggestionsProvider::OnStatusChanged(
         // 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);
@@ -1104,7 +1028,9 @@ void RemoteSuggestionsProvider::OnStatusChanged(
         // 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);
@@ -1123,7 +1049,7 @@ void RemoteSuggestionsProvider::OnStatusChanged(
   }
 }
 
-void RemoteSuggestionsProvider::EnterState(State state) {
+void RemoteSuggestionsProviderImpl::EnterState(State state) {
   if (state == state_) {
     return;
   }
@@ -1165,11 +1091,34 @@ void RemoteSuggestionsProvider::EnterState(State state) {
       break;
   }
 
-  // Schedule or un-schedule background fetching after each state change.
-  RescheduleFetching(false);
+  NotifyStateChanged();
+}
+
+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 RemoteSuggestionsProvider::NotifyNewSuggestions(
+void RemoteSuggestionsProviderImpl::NotifyNewSuggestions(
     Category category,
     const CategoryContent& content) {
   DCHECK(IsCategoryStatusAvailable(content.status));
@@ -1182,8 +1131,9 @@ void RemoteSuggestionsProvider::NotifyNewSuggestions(
   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;
@@ -1199,7 +1149,8 @@ void RemoteSuggestionsProvider::UpdateCategoryStatus(Category category,
   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);
   }
@@ -1220,7 +1171,7 @@ typename T::const_iterator FindSnippetInContainer(
 
 }  // 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);
@@ -1238,9 +1189,9 @@ const NTPSnippet* RemoteSuggestionsProvider::CategoryContent::FindSnippet(
   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_
@@ -1252,7 +1203,7 @@ RemoteSuggestionsProvider::UpdateCategoryInfo(Category category,
   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());
 
@@ -1305,7 +1256,7 @@ void RemoteSuggestionsProvider::RestoreCategoriesFromPrefs() {
   }
 }
 
-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_) {
@@ -1336,17 +1287,17 @@ void RemoteSuggestionsProvider::StoreCategoriesToPrefs() {
   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