[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/content_suggestions_service.h

Index: components/ntp_snippets/content_suggestions_service.h
diff --git a/components/ntp_snippets/content_suggestions_service.h b/components/ntp_snippets/content_suggestions_service.h
index f8a96a69e416f5bef9d5bc343fa20e156febe01b..200fccc1574292fffec4644dbda39b074d7b5253 100644
--- a/components/ntp_snippets/content_suggestions_service.h
+++ b/components/ntp_snippets/content_suggestions_service.h
@@ -23,6 +23,7 @@
 #include "components/ntp_snippets/category_factory.h"
 #include "components/ntp_snippets/category_status.h"
 #include "components/ntp_snippets/content_suggestions_provider.h"
+#include "components/ntp_snippets/remote/persistent_scheduler.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/signin/core/browser/signin_manager.h"
 
@@ -140,10 +141,18 @@ class ContentSuggestionsService : public KeyedService,
   // Fetches additional contents for the given |category|. If the fetch was
   // completed, the given |callback| is called with the updated content.
   // This includes new and old data.
+  // TODO(jkrcal): Consider either renaming this to FetchMore or unify the ways
+  // to get suggestions to just this async Fetch() API.
   void Fetch(const Category& category,
              const std::set<std::string>& known_suggestion_ids,
              const FetchDoneCallback& callback);
 
+  // Reloads suggestions from all categories. Makes sense to call only after

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

what does "reload" mean? Would this include fetching new suggestions? Or is this
for restoring dismissed suggestions?

Isn't this just a FetchAll() functionality?

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

I've rewritten the comment. Hope it is clearer now. The FetchAll naming is
discussed above. Nevertheless, I am open to renaming suggestions.

+  // some suggestions have been dismissed. The provider may fill in the vacant
+  // space by new suggestions that haven't been included previously due to space
+  // limits.
+  void ReloadSuggestions();
+
   // Observer accessors.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
@@ -193,15 +202,24 @@ class ContentSuggestionsService : public KeyedService,
 
   CategoryFactory* category_factory() { return &category_factory_; }
 
-  // The reference to the RemoteSuggestionsProvider provider should only be set
-  // by the factory and only be used for scheduling, periodic fetching and
-  // debugging.
-  RemoteSuggestionsProvider* ntp_snippets_service() {
-    return ntp_snippets_service_;
+  // The reference to the RemoteSuggestionsProvider provider should
+  // only be set by the factory and only used for debugging.
+  void set_remote_suggestions_provider(

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

please add a TODO() to remove this setter. These instances should be injected
into the constructor.  Not sure which debugging use cases would justify changing
 those internals.

I see that right now, we have a cyclic dependency as the service gets injected
as an observer into the RemoteSuggestionProvider. IMO, it would be more
appropriate to hand the RemoteSuggetsionsProvider into the
ContentSuggestionsService's constructor and have a setter on the
RemoteSuggestionsProvider to set the observer. But like I said, that's a TODO().

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

Yes, there is a cyclic dependency. Adding a setter for the observer actually
influences all providers whereas this setter influences just one provider. Why
do you feel the former way is better? (maybe better to discuss offline)

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

The depdencies just feel wrong: Needing the service first before being able to
construct the provider only complicates things. The most important flow is that
the service forwards data to the providers.The depdency back from the provider
to the service is of lower importance IMO.
But I might also see this from a biased view. Probably best to leave it as is
for now :-)
(because you can argue that the provider's job is to push data to observers and
not being called by the service ;-)).

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

Thanks for the explanation. Still not totally convinced it is worth the effort.
Added a (mildly phrased) TODO so that we do not forget this discussion.

+      RemoteSuggestionsProvider* remote_suggestions_provider) {
+    remote_suggestions_provider_ = remote_suggestions_provider;
   }
-  void set_ntp_snippets_service(
-      RemoteSuggestionsProvider* ntp_snippets_service) {
-    ntp_snippets_service_ = ntp_snippets_service;
+  RemoteSuggestionsProvider* remote_suggestions_provider_for_debugging() {
+    return remote_suggestions_provider_;
+  }
+
+  // The reference to PersistentScheduler::Listener should
+  // only be set by the factory and only used for scheduling, periodic fetching.
+  void set_persistent_scheduler_listener(
+      PersistentScheduler::Listener* persistent_scheduler_listener) {
+    persistent_scheduler_listener_ = persistent_scheduler_listener;
+  }
+  PersistentScheduler::Listener* persistent_scheduler_listener() {
+    return persistent_scheduler_listener_;
   }
 
   UserClassifier* user_classifier() { return &user_classifier_; }
@@ -312,11 +330,13 @@ class ContentSuggestionsService : public KeyedService,
 
   const std::vector<ContentSuggestion> no_suggestions_;
 
-  // Keep a direct reference to this special provider to redirect scheduling,
-  // background fetching and debugging calls to it. If the
-  // RemoteSuggestionsProvider is loaded, it is also present in |providers_|,
-  // otherwise this is a nullptr.
-  RemoteSuggestionsProvider* ntp_snippets_service_;
+  // Keep a direct reference to this special provider to redirect debugging
+  // calls to it. If the RemoteSuggestionsProvider is loaded, it is also present
+  // in |providers_|, otherwise this is a nullptr.
+  RemoteSuggestionsProvider* remote_suggestions_provider_;
+
+  // Interface for OS-dependent scheduling and background fetching.
+  PersistentScheduler::Listener* persistent_scheduler_listener_;
 
   PrefService* pref_service_;