Use multiple IDs when discarding or merging snippets.

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>
 
@@ skipping 143 matching lines @@
 std::unique_ptr<base::ListValue> SnippetsToListValue(
     const NTPSnippet::PtrVector& snippets) {
   std::unique_ptr<base::ListValue> list(new base::ListValue);
   for (const auto& snippet : snippets) {
     std::unique_ptr<base::DictionaryValue> dict = snippet->ToDictionary();
     list->Append(std::move(dict));
   }
   return list;
 }
 
-bool ContainsSnippet(const NTPSnippet::PtrVector& haystack,
-                     const std::unique_ptr<NTPSnippet>& needle) {
-  const std::string& id = needle->id();
-  return std::find_if(haystack.begin(), haystack.end(),
-                      [&id](const std::unique_ptr<NTPSnippet>& snippet) {
-                        return snippet->id() == id;
-                      }) != haystack.end();
+void InsertAllIDs(const NTPSnippet::PtrVector& snippets,
+                  std::set<std::string>* ids) {
+  for (const std::unique_ptr<NTPSnippet>& snippet : snippets) {
+    ids->insert(snippet->id());
+    for (const SnippetSource& source : snippet->sources()) {

[Marc Treib, 2016/05/18 18:58:05]

nit: no braces required (we usually leave them out if both the condition and the
body fit on one line each)

[tschumann, 2016/05/18 19:23:28]

hmmm... Ok, done for consistency.
One day (in the not-so-distant future) I will challenge this convention, though
=)

[Marc Treib, 2016/05/18 19:29:29]

You'll have to fight Bernhard over it :D
Personally, I don't really mind either way, as long as it's consistent.

+      ids->insert(source.url.spec());
+    }
+  }
 }
 
 void WrapImageFetchedCallback(
     const NTPSnippetsService::ImageFetchedCallback& callback,
     const GURL& snippet_id_url,
     const SkBitmap* bitmap) {
   callback.Run(snippet_id_url.spec(), bitmap);
 }
 
 }  // namespace
@@ skipping 205 matching lines @@
     DCHECK_LE(snippets->size(), static_cast<size_t>(kMaxSnippetCount));
     UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticlesFetched",
                                 snippets->size());
     MergeSnippets(std::move(*snippets));
   }
   LoadingSnippetsFinished();
 }
 
 void NTPSnippetsService::MergeSnippets(NTPSnippet::PtrVector new_snippets) {
   // Remove new snippets that we already have, or that have been discarded.
+  std::set<std::string> old_snippet_ids;
+  InsertAllIDs(discarded_snippets_, &old_snippet_ids);
+  InsertAllIDs(snippets_, &old_snippet_ids);
   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);
-                     }),
+      std::remove_if(
+          new_snippets.begin(), new_snippets.end(),
+          [&old_snippet_ids](const std::unique_ptr<NTPSnippet>& snippet) {
+            if (old_snippet_ids.count(snippet->id())) {

[Marc Treib, 2016/05/18 18:58:05]

Also here: no braces required

[tschumann, 2016/05/18 19:23:28]

Done.

+              return true;
+            }
+            for (const SnippetSource& source : snippet->sources()) {
+              if (old_snippet_ids.count(source.url.spec())) {

[Marc Treib, 2016/05/18 18:58:05]

And here. (The "for" should keep the braces though, since its body has two
lines)

[tschumann, 2016/05/18 19:23:28]

Done.

+                return true;
+              }
+            }
+            return false;
+          }),
       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));
@@ skipping 14 matching lines @@
                        }),
         new_snippets.end());
     int num_snippets_discarded = num_new_snippets - new_snippets.size();
     UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.IncompleteSnippetsAfterFetch",
                           num_snippets_discarded > 0);
     if (num_snippets_discarded > 0) {
       UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumIncompleteSnippets",
                                   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()));
 }
 
 void NTPSnippetsService::LoadSnippetsFromPrefs() {
   NTPSnippet::PtrVector prefs_snippets;
   bool success = NTPSnippet::AddFromListValue(
       *pref_service_->GetList(prefs::kSnippets), &prefs_snippets);
@@ skipping 87 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