Add favicon and publisher name to snippet cards

components/ntp_snippets/ntp_snippets_service.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_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #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"
 #include "base/task_runner_util.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/switches.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
@@ skipping 95 matching lines @@
     const ChromeSuggestion& suggestion = suggestions.suggestions(i);
     GURL url(suggestion.url());
     if (url.is_valid())
       hosts.insert(url.host());
   }
   return hosts;
 }
 
 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))
       return false;
 
     const base::DictionaryValue* content = nullptr;
     if (!dict->GetDictionary(kContentInfo, &content))
       return false;
     std::unique_ptr<NTPSnippet> snippet =
         NTPSnippet::CreateFromDictionary(*content);
     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;
 }
 
 std::unique_ptr<base::ListValue> SnippetsToListValue(
     const NTPSnippetsService::NTPSnippetStorage& snippets) {
   std::unique_ptr<base::ListValue> list(new base::ListValue);
   for (const auto& snippet : snippets) {
     std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue);
     dict->Set(kContentInfo, snippet->ToDictionary());
     list->Append(std::move(dict));
@@ skipping 117 matching lines @@
   // |suggestions_service_| can be null in tests.
   if (!suggestions_service_)
     return std::set<std::string>();
 
   // TODO(treib) this should just call GetSnippetHostsFromPrefs
   return GetSuggestionsHostsImpl(
       suggestions_service_->GetSuggestionsDataFromCache());
 }
 
 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));
   snippets_.erase(it);
   StoreDiscardedSnippetsToPrefs();
   StoreSnippetsToPrefs();
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
   return true;
 }
@@ skipping 86 matching lines @@
 
   const base::ListValue* list = nullptr;
   if (!top_dict->GetList("recos", &list))
     return false;
 
   return LoadFromListValue(*list);
 }
 
 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.
   new_snippets.erase(
       std::remove_if(new_snippets.begin(), new_snippets.end(),
                      [this](const std::unique_ptr<NTPSnippet>& snippet) {
                        return ContainsSnippet(discarded_snippets_, snippet) ||
                               ContainsSnippet(snippets_, snippet);
                      }),
       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()),
                    std::make_move_iterator(new_snippets.end()));
 
   // If there are more snippets now than we want to show, drop the extra ones
   // from the end of the list.
   if (snippets_.size() > kMaxSnippetCount)
     snippets_.resize(kMaxSnippetCount);
 
   return true;
 }
 
 void NTPSnippetsService::LoadSnippetsFromPrefs() {
   bool success = LoadFromListValue(*pref_service_->GetList(prefs::kSnippets));
   DCHECK(success) << "Failed to parse snippets from prefs";
 
   LoadingSnippetsFinished();
 }
 
 void NTPSnippetsService::StoreSnippetsToPrefs() {
   pref_service_->Set(prefs::kSnippets, *SnippetsToListValue(snippets_));
 }
 
 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";
 }
 
 void NTPSnippetsService::StoreDiscardedSnippetsToPrefs() {
   pref_service_->Set(prefs::kDiscardedSnippets,
                      *SnippetsToListValue(discarded_snippets_));
 }
 
 std::set<std::string> NTPSnippetsService::GetSnippetHostsFromPrefs() const {
   std::set<std::string> hosts;
@@ skipping 51 matching lines @@
     if (snippet->expiry_date() < next_expiry)
       next_expiry = snippet->expiry_date();
   }
   DCHECK_GT(next_expiry, expiry);
   expiry_timer_.Start(FROM_HERE, next_expiry - expiry,
                       base::Bind(&NTPSnippetsService::LoadingSnippetsFinished,
                                  base::Unretained(this)));
 }
 
 }  // namespace ntp_snippets