Add ContentSuggestionsService

components/ntp_snippets/content_suggestions_service.h

Index: components/ntp_snippets/content_suggestions_service.h
diff --git a/components/ntp_snippets/content_suggestions_service.h b/components/ntp_snippets/content_suggestions_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..25ec7be97a3f037cdba884959880a0bd595354fb
--- /dev/null
+++ b/components/ntp_snippets/content_suggestions_service.h
@@ -0,0 +1,138 @@
+// Copyright 2016 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_CONTENT_SUGGESTIONS_SERVICE_H_
+#define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_
+
+#include <stddef.h>
+
+#include <map>
+#include <set>

[Marc Treib, 2016/06/28 11:29:10]

not needed

[Philipp Keck, 2016/06/28 14:18:57]

Done.

+#include <string>
+#include <vector>
+

[Marc Treib, 2016/06/28 11:29:10]

#include "base/callback_forward.h"

[Philipp Keck, 2016/06/28 14:18:56]

Done.

+#include "base/observer_list.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "components/ntp_snippets/content_suggestions_provider.h"
+#include "components/ntp_snippets/content_suggestions_provider_type.h"
+
+namespace gfx {
+class Image;
+}
+
+namespace ntp_snippets {
+
+class ContentSuggestionsServiceObserver;
+
+// Retrieves suggestions from a number of ContentSuggestionsProviders and serves
+// them to the NTP.

[Marc Treib, 2016/06/28 11:29:09]

I'd remove the reference to the NTP - that is not the service's business.
Instead, you *could* give some more info on what it does, like grouping into
categories, possibly ranking (though it doesn't really do that yet).

[Philipp Keck, 2016/06/28 14:18:56]

Done.

+class ContentSuggestionsService : public KeyedService,
+                                  public ContentSuggestionsProvider::Delegate {
+ public:
+  using ImageFetchedCallback =
+      base::Callback<void(const std::string& suggestion_id, const gfx::Image&)>;
+
+  ContentSuggestionsService(bool enabled);
+  ~ContentSuggestionsService() override;
+
+  // Inherited from KeyedService.
+  void Shutdown() override;
+
+  // Available suggestions
+  const std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>&

[Marc Treib, 2016/06/28 11:29:09]

Does this need to be exposed at all? If so, maybe worth a typedef ("using ... =
std::map<...>"), though I'm leaning towards not exposing it at all. E.g. what
about the order of categories? Like this, the service has no way of influencing
it.

[Philipp Keck, 2016/06/28 14:18:56]

You're right, but I'm not sure which solution is the best.
A "GetCategories" would make sense if the service had an idea of which
categories are enabled at all. Otherwise it would just return the categories
enum, which would probably be hard-coded in the UI anyway.

Iirc, we decided that the service doesn't decide the order of the suggestions
(within a category). So probably it wouldn't decided the order of the categories
either?

Depending on how much the service knows about and does with categories, I'd
either just remove this method and only use GetSuggestionsForCategory, or
rethink how the category handling works. Maybe we need an "OnCategoryAdded"
event, etc.?

[Marc Treib, 2016/06/28 15:04:55]

No, it would go over suggestions_by_category_ and collect all categories that
have any suggestions.
I think it will, because who else would? But whoever makes the decision in the
end, the service will have to expose it to the UI.
I'm all for removing this method and just exposing GetSuggestionsForCategory.
I don't see why we'd need OnCategoryAdded - the existing OnSuggestionsChanged
should cover that already?

[Philipp Keck, 2016/06/30 09:35:35]

Done. Changed OnSuggestionsChanged to not be called per-category.

+  suggestions() const {
+    return suggestions_;
+  }
+  const std::vector<ContentSuggestion>& suggestions(

[Marc Treib, 2016/06/28 11:29:10]

Please give this a better name, something like "GetSuggestionsForCategory"?
Also, to make it useful, there really should be a "GetCategories" too.

[Philipp Keck, 2016/06/28 14:18:56]

Acknowledged.

[Philipp Keck, 2016/06/30 09:35:35]

Done.

+      ContentSuggestionCategory category) {

[Marc Treib, 2016/06/28 11:29:09]

Should be const.

[Philipp Keck, 2016/06/28 14:18:56]

Acknowledged.

+    return suggestions_[category];
+  }
+
+  // Fetches the image for the suggestion with the given |suggestion_id| and
+  // runs the |callback|. If that suggestion doesn't exist or the fetch fails,
+  // the callback gets an empty image.
+  void FetchImage(const ContentSuggestionsProviderType provider_type,
+                  const std::string& suggestion_id,
+                  const ImageFetchedCallback& callback);
+
+  // Removes all suggestions from all caches or internal stores in all
+  // providers. It does, however, not remove any suggestions from the provider's
+  // sources, so if their configuration hasn't changed, they should return the
+  // same results when they fetch the next time. In particular, calling this
+  // method will not mark any suggestions as discarded.
+  void ClearSuggestions();

[Marc Treib, 2016/06/28 11:29:09]

I think this is also used only for debugging purposes?
(ClearDiscardedSuggestions has a comment saying that)

Maybe rename to ClearCachedSuggestions, since that's closer to what actually
happens?

[Philipp Keck, 2016/06/28 14:18:57]

Done, renamed here and in the provider interface.

+
+  // Only for debugging use through the internals page. Some providers
+  // internally store a list of discarded suggestions to prevent them from
+  // reappearing. This function clears all such lists in all providers, making
+  // discarded suggestions reappear (only for certain providers).
+  void ClearDiscardedSuggestions();
+
+  // Discards the suggestion with the given |suggestion_id|, if it exists.
+  void Discard(const ContentSuggestionsProviderType provider_type,

[Marc Treib, 2016/06/28 11:29:09]

No "const" if you pass by value.

[Philipp Keck, 2016/06/28 14:18:56]

Done, fixed here and in FetchImage() above.

+               const std::string& suggestion_id);
+
+  // Observer accessors (outgoing observers, from this service towards UI)

[Marc Treib, 2016/06/28 11:29:09]

I'd remove the part in parens - "outgoing" is obvious, that's what observers are
for, and "towards the UI" is not necessarily correct, and anyway not the
service's responsibility.
Also, nit: period after comment.

[Philipp Keck, 2016/06/28 14:18:56]

Done.

+  void AddObserver(ContentSuggestionsServiceObserver* observer);
+  void RemoveObserver(ContentSuggestionsServiceObserver* observer);
+
+  // Implementation of ContentSuggestionsProvider::Delegate (incoming data)

[Marc Treib, 2016/06/28 11:29:09]

These can (and should) be private.
Also, nit: period after comment.

[Philipp Keck, 2016/06/28 14:18:56]

Aha ok? Done.

+  void OnContentChanged(const ContentSuggestionsProvider& source,
+                        ContentSuggestionCategory changed_category,
+                        std::vector<ContentSuggestion> suggestions) override;
+  void OnProviderShutdown(const ContentSuggestionsProvider& source) override;
+
+  // Registers a new ContentSuggestionsProvider. The service will call
+  // ContentSuggestionsProvider::setDelegate with a delegate object, through

[Marc Treib, 2016/06/28 11:29:10]

I'd remove the "The service..." sentence, since it's not really relevant for the
interface.

[Philipp Keck, 2016/06/28 14:18:57]

Done.

+  // which the registered provider should report its new suggestions. This
+  // method will fail when a provider of the same type is already registered.
+  void RegisterProvider(ContentSuggestionsProvider* provider);
+
+  // For use by the snippets-internals page only.
+  const std::map<ContentSuggestionsProviderType, ContentSuggestionsProvider*>&
+  providers() const {
+    return providers_;
+  }
+
+ private:
+  // When |enabled_| is true the service will retrieve suggestions from the
+  // providers.
+  bool enabled_;
+
+  // All current suggestions.
+  std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>
+      suggestions_;

[Marc Treib, 2016/06/28 11:29:09]

suggestions_by_category_?

[Philipp Keck, 2016/06/28 14:18:56]

Done.

+
+  std::map<ContentSuggestionsProviderType, ContentSuggestionsProvider*>
+      providers_;
+
+  // The observers.
+  base::ObserverList<ContentSuggestionsServiceObserver> observers_;
+
+  // Helper method to remove all suggestions of the given |provider_type| from
+  // the given |list|. Returns true if anything changed.
+  bool RemoveSuggestionsOfProvider(
+      std::vector<ContentSuggestion>* list,
+      ContentSuggestionsProviderType provider_type);

[Marc Treib, 2016/06/28 11:29:10]

Private methods come before member variables. But this probably doesn't need to
be a member at all, in which case it should be a function in an anonymous
namespace in the .cc file.

[Philipp Keck, 2016/06/28 14:18:56]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(ContentSuggestionsService);
+};
+
+class ContentSuggestionsServiceObserver {

[Marc Treib, 2016/06/28 11:29:10]

I'd make this an inner class of the service (just called Observer).

AFAIK, the main motivation of making an observer a separate class is that it can
be moved to a separate file, so clients can include only the observer without
getting the whole service too. We could do that too here, but I don't see a
particular reason to.

[Philipp Keck, 2016/06/28 14:18:56]

This implementation is analogous to NTPSnippetsService, so the reasons for
having a separate would be the same.

[Marc Treib, 2016/06/28 15:04:55]

I don't know of any reasons for having it separate there either :)
Also, the SnippetsServiceObserver will probably go away? (or rather, be replaced
by Provider::Delegate)

[Philipp Keck, 2016/06/30 09:35:35]

Yes, as soon as the internals page is able to get the info it needs without the
NTPSnippetsServiceObserver and as soon as the UI gets the proper
disabled/shutdown from the new service, the old observer will go away. So I'll
just make this one an inner class and not change the existing one.

+ public:
+  // Fired every time the service loads a new set of data. This event is fired
+  // per |changed_category|. The new data is then available through
+  // |suggestions()[changed_category]|.

[Marc Treib, 2016/06/28 11:29:09]

Please update to refer to the (proposed) GetSuggestionsForCategory method.

[Philipp Keck, 2016/06/28 14:18:56]

Acknowledged.

+  virtual void OnSuggestionsChanged(
+      ContentSuggestionCategory changed_category) = 0;
+  // Sent when the service is shutting down.
+  virtual void ContentSuggestionsServiceShutdown() = 0;
+
+ protected:
+  virtual ~ContentSuggestionsServiceObserver() {}
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_