Add favicon and publisher name to snippet cards

components/ntp_snippets/ntp_snippet.cc

 // 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.
 
 #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";
 
 }  // namespace
 
 namespace ntp_snippets {
 
 NTPSnippet::NTPSnippet(const GURL& url) : url_(url) {
   DCHECK(url_.is_valid());
 }
 
 NTPSnippet::~NTPSnippet() {}
 
 // static
 std::unique_ptr<NTPSnippet> NTPSnippet::CreateFromDictionary(
     const base::DictionaryValue& dict) {
   // Need at least the url.
   std::string url_str;
   if (!dict.GetString("url", &url_str))
     return nullptr;
   GURL url(url_str);
   if (!url.is_valid())
     return nullptr;
 
   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));
   std::string snippet_str;
   if (dict.GetString(kSnippet, &snippet_str))
     snippet->set_snippet(snippet_str);
   // The creation and expiry timestamps are uint64s which are stored as strings.
   std::string creation_timestamp_str;
   if (dict.GetString(kPublishDate, &creation_timestamp_str))
     snippet->set_publish_date(TimeFromJsonString(creation_timestamp_str));
   std::string expiry_timestamp_str;
   if (dict.GetString(kExpiryDate, &expiry_timestamp_str))
     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;
 }
 
 std::unique_ptr<base::DictionaryValue> NTPSnippet::ToDictionary() const {
   std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue);
 
   dict->SetString(kUrl, url_.spec());

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

Hm. So the URL is now effectively stored twice, but the other per-source items
aren't.

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

yeah, mainly because the original url that we get from ChromeReader could be
different from all the source_urls. It's useful for deduping and debugging.

   if (!site_title_.empty())
     dict->SetString(kSiteTitle, site_title_);
   if (!title_.empty())
     dict->SetString(kTitle, title_);
   if (favicon_url_.is_valid())
     dict->SetString(kFaviconUrl, favicon_url_.spec());
   if (salient_image_url_.is_valid())
     dict->SetString(kSalientImageUrl, salient_image_url_.spec());
   if (!snippet_.empty())
     dict->SetString(kSnippet, snippet_);
   if (!publish_date_.is_null())
     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;
 }
 
 // static
 base::Time NTPSnippet::TimeFromJsonString(const std::string& timestamp_str) {
   int64_t timestamp;
   if (!base::StringToInt64(timestamp_str, &timestamp)) {
     // Even if there's an error in the conversion, some garbage data may still
     // be written to the output var, so reset it.
     timestamp = 0;
   }
   return base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(timestamp);
 }
 
 // static
 std::string NTPSnippet::TimeToJsonString(const base::Time& time) {
   return base::Int64ToString((time - base::Time::UnixEpoch()).InSeconds());
 }
 
 }  // namespace ntp_snippets