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 117 matching lines @@
 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 = snippet->ToDictionary();
     list->Append(std::move(dict));
   }
   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(
     const NTPSnippetsService::ImageFetchedCallback& callback,
     const GURL& snippet_id_url,
     const SkBitmap* bitmap) {
   callback.Run(snippet_id_url.spec(), bitmap);
 }
 
 }  // namespace
@@ skipping 190 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(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.
   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);
     }
   }
-

[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()),
                    std::make_move_iterator(new_snippets.end()));
 }
 
 void NTPSnippetsService::LoadSnippetsFromPrefs() {
   NTPSnippetStorage 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