Change NTPSnippetsService to implement ContentSuggestionsProvider

components/ntp_snippets/ntp_snippets_service.h

Index: components/ntp_snippets/ntp_snippets_service.h
diff --git a/components/ntp_snippets/ntp_snippets_service.h b/components/ntp_snippets/ntp_snippets_service.h
index 5c0ed8c0a6a88c89120da5c94f692c040c1fea0d..c73ba6b650acfe8efcc25dd929095f0d4c1a49a7 100644
--- a/components/ntp_snippets/ntp_snippets_service.h
+++ b/components/ntp_snippets/ntp_snippets_service.h
@@ -19,6 +19,8 @@
 #include "base/timer/timer.h"
 #include "components/image_fetcher/image_fetcher_delegate.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/ntp_snippets/content_suggestion.h"
+#include "components/ntp_snippets/content_suggestions_provider.h"
 #include "components/ntp_snippets/ntp_snippet.h"
 #include "components/ntp_snippets/ntp_snippets_fetcher.h"
 #include "components/ntp_snippets/ntp_snippets_scheduler.h"
@@ -59,10 +61,11 @@ class NTPSnippetsServiceObserver;
 
 // Stores and vends fresh content data for the NTP.
 class NTPSnippetsService : public KeyedService,
-                           public image_fetcher::ImageFetcherDelegate {
+                           public image_fetcher::ImageFetcherDelegate,
+                           public ContentSuggestionsProvider {
  public:
   using ImageFetchedCallback =
-      base::Callback<void(const std::string& snippet_id, const gfx::Image&)>;
+      base::Callback<void(const std::string& suggestion_id, const gfx::Image&)>;
 
   // |application_language_code| should be a ISO 639-1 compliant string, e.g.
   // 'en' or 'en-US'. Note that this code should only specify the language, not
@@ -126,21 +129,17 @@ class NTPSnippetsService : public KeyedService,
   // the schedule depends on the time of day.
   void RescheduleFetching();
 
-  // Fetches the image for the snippet with the given |snippet_id| and runs the
-  // |callback|. If that snippet doesn't exist or the fetch fails, the callback
-  // gets a null image.
-  void FetchSnippetImage(const std::string& snippet_id,
-                         const ImageFetchedCallback& callback);
-
-  // Deletes all currently stored snippets.
-  void ClearSnippets();
-
-  // Discards the snippet with the given |snippet_id|, if it exists. Returns
-  // true iff a snippet was discarded.
-  bool DiscardSnippet(const std::string& snippet_id);
-
-  // Clears the lists of snippets previously discarded by the user.
-  void ClearDiscardedSnippets();
+  // ContentSuggestionsProvider implementation
+  // TODO(pke): At some point reorder the implementations in the .cc file
+  // accordingly.
+  void SetObserver(Observer* observer) override;
+  ContentSuggestionsCategoryStatus GetCategoryStatus(
+      ContentSuggestionsCategory category) override;
+  void DiscardSuggestion(const std::string& suggestion_id) override;
+  void FetchSuggestionImage(const std::string& suggestion_id,
+                            const ImageFetchedCallback& callback) override;
+  void ClearCachedSuggestionsForDebugging() override;
+  void ClearDiscardedSuggestionsForDebugging() override;
 
   // Returns the lists of suggestion hosts the snippets are restricted to.
   std::set<std::string> GetSuggestionsHosts() const;
@@ -254,8 +253,19 @@ class NTPSnippetsService : public KeyedService,
 
   void ClearDeprecatedPrefs();
 
+  // Converts the cached snippets to article content suggestions and notifies
+  // the observers.
+  void NotifyNewSuggestions();
+
+  // Notifies the content suggestions observer about a change in the
+  // |category_status_|.
+  void NotifyCategoryStatusChanged();
+
   State state_;
 
+  ContentSuggestionsCategoryStatus category_status_ =
+      ContentSuggestionsCategoryStatus::LOADING;

[Marc Treib, 2016/07/08 13:26:02]

Do we need both this and state_? Seems that it should be possible to infer one
from the other?

[Philipp Keck, 2016/07/08 14:02:55]

Yes, state_ and the "disabled reason" together are roughly equivalent to the
category_status_. I didn't want to change too much of the code at the moment
(especially because the NTPSnippetsStatusService is rather new). After the UI
starts talking to the ContentSuggestionsService instead of the
NTPSnippetsService, the state_ will become obsolete, the
NTPSnippetsStatusService will become available to all providers as a background
service (it gives providers the statuses of dependencies like sync, etc.) and
the NTPSnippetsService (ArticleSuggestionsProvider) will use that status service
to determine the correct category_status_. So then the entire old interface of
the NTPSnippetsService including the old observers and state_ can be removed.

[Marc Treib, 2016/07/08 15:17:23]

Hm. We have fairly nice handling of state transitions though, while
category_status_ is updated "wherever" right now. Can we unify updates to state_
and category_status_ somehow?

[Philipp Keck, 2016/07/08 16:17:01]

The NTPSnippetsService's state diagram is quite neat because it is so small. The
service could use the statuses for its state (grouping a lot of status to the
disabled state, using the new INITIALIZING state and using NOT_PROVIDED as the
shut-down state). I'm not sure though if it's a good idea to change that already
because the UI currently still accesses the state_ and expects it to behave in a
certain way. While the state_ might be derivable easily from the
category_status_, there are other changes to the state handling that could be
made when transforming the NTPSnippetsService to a pure provider (i.e. when it
does not have to deliver the state_ anymore): For example, the service could
first check if its dependencies (especially sync service) are met, and only if
that's the case it would load the database, etc. Changing that requires a lot of
refactoring and then testing to make sure that it works in all cases. So I'd
rather do it when I have to test anyway, which is when the UI is switched to
talk to the ContentSuggestionsService and understand the new statuses that come
from there. Currently, the statuses that the NTPSnippetsService reports to the
new service might not be perfectly accurate, but since nobody really relies on
that anyway (only me when viewing the debug outputs of
ContentSuggestionsService), it doesn't matter too much.

Finding a solid way to implement the states for the new
ArticleSuggestionsProvider is more important than unifying state_ and
category_status_ for the meantime. Maybe the new way will be cleaner if it
doesn't take the historically grown "state" and "status" of the
NTPSnippetsService as a reference.

[Marc Treib, 2016/07/11 08:41:33]

Alright. But I still don't like how category_status_ is updated in lots of
random places, and each of them needs to remember to call Notify..
Can we move the updating and notifying into EnterState? It looks like state_ and
category_status_ are always updated together now anyway.

[Philipp Keck, 2016/07/11 09:41:49]

Done.

+
   PrefService* pref_service_;
 
   suggestions::SuggestionsService* suggestions_service_;
@@ -271,8 +281,12 @@ class NTPSnippetsService : public KeyedService,
   const std::string application_language_code_;
 
   // The observers.
+  // TODO(pke): This is an old kind of observers that will be removed.
   base::ObserverList<NTPSnippetsServiceObserver> observers_;
 
+  // The observer to deliver data to.
+  Observer* observer_ = nullptr;
+
   // Scheduler for fetching snippets. Not owned.
   NTPSnippetsScheduler* scheduler_;
 
@@ -308,6 +322,10 @@ class NTPSnippetsService : public KeyedService,
 class NTPSnippetsServiceObserver {
  public:
   // Sent every time the service loads a new set of data.
+  // This is called in addition to the ContentSuggestionsProvider::Observer and
+  // should only be used by snippets-internals.
+  // TODO(pke): Remove this when snippets internals don't access this service

[Marc Treib, 2016/07/08 13:26:02]

The whole class should be removed, right? Then I'd move the comment up there.

[Philipp Keck, 2016/07/08 14:02:55]

Done.

+  // directly anymore.
   virtual void NTPSnippetsServiceLoaded() = 0;
 
   // Sent when the service is shutting down.