Add favicon and publisher name to snippet cards

components/ntp_snippets/ntp_snippets_service_unittest.cc

Index: components/ntp_snippets/ntp_snippets_service_unittest.cc
diff --git a/components/ntp_snippets/ntp_snippets_service_unittest.cc b/components/ntp_snippets/ntp_snippets_service_unittest.cc
index b8f4e7161472b1adbccfbfe46d54e92b7a199f5e..fba0294ad225ce408f92edff3acb38ae7948bce0 100644
--- a/components/ntp_snippets/ntp_snippets_service_unittest.cc
+++ b/components/ntp_snippets/ntp_snippets_service_unittest.cc
@@ -5,6 +5,7 @@
 #include "components/ntp_snippets/ntp_snippets_service.h"
 
 #include <memory>
+#include <vector>
 
 #include "base/json/json_reader.h"
 #include "base/macros.h"
@@ -41,16 +42,15 @@ std::string GetTestJson(const std::string& content_creation_time_str,
       "{ \"recos\": [ "
       "{ \"contentInfo\": {"
       "\"url\" : \"http://localhost/foobar\","
-      "\"site_title\" : \"Site Title\","
-      "\"favicon_url\" : \"http://localhost/favicon\","
       "\"title\" : \"Title\","
       "\"snippet\" : \"Snippet\","
       "\"thumbnailUrl\" : \"http://localhost/salient_image\","
       "\"creationTimestampSec\" : \"%s\","
       "\"expiryTimestampSec\" : \"%s\","
       "\"sourceCorpusInfo\" : [ "
-      "{\"ampUrl\" : \"http://localhost/amp\"},"
-      "{\"corpusId\" : \"id\"}]"
+      "{\"ampUrl\" : \"http://localhost/amp\","
+      "\"corpusId\" : \"http://localhost/foobar\","
+      "\"publisherData\": { \"sourceName\" : \"Foo News\"}}]"
       "}}"
       "]}";
 
@@ -58,6 +58,58 @@ std::string GetTestJson(const std::string& content_creation_time_str,
                             expiry_time_str.c_str());
 }
 
