Finalize backend for fetching more NTPSnippets.

components/ntp_snippets/remote/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/remote/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 165 matching lines @@
   }
   int num_snippets = snippets->size();
   // Remove snippets that do not have all the info we need to display it to
   // the user.
   snippets->erase(
       std::remove_if(snippets->begin(), snippets->end(),
                      [](const std::unique_ptr<NTPSnippet>& snippet) {
                        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);
   }
 }
 
 std::vector<ContentSuggestion> ConvertToContentSuggestions(
     Category category,
     const NTPSnippet::PtrVector& snippets) {
   std::vector<ContentSuggestion> result;
   for (const std::unique_ptr<NTPSnippet>& snippet : snippets) {
     // TODO(sfiera): if a snippet is not going to be displayed, move it
     // directly to content.dismissed on fetch. Otherwise, we might prune
@@ skipping 95 matching lines @@
   if (ready())
     FetchSnippetsFromHosts(std::set<std::string>(), interactive_request);
   else
     fetch_when_ready_ = true;
 }
 
 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;
   }
 
   MarkEmptyCategoriesAsLoading();
 
   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;
   params.exclusive_category = std::move(exclusive_category);
 
   snippets_fetcher_->FetchSnippets(
       params, base::BindOnce(&NTPSnippetsService::OnFetchFinished,
                              base::Unretained(this), fetch_more, callback));
 }
 
 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;
     for (const auto& snippet : content.dismissed)
       ids.insert(snippet->id());
     if (!fetch_more)
       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;
 }
 
 void NTPSnippetsService::MarkEmptyCategoriesAsLoading() {
   for (const auto& item : categories_) {
     Category category = item.first;
     const CategoryContent& content = item.second;
     if (content.snippets.empty())
       UpdateCategoryStatus(category, CategoryStatus::AVAILABLE_LOADING);
   }
@@ skipping 255 matching lines @@
   FinishInitialization();
 }
 
 void NTPSnippetsService::OnDatabaseError() {
   EnterState(State::ERROR_OCCURRED);
   UpdateAllCategoryStatus(CategoryStatus::LOADING_ERROR);
 }
 
 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;
 
   // TODO(fhorschig): Check which of the things here should actually happen for
   // |fetch_more| requests. Maybe it makes sense to have two separate
   // "Finished" methods?
 
   // Mark all categories as not provided by the server in the latest fetch. The
   // ones we got will be marked again below.
@@ skipping 420 matching lines @@
 
   std::vector<ContentSuggestion> result =
       ConvertToContentSuggestions(category, content.snippets);
 
   DVLOG(1) << "NotifyNewSuggestions(): " << result.size()
            << " items in category " << category;
   observer()->OnNewSuggestions(this, category, std::move(result));
 }
 
 void NTPSnippetsService::NotifyMoreSuggestions(Category category,
-                                               FetchedMoreCallback callback) {
+                                               FetchingCallback callback) {
   DCHECK(base::ContainsKey(categories_, category));
   const CategoryContent& content = categories_[category];
   DCHECK(IsCategoryStatusAvailable(content.status));
 
   std::vector<ContentSuggestion> result =
       ConvertToContentSuggestions(category, content.snippets);
 
   DVLOG(1) << "NotifyMoreSuggestions(): " << result.size()
            << " items in category " << category;
   DCHECK(!callback.is_null());
@@ skipping 110 matching lines @@
 }
 
 NTPSnippetsService::CategoryContent::CategoryContent() = default;
 NTPSnippetsService::CategoryContent::CategoryContent(CategoryContent&&) =
     default;
 NTPSnippetsService::CategoryContent::~CategoryContent() = default;
 NTPSnippetsService::CategoryContent& NTPSnippetsService::CategoryContent::
 operator=(CategoryContent&&) = default;
 
 }  // namespace ntp_snippets