Add favicon and publisher name to snippet cards

components/ntp_snippets/ntp_snippets_service.cc

Index: components/ntp_snippets/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/ntp_snippets_service.cc b/components/ntp_snippets/ntp_snippets_service.cc
index af047ac3af7da517dda41c378aa711cfd3cecd9c..2a3be2ee63132f71b99b19aafbe0b9a15ea4c5c0 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -13,6 +13,7 @@
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/location.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
@@ -128,10 +129,14 @@ std::set<std::string> GetSuggestionsHostsImpl(
 
 const char kContentInfo[] = "contentInfo";
 
-// Parses snippets from |list| and adds them to |snippets|. Returns true on
-// success, false if anything went wrong.
+// Parses snippets from |list| and adds them to |snippets|. |downloaded| should
+// be true if we are adding snippets that have been freshly downloaded, and
+// false if we are loading from prefs. Returns true on success, false if
+// anything went wrong.
 bool AddSnippetsFromListValue(const base::ListValue& list,
-                              NTPSnippetsService::NTPSnippetStorage* snippets) {
+                              NTPSnippetsService::NTPSnippetStorage* snippets,
+                              bool downloaded) {
+  int num_incomplete_snippets = 0;
   for (const base::Value* const value : list) {
     const base::DictionaryValue* dict = nullptr;
     if (!value->GetAsDictionary(&dict))
@@ -145,8 +150,29 @@ bool AddSnippetsFromListValue(const base::ListValue& list,
     if (!snippet)
       return false;
 
-    snippets->push_back(std::move(snippet));
+    // Fill in default publish/expiry dates where required.

[Marc Treib, 2016/04/29 12:11:20]

This will now also run when we're loading snippets from prefs, which we IMO
don't want - those should always already have the dates.
I think the problem is just with the "incomplete" block that you introduced just
below, right? Then I'd move that block into LoadFromListValue, just below where
the "default publish/expiry" block used to be.

[May, 2016/04/29 12:19:09]

We should never store snippets without a publish/expiry date, right? So this
should be no-op for those. But I can change it back if you feel strongly about
it.

[Marc Treib, 2016/04/29 12:33:32]

Yes, please. It "should" be a no-op in those cases, but it has the potential to
hide some other bug. I'd like to run this code only in the place where it's
actually required, and expected to do something.

[May, 2016/04/29 17:00:15]

Sure, moved the snippet discarding logic into LoadFromListValue

+    if (snippet->publish_date().is_null())
+      snippet->set_publish_date(base::Time::Now());
+    if (snippet->expiry_date().is_null()) {
+      snippet->set_expiry_date(
+          snippet->publish_date() +
+          base::TimeDelta::FromMinutes(kDefaultExpiryTimeMins));
+    }
+
+    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kAddIncompleteSnippets)) {
+      snippets->push_back(std::move(snippet));
+    } else if (snippet->is_complete()) {
+      snippets->push_back(std::move(snippet));
+    } else {
+      ++num_incomplete_snippets;
+    }
   }
+
+  if (num_incomplete_snippets > 0)
+    UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.IncompleteSnippets",
+                                num_incomplete_snippets);
+
   return true;
 }
 
@@ -284,10 +310,11 @@ std::set<std::string> NTPSnippetsService::GetSuggestionsHosts() const {
 }
 
 bool NTPSnippetsService::DiscardSnippet(const GURL& url) {
-  auto it = std::find_if(snippets_.begin(), snippets_.end(),
-                         [&url](const std::unique_ptr<NTPSnippet>& snippet) {
-                           return snippet->url() == url;
-                         });
+  auto it =
+      std::find_if(snippets_.begin(), snippets_.end(),
+                   [&url](const std::unique_ptr<NTPSnippet>& snippet) {
+                     return snippet->url() == url || snippet->best_source().url;

[Marc Treib, 2016/04/29 12:11:20]

This will just check if |snippet->best_source().url| evaluates to true, you're
missing a "== url". I'm surprised this even compiles - can GURL be
auto-converted to bool or int?!

[May, 2016/04/29 12:19:09]

Ack -- fixed. And apparently it can be turned into a bool/int, because it
definitely compiles!

+                   });
   if (it == snippets_.end())
     return false;
   discarded_snippets_.push_back(std::move(*it));
@@ -394,7 +421,7 @@ bool NTPSnippetsService::LoadFromValue(const base::Value& value) {
 
 bool NTPSnippetsService::LoadFromListValue(const base::ListValue& list) {
   NTPSnippetStorage new_snippets;
-  if (!AddSnippetsFromListValue(list, &new_snippets))
+  if (!AddSnippetsFromListValue(list, &new_snippets, true))
     return false;
 
   // Remove new snippets that we already have, or that have been discarded.
@@ -406,17 +433,6 @@ bool NTPSnippetsService::LoadFromListValue(const base::ListValue& list) {
                      }),
       new_snippets.end());
 
-  // Fill in default publish/expiry dates where required.
-  for (std::unique_ptr<NTPSnippet>& snippet : new_snippets) {
-    if (snippet->publish_date().is_null())
-      snippet->set_publish_date(base::Time::Now());
-    if (snippet->expiry_date().is_null()) {
-      snippet->set_expiry_date(
-          snippet->publish_date() +
-          base::TimeDelta::FromMinutes(kDefaultExpiryTimeMins));
-    }
-  }
-
   // Insert the new snippets at the front.
   snippets_.insert(snippets_.begin(),
                    std::make_move_iterator(new_snippets.begin()),
@@ -444,8 +460,8 @@ void NTPSnippetsService::StoreSnippetsToPrefs() {
 void NTPSnippetsService::LoadDiscardedSnippetsFromPrefs() {
   discarded_snippets_.clear();
   bool success = AddSnippetsFromListValue(
-      *pref_service_->GetList(prefs::kDiscardedSnippets),
-      &discarded_snippets_);
+      *pref_service_->GetList(prefs::kDiscardedSnippets), &discarded_snippets_,
+      false);
   DCHECK(success) << "Failed to parse discarded snippets from prefs";
 }