[NTP Snippets] Refactor background scheduling for remote suggestions

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.h b/components/ntp_snippets/remote/remote_suggestions_provider_impl.h
similarity index 89%
copy from components/ntp_snippets/remote/remote_suggestions_provider.h
copy to components/ntp_snippets/remote/remote_suggestions_provider_impl.h
index 1c39bb8b8c8972a48d2b82db23751c7c0c9a92bc..d918733cacb096ee50a2ea30cf516b0cb8f0fd2d 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider.h
+++ b/components/ntp_snippets/remote/remote_suggestions_provider_impl.h
@@ -2,8 +2,8 @@
 // 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_
+#ifndef COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_IMPL_H_
+#define COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_IMPL_H_
 
 #include <cstddef>
 #include <deque>
@@ -27,7 +27,7 @@
 #include "components/ntp_snippets/content_suggestions_provider.h"
 #include "components/ntp_snippets/remote/ntp_snippet.h"
 #include "components/ntp_snippets/remote/ntp_snippets_fetcher.h"
-#include "components/ntp_snippets/remote/ntp_snippets_scheduler.h"
+#include "components/ntp_snippets/remote/remote_suggestions_provider.h"
 #include "components/ntp_snippets/remote/remote_suggestions_status_service.h"
 #include "components/ntp_snippets/remote/request_throttler.h"
 
