Add favicon and publisher name to snippet cards

components/ntp_snippets/ntp_snippet.cc

Index: components/ntp_snippets/ntp_snippet.cc
diff --git a/components/ntp_snippets/ntp_snippet.cc b/components/ntp_snippets/ntp_snippet.cc
index dcac6d27908fa1b2f51124e3502818f6668543ed..ece9563a858f9ed1816ae5d1a07dd74ea04825b2 100644
--- a/components/ntp_snippets/ntp_snippet.cc
+++ b/components/ntp_snippets/ntp_snippet.cc
@@ -4,19 +4,23 @@
 
 #include "components/ntp_snippets/ntp_snippet.h"
 
+#include "base/memory/scoped_ptr.h"

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

Not needed anymore (bad merge?)

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

Done.

 #include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
 #include "base/values.h"
 
 namespace {
 
 const char kUrl[] = "url";
-const char kSiteTitle[] = "site_title";
+const char kSiteTitle[] = "sourceName";

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

nit: I'd move these two down to the other sourcecorpus stuff.

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

Done. Removed sourceLogoUrl since we don't use it.

 const char kTitle[] = "title";
-const char kFaviconUrl[] = "favicon_url";
+const char kFaviconUrl[] = "sourceLogoUrl";
 const char kSalientImageUrl[] = "thumbnailUrl";
 const char kSnippet[] = "snippet";
 const char kPublishDate[] = "creationTimestampSec";
 const char kExpiryDate[] = "expiryTimestampSec";
+const char kPublisherData[] = "publisherData";
+const char kCorpusId[] = "corpusId";
 const char kSourceCorpusInfo[] = "sourceCorpusInfo";
 const char kAmpUrl[] = "ampUrl";
 
@@ -43,15 +47,9 @@ std::unique_ptr<NTPSnippet> NTPSnippet::CreateFromDictionary(
 
   std::unique_ptr<NTPSnippet> snippet(new NTPSnippet(url));
 
-  std::string site_title;
-  if (dict.GetString(kSiteTitle, &site_title))
-    snippet->set_site_title(site_title);
   std::string title;
   if (dict.GetString(kTitle, &title))
     snippet->set_title(title);
-  std::string favicon_url;
-  if (dict.GetString(kFaviconUrl, &favicon_url))
-    snippet->set_favicon_url(GURL(favicon_url));
   std::string salient_image_url;
   if (dict.GetString(kSalientImageUrl, &salient_image_url))
     snippet->set_salient_image_url(GURL(salient_image_url));
@@ -67,17 +65,79 @@ std::unique_ptr<NTPSnippet> NTPSnippet::CreateFromDictionary(
     snippet->set_expiry_date(TimeFromJsonString(expiry_timestamp_str));
 
   const base::ListValue* corpus_infos_list = nullptr;
-  if (dict.GetList(kSourceCorpusInfo, &corpus_infos_list)) {
+  if (!dict.GetList(kSourceCorpusInfo, &corpus_infos_list)) {
+    DLOG(WARNING) << "No sources found for article " << url_str;

[Bernhard Bauer, 2016/04/27 13:22:05]

You could `return snippet` here to avoid having to indent the rest of the
method.

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

Done.

+  } else {
     for (base::Value* value : *corpus_infos_list) {
       const base::DictionaryValue* dict_value = nullptr;
       if (value->GetAsDictionary(&dict_value)) {

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

nit: "if (!Get...) continue;", to remove one level of indentation?

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

Done.

-        std::string amp_url;
-        if (dict_value->GetString(kAmpUrl, &amp_url)) {
-          snippet->set_amp_url(GURL(amp_url));
-          break;
+        std::string corpus_id_str;
+        GURL corpus_id;
+        if (dict_value->GetString(kCorpusId, &corpus_id_str)) {

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

You probably also want to handle the "else" here, i.e. the corpus ID is missing
or not a string?

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

Done.

+          corpus_id = GURL(corpus_id_str);
+          if (!corpus_id.is_valid()) {
+            // We must at least have a valid source URL
+            DLOG(WARNING) << "Invalid article url " << corpus_id.spec();

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

I think if a GURL is invalid, then it's spec will be empty. I'd use
corpus_id_str here.

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

Eep, good catch. Done.

+            continue;
+          }
         }
+
+        const base::DictionaryValue* publisher_data = nullptr;
+        std::string site_title;
+        if (dict_value->GetDictionary(kPublisherData, &publisher_data)) {

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

Also here: Do we want to warn also if the publisher data dict is missing?

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

Done.

+          if (!publisher_data->GetString(kSiteTitle, &site_title)) {
+            // It's possible but not desirable to have no publisher data
+            DLOG(WARNING) << "No publisher data for article "
+                         << corpus_id.spec();

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

nit: misaligned

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

Done.

+          }
+        }
+
+        std::string amp_url_str;
+        GURL amp_url;
+        // Expected to not have AMP url sometimes
+        if (dict_value->GetString(kAmpUrl, &amp_url_str)) {
+          amp_url = GURL(amp_url_str);
+          if (!amp_url.is_valid())
+            DLOG(WARNING) << "Invalid AMP url " << amp_url.spec();

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

nit: DLOG_IF

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

Done.

+        }
+        snippet->sources_.push_back(SnippetSource(
+            corpus_id, site_title, amp_url.is_valid() ? amp_url : GURL()));
       }
     }
+    // The previous url we have saved can be one of several sources for the
+    // article. For example, the same article can be hosted by nytimes.com,
+    // cnn.com, etc. We need to parse the list of sources for this article and
+    // find the best match. In order of preference:
+    //  1) A source that has url, publisher name, AMP url
+    //  2) A source that has url, publisher name
+    //  3) A source that has url and AMP url, or url only

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

3) ...so, just URL (which is required anyway), and we don't care about AMP?

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

yeah, clarified a bit in the comments.

+    if (snippet->sources_.size() > 0) {

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

nit: !.empty()

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

Done.

+      SnippetSource best_source_found = snippet->sources_[0];
+      for (size_t i = 1; i < snippet->sources_.size(); ++i) {
+        SnippetSource current_source = snippet->sources_[i];

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

const SnippetSource& ?

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

Done.

+        if (!best_source_found.publisher_name.empty()) {

[Bernhard Bauer, 2016/04/27 13:22:05]

I feel like this would be best done with a comparison function, and then just
picking the maximum element. If you wanted to get real fancy, you might be able
to do that with std::max_element, in fact.

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

Pshaw, you kids with your fancy al-gow-rhythms. Given the discussion today re:
asking ChromeReader to just give us a definitive source, I will wait on updating
the CR to do this as it's possible I will be updating the CR shortly to do
something completely different...

+          if (!best_source_found.amp_url.is_empty()) {
+            // We already have the best source
+            break;
+          }
+          if (!current_source.publisher_name.empty() &&
+              !current_source.amp_url.is_empty()) {
+            best_source_found = current_source;

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

Here we can break directly, no?

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

Done.

+          }
+        } else {
+          if (!current_source.publisher_name.empty())
+            best_source_found = current_source;
+        }
+      }
+
+      // The url from source_info is a url for a site that is one of the
+      // HOST_RESTRICT parameters, so if we have this, we need to replace the
+      // previously saved url with this one as this is hosted on a site that the
+      // user actually visits.
+      snippet->set_url(best_source_found.url);
+      snippet->set_site_title(best_source_found.publisher_name);
+      snippet->set_amp_url(best_source_found.amp_url);
+    }
   }
 
   return snippet;
@@ -101,14 +161,25 @@ std::unique_ptr<base::DictionaryValue> NTPSnippet::ToDictionary() const {
     dict->SetString(kPublishDate, TimeToJsonString(publish_date_));
   if (!expiry_date_.is_null())
     dict->SetString(kExpiryDate, TimeToJsonString(expiry_date_));
-  if (amp_url_.is_valid()) {
-    std::unique_ptr<base::ListValue> corpus_infos_list(new base::ListValue);
+
+  std::unique_ptr<base::ListValue> corpus_infos_list(new base::ListValue);
+  for (const SnippetSource source : sources_) {
     std::unique_ptr<base::DictionaryValue> corpus_info_dict(
         new base::DictionaryValue);
-    corpus_info_dict->SetString(kAmpUrl, amp_url_.spec());
-    corpus_infos_list->Set(0, std::move(corpus_info_dict));
-    dict->Set(kSourceCorpusInfo, std::move(corpus_infos_list));
+
+    corpus_info_dict->SetString(kCorpusId, source.url.spec());
+    if (!source.amp_url.is_empty())
+      corpus_info_dict->SetString(kAmpUrl, source.amp_url.spec());
+    if (!source.publisher_name.empty())
+      corpus_info_dict->SetString(
+          base::StringPrintf("%s.%s", kPublisherData, kSiteTitle),
+          source.publisher_name);
+
+    corpus_infos_list->Append(std::move(corpus_info_dict));
   }
+
+  dict->Set(kSourceCorpusInfo, std::move(corpus_infos_list));
+
   return dict;
 }