[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/content_suggestions_service.h

 // Copyright 2016 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.
 
 #ifndef COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_
 #define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_
 
 #include <map>
 #include <memory>
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/callback_forward.h"
 #include "base/observer_list.h"
 #include "base/optional.h"
 #include "base/scoped_observer.h"
 #include "base/time/time.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/history_service_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/ntp_snippets/callbacks.h"
 #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"
 
 class PrefService;
 class PrefRegistrySimple;
 
 namespace ntp_snippets {
 
 class RemoteSuggestionsProvider;
 
@@ skipping 97 matching lines @@
   // Restores all dismissed categories.
   // This will not trigger an update through the observers.
   void RestoreDismissedCategories();
 
   // Returns whether |category| is dismissed.
   bool IsCategoryDismissed(Category category) const;
 
   // 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);
 
   // Registers a new ContentSuggestionsProvider. It must be ensured that at most
   // one provider is registered for every category and that this method is
   // called only once per provider.
   void RegisterProvider(std::unique_ptr<ContentSuggestionsProvider> provider);
 
   // Removes history from the specified time range where the URL matches the
@@ skipping 29 matching lines @@
 
   // Only for debugging use through the internals page. Some providers
   // internally store a list of dismissed suggestions to prevent them from
   // reappearing. This function clears all suggestions of the given |category|
   // from such lists, making dismissed suggestions reappear (if the provider
   // supports it).
   void ClearDismissedSuggestionsForDebugging(Category category);
 
   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_; }
 
  private:
   friend class ContentSuggestionsServiceTest;
 
   // Implementation of ContentSuggestionsProvider::Observer.
   void OnNewSuggestions(ContentSuggestionsProvider* provider,
                         Category category,
@@ skipping 90 matching lines @@
 
   // Observer for the HistoryService. All providers are notified when history is
   // deleted.
   ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
       history_service_observer_;
 
   base::ObserverList<Observer> observers_;
 
   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_;
 
   UserClassifier user_classifier_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentSuggestionsService);
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_