Add favicon and publisher name to snippet cards

components/ntp_snippets/ntp_snippet.h

Index: components/ntp_snippets/ntp_snippet.h
diff --git a/components/ntp_snippets/ntp_snippet.h b/components/ntp_snippets/ntp_snippet.h
index 688098417031b001a998594a0b29b9fa780c18ee..cd796ffc338ada6a9bf5966f28f022e66aaa0622 100644
--- a/components/ntp_snippets/ntp_snippet.h
+++ b/components/ntp_snippets/ntp_snippet.h
@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/time/time.h"
@@ -18,6 +19,16 @@ class DictionaryValue;
 
 namespace ntp_snippets {
 
+struct SnippetSource {
+  SnippetSource(const GURL& url,
+                const std::string& publisher_name,
+                const GURL& amp_url)
+      : url(url), publisher_name(publisher_name), amp_url(amp_url) {}
+  GURL url;
+  std::string publisher_name;
+  GURL amp_url;
+};
+
 // Stores and vend fresh content data for the NTP. This is a dumb class with no
 // smarts at all, all the logic is in the service.
 class NTPSnippet {
@@ -38,6 +49,7 @@ class NTPSnippet {
 
   // URL of the page described by this snippet.
   const GURL& url() const { return url_; }
+  void set_url(const GURL& url) { url_ = url; }
 
   // Subtitle to identify the site the snippet is from.
   const std::string& site_title() const { return site_title_; }
@@ -46,6 +58,7 @@ class NTPSnippet {
   }
 
   // Favicon for the site. Do not use to directly retrieve the favicon.
+  // Optional data

[Marc Treib, 2016/04/27 07:03:09]

So far all of the fields, except for the URL, have been optional. Are we
changing that? IIRC, we usually don't get expiry_date from ChromeReader for
example?

[May, 2016/04/27 16:45:09]

Removed favicon actually, since we don't get that data anymore from
ChromeReader. But to answer the general question: I count as 'optional' fields
that we don't need to display the snippet to the user. In your example, expiry
date is not optional (since we need it to know when to stop showing it to the
user). Also, we calculate that each time so it should never be blank. Given the
insistence that we only get data from ChromeReader that is "perfect", I'm taking
the route of erring on the side of only showing the user something that is
complete.

[Marc Treib, 2016/04/28 08:50:13]

Okay, makes sense, thanks!

   const GURL& favicon_url() const { return favicon_url_; }
   void set_favicon_url(const GURL& favicon_url) { favicon_url_ = favicon_url; }
 
@@ -79,15 +92,24 @@ class NTPSnippet {
     expiry_date_ = expiry_date;
   }
 
+  // Optional data
   const GURL& amp_url() const { return amp_url_; }
   void set_amp_url(const GURL& amp_url) { amp_url_ = amp_url; }
 
+  const std::vector<SnippetSource> get_sources() const { return sources_; }

[Marc Treib, 2016/04/27 07:03:09]

Return by ref? Also just "sources", no "get".

[May, 2016/04/27 16:45:08]

Done.

+
+  bool is_valid_snippet() const {

[Marc Treib, 2016/04/27 07:03:09]

nit: just "is_valid" please (though IMO "valid" might not be the right term -
isn't a snippet without publisher still valid for example?)

[May, 2016/04/27 16:45:09]

Changed to complete(). Better..?

[Marc Treib, 2016/04/28 08:50:13]

Yes, thank you!

+    return url_.is_valid() && !site_title_.empty() && !title_.empty() &&
+           !snippet_.empty() && salient_image_url_.is_valid() &&
+           !publish_date_.is_null() && !expiry_date_.is_null();
+  }
+
   // Public for testing.
   static base::Time TimeFromJsonString(const std::string& timestamp_str);
   static std::string TimeToJsonString(const base::Time& time);
 
  private:
-  const GURL url_;
+  GURL url_;
   std::string site_title_;
   std::string title_;
   GURL favicon_url_;
@@ -97,6 +119,8 @@ class NTPSnippet {
   base::Time expiry_date_;
   GURL amp_url_;

[Marc Treib, 2016/04/27 07:03:09]

A few fields are now effectively stored twice. Can we get rid of them here, and
just store a "best source index" instead?

[May, 2016/04/27 16:45:08]

Done. I kept url_ because we do have a url from ChromeReader that's separate
from the source info, it's just that we prefer the source info data.

[Marc Treib, 2016/04/28 08:50:13]

Alright, fair enough.

 
+  std::vector<SnippetSource> sources_;

[Marc Treib, 2016/04/27 07:03:09]

Or in fact: Do we actually need to store all the sources? Don't we just need the
best one?

[May, 2016/04/27 16:45:08]

I do it so that we can effectively de-dupe later as new snippets come in (see
crbug/603884). We can't guarantee that the sources we receive from ChromeReader
will always be in the same order (or that we have the same exact sources even --
it's possible that in a later fetch a new source came in for the same article)
so we won't always choose the same best source.

De-duping not in this CL yet, planning to submit a separate one for that.

[Marc Treib, 2016/04/28 08:50:13]

Acknowledged.

+
   DISALLOW_COPY_AND_ASSIGN(NTPSnippet);
 };