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

components/ntp_snippets/remote/remote_suggestions_provider_impl.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_IMPL_H_
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_IMPL_H_
 
 #include <cstddef>
 #include <deque>
 #include <map>
@@ skipping 29 matching lines @@
 
 namespace image_fetcher {
 class ImageDecoder;
 class ImageFetcher;
 }  // namespace image_fetcher
 
 namespace ntp_snippets {
 
 class CategoryRanker;
 class RemoteSuggestionsDatabase;
+class RemoteSuggestionsScheduler;
 
 // CachedImageFetcher takes care of fetching images from the network and caching
 // them in the database.
 // TODO(tschumann): Move into a separate library and inject the
 // CachedImageFetcher into the RemoteSuggestionsProvider. This allows us to get
 // rid of exposing this member for testing and lets us test the caching logic
 // separately.
 class CachedImageFetcher : public image_fetcher::ImageFetcherDelegate {
  public:
   // |pref_service| and |database| need to outlive the created image fetcher
@@ skipping 70 matching lines @@
   // Returns whether the service is ready. While this is false, the list of
   // suggestions will be empty, and all modifications to it (fetch, dismiss,
   // etc) will be ignored.
   bool ready() const { return state_ == State::READY; }
 
   // Returns whether the service is successfully initialized. While this is
   // false, some calls may trigger DCHECKs.
   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;
 
   // TODO(fhorschig): Remove this getter when there is an interface for the
   // fetcher that allows better mocks.
   const RemoteSuggestionsFetcher* suggestions_fetcher_for_debugging()
       const override;
 
   // ContentSuggestionsProvider implementation.
   CategoryStatus GetCategoryStatus(Category category) override;
@@ skipping 38 matching lines @@
   }
 
   // TODO(tschumann): remove this method as soon as we inject the fetcher into
   // the constructor.
   CachedImageFetcher& GetImageFetcherForTesting() { return image_fetcher_; }
 
  private:
   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,
                            DontNotifyIfNotAvailable);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
                            RemoveExpiredDismissedContent);
   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 --------+
   //       /       \          |
   //      v         v         |
   //   READY <--> DISABLED    |
   //       \       /          |
   //        v     v           |
   //     ERROR_OCCURRED <-----+
   enum class State {
@@ skipping 121 matching lines @@
 
   // Removes expired dismissed suggestions from the service and the database.
   void ClearExpiredDismissedSuggestions();
 
   // Removes images from the DB that are not referenced from any known
   // suggestion. Needs to iterate the whole suggestion database -- so do it
   // often enough to keep it small but not too often as it still iterates over
   // 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();
 
   // Completes the initialization phase of the service, registering the last
   // observers. This is done after construction, once the database is loaded.
   void FinishInitialization();
 
   // Triggers a state transition depending on the provided status. This method
   // is called when a change is detected by |status_service_|.
   void OnStatusChanged(RemoteSuggestionsStatus old_status,
@@ skipping 70 matching lines @@
 
   // Set to true if FetchSuggestions is called while the service isn't ready.
   // The fetch will be executed once the service enters the READY state.
   // TODO(jkrcal): create a struct and have here just one base::Optional<>?
   bool fetch_when_ready_;
 
   // The parameters for the fetch to perform later.
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsProviderImpl);
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_IMPL_H_