Add ContentSuggestionsService

components/ntp_snippets/content_suggestion.h

Index: components/ntp_snippets/content_suggestion.h
diff --git a/components/ntp_snippets/content_suggestion.h b/components/ntp_snippets/content_suggestion.h
index 60ab27285d39153f84cbc3440fa5346646f6a7d5..d7ab4293153e001955efa5c107e5df023a5ddbfb 100644
--- a/components/ntp_snippets/content_suggestion.h
+++ b/components/ntp_snippets/content_suggestion.h
@@ -23,21 +23,23 @@ namespace ntp_snippets {
 // (see ntp_snippet.h).
 class ContentSuggestion {
  public:
-  ContentSuggestion(const std::string& id,
-                    const ContentSuggestionsProviderType provider,
+  // This is the constructor that should be used by providers.
+  // Note that the |original_id| provided here is different from the ID that is
+  // returned from  |id()| because it will be combined with the |provider_type|
+  // to obtain an ID that is unique application-wide.
+  ContentSuggestion(const std::string& original_id,
+                    const ContentSuggestionsProviderType provider_type,

[tschumann, 2016/07/01 09:37:10]

IMO the suggestion constructor should only get a single ID. The service
shouldn't leak any kind of provider information.

[Philipp Keck, 2016/07/01 13:00:04]

I can't think of any good solution in which this constructor receives a single
ID. That would require the "merging" of the two to happen before the constructor
is called. The constructor is called by the provider. But we don't want to leak
the merging logic into all providers, so we would need a helper function
somewhere that does "my_provider_type+id=>combined_id", but that would leak just
as this constructor does.

The only thing that's leaking with this solution is an "incomprehensible"
constructor towards the UI, but the UI won't use any constructors of this class
anyway because it only consumes the instances.

Again, all of this would be resolved if we enforced a category->provider_type
mapping ;-) And that mapping wouldn't even need to be static, there just
couldn't be any mixing of suggestions into one category from different providers
at runtime. And that makes sense because "mixing" could only be done if there
was a sensible sorting criterion (like score), which ContentSuggestion of course
doesn't and shouldn't have. So if we mixed with the current code, what would
happen is: "aaaaabbbb" (all from provider A, then all from provider B). Then
provider A triggers an update and we have "bbbbaaaaa" (even if the a's and the
b's still are exactly the same) and so on.

Mixing into one category from different sources is not unrealistic. It could
especially happen with "articles for you", where many different sources could
suggest interesting articles, but they're all the same to the user. Like RSS
readers mix your RSS feeds.
Such mixing would still be possible if we require a category->provider_type
mapping, but it would need to happen in or before the provider and not in the
service. And due to the general nature of ContentSuggestion, that doesn't seem
like a bad thing (imagine an ArticlesMixerProvider that knows what articles are
and what scores are, etc.). Articles are also the only use case that I can think
of where a "merging" of the "same content URL" from different sources, like the
NTPSnippetsService does it today, makes any sense; so that logic could also live
in an ArticlesMixerProvider.

As long as such article mixing is required, I don't see any other use cases or
reasons why multiple providers should be able to feed a single category.

[Marc Treib, 2016/07/01 13:29:17]

For "Articles for you", the mixing will happen on the server. For all other
categories, we don't have any plans for mixing. So, once more: +1 for
category->provider mapping :)

[Philipp Keck, 2016/07/01 13:45:11]

Acknowledged.

                     const ContentSuggestionCategory category,
                     const GURL& url);
+  ContentSuggestion(ContentSuggestion&&);
+  ContentSuggestion& operator=(ContentSuggestion&&);
 
   ~ContentSuggestion();
 
   // An ID for identifying the suggestion. The ID is unique among all
-  // suggestions from the same provider, so to determine a globally unique
-  // identifier, combine this ID with the provider type.
+  // suggestions of all providers.
   const std::string& id() const { return id_; }
 
-  // The provider that created this suggestion.
-  ContentSuggestionsProviderType provider() const { return provider_; }
-
   // The category that this suggestion belongs to.
   ContentSuggestionCategory category() const { return category_; }
 
@@ -82,7 +84,6 @@ class ContentSuggestion {
 
  private:
   std::string id_;
-  ContentSuggestionsProviderType provider_;
   ContentSuggestionCategory category_;
   GURL url_;
   GURL amp_url_;