Add favicon and publisher name to snippet cards

components/ntp_snippets/ntp_snippet.h

 // Copyright 2015 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_NTP_SNIPPET_H_
 #define COMPONENTS_NTP_SNIPPETS_NTP_SNIPPET_H_
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "url/gurl.h"
 
 namespace base {
 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 {
  public:
   // Creates a new snippet with the given URL. URL must be valid.
   NTPSnippet(const GURL& url);
 
   ~NTPSnippet();
 
   // Creates an NTPSnippet from a dictionary. Returns a null pointer if the
   // dictionary doesn't contain at least a url. The keys in the dictionary are
   // expected to be the same as the property name, with exceptions documented in
   // the property comment.
   static std::unique_ptr<NTPSnippet> CreateFromDictionary(
       const base::DictionaryValue& dict);
 
   std::unique_ptr<base::DictionaryValue> ToDictionary() const;
 
   // 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_; }
   void set_site_title(const std::string& site_title) {
     site_title_ = site_title;
   }
 
   // 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; }
 
   // 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& snippet() const { return snippet_; }
   void set_snippet(const std::string& snippet) { snippet_ = snippet; }
@@ skipping 13 matching lines @@
     publish_date_ = publish_date;
   }
 
   // After this expiration date this snippet should no longer be presented to
   // the user.
   const base::Time& expiry_date() const { return expiry_date_; }
   void set_expiry_date(const base::Time& expiry_date) {
     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_;
   GURL salient_image_url_;
   std::string snippet_;
   base::Time publish_date_;
   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);
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_NTP_SNIPPET_H_