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/histogram_macros.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"
@@ skipping 254 matching lines @@
     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;
+                           return snippet->url() == url ||
+                                  snippet->best_source().url == url;
                          });
   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 36 matching lines @@
   StoreSnippetHostsToPrefs(hosts);
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
 
   FetchSnippetsFromHosts(hosts);
 }
 
 void NTPSnippetsService::OnSnippetsDownloaded(
     const std::string& snippets_json, const std::string& status) {
-
   if (!snippets_json.empty()) {
     DCHECK(status.empty());
 
     last_fetch_json_ = snippets_json;
 
     parse_json_callback_.Run(
         snippets_json,
         base::Bind(&NTPSnippetsService::OnJsonParsed,
                    weak_ptr_factory_.GetWeakPtr(), snippets_json),
         base::Bind(&NTPSnippetsService::OnJsonError,
@@ skipping 55 matching lines @@
   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));
     }
   }
 
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kAddIncompleteSnippets)) {
+    int num_new_snippets = new_snippets.size();
+    // Remove snippets that do not have all the info we need to display it to
+    // the user.
+    new_snippets.erase(
+        std::remove_if(new_snippets.begin(), new_snippets.end(),
+                       [](const std::unique_ptr<NTPSnippet>& snippet) {
+                         return !snippet->is_complete();
+                       }),
+        new_snippets.end());
+    int num_snippets_discarded = num_new_snippets - new_snippets.size();
+    UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.SnippetsDiscardedAfterFetch",

[Marc Treib, 2016/05/04 07:42:43]

I wouldn't use "Discarded" here, since that term is also user for "discarded by
the user".
Any particular reason for having a separate boolean histogram, instead of just
unconditionally recording .IncompleteSnippets?

[May, 2016/05/04 09:00:06]

Recommended by jwd@ to just have a separate boolean for this to use as the
denominator for figuring out % of snippet fetches that have issues

Changed name to IncompleteSnippetsAfterFetch, and changed the other one to
NumIncompleteSnippets to clarify that it's a count.

[Marc Treib, 2016/05/04 09:01:36]

Couldn't we do that anyway by comparing the number in the "0" bucket to the
total number of samples? Anyway, I don't have strong feelings, I was just
wondering.
SGTM, thanks!

[Bernhard Bauer, 2016/05/04 09:06:54]

Suggestion for consistency: Use "dismissed" for snippets swiped away by the
user, and "discarded" for the incomplete ones thrown away by us?

[Marc Treib, 2016/05/04 09:08:29]

Ah indeed, I mixed up "dismissed" and "discarded". Still, I like "Incomplete"
more since it's less ambiguous.
Also, the bike shed should be sky blue.

[May, 2016/05/04 09:26:34]

Stop painting my bike shed!! I don't even use a bike!!

Also, executive decision to use "incomplete" as much as grammatically possible
for this scenario, and "dismissed" for the other ones.

+                          num_snippets_discarded > 0);
+    if (num_snippets_discarded > 0)

[Marc Treib, 2016/05/04 07:42:43]

nit: braces if the body doesn't fit on one line

[May, 2016/05/04 09:00:06]

Done.

+      UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.IncompleteSnippets",
+                                  num_snippets_discarded);
+  }
+
   // 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()));
 
   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_);
   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 60 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