[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/remote/remote_suggestions_provider.h

Index: components/ntp_snippets/remote/remote_suggestions_provider.h
diff --git a/components/ntp_snippets/remote/remote_suggestions_provider.h b/components/ntp_snippets/remote/remote_suggestions_provider.h
index e5b2e6d2d68527dbdef6dee6707c088d4226dd99..f99dfbaf6b6b09a745ffe378b051654b6276381e 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider.h
+++ b/components/ntp_snippets/remote/remote_suggestions_provider.h
@@ -26,7 +26,6 @@
 #include "components/ntp_snippets/content_suggestions_provider.h"
 #include "components/ntp_snippets/remote/ntp_snippet.h"
 #include "components/ntp_snippets/remote/ntp_snippets_fetcher.h"
-#include "components/ntp_snippets/remote/ntp_snippets_scheduler.h"
 #include "components/ntp_snippets/remote/remote_suggestions_status_service.h"
 #include "components/ntp_snippets/remote/request_throttler.h"
 
@@ -45,7 +44,6 @@ class ImageFetcher;
 namespace ntp_snippets {
 
 class RemoteSuggestionsDatabase;
-class UserClassifier;
 
 // Retrieves fresh content data (articles) from the server, stores them and
 // provides them as content suggestions.
@@ -62,6 +60,8 @@ class RemoteSuggestionsProvider final
     : public ContentSuggestionsProvider,
       public image_fetcher::ImageFetcherDelegate {
  public:
+  using FetchStatusCallback = base::Callback<void(Status status_code)>;
+  using ProviderActiveCallback = base::RepeatingCallback<void(bool active)>;
   // |application_language_code| should be a ISO 639-1 compliant string, e.g.
   // 'en' or 'en-US'. Note that this code should only specify the language, not
   // the locale, so 'en_US' (English language with US locale) and 'en-GB_US'
@@ -71,8 +71,6 @@ class RemoteSuggestionsProvider final
       CategoryFactory* category_factory,
       PrefService* pref_service,
       const std::string& application_language_code,
-      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,
@@ -88,29 +86,24 @@ class RemoteSuggestionsProvider final
   // will be ignored.
   bool ready() const { return state_ == State::READY; }
 
-  // Returns whether the service is initialized. While this is false, some
-  // calls may trigger DCHECKs.
+  // Returns whether the service is successfuly initialized. While this is
+  // false, some calls may trigger DCHECKs.
   bool initialized() const { return ready() || state_ == State::DISABLED; }
 
-  // Fetchs content suggestions from the Content Suggestions server for all
-  // remote categories in the background.
-  void FetchSnippetsInTheBackground();
+  void RegisterActivenessObserver(const ProviderActiveCallback& callback);

[tschumann, 2016/12/15 19:27:00]

naming is hard. Maybe we change this to 
rename the callback to
ProviderStatusCallback() and probably introduce an enum for readability:
enum ProviderStatus { ACTIVE, INACTIVE } or ENABLED, DISABLED, whatever.

then we can call this:
SetProviderStatusListener or SetProviderStatusCallback().

As we only store a single callback, we should call it Set and not Register.

How do we distinguish if such a callback was provided? How does the default
constructed callback react if we call it? I guess it's best to make this
function take a unique_ptr<> and store the unique ptr so that we can 
a) check against nullptr
b) clients can unregister themselves before they get deleted (we don't want
callbacks into deleted objects).

[jkrcal, 2016/12/19 09:40:24]

Renamed as suggested. Totally agree with both "Set" and "ProviderStatus".

I believe that Callback internally handles this (keeping a pointer to a context,
being nullptr initially). Do you still want me to change it here to clarify the
contract here?

I do not think that clearing / uneregistering the callback is needed here. The
only client so far keeps RemoteSuggestionsProviderImpl as a unique_ptr so it
will delete the whole class before deleting itself. 

[tschumann, 2016/12/19 11:19:23]

I see. Well in this case we're in good shape. If that changes, that party can
still call SetProviderStatusCallback() with a dummy callback to unregister.

can you add a unit test making sure we play nicely with the default constructed
callback?

[jkrcal, 2016/12/20 16:39:45]

Done.

 
-  // Fetchs content suggestions from the Content Suggestions server for all
-  // remote categories. The request to the server is performed as an interactive
-  // request. Interactive requests are used for actions triggered by the user
-  // and request lower latency processing.
-  void FetchSnippetsForAllCategories();
+  // Fetches snippets from the server for all remote categories and replaces old
+  // snippets by the new ones. The request to the server is performed as an
+  // background request. Background requests are used for actions not triggered
+  // by the user and have lower priority on the server. After the fetch
+  // finished, the provided |callback| will be triggered with the status of the
+  // fetch.
+  void RefetchInTheBackground(const FetchStatusCallback& callback);
 
   const NTPSnippetsFetcher* snippets_fetcher() const {
     return snippets_fetcher_.get();
   }
 
-  // (Re)schedules the periodic fetching of snippets. If |force| is true, the
-  // tasks will be re-scheduled even if they already exist and have the correct
-  // periods.
-  void RescheduleFetching(bool force);
-
   // ContentSuggestionsProvider implementation
   CategoryStatus GetCategoryStatus(Category category) override;
   CategoryInfo GetCategoryInfo(Category category) override;
@@ -120,6 +113,7 @@ class RemoteSuggestionsProvider final
   void Fetch(const Category& category,
              const std::set<std::string>& known_suggestion_ids,
              const FetchDoneCallback& callback) override;
+  void ReloadSuggestions() override;
   void ClearHistory(
       base::Time begin,
       base::Time end,
@@ -242,8 +236,10 @@ class RemoteSuggestionsProvider final
   // Fetches snippets from the server and replaces old snippets by the new ones.
   // Requests can be marked more important by setting |interactive_request| to
   // true (such request might circumvent the daily quota for requests, etc.)
-  // Useful for requests triggered by the user.
-  void FetchSnippets(bool interactive_request);
+  // Useful for requests triggered by the user. After the fetch finished, the
+  // provided |callback| will be triggered with the status of the fetch.
+  void FetchSnippets(bool interactive_request,
+                     const FetchStatusCallback& callback);
 
   // Returns the URL of the image of a snippet if it is among the current or
   // among the archived snippets in the matching category. Returns an empty URL
@@ -266,6 +262,7 @@ class RemoteSuggestionsProvider final
 
   // Callback for regular fetch requests with the NTPSnippetsFetcher.
   void OnFetchFinished(
+      const FetchStatusCallback& callback,
       bool interactive_request,
       NTPSnippetsFetcher::FetchResult fetch_status,
       NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories);
@@ -379,12 +376,6 @@ class RemoteSuggestionsProvider final
   // The ISO 639-1 code of the language used by the application.
   const std::string application_language_code_;
 
-  // Classifier that tells us how active the user is. Not owned.
-  const UserClassifier* user_classifier_;
-
-  // Scheduler for fetching snippets. Not owned.
-  NTPSnippetsScheduler* scheduler_;
-
   // The snippets fetcher.
   std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher_;
 
@@ -401,6 +392,11 @@ class RemoteSuggestionsProvider final
   // Set to true if FetchSnippets is called while the service isn't ready.
   // The fetch will be executed once the service enters the READY state.
   bool fetch_when_ready_;
+  // The parameters for the fetch to perform later.
+  bool fetch_when_ready_interactive_;
+  FetchStatusCallback fetch_when_ready_callback_;
+
+  ProviderActiveCallback provider_active_callback_;
 
   // Set to true if NukeAllSnippets is called while the service isn't ready.
   // The nuke will be executed once the service finishes initialization or