Use multiple IDs when discarding or merging snippets.

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 02524508ceca77bf9620436e54bc6604c47ab25e..6d7e2f62b14f3a3a30b8c6b54836a58398d0bcdb 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -135,13 +135,16 @@ std::unique_ptr<base::ListValue> SnippetsToListValue(
   return list;
 }
 
-bool ContainsSnippet(const NTPSnippetsService::NTPSnippetStorage& 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 NTPSnippetsService::NTPSnippetStorage& snippets,

[Marc Treib, 2016/05/18 15:19:33]

Heads-up: This will require a rebase onto
https://codereview.chromium.org/1976153002/ -
NTPSnippetsService::NTPSnippetStorage is now NTPSnippet::PtrVector

+                  std::set<std::string>* ids) {
+  for (const std::unique_ptr<NTPSnippet>& snippet : snippets) {
+    ids->insert(snippet->id());
+    for (const SnippetSource& source : snippet->sources()) {
+      if (source.url.is_valid()) {

[Marc Treib, 2016/05/18 15:19:33]

We check that the source URL is valid when building the snippet, no need to
check here again. (You could DCHECK() it if you want)

[tschumann, 2016/05/18 17:09:16]

i thought I saw some code where we'd fall back to an empty one... Oh, that was
the AMP one =).
Well I guess we can just ignore it. Just wanted to make sure we're not adding
empty strings unnecessarily.

+        ids->insert(source.url.spec());
+      }
+    }
+  }
 }
 
 void WrapImageFetchedCallback(
@@ -352,12 +355,15 @@ void NTPSnippetsService::OnFetchFinished(
 
 void NTPSnippetsService::MergeSnippets(NTPSnippetStorage 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) {
+            return old_snippet_ids.find(snippet->id()) != old_snippet_ids.end();

[Marc Treib, 2016/05/18 15:19:33]

We should probably check if *any* of the new snippet's IDs/URLs are in the old
IDs?

Also, optional nit: I find
old_snippet_ids.count(snippet->id()) > 0
a bit easier to read.

[tschumann, 2016/05/18 17:09:16]

Good catch! 
Done.

+          }),
       new_snippets.end());
 
   // Fill in default publish/expiry dates where required.
@@ -392,7 +398,6 @@ void NTPSnippetsService::MergeSnippets(NTPSnippetStorage new_snippets) {
                                   num_snippets_discarded);
     }
   }
-

[Marc Treib, 2016/05/18 15:19:33]

nitty nit: I'd keep the empty line here between "unrelated" blocks.

   // Insert the new snippets at the front.
   snippets_.insert(snippets_.begin(),
                    std::make_move_iterator(new_snippets.begin()),