+std::string GetTestJsonWithSources(const std::string& content_creation_time_str,
+                                   const std::string& expiry_time_str,
+                                   const std::vector<std::string>& source_urls,
+                                   const std::vector<std::string>& publishers,
+                                   const std::vector<std::string>& amp_urls) {
+  char json_str_format[] =
+      "{ \"recos\": [ "
+      "{ \"contentInfo\": {"
+      "\"url\" : \"http://localhost/foobar\","
+      "\"title\" : \"Title\","
+      "\"snippet\" : \"Snippet\","
+      "\"thumbnailUrl\" : \"http://localhost/salient_image\","
+      "\"creationTimestampSec\" : \"%s\","
+      "\"expiryTimestampSec\" : \"%s\","
+      "\"sourceCorpusInfo\" : [%s]"
+      "}}"
+      "]}";
+
+  char source_corpus_info_format[] =
+      "{\"corpusId\": \"%s\","
+      "\"publisherData\": {"
+      "\"sourceName\": \"%s\""
+      "},"
+      "\"ampUrl\": \"%s\"}";
+
+  std::string source_corpus_info_list_str;
+  for (size_t i = 0; i < source_urls.size(); ++i) {
+    std::string source_corpus_info_str =
+        base::StringPrintf(source_corpus_info_format,
+                           source_urls[i].empty() ? "" : source_urls[i].c_str(),
+                           publishers[i].empty() ? "" : publishers[i].c_str(),
+                           amp_urls[i].empty() ? "" : amp_urls[i].c_str());
+    source_corpus_info_list_str.append(source_corpus_info_str);
+    source_corpus_info_list_str.append(",");
+  }
+  // Remove the last comma
+  source_corpus_info_list_str.pop_back();
+  return base::StringPrintf(json_str_format, content_creation_time_str.c_str(),
+                            expiry_time_str.c_str(),
+                            source_corpus_info_list_str.c_str());
+}
+
+std::string GetTestJsonWithSources(const std::vector<std::string>& source_urls,
+                                   const std::vector<std::string>& publishers,
+                                   const std::vector<std::string>& amp_urls) {
+  base::Time expiry_time = base::Time::Now() + base::TimeDelta::FromHours(1);
+  return GetTestJsonWithSources(
+      NTPSnippet::TimeToJsonString(GetDefaultCreationTime()),
+      NTPSnippet::TimeToJsonString(expiry_time), source_urls, publishers,
+      amp_urls);
+}
+
 std::string GetTestJson(const std::string& content_creation_time_str) {
   base::Time expiry_time = base::Time::Now() + base::TimeDelta::FromHours(1);
   return GetTestJson(content_creation_time_str,
@@ -219,14 +271,13 @@ TEST_F(NTPSnippetsServiceTest, Full) {
   for (auto& snippet : *service()) {
     // Snippet here is a const.
     EXPECT_EQ(snippet.url(), GURL("http://localhost/foobar"));
-    EXPECT_EQ(snippet.site_title(), "Site Title");
-    EXPECT_EQ(snippet.favicon_url(), GURL("http://localhost/favicon"));
+    EXPECT_EQ(snippet.site_title(), "Foo News");
     EXPECT_EQ(snippet.title(), "Title");
     EXPECT_EQ(snippet.snippet(), "Snippet");
     EXPECT_EQ(snippet.salient_image_url(),
               GURL("http://localhost/salient_image"));
     EXPECT_EQ(GetDefaultCreationTime(), snippet.publish_date());
-    EXPECT_EQ(snippet.amp_url(), GURL("http://localhost/amp"));
+    EXPECT_EQ(snippet.amp_url().spec(), GURL("http://localhost/amp").spec());
   }
 }
 
@@ -241,18 +292,28 @@ TEST_F(NTPSnippetsServiceTest, Clear) {
 }
 
 TEST_F(NTPSnippetsServiceTest, InsertAtFront) {
+  std::vector<std::string> source_urls;

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

I think you could use an initializer list here to construct the vector in one
go?

[Bernhard Bauer, 2016/04/28 14:19:20]

Ping :)

[May, 2016/04/28 18:01:52]

Done.

+  source_urls.push_back(std::string("http://first"));
+  std::vector<std::string> publishers;
+  publishers.push_back(std::string("Source 1"));
+  std::vector<std::string> amp_urls;
+  amp_urls.push_back(std::string(""));

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

The empty string literal is unnecessary.

[Bernhard Bauer, 2016/04/28 14:19:20]

Ping :)

[May, 2016/04/28 18:01:52]

Done.

   std::string json_str(
-      "{ \"recos\": [ "
-      "  { \"contentInfo\": { \"url\" : \"http://first\" }}"
-      "]}");
+      GetTestJsonWithSources(source_urls, publishers, amp_urls));
+
   LoadFromJSONString(json_str);
+
   ASSERT_EQ(service()->size(), 1u);
 
-  std::string json_str2(
-      "{ \"recos\": [ "
-      "  { \"contentInfo\": { \"url\" : \"http://second\" }}"
-      "]}");
-  LoadFromJSONString(json_str2);
+  source_urls.clear();
+  source_urls.push_back(std::string("http://second"));
+  publishers.clear();
+  publishers.push_back(std::string("Source 1"));
+  amp_urls.clear();
+  amp_urls.push_back(std::string(""));
+  json_str = GetTestJsonWithSources(source_urls, publishers, amp_urls);
+
+  LoadFromJSONString(json_str);
   ASSERT_EQ(service()->size(), 2u);
 
   // The snippet loaded last should be at the first position in the list now.
@@ -314,8 +375,15 @@ TEST_F(NTPSnippetsServiceTest, LoadIncompleteJsonWithExistingSnippets) {
 }
 
 TEST_F(NTPSnippetsServiceTest, Discard) {
+  std::vector<std::string> source_urls;
+  source_urls.push_back(std::string("http://site.com"));
+  std::vector<std::string> publishers;
+  publishers.push_back(std::string("Source 1"));
+  std::vector<std::string> amp_urls;
+  amp_urls.push_back(std::string(""));
   std::string json_str(
-      "{ \"recos\": [ { \"contentInfo\": { \"url\" : \"http://site.com\" }}]}");
+      GetTestJsonWithSources(source_urls, publishers, amp_urls));
+
   LoadFromJSONString(json_str);
 
   ASSERT_EQ(1u, service()->size());