@@ -46,7 +46,6 @@ class ImageFetcher;
 namespace ntp_snippets {
 
 class RemoteSuggestionsDatabase;
-class UserClassifier;
 
 // CachedImageFetcher takes care of fetching images from the network and caching
 // them in the database.
@@ -106,26 +105,26 @@ class CachedImageFetcher : public image_fetcher::ImageFetcherDelegate {
 // This class is final because it does things in its constructor which make it
 // unsafe to derive from it.
 // TODO(treib): Introduce two-phase initialization and make the class not final?
-class RemoteSuggestionsProvider final : public ContentSuggestionsProvider {
+class RemoteSuggestionsProviderImpl final : public RemoteSuggestionsProvider {
  public:
+  using FetchStatusCallback = base::Callback<void(Status status_code)>;
+  using ProviderActiveCallback = base::RepeatingCallback<void(bool active)>;

[Marc Treib, 2016/12/19 12:59:23]

AFAIK Callback and RepeatingCallback are the same thing. We should be
consistent.
Also seems that ProviderActiveCallback isn't actually used?

[jkrcal, 2016/12/20 16:39:46]

Removed ProviderActiveCallback. Even FetchStatusCallback should be defined here.
Still your comment applies to their definition in RemoteSuggestionsProvider. I
added a TODO to change the Callback to OnceCallback there (I had problems with
move-constructors and google mock that avoided me from doing it right away).

Explanation (from the docs):

The legacy `Callback<>` is currently aliased to `RepeatingCallback<>`. In new
code, prefer `OnceCallback<>` where possible, and use `RepeatingCallback<>`
otherwise. Once the migration is complete, the type alias will be removed and
`OnceCallback<>` will be renamed to `Callback<>` to emphasize that it should be
preferred.

   // |application_language_code| should be a ISO 639-1 compliant string, e.g.

[Marc Treib, 2016/12/19 12:59:23]

nit: empty line before

[jkrcal, 2016/12/20 16:39:46]

Done.

   // 'en' or 'en-US'. Note that this code should only specify the language, not
   // the locale, so 'en_US' (English language with US locale) and 'en-GB_US'
   // (British English person in the US) are not language codes.
-  RemoteSuggestionsProvider(
+  RemoteSuggestionsProviderImpl(
       Observer* observer,
       CategoryFactory* category_factory,
       PrefService* pref_service,
       const std::string& application_language_code,
-      const UserClassifier* user_classifier,
-      NTPSnippetsScheduler* scheduler,
       std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
       std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
       std::unique_ptr<image_fetcher::ImageDecoder> image_decoder,
       std::unique_ptr<RemoteSuggestionsDatabase> database,
       std::unique_ptr<RemoteSuggestionsStatusService> status_service);
 
-  ~RemoteSuggestionsProvider() override;
+  ~RemoteSuggestionsProviderImpl() override;
 
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
@@ -134,28 +133,23 @@ class RemoteSuggestionsProvider final : public ContentSuggestionsProvider {
   // will be ignored.
   bool ready() const { return state_ == State::READY; }
 
-  // Returns whether the service is initialized. While this is false, some
-  // calls may trigger DCHECKs.
+  // Returns whether the service is successfuly initialized. While this is

[Marc Treib, 2016/12/19 12:59:23]

successfully (two "l"s)

[jkrcal, 2016/12/20 16:39:46]

Huh, I have repetitive problems with "success" ;D

+  // false, some calls may trigger DCHECKs.
   bool initialized() const { return ready() || state_ == State::DISABLED; }
 
-  // Fetchs content suggestions from the Content Suggestions server for all
-  // remote categories in the background.
-  void FetchSnippetsInTheBackground();
+  void SetProviderStatusCallback(
+      const ProviderStatusCallback& callback) override;
 
-  // Fetchs content suggestions from the Content Suggestions server for all
-  // remote categories. The request to the server is performed as an interactive
-  // request. Interactive requests are used for actions triggered by the user
-  // and request lower latency processing.
-  void FetchSnippetsForAllCategories();
+  // Fetches snippets from the server for all remote categories and replaces old
+  // snippets 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.
+  void RefetchInTheBackground(const FetchStatusCallback& callback) override;
 
-  const NTPSnippetsFetcher* snippets_fetcher() const {
-    return snippets_fetcher_.get();
-  }
-
-  // (Re)schedules the periodic fetching of snippets. If |force| is true, the
-  // tasks will be re-scheduled even if they already exist and have the correct
-  // periods.
-  void RescheduleFetching(bool force);
+  const NTPSnippetsFetcher* snippets_fetcher_for_testing_and_debugging()
+      const override;
 
   // ContentSuggestionsProvider implementation
   CategoryStatus GetCategoryStatus(Category category) override;
@@ -166,6 +160,7 @@ class RemoteSuggestionsProvider final : public ContentSuggestionsProvider {
   void Fetch(const Category& category,
              const std::set<std::string>& known_suggestion_ids,
              const FetchDoneCallback& callback) override;
+  void ReloadSuggestions() override;
   void ClearHistory(
       base::Time begin,
       base::Time end,
@@ -205,11 +200,15 @@ class RemoteSuggestionsProvider final : public ContentSuggestionsProvider {
   friend class RemoteSuggestionsProviderTest;
 
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest,
+                           CallsProviderActiveCallbackWhenReady);
+  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest,
+                           CallsProviderActiveCallbackOnError);
+  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest,
+                           CallsProviderActiveCallbackWhenDisabled);
+  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest,
                            DontNotifyIfNotAvailable);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest,
                            RemoveExpiredDismissedContent);
-  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest,
-                           RescheduleOnStateChange);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest, StatusChanges);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest,
                            SuggestionsFetchedOnSignInAndSignOut);
@@ -292,8 +291,10 @@ class RemoteSuggestionsProvider final : public ContentSuggestionsProvider {
   // Fetches snippets from the server and replaces old snippets by the new ones.
   // Requests can be marked more important by setting |interactive_request| to
   // true (such request might circumvent the daily quota for requests, etc.)
-  // Useful for requests triggered by the user.
-  void FetchSnippets(bool interactive_request);
+  // Useful for requests triggered by the user. After the fetch finished, the
+  // provided |callback| will be triggered with the status of the fetch.
+  void FetchSnippets(bool interactive_request,
+                     const FetchStatusCallback& callback);
 
   // Returns the URL of the image of a snippet if it is among the current or
   // among the archived snippets in the matching category. Returns an empty URL
@@ -312,6 +313,7 @@ class RemoteSuggestionsProvider final : public ContentSuggestionsProvider {
 
   // Callback for regular fetch requests with the NTPSnippetsFetcher.
   void OnFetchFinished(
+      const FetchStatusCallback& callback,
       bool interactive_request,
       Status status,
       NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories);
@@ -408,12 +410,6 @@ class RemoteSuggestionsProvider final : public ContentSuggestionsProvider {
   // The ISO 639-1 code of the language used by the application.
   const std::string application_language_code_;
 
-  // Classifier that tells us how active the user is. Not owned.
-  const UserClassifier* user_classifier_;
-
-  // Scheduler for fetching snippets. Not owned.
-  NTPSnippetsScheduler* scheduler_;
-
   // The snippets fetcher.
   std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher_;
 
@@ -430,6 +426,11 @@ class RemoteSuggestionsProvider final : public ContentSuggestionsProvider {
   // Set to true if FetchSnippets is called while the service isn't ready.
   // The fetch will be executed once the service enters the READY state.
   bool fetch_when_ready_;
+  // The parameters for the fetch to perform later.
+  bool fetch_when_ready_interactive_;
+  FetchStatusCallback fetch_when_ready_callback_;

[Marc Treib, 2016/12/19 12:59:23]

Maybe package all these into a struct, and have a base::Optional instance of it
here?

[jkrcal, 2016/12/20 16:39:46]

I added a TODO. I really want to finish this piece.
(Actually I struggled for a while and reverted back. Not so straightforward
because the OnceCallback needs to be moved.)

+
+  ProviderStatusCallback provider_status_callback_;
 
   // Set to true if NukeAllSnippets is called while the service isn't ready.
   // The nuke will be executed once the service finishes initialization or
@@ -439,9 +440,9 @@ class RemoteSuggestionsProvider final : public ContentSuggestionsProvider {
   // A clock for getting the time. This allows to inject a clock in tests.
   std::unique_ptr<base::Clock> clock_;
 
-  DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsProvider);
+  DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsProviderImpl);
 };
 
 }  // namespace ntp_snippets
 
-#endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_H_
+#endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_IMPL_H_