Finalize backend for fetching more NTPSnippets.

components/ntp_snippets/remote/ntp_snippets_service.cc

Index: components/ntp_snippets/remote/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/remote/ntp_snippets_service.cc b/components/ntp_snippets/remote/ntp_snippets_service.cc
index 167bed2887a140ccdb79d895df07da2e29a78779..4f459f6bd9538296175f76123eef34f783de1869 100644
--- a/components/ntp_snippets/remote/ntp_snippets_service.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_service.cc
@@ -183,12 +183,12 @@ void RemoveIncompleteSnippets(NTPSnippet::PtrVector* snippets) {
                        return !snippet->is_complete();
                      }),
       snippets->end());
-  int num_snippets_dismissed = num_snippets - snippets->size();
+  int num_snippets_removed = num_snippets - snippets->size();
   UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.IncompleteSnippetsAfterFetch",
-                        num_snippets_dismissed > 0);
-  if (num_snippets_dismissed > 0) {
+                        num_snippets_removed > 0);
+  if (num_snippets_removed > 0) {
     UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumIncompleteSnippets",
-                                num_snippets_dismissed);
+                                num_snippets_removed);
   }
 }
 
@@ -304,26 +304,30 @@ void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts,
     bool interactive_request) {
   FetchSnippetsFromHostsImpl(hosts, interactive_request,
-                             /*fetch_more=*/false, FetchedMoreCallback(),
-                             base::Optional<Category>());
+                             /*fetch_more=*/false, std::set<std::string>(),
+                             base::Optional<Category>(), FetchingCallback());
 }
 
-void NTPSnippetsService::FetchMore(const Category& category,
-                                   FetchedMoreCallback callback) {
+void NTPSnippetsService::Fetch(const Category& category,
+                               std::set<std::string> known_suggestion_ids,
+                               FetchingCallback callback) {
   FetchSnippetsFromHostsImpl(std::set<std::string>(),
                              /*interactive_request=*/true,
-                             /*fetch_more=*/true, callback,
-                             base::Optional<Category>(category));
+                             /*fetch_more=*/true,
+                             std::move(known_suggestion_ids),
+                             base::Optional<Category>(category), callback);
 }
 
 void NTPSnippetsService::FetchSnippetsFromHostsImpl(
     const std::set<std::string>& hosts,
     bool interactive_request,
     bool fetch_more,
-    FetchedMoreCallback callback,
-    base::Optional<Category> exclusive_category) {
+    std::set<std::string> known_suggestion_ids,
+    base::Optional<Category> exclusive_category,
+    FetchingCallback callback) {
   if (!ready()) {
-    // TODO(fhorschig): Call |callback| if it's non-null.
+    if (!callback.is_null())
+      std::move(callback).Run(std::vector<ContentSuggestion>());

[dgn, 2016/11/02 11:10:11]

why is std::move necessary here?

[dgn, 2016/11/02 12:04:33]

Okay, this is explained in
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md

It still looks super dodgy to my untrained c++ eyes. Maybe add a comment to
explain why this is WAI to not move it anywhere or wrap it in a method that will
have that explanation?

[fhorschig, 2016/11/03 01:53:14]

Thank you very much!
You discovered (maybe unwillingly) a potential bug: this callback used to be a
OnceCallback, but it isn't one any longer. So the outer move used to be
necessary but is ABSOLUTELY WRONG currently. I move it into an explaining
function as this construct will return in the near future.

     return;
   }
 
@@ -331,7 +335,8 @@ void NTPSnippetsService::FetchSnippetsFromHostsImpl(
 
   NTPSnippetsFetcher::Params params;
   params.language_code = application_language_code_;
-  params.excluded_ids = CollectIdsToExclude(fetch_more);
+  params.excluded_ids =
+      CollectIdsToExclude(fetch_more, std::move(known_suggestion_ids));
   params.count_to_fetch = kMaxSnippetCount;
   params.hosts = hosts;
   params.interactive_request = interactive_request;
@@ -343,7 +348,8 @@ void NTPSnippetsService::FetchSnippetsFromHostsImpl(
 }
 
 std::set<std::string> NTPSnippetsService::CollectIdsToExclude(
-    bool fetch_more) const {
+    bool fetch_more,
+    std::set<std::string> additional_ids) const {
   std::set<std::string> ids;
   for (const auto& item : categories_) {
     const CategoryContent& content = item.second;
@@ -353,9 +359,9 @@ std::set<std::string> NTPSnippetsService::CollectIdsToExclude(
       continue;
     for (const auto& snippet : content.archived)
       ids.insert(snippet->id());
-    for (const auto& snippet : content.snippets)
-      ids.insert(snippet->id());
   }
+  std::move(additional_ids.begin(), additional_ids.end(),
+            std::insert_iterator<std::set<std::string>>(ids, ids.end()));
   return ids;
 }
 
@@ -631,7 +637,7 @@ void NTPSnippetsService::OnDatabaseError() {
 
 void NTPSnippetsService::OnFetchFinished(
     bool fetched_more,
-    FetchedMoreCallback fetched_more_callback,
+    FetchingCallback fetched_more_callback,

[dgn, 2016/11/02 11:10:11]

Can this argument be a base::Optional<FetchingCallback> ? we don't care about it
if |fetched_more| is false, right? that would also avoid giving a dummy callback
as argument.

[fhorschig, 2016/11/03 01:53:14]

Done.

Yes. This whole set of parameters is planned to change when supporting
concurrent fetches (Bug 659931).

     NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
   if (!ready())
     return;
@@ -1072,7 +1078,7 @@ void NTPSnippetsService::NotifyNewSuggestions(Category category) {
 }
 
 void NTPSnippetsService::NotifyMoreSuggestions(Category category,
-                                               FetchedMoreCallback callback) {
+                                               FetchingCallback callback) {
   DCHECK(base::ContainsKey(categories_, category));
   const CategoryContent& content = categories_[category];
   DCHECK(IsCategoryStatusAvailable(content.status));