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..685b4c1e0a53627c883c7edc6d6506a6161f9a87
--- /dev/null
+++ b/components/ntp_snippets/content_suggestions_service.h
@@ -0,0 +1,163 @@
+// 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 <string>
+#include <vector>
+
+#include "base/callback_forward.h"
+#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_state.h"
+
+namespace gfx {
+class Image;
+}
+
+namespace ntp_snippets {
+
+// Retrieves suggestions from a number of ContentSuggestionsProviders and serves
+// them grouped into categories. There can be at most one provider per category.
+// NOTE: This class is not yet in use, please use NTPSnippetsService for now
+// (see ntp_snippets_service.h).
+class ContentSuggestionsService : public KeyedService,
+                                  public ContentSuggestionsProvider::Observer {
+ public:
+  using ImageFetchedCallback =
+      base::Callback<void(const std::string& suggestion_id, const gfx::Image&)>;
+
+  class Observer {
+   public:
+    // Fired every time the service receives a new set of data, replacing any
+    // previously available data (though in most cases there will be an overlap
+    // and only a few changes within the data). The new data is then available
+    // through the getters of the service.
+    virtual void OnNewSuggestions() = 0;
+
+    // Fired when the state of a suggestions category changed. If the state
+    // changes to an unavailable state, the suggestions of the respective
+    // category have been invalidated, which means that they must no longer be
+    // displayed to the user. The UI must immediately clear any suggestions of
+    // that category.
+    virtual void OnCategoryStateChanged(ContentSuggestionCategory category,
+                                        ContentSuggestionsState new_state) = 0;
+
+    // Sent when the service is shutting down. After the service has shut down,
+    // it will not provide any data anymore, though calling the getters is still
+    // safe.
+    virtual void ContentSuggestionsServiceShutdown() = 0;
+
+   protected:
+    virtual ~Observer() {}
+  };
+
+  enum Enabled : bool {

[Marc Treib, 2016/07/05 13:17:12]

This should still be called State or something like that.

[Philipp Keck, 2016/07/05 13:52:00]

Done. Maybe should really rename ContentSuggestionsState to
ContentSuggestionsStatus to avoid confusion.

+    ENABLE = true,
+    DISABLE = false,

[Marc Treib, 2016/07/05 13:17:12]

And these should be ENABLED and DISABLED - they describe a state, not an action.

[Philipp Keck, 2016/07/05 13:52:00]

Done.

+  };
+
+  ContentSuggestionsService(Enabled enabled);
+  ~ContentSuggestionsService() override;
+
+  // Inherited from KeyedService.
+  void Shutdown() override;
+
+  // Gets all categories for which a provider is registered. The categories
+  // may or may not be available, see |GetCategoryState()|.
+  const std::vector<ContentSuggestionCategory>& GetCategories() const {
+    return categories_;
+  }
+
+  // Gets the status of a category.
+  ContentSuggestionsState GetCategoryState(
+      ContentSuggestionCategory category) const;
+
+  // Gets the available suggestions for a category. The result is empty if the
+  // category is available and empty, but also if the category is unavailable
+  // for any reason, see |GetCategoryState()|.
+  const std::vector<ContentSuggestion>& GetSuggestionsForCategory(
+      ContentSuggestionCategory category) const;
+
+  // 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 FetchSuggestionImage(const std::string& suggestion_id,
+                            const ImageFetchedCallback& callback);
+
+  // Discards the suggestion with the given |suggestion_id|, if it exists.
+  // This will not trigger an update through the observers.
+  void DiscardSuggestion(const std::string& suggestion_id);
+
+  // Observer accessors.
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+  // Registers a new ContentSuggestionsProvider. This method will fail if there

[Marc Treib, 2016/07/05 13:17:12]

"will fail" sounds like some runtime error, like "return an error code" or
something like that. For hard constraints like this, use a formulation with
"must", like "no provider must already be registered...".

[Philipp Keck, 2016/07/05 13:52:00]

Done.

+  // is an overlap of the provided categories with another provider.
+  void RegisterProvider(ContentSuggestionsProvider* provider);
+
+  // Only for debugging use through the internals page.
+  // 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 ClearCachedSuggestionsForDebugging();
+
+  // 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 ClearDiscardedSuggestionsForDebugging();
+
+ private:
+  // Implementation of ContentSuggestionsProvider::Observer.
+  void OnNewSuggestions(ContentSuggestionCategory changed_category,
+                        std::vector<ContentSuggestion> suggestions) override;
+  void OnCategoryStateChanged(ContentSuggestionCategory changed_category,
+                              ContentSuggestionsState new_state) override;
+  void OnProviderShutdown(ContentSuggestionsProvider* provider) override;
+
+  bool CategoryExists(ContentSuggestionCategory category) const;

[Marc Treib, 2016/07/05 13:17:12]

CategoryIsRegistered?

[Philipp Keck, 2016/07/05 13:52:00]

Done. IsCategoryRegistered.

+
+  // Whether the content suggestions feature is enabled.
+  Enabled enabled_;
+
+  // All current suggestion categories, in order. This contains exactly the same
+  // categories as |providers_|.
+  std::vector<ContentSuggestionCategory> categories_;
+
+  // All current suggestions grouped by category. This contains an entry for
+  // every category in |categories_| whose state is an available state. It may
+  // contain an empty vector if the category is available but empty (or still
+  // loading).

[Marc Treib, 2016/07/05 13:17:12]

It might be easier now if it always contained a (possibly empty) vector for each
registered category? Then you could get rid of empty_categories_list_ (which is
poorly named btw...)

[Philipp Keck, 2016/07/05 13:52:00]

empty_categories_list_ will need to stay because there are the cases when the
service is disabled entirely and when a provider simply isn't registered. Also
in these cases we wouldn't want the call to this method to fail. (At least we
previously discussed that calling this method with a category that is not
returned from GetCategories() shouldn't fail.)
So what would be a better name for empty_categories_list_?

[Marc Treib, 2016/07/05 14:50:50]

It's not a list of categories, but a list of suggestions. So, |no_suggestions_|
maybe? (I don't much like putting the type in the name, in particular if it's
not even the same type :D)

[Philipp Keck, 2016/07/05 16:53:34]

Ah ok, I see what you mean. I named it after its usage, not after its
(non-)contents. Done.

+  std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>
+      suggestions_by_category_;
+
+  // All registered providers. A provider may be contained multiple times, if it
+  // provides multiple categories. The keys of this map are exactly the entries
+  // of |categories_|.
+  std::map<ContentSuggestionCategory, ContentSuggestionsProvider*> providers_;
+
+  // Map used to determine the category of a suggestion (of which only the ID
+  // is available). This also determines the provider that delivered the
+  // suggestion.
+  std::map<std::string, ContentSuggestionCategory> id_category_map_;

[Marc Treib, 2016/07/05 13:17:12]

Hm. So you need this because you don't want to parse the category out of the ID?
Not sure I like this any more... too many data structures here that have to be
kept in sync.

[Philipp Keck, 2016/07/05 13:52:00]

Tim's suggestion :-) Tim, wdyt?

I was already in doubt when he suggested it this morning. The advantage is that
the format of the ID now doesn't matter to anyone anymore and providers can, but
do not have to use MakeUniqueID().

One solution would be to add the parsing back in.
Another solution would be to add the category-parameter back on the FetchImage
and Discard methods accross the Android bridge, which is less problematic since
the category is present there anyways. Though if we didn't do that, we could
maybe even remove the .category() getter and field from the ContentSuggestion,
so that it's category would implicitly be determined by the data structure it
lives in. Currently, a provider could pass rubbish data into OnNewSuggestions()
because the category information is redundant (and nobody checks for that).

[Philipp Keck, 2016/07/05 14:28:34]

Tim still prefers more data structures over parsing and notes that we can merge
the data structures in the future if consistency becomes a problem.

[Marc Treib, 2016/07/05 14:50:50]

Alright.

Removing category() from ContentSuggestion sounds like a good idea. The way the
ContentSuggestionService API is built right now, it shouldn't be required.

[Philipp Keck, 2016/07/05 16:53:34]

Done.

+
+  base::ObserverList<Observer> observers_;
+
+  const std::vector<ContentSuggestion> empty_categories_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(ContentSuggestionsService);
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_