Add ContentSuggestionsService

components/ntp_snippets/content_suggestion.h

 // 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_SUGGESTION_H_
 #define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTION_H_
 
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "components/ntp_snippets/content_suggestion_category.h"
 #include "components/ntp_snippets/content_suggestions_provider_type.h"
 #include "url/gurl.h"
 
 namespace ntp_snippets {
 
 // A content suggestion for the new tab page, which can be an article or an
 // offline page, for example.
 // NOTE: This class is not yet in use, please use NTPSnippet for now
 // (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_; }
 
   // The normal content URL where the content referenced by the suggestion can
   // be accessed.
   const GURL& url() const { return url_; }
 
   // If available, this contains an URL to an AMP version of the same content.
   // Otherwise, this is an empty GURL().
   const GURL& amp_url() const { return amp_url_; }
@@ skipping 24 matching lines @@
   // TODO(pke) Remove the score from the ContentSuggestion class. The UI only
   // uses it to track user clicks (histogram data). Instead, the providers
   // should be informed about clicks and do appropriate logging themselves.
   // IMPORTANT: The score may simply be 0 for suggestions from providers which
   // cannot provide score values.
   float score() const { return score_; }
   void set_score(float score) { score_ = score; }
 
  private:
   std::string id_;
-  ContentSuggestionsProviderType provider_;
   ContentSuggestionCategory category_;
   GURL url_;
   GURL amp_url_;
   std::string title_;
   std::string snippet_text_;
   GURL salient_image_url_;
   base::Time publish_date_;
   std::string publisher_name_;
   float score_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentSuggestion);
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTION_H_