[Remote suggestions] Move all decisions to fetch to the scheduler

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 c9f58c45fb10639a62bdddc4a7131906f75552a7..e94d5e6fc26abfb4990bc7c17767e1e35d9130cb 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider.h
+++ b/components/ntp_snippets/remote/remote_suggestions_provider.h
@@ -14,19 +14,12 @@
 namespace ntp_snippets {
 
 class RemoteSuggestionsFetcher;
+class RemoteSuggestionsScheduler;
 
 // Retrieves fresh content data (articles) from the server, stores them and
 // provides them as content suggestions.
 class RemoteSuggestionsProvider : public ContentSuggestionsProvider {
  public:
-  // TODO(jkrcal): Would be nice to get rid of this another level of statuses.
-  // Maybe possible while refactoring the RemoteSuggestionsStatusService? (and
-  // letting it notify both the SchedulingRemoteSuggestionsProvider and
-  // RemoteSuggestionsProviderImpl or just the scheduling one).
-  enum class ProviderStatus { ACTIVE, INACTIVE };
-  using ProviderStatusCallback =
-      base::RepeatingCallback<void(ProviderStatus status)>;
-
   // Callback to notify with the result of a fetch.
   // TODO(jkrcal): Change to OnceCallback? A OnceCallback does only have a
   // move-constructor which seems problematic for google mock.
@@ -34,13 +27,12 @@ class RemoteSuggestionsProvider : public ContentSuggestionsProvider {
 
   ~RemoteSuggestionsProvider() override;
 
-  // Set a callback to be notified whenever the status of the provider changes.
-  // The initial change is also notified (switching from an initial undecided
-  // status). If the callback is set after the first change, it is called back
-  // immediately.
-  // TODO(treib): Get rid of unique_ptrs to callbacks.
-  virtual void SetProviderStatusCallback(
-      std::unique_ptr<ProviderStatusCallback> callback) = 0;
+  // Set the scheduler to be notified whenever the provider becomes active /
+  // in-active and whenever history is deleted. The initial change is also
+  // notified (switching from an initial undecided status). If the scheduler is
+  // set after the first change, it is called back immediately.
+  virtual void SetRemoteSuggestionsScheduler(

[tschumann, 2017/02/24 09:37:12]

Completely missed this (and thanks Marc for pointing it out).
This method should not be on the RemoteSuggestionsProvider interface anymore. We
only need it to tell the Impl about the Scheduler.

We should move the function down to the Impl and then have the factory call it
with the scheduler instance.

[jkrcal, 2017/02/24 09:47:04]

I am puzzled: How is this different from the previous SetProviderStatusCallback?
Why is it cleaner in the factory (which results in code duplication)?

[tschumann, 2017/02/24 10:22:59]

The main issue was here before (guess we missed that when we extracted the
interface). It got slightly worse has we have now a dependency from a base class
to its subclass.

In the spirit of dependency injection, such wiring should be done as far outside
as possible and a factory seems the proper place. A single instance should know
as little as possible about how it's precisely wired to other instances. By
having the wiring in a central place (where you actually can see the instances
with their specific types) the system gets much easier to reason about.

+      RemoteSuggestionsScheduler* scheduler) = 0;
 
   // Fetches suggestions from the server for all remote categories and replaces
   // old suggestions by the new ones. The request to the server is performed as