@@ -345,17 +413,15 @@ TEST_F(NTPSnippetsServiceTest, Discard) {
 }
 
 TEST_F(NTPSnippetsServiceTest, GetDiscarded) {
-  std::string json_str(
-      "{ \"recos\": [ { \"contentInfo\": { \"url\" : \"http://site.com\" }}]}");
-  LoadFromJSONString(json_str);
+  LoadFromJSONString(GetTestJson());
 
   // For the test, we need the snippet to get discarded.
-  ASSERT_TRUE(service()->DiscardSnippet(GURL("http://site.com")));
+  ASSERT_TRUE(service()->DiscardSnippet(GURL("http://localhost/foobar")));
   const NTPSnippetsService::NTPSnippetStorage& snippets =
       service()->discarded_snippets();
   EXPECT_EQ(1u, snippets.size());
   for (auto& snippet : snippets) {
-    EXPECT_EQ(GURL("http://site.com"), snippet->url());
+    EXPECT_EQ(GURL("http://localhost/foobar"), snippet->url());
   }
 
   // There should be no discarded snippet after clearing the list.
@@ -386,4 +452,258 @@ TEST_F(NTPSnippetsServiceTest, RemoveExpiredContent) {
   EXPECT_EQ(service()->size(), 0u);
 }
 
+TEST_F(NTPSnippetsServiceTest, TestSingleSource) {
+  std::vector<std::string> source_urls;
+  source_urls.push_back(std::string("http://source1.com"));
+  std::vector<std::string> publishers;
+  publishers.push_back(std::string("Source 1"));
+  std::vector<std::string> amp_urls;
+  amp_urls.push_back(std::string("http://source1.amp.com"));
+  std::string json_str(
+      GetTestJsonWithSources(source_urls, publishers, amp_urls));
+
+  LoadFromJSONString(json_str);
+  EXPECT_EQ(service()->size(), 1u);
+
+  for (auto& snippet : *service()) {
+    EXPECT_EQ(snippet.url(), GURL("http://source1.com"));
+    EXPECT_EQ(snippet.site_title(), std::string("Source 1"));
+    EXPECT_EQ(snippet.amp_url(), GURL("http://source1.amp.com"));
+    EXPECT_EQ(snippet.get_sources().size(), 1u);
+  }
+}
+
+TEST_F(NTPSnippetsServiceTest, TestSingleSourceWithMalformedUrl) {
+  std::vector<std::string> source_urls;
+  source_urls.push_back(std::string("aaaa"));
+  std::vector<std::string> publishers;
+  publishers.push_back(std::string("Source 1"));
+  std::vector<std::string> amp_urls;
+  amp_urls.push_back(std::string("http://source1.amp.com"));
+  std::string json_str(
+      GetTestJsonWithSources(source_urls, publishers, amp_urls));
+
+  LoadFromJSONString(json_str);
+// If we're running a release build, we only add snippets that have a well-
+// formed source url
+#if !DCHECK_IS_ON()
+  EXPECT_EQ(service()->size(), 0u);
+#else
+  EXPECT_EQ(service()->size(), 1u);
+
+  for (auto& snippet : *service()) {
+    EXPECT_EQ(snippet.site_title(), std::string());
+    EXPECT_EQ(snippet.get_sources().size(), 0u);
+  }
+#endif
+}
+
+TEST_F(NTPSnippetsServiceTest, TestSingleSourceWithMissingData) {
+  std::vector<std::string> source_urls;
+  source_urls.push_back(std::string("http://source1.com"));
+  std::vector<std::string> publishers;
+  publishers.push_back(std::string(""));
+  std::vector<std::string> amp_urls;
+  amp_urls.push_back(std::string(""));
+  std::string json_str(
+      GetTestJsonWithSources(source_urls, publishers, amp_urls));
+
+  LoadFromJSONString(json_str);
+// If we're running a release build, we only add snippets have a source url
+// and publisher
+#if !DCHECK_IS_ON()
+  EXPECT_EQ(service()->size(), 0u);
+#else
+  EXPECT_EQ(service()->size(), 1u);
+
+  for (auto& snippet : *service()) {
+    EXPECT_EQ(snippet.url(), GURL("http://source1.com"));
+    EXPECT_EQ(snippet.site_title(), std::string());
+    EXPECT_EQ(snippet.amp_url(), GURL());
+    EXPECT_EQ(snippet.get_sources().size(), 1u);
+  }
+#endif
+}
+
+TEST_F(NTPSnippetsServiceTest, TestMultipleSources) {
+  std::vector<std::string> source_urls;
+  source_urls.push_back(std::string("http://source1.com"));
+  source_urls.push_back(std::string("http://source2.com"));
+  std::vector<std::string> publishers;
+  publishers.push_back(std::string("Source 1"));
+  publishers.push_back(std::string("Source 2"));
+  std::vector<std::string> amp_urls;
+  amp_urls.push_back(std::string("http://source1.amp.com"));
+  amp_urls.push_back(std::string("http://source2.amp.com"));
+  std::string json_str(
+      GetTestJsonWithSources(source_urls, publishers, amp_urls));
+
+  LoadFromJSONString(json_str);
+  EXPECT_EQ(service()->size(), 1u);
+
+  // Expect the first source to be chosen
+  for (auto& snippet : *service()) {
+    EXPECT_EQ(snippet.get_sources().size(), 2u);
+    EXPECT_EQ(snippet.url(), GURL("http://source1.com"));
+    EXPECT_EQ(snippet.site_title(), std::string("Source 1"));
+    EXPECT_EQ(snippet.amp_url(), GURL("http://source1.amp.com"));
+  }
+}
+
+TEST_F(NTPSnippetsServiceTest, TestMultipleIncompleteSources) {
+  // Set Source 2 to have no AMP url, and Source 1 to have no publisher name
+  // Source 2 should win since we favor publisher name over amp url
+  std::vector<std::string> source_urls;
+  source_urls.push_back(std::string("http://source1.com"));
+  source_urls.push_back(std::string("http://source2.com"));
+  std::vector<std::string> publishers;
+  publishers.push_back(std::string(""));
+  publishers.push_back(std::string("Source 2"));
+  std::vector<std::string> amp_urls;
+  amp_urls.push_back(std::string("http://source1.amp.com"));
+  amp_urls.push_back(std::string(""));
+  std::string json_str(
+      GetTestJsonWithSources(source_urls, publishers, amp_urls));
+
+  LoadFromJSONString(json_str);
+  EXPECT_EQ(service()->size(), 1u);
+
+  for (auto& snippet : *service()) {
+    EXPECT_EQ(snippet.get_sources().size(), 2u);
+    EXPECT_EQ(snippet.url(), GURL("http://source2.com"));
+    EXPECT_EQ(snippet.site_title(), std::string("Source 2"));
+    EXPECT_EQ(snippet.amp_url(), GURL());
+  }
+
+  service()->ClearSnippets();
+  // Set Source 1 to have no AMP url, and Source 2 to have no publisher name
+  // Source 1 should win in this case since we prefer publisher name to AMP url
+  source_urls.clear();
+  source_urls.push_back(std::string("http://source1.com"));
+  source_urls.push_back(std::string("http://source2.com"));
+  publishers.clear();
+  publishers.push_back(std::string("Source 1"));
+  publishers.push_back(std::string(""));
+  amp_urls.clear();
+  amp_urls.push_back(std::string(""));
+  amp_urls.push_back(std::string("http://source2.amp.com"));
+  json_str = GetTestJsonWithSources(source_urls, publishers, amp_urls);
+
+  LoadFromJSONString(json_str);
+  EXPECT_EQ(service()->size(), 1u);
+
+  for (auto& snippet : *service()) {
+    EXPECT_EQ(snippet.get_sources().size(), 2u);
+    EXPECT_EQ(snippet.url(), GURL("http://source1.com"));
+    EXPECT_EQ(snippet.site_title(), std::string("Source 1"));
+    EXPECT_EQ(snippet.amp_url(), GURL());
+  }
+
+  service()->ClearSnippets();
+  // Set source 1 to have no AMP url and no source. Source 1 should win since
+  // all sources without publisher name are ranked equally
+  source_urls.clear();
+  source_urls.push_back(std::string("http://source1.com"));
+  source_urls.push_back(std::string("http://source2.com"));
+  publishers.clear();
+  publishers.push_back(std::string(""));
+  publishers.push_back(std::string(""));
+  amp_urls.clear();
+  amp_urls.push_back(std::string(""));
+  amp_urls.push_back(std::string("http://source2.amp.com"));
+  json_str = GetTestJsonWithSources(source_urls, publishers, amp_urls);
+
+  LoadFromJSONString(json_str);
+#if !DCHECK_IS_ON()
+  EXPECT_EQ(service()->size(), 0u);
+#else
+  EXPECT_EQ(service()->size(), 1u);
+  for (auto& snippet : *service()) {
+    EXPECT_EQ(snippet.get_sources().size(), 2u);
+    EXPECT_EQ(snippet.url(), GURL("http://source1.com"));
+    EXPECT_EQ(snippet.site_title(), std::string());
+    EXPECT_EQ(snippet.amp_url(), GURL());
+  }
+#endif
+}
+
+TEST_F(NTPSnippetsServiceTest, TestMultipleCompleteSources) {
+  // Test 2 complete sources, we should choose the first complete source
+  std::vector<std::string> source_urls;
+  source_urls.push_back(std::string("http://source1.com"));
+  source_urls.push_back(std::string("http://source2.com"));
+  source_urls.push_back(std::string("http://source3.com"));
+  std::vector<std::string> publishers;
+  publishers.push_back(std::string("Source 1"));
+  publishers.push_back(std::string(""));
+  publishers.push_back(std::string("Source 3"));
+  std::vector<std::string> amp_urls;
+  amp_urls.push_back(std::string("http://source1.amp.com"));
+  amp_urls.push_back(std::string("http://source2.amp.com"));
+  amp_urls.push_back(std::string("http://source3.amp.com"));
+  std::string json_str(
+      GetTestJsonWithSources(source_urls, publishers, amp_urls));
+
+  LoadFromJSONString(json_str);
+  EXPECT_EQ(service()->size(), 1u);
+
+  for (auto& snippet : *service()) {
+    EXPECT_EQ(snippet.get_sources().size(), 3u);
+    EXPECT_EQ(snippet.url(), GURL("http://source1.com"));
+    EXPECT_EQ(snippet.site_title(), std::string("Source 1"));
+    EXPECT_EQ(snippet.amp_url(), GURL("http://source1.amp.com"));
+  }
+
+  // Test 2 complete sources, we should choose the first complete source
+  service()->ClearSnippets();
+  source_urls.clear();
+  source_urls.push_back(std::string("http://source1.com"));
+  source_urls.push_back(std::string("http://source2.com"));
+  source_urls.push_back(std::string("http://source3.com"));
+  publishers.clear();
+  publishers.push_back(std::string(""));
+  publishers.push_back(std::string("Source 2"));
+  publishers.push_back(std::string("Source 3"));
+  amp_urls.clear();
+  amp_urls.push_back(std::string("http://source1.amp.com"));
+  amp_urls.push_back(std::string("http://source2.amp.com"));
+  amp_urls.push_back(std::string("http://source3.amp.com"));
+  json_str = GetTestJsonWithSources(source_urls, publishers, amp_urls);
+
+  LoadFromJSONString(json_str);
+  EXPECT_EQ(service()->size(), 1u);
+
+  for (auto& snippet : *service()) {
+    EXPECT_EQ(snippet.get_sources().size(), 3u);
+    EXPECT_EQ(snippet.url(), GURL("http://source2.com"));
+    EXPECT_EQ(snippet.site_title(), std::string("Source 2"));
+    EXPECT_EQ(snippet.amp_url(), GURL("http://source2.amp.com"));
+  }
+
+  // Test 3 complete sources, we should choose the first complete source
+  service()->ClearSnippets();
+  source_urls.clear();
+  source_urls.push_back(std::string("http://source1.com"));
+  source_urls.push_back(std::string("http://source2.com"));
+  source_urls.push_back(std::string("http://source3.com"));
+  publishers.clear();
+  publishers.push_back(std::string("Source 1"));
+  publishers.push_back(std::string("Source 2"));
+  publishers.push_back(std::string("Source 3"));
+  amp_urls.clear();
+  amp_urls.push_back(std::string(""));
+  amp_urls.push_back(std::string("http://source2.amp.com"));
+  amp_urls.push_back(std::string("http://source3.amp.com"));
+  json_str = GetTestJsonWithSources(source_urls, publishers, amp_urls);
+
+  LoadFromJSONString(json_str);
+  EXPECT_EQ(service()->size(), 1u);
+
+  for (auto& snippet : *service()) {
+    EXPECT_EQ(snippet.get_sources().size(), 3u);
+    EXPECT_EQ(snippet.url(), GURL("http://source2.com"));
+    EXPECT_EQ(snippet.site_title(), std::string("Source 2"));
+    EXPECT_EQ(snippet.amp_url(), GURL("http://source2.amp.com"));
+  }
+}
 }  // namespace ntp_snippets