FetchMore functionality backend

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 6d7c5418da939ae436a566f9fd329852dea18c03..67baaeb4b21bdc1a3d101735be7e0002dfb0ac65 100644
--- a/components/ntp_snippets/remote/ntp_snippets_service.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_service.cc
@@ -157,6 +157,75 @@ void RemoveNullPointers(NTPSnippet::PtrVector* snippets) {
       snippets->end());
 }
 
+void AssignExpiryAndPublishDates(NTPSnippet::PtrVector* snippets) {
+  for (std::unique_ptr<NTPSnippet>& snippet : *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));
+    }
+  }
+}
+
+void RemoveIncompleteSnippets(NTPSnippet::PtrVector* snippets) {
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kAddIncompleteSnippets)) {
+    return;
+  }
+  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_removed = num_snippets - snippets->size();
+  UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.IncompleteSnippetsAfterFetch",
+                        num_snippets_removed > 0);
+  if (num_snippets_removed > 0) {
+    UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumIncompleteSnippets",
+                                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
+    // other snippets to get down to kMaxSnippetCount, only to hide one of the
+    // incomplete ones we kept.
+    if (!snippet->is_complete())
+      continue;
+    ContentSuggestion suggestion(category, snippet->id(),
+                                 snippet->best_source().url);
+    suggestion.set_amp_url(snippet->best_source().amp_url);
+    suggestion.set_title(base::UTF8ToUTF16(snippet->title()));
+    suggestion.set_snippet_text(base::UTF8ToUTF16(snippet->snippet()));
+    suggestion.set_publish_date(snippet->publish_date());
+    suggestion.set_publisher_name(
+        base::UTF8ToUTF16(snippet->best_source().publisher_name));
+    suggestion.set_score(snippet->score());
+    result.emplace_back(std::move(suggestion));
+  }
+  return result;
+}
+
+void CallWithEmptyResults(
+    base::Optional<ContentSuggestionsProvider::FetchingCallback> callback) {
+  if (!callback)
+    return;
+  if (callback->is_null())
+    return;
+  callback->Run(std::vector<ContentSuggestion>());
+}
+
 }  // namespace
 
 NTPSnippetsService::NTPSnippetsService(
@@ -243,28 +312,74 @@ void NTPSnippetsService::FetchSnippets(bool interactive_request) {
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts,
     bool interactive_request) {
-  if (!ready())
-    return;
+  FetchSnippetsFromHostsImpl(hosts, interactive_request,
+                             /*fetch_more=*/false, std::set<std::string>(),
+                             base::Optional<Category>(),
+                             base::Optional<FetchingCallback>());
+}
 
-  // Empty categories are marked as loading; others are unchanged.
-  for (const auto& item : categories_) {
-    Category category = item.first;
-    const CategoryContent& content = item.second;
-    if (content.snippets.empty())
-      UpdateCategoryStatus(category, CategoryStatus::AVAILABLE_LOADING);
+void NTPSnippetsService::Fetch(
+    const Category& category,
+    const std::set<std::string>& known_suggestion_ids,
+    FetchingCallback callback) {
+  FetchSnippetsFromHostsImpl(std::set<std::string>(),
+                             /*interactive_request=*/true,
+                             /*fetch_more=*/true, known_suggestion_ids,
+                             base::Optional<Category>(category),
+                             base::Optional<FetchingCallback>(callback));
+}
+
+void NTPSnippetsService::FetchSnippetsFromHostsImpl(
+    const std::set<std::string>& hosts,
+    bool interactive_request,
+    bool fetch_more,
+    const std::set<std::string>& known_suggestion_ids,
+    base::Optional<Category> exclusive_category,
+    base::Optional<FetchingCallback> callback) {
+  if (!ready()) {
+    CallWithEmptyResults(callback);
+    return;
   }
 
+  MarkEmptyCategoriesAsLoading();
+
   NTPSnippetsFetcher::Params params;
   params.language_code = application_language_code_;
+  params.excluded_ids = CollectIdsToExclude(fetch_more, 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 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)
-      params.excluded_ids.insert(snippet->id());
+      ids.insert(snippet->id());
+    if (!fetch_more)
+      continue;
+    for (const auto& snippet : content.archived)
+      ids.insert(snippet->id());
+  }
+  ids.insert(additional_ids.begin(), additional_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);
   }
-  snippets_fetcher_->FetchSnippets(params);
 }
 
 void NTPSnippetsService::RescheduleFetching(bool force) {
@@ -529,10 +644,16 @@ void NTPSnippetsService::OnDatabaseError() {
 }
 
 void NTPSnippetsService::OnFetchFinished(
+    bool fetched_more,
+    base::Optional<FetchingCallback> fetching_callback,
     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.
   for (auto& item : categories_) {
@@ -573,7 +694,8 @@ void NTPSnippetsService::OnFetchFinished(
             std::min(fetched_category.snippets.size(),
                      static_cast<size_t>(kMaxSnippetCount + 1)));
       }
-      ReplaceSnippets(category, std::move(fetched_category.snippets));
+      IncludeSnippets(category, std::move(fetched_category.snippets),
+                      /*replace_snippets=*/!fetched_more);
     }
   }
 
