Add ContentSuggestion, ContentSuggestionCategory and ContentSuggestionProviderType

components/ntp_snippets/snippet.h

Index: components/ntp_snippets/snippet.h
diff --git a/components/ntp_snippets/snippet.h b/components/ntp_snippets/snippet.h
new file mode 100644
index 0000000000000000000000000000000000000000..82f2f9b39d49cde71ea54f8557725d3f70cf4357
--- /dev/null
+++ b/components/ntp_snippets/snippet.h
@@ -0,0 +1,103 @@
+// 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_SNIPPET_H_
+#define COMPONENTS_NTP_SNIPPETS_SNIPPET_H_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/time/time.h"
+#include "components/ntp_snippets/snippet_category.h"
+#include "components/ntp_snippets/snippet_provider_type.h"
+#include "url/gurl.h"
+
+namespace ntp_snippets {
+
+// A snippet for the new tab page, which can be an article, an offline page or
+// a snippet from another source.

[Marc Treib, 2016/06/13 15:11:10]

nit: I wouldn't say "source", since that term is too overloaded. Maybe just
"which can be e.g. an article or an offline page." ?

[Philipp Keck, 2016/06/13 15:27:50]

Done.

+class Snippet {
+ public:
+  using PtrVector = std::vector<std::unique_ptr<Snippet>>;
+
+  // Creates a new snippet with the given |id|.
+  Snippet(const std::string& id,
+          const SnippetProviderType provider,
+          const SnippetCategory category);

[Marc Treib, 2016/06/13 15:11:10]

Is the idea here to pass in all strictly-required fields? If so, URL should
probably be added.
In general, we should clearly define which fields we consider mandatory and
which are optional. That can happen later though (the old NTPSnippet also
doesn't really do that).

[Philipp Keck, 2016/06/13 15:27:50]

If the two enum values are not passed in, they need to be initialized with
something else (like 0). Would that be okay?
Otherwise, I see no other solution but to pass them in here. Then adding all
required fields to the constructor seems like a good idea.

My intention was: Constructor gets everything for the object to be able to exist
(though required fields maybe should be part of that) and setters exist for all
fields that are not necessarily constant.

In Java, I'd make all fields final, no setters, and overloaded constructors or
factory methods to account for optional fields. But C++ style seems to be
different.

[Marc Treib, 2016/06/13 15:30:42]

No, initializing to 0 doesn't seem like a good idea.
But I wasn't complaining about those being passed in, I was saying that maybe
more things should be passed to the constructor :)
Your intention was exactly right, the question is just what exactly we consider
"able to exist". I'd argue that at least the URL is necessary for that.

[Philipp Keck, 2016/06/13 15:38:56]

Maybe also the title, as it doesn't make much sense to display a card on the UI
without a title. Unless there might be cards with only text_extract but without
title? Seems unlikely, the only cards of this kind that Google Now has, for
example, are "read more" cards, and sth. like that probably wouldn't be backed
by the Snippet class.

amp_url and score are clearly optional and the remaining fields are almost
always present, but not "required to be able to display a meaningful card".

[Marc Treib, 2016/06/13 15:54:35]

text_extract will definitely be optional (since Offline Pages cards won't have
it at first); not sure about title.
Maybe it's best to keep the ctor as-is (pass in only the things that don't have
any reasonable "empty" state), and maybe add an "is_complete" method, like
NTPSnippet has? WDYT?

[Philipp Keck, 2016/06/13 16:12:20]

I will go by your previous suggestion. The constructor will take ID,
provider_type, category and url. Those will be unchangeable (no setters). An
additional reason for this is the fact that the mixer service or the UI may need
to rely on the URL being unchangeable (just like those other three fields) for
properly merging identical content from different providers.

+
+  ~Snippet();
+
+  // An ID for identifying the snippet. The ID is unique among all snippets
+  // from the same provider, so to determine a globally unique identifier,
+  // combine this ID with the provider type.
+  const std::string& id() const { return id_; }
+
+  // The provider that created this snippet.
+  SnippetProviderType provider() const { return provider_; }
+
+  // The category that this snippet belongs to.
+  SnippetCategory category() const { return category_; }
+  void set_category(const SnippetCategory category) { category_ = category; }

[Marc Treib, 2016/06/13 15:11:10]

I think this setter isn't required?

[Philipp Keck, 2016/06/13 15:27:50]

Depends on decision above.

[Philipp Keck, 2016/06/13 16:12:20]

Done.

+
+  // The normal content URL where the content referenced by the snippet can be
+  // accessed.
+  const GURL& url() const { return url_; }
+  void set_url(const GURL& url) { url_ = 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_; }
+  void set_amp_url(const GURL& amp_url) { amp_url_ = amp_url; }
+
+  // Title of the snippet.
+  const std::string& title() const { return title_; }
+  void set_title(const std::string& title) { title_ = title; }
+
+  // Summary or relevant extract from the content.
+  const std::string& text_extract() const { return text_extract_; }
+  void set_text_extract(const std::string& text_extract) {
+    text_extract_ = text_extract;
+  }
+
+  // The time when the content represented by this snippet was published.
+  const base::Time& publish_date() const { return publish_date_; }
+  void set_publish_date(const base::Time& publish_date) {
+    publish_date_ = publish_date;
+  }
+
+  // The name of the source/publisher of this snippet.
+  const std::string& publisher_name() const { return publisher_name_; }
+  void set_publisher_name(const std::string& publisher_name) {
+    publisher_name_ = publisher_name;
+  }
+
+  // TODO(pke) Remove the score from the Snippet 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 snippets from providers which
+  // cannot provide score values.
+  float score() const { return score_; }
+  void set_score(float score) { score_ = score; }
+
+ private:
+  std::string id_;
+  SnippetProviderType provider_;
+  SnippetCategory category_;
+  GURL url_;
+  GURL amp_url_;
+  std::string title_;
+  std::string text_extract_;
+  GURL salient_image_url_;
+  base::Time publish_date_;
+  std::string publisher_name_;
+  float score_;
+
+  DISALLOW_COPY_AND_ASSIGN(Snippet);
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_SNIPPET_H_