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

components/ntp_snippets/remote/remote_suggestions_provider_impl.h

Index: components/ntp_snippets/remote/remote_suggestions_provider_impl.h
diff --git a/components/ntp_snippets/remote/remote_suggestions_provider_impl.h b/components/ntp_snippets/remote/remote_suggestions_provider_impl.h
index 5fc9f77a9ebd8ccfe401e24fdeb321d0a2522c32..172b9b2b398ccb0d8caad61db52477ea840d8e37 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider_impl.h
+++ b/components/ntp_snippets/remote/remote_suggestions_provider_impl.h
@@ -47,6 +47,7 @@ namespace ntp_snippets {
 
 class CategoryRanker;
 class RemoteSuggestionsDatabase;
+class RemoteSuggestionsScheduler;
 
 // CachedImageFetcher takes care of fetching images from the network and caching
 // them in the database.
@@ -137,8 +138,8 @@ class RemoteSuggestionsProviderImpl final : public RemoteSuggestionsProvider {
   bool initialized() const { return ready() || state_ == State::DISABLED; }
 
   // RemoteSuggestionsProvider implementation.
-  void SetProviderStatusCallback(
-      std::unique_ptr<ProviderStatusCallback> callback) override;
+  void SetRemoteSuggestionsScheduler(
+      RemoteSuggestionsScheduler* scheduler) override;
   void RefetchInTheBackground(
       std::unique_ptr<FetchStatusCallback> callback) override;
 
@@ -197,11 +198,9 @@ class RemoteSuggestionsProviderImpl final : public RemoteSuggestionsProvider {
   friend class RemoteSuggestionsProviderImplTest;
 
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
-                           CallsProviderStatusCallbackWhenReady);
+                           CallsSchedulerOnError);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
-                           CallsProviderStatusCallbackOnError);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
-                           CallsProviderStatusCallbackWhenDisabled);
+                           CallsSchedulerWhenDisabled);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
                            ShouldNotCrashWhenCallingEmptyCallback);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
@@ -211,6 +210,10 @@ class RemoteSuggestionsProviderImpl final : public RemoteSuggestionsProvider {
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest, StatusChanges);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
                            SuggestionsFetchedOnSignInAndSignOut);
+  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,

[tschumann, 2017/02/22 11:31:34]

please add a comment why these tests need to be friends.
Honestly we should try to avoid them. Seems we only use these methods for
injecting the error states. Honestly, I'd be happier if we could exercise the
whole error path in tests (and make the provider get into the error condition by
itself).

Not necessarily in this CL, however, it would be nice if the new tests could
already trigger the error through public interfaces. 

[jkrcal, 2017/02/22 13:17:03]

Agreed. I did a clean-up in the friends, removed some stale ones. Most of these
friends (incl. the new ones) boil down to mocking the status service. This
should be done. I vote for "not-in-this-CL". Added TODOs.

+                           CallsSchedulerWhenSignedIn);
+  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
+                           CallsSchedulerWhenSignedOut);
 
   // Possible state transitions:
   //       NOT_INITED --------+
@@ -352,6 +355,12 @@ class RemoteSuggestionsProviderImpl final : public RemoteSuggestionsProvider {
   // the file system.
   void ClearOrphanedImages();
 
+  // Clears suggestions because any history item has been removed.
+  void ClearAnyHistory();
+
+  // Clears suggestions because they became obsolete.
+  void ObsoleteSuggestions();

[tschumann, 2017/02/22 11:31:34]

i'd like to avoid coining new terms -- it's pretty unclear what obsolete means.
Why not simply ClearSuggestions()? -- the reason should be irrelevant. 
Then we have 2 distinct events -- ClearAnyHistory() [==leave no traces] and
ClearSuggestions() [just removes what we currently have].

[jkrcal, 2017/02/22 13:17:03]

Makes sense. Done.

+
   // Clears all stored suggestions and updates the observer.
   void NukeAllSuggestions();
 
@@ -442,12 +451,12 @@ class RemoteSuggestionsProviderImpl final : public RemoteSuggestionsProvider {
   bool fetch_when_ready_interactive_;
   std::unique_ptr<FetchStatusCallback> fetch_when_ready_callback_;
 
-  std::unique_ptr<ProviderStatusCallback> provider_status_callback_;
+  RemoteSuggestionsScheduler* remote_suggestions_scheduler_;
 
-  // Set to true if NukeAllSuggestions is called while the service isn't ready.
+  // Set to true if ClearAnyHistory is called while the service isn't ready.
   // The nuke will be executed once the service finishes initialization or
   // enters the READY state.
-  bool nuke_when_initialized_;
+  bool clear_any_history_when_initialized_;
 
   // A clock for getting the time. This allows to inject a clock in tests.
   std::unique_ptr<base::Clock> clock_;