@@ -585,7 +707,10 @@ void NTPSnippetsService::OnFetchFinished(
     Category category = item.first;
     UpdateCategoryStatus(category, CategoryStatus::AVAILABLE);
     // TODO(sfiera): notify only when a category changed above.
-    NotifyNewSuggestions(category);
+    if (fetched_more)
+      NotifyMoreSuggestions(category, fetching_callback);
+    else
+      NotifyNewSuggestions(category);
   }
 
   // TODO(sfiera): equivalent metrics for non-articles.
@@ -629,44 +754,17 @@ void NTPSnippetsService::ArchiveSnippets(Category category,
   }
 }
 
-void NTPSnippetsService::ReplaceSnippets(Category category,
-                                         NTPSnippet::PtrVector new_snippets) {
+void NTPSnippetsService::IncludeSnippets(const Category& category,
+                                         NTPSnippet::PtrVector new_snippets,
+                                         bool replace_snippets) {
   DCHECK(ready());
   CategoryContent* content = &categories_[category];
 
   // Remove new snippets that have been dismissed.
   EraseMatchingSnippets(&new_snippets, content->dismissed);
 
-  // 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));
-    }
-  }
-
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kAddIncompleteSnippets)) {
-    int num_new_snippets = new_snippets.size();
-    // Remove snippets that do not have all the info we need to display it to
-    // the user.
-    new_snippets.erase(
-        std::remove_if(new_snippets.begin(), new_snippets.end(),
-                       [](const std::unique_ptr<NTPSnippet>& snippet) {
-                         return !snippet->is_complete();
-                       }),
-        new_snippets.end());
-    int num_snippets_dismissed = num_new_snippets - new_snippets.size();
-    UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.IncompleteSnippetsAfterFetch",
-                          num_snippets_dismissed > 0);
-    if (num_snippets_dismissed > 0) {
-      UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumIncompleteSnippets",
-                                  num_snippets_dismissed);
-    }
-  }
+  AssignExpiryAndPublishDates(&new_snippets);
+  RemoveIncompleteSnippets(&new_snippets);
 
   // Do not touch the current set of snippets if the newly fetched one is empty.
   if (new_snippets.empty())
@@ -679,14 +777,18 @@ void NTPSnippetsService::ReplaceSnippets(Category category,
   // appear with the same ID in the new suggestions (it's fine for additional
   // IDs though).
   EraseByPrimaryID(&content->snippets, *GetSnippetIDVector(new_snippets));
-  ArchiveSnippets(category, &content->snippets);
 
-  // Save new articles to the DB.
   database_->SaveSnippets(new_snippets);
 
-  content->snippets = std::move(new_snippets);
+  if (replace_snippets) {
+    ArchiveSnippets(category, &content->snippets);
+    content->snippets = std::move(new_snippets);
+  } else {
+    content->snippets.insert(content->snippets.end(),

[tschumann, 2016/11/03 09:20:58]

what happens if I open a new NTP after hitting 'more' on an old one?
It looks like the new NTP would now also show more articles.
I believe the simplest thing for now would be to only return a truncated list of
snippets

+                             std::make_move_iterator(new_snippets.begin()),
+                             std::make_move_iterator(new_snippets.end()));
+  }
 }
-
 void NTPSnippetsService::ClearExpiredDismissedSnippets() {
   std::vector<Category> categories_to_erase;
 
@@ -870,9 +972,6 @@ void NTPSnippetsService::FinishInitialization() {
     nuke_when_initialized_ = false;
   }
 
-  snippets_fetcher_->SetCallback(
-      base::Bind(&NTPSnippetsService::OnFetchFinished, base::Unretained(this)));
-
   // |image_fetcher_| can be null in tests.
   if (image_fetcher_) {
     image_fetcher_->SetImageFetcherDelegate(this);
@@ -977,31 +1076,31 @@ void NTPSnippetsService::NotifyNewSuggestions(Category category) {
   const CategoryContent& content = categories_[category];
   DCHECK(IsCategoryStatusAvailable(content.status));
 
-  std::vector<ContentSuggestion> result;
-  for (const std::unique_ptr<NTPSnippet>& snippet : content.snippets) {
-    // TODO(sfiera): if a snippet is not going to be displayed, move it
-    // directly to content.dismissed on fetch. Otherwise, we might prune
-    // other snippets to get down to kMaxSnippetCount, only to hide one of the
-    // incomplete ones we kept.
-    if (!snippet->is_complete())
-      continue;
-    ContentSuggestion suggestion(category, snippet->id(),
-                                 snippet->best_source().url);
-    suggestion.set_amp_url(snippet->best_source().amp_url);
-    suggestion.set_title(base::UTF8ToUTF16(snippet->title()));
-    suggestion.set_snippet_text(base::UTF8ToUTF16(snippet->snippet()));
-    suggestion.set_publish_date(snippet->publish_date());
-    suggestion.set_publisher_name(
-        base::UTF8ToUTF16(snippet->best_source().publisher_name));
-    suggestion.set_score(snippet->score());
-    result.emplace_back(std::move(suggestion));
-  }
+  std::vector<ContentSuggestion> result =
+      ConvertToContentSuggestions(category, content.snippets);

[tschumann, 2016/11/03 09:20:58]

i believe we need to limit this to the first 10 results so that newly opened
NTPs don't show more items in case we clicked More on a different one.
Can sync up here later.

[dgn, 2016/11/03 10:28:04]

AFAICT, NotifyNewSuggestions() reports the suggestions to the
ContentSuggestionService, and that one will store them. We reach there when we
replace the current list of suggestion instead of appending, so we should still
have 10.

When we go through NotifyMoreSuggestions(), we don't notify the
ContentSuggestionService, so the regular GetSuggestionsForCategory() will not
see these extra suggestions.

But a DCHECK or a test would be useful to ensure that it stays correct.

[tschumann, 2016/11/03 10:35:06]

Ok got it. I was a bit confused thinking we would store the results inside the
NTPSnippetsService which might have turned out problematic. This way it's fine.
Will remove my TODO.

 
-  DVLOG(1) << "NotifyNewSuggestions(" << category << "): " << result.size()
-           << " items.";
+  DVLOG(1) << "NotifyNewSuggestions(): " << result.size()
+           << " items in category " << category;
   observer()->OnNewSuggestions(this, category, std::move(result));
 }
 
+void NTPSnippetsService::NotifyMoreSuggestions(
+    Category category,
+    base::Optional<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);
+  DCHECK(!callback->is_null());
+  callback->Run(std::move(result));
+}
+
 void NTPSnippetsService::UpdateCategoryStatus(Category category,
                                               CategoryStatus status) {
   DCHECK(base::ContainsKey(categories_, category));