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

components/ntp_snippets/remote/remote_suggestions_provider.h

 // Copyright 2015 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_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_H_
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_H_
 
 #include <memory>
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "components/ntp_snippets/content_suggestions_provider.h"
 
 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.
   using FetchStatusCallback = base::Callback<void(Status status_code)>;
 
   ~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
   // 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 (unless nullptr).
   virtual void RefetchInTheBackground(
       std::unique_ptr<FetchStatusCallback> callback) = 0;
 
   virtual const RemoteSuggestionsFetcher* suggestions_fetcher_for_debugging()
       const = 0;
 
  protected:
   RemoteSuggestionsProvider(Observer* observer);
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_H_