[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 37 matching lines @@
     clock_ = std::move(clock);
   }
 
   // TODO(tschumann): remove this method as soon as we inject the fetcher into
   // the constructor.
   CachedImageFetcher& GetImageFetcherForTesting() { return image_fetcher_; }
 
  private:
   friend class RemoteSuggestionsProviderImplTest;
 
+  // TODO(jkrcal): Mock the database to trigger the error naturally (or remote

[Marc Treib, 2017/02/23 13:52:24]

s/remote/remove/ ?

[jkrcal, 2017/02/24 09:21:43]

Done.

+  // the error state and get rid of the test).
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
-                           CallsProviderStatusCallbackWhenReady);
+                           CallsSchedulerOnError);
+  // TODO(jkrcal): Mock the status service and remove these friend declarations.
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
-                           CallsProviderStatusCallbackOnError);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
-                           CallsProviderStatusCallbackWhenDisabled);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
-                           ShouldNotCrashWhenCallingEmptyCallback);
+                           CallsSchedulerWhenDisabled);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
                            DontNotifyIfNotAvailable);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
-                           RemoveExpiredDismissedContent);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest, StatusChanges);
+                           CallsSchedulerWhenSignedIn);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
-                           SuggestionsFetchedOnSignInAndSignOut);
+                           CallsSchedulerWhenSignedOut);
 
   // Possible state transitions:
   //       NOT_INITED --------+
   //       /       \          |
   //      v         v         |
   //   READY <--> DISABLED    |
   //       \       /          |
   //        v     v           |
   //     ERROR_OCCURRED <-----+
+  // TODO(jkrcal): Do we need to keep the distinction between states DISABLED
+  // and ERROR_OCCURED?
   enum class State {
     // The service has just been created. Can change to states:
     // - DISABLED: After the database is done loading,
     //             GetStateForDependenciesStatus can identify the next state to
     //             be DISABLED.
     // - READY: if GetStateForDependenciesStatus returns it, after the database
     //          is done loading.
     // - ERROR_OCCURRED: when an unrecoverable error occurred.
     NOT_INITED,
 
@@ skipping 112 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();

[Marc Treib, 2017/02/23 13:52:24]

Should this be "AnyHistoryCleared"? It doesn't clear history itself; it's called
when a history clear event occurs (right?)
Or alternatively something like ClearHistoryDependentState? (with a less-awkward
name, if possible :D)

[jkrcal, 2017/02/24 09:21:43]

I prefer the active form, stick to ClearHistoryDependentState().

+
+  // Clears suggestions for any non-history related reason (e.g., sign-in status
+  // change, etc.).
+  void ClearSuggestions();
+
   // 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_