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 63b79593e1d0730564daea882de936a42c83adc5..f0810d817dc06c887aa90ac0630e33248b0fdade 100644
--- a/components/ntp_snippets/remote/ntp_snippets_service.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_service.cc
@@ -157,6 +157,45 @@ 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_dismissed = num_snippets - 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);
+  }
+}
+
+NTPSnippetsService::FetchedMoreCallback NullCallback() {
+  return NTPSnippetsService::FetchedMoreCallback();

[Marc Treib, 2016/10/20 16:51:40]

Just inline this below. Also "NTPSnippetsService::" isn't needed.

[Marc Treib, 2016/10/28 14:49:49]

Done.

+}
+
 }  // namespace
 
 NTPSnippetsService::NTPSnippetsService(
@@ -242,26 +281,63 @@ void NTPSnippetsService::FetchSnippets(bool interactive_request) {
 void NTPSnippetsService::FetchSnippetsFromHosts(

[Marc Treib, 2016/10/20 16:51:40]

This doesn't do anything now - might as well remove it and just call the Impl
directly. (And then maybe rename that to FetchSnippetsImpl, no "FromHosts")

[Marc Treib, 2016/10/28 14:49:49]

...buut it's public, and used by the internals page, so leaving it there for
now.

     const std::set<std::string>& hosts,
     bool interactive_request) {
+  FetchSnippetsFromHostsImpl(hosts, interactive_request,
+                             /*fetch_more=*/false, NullCallback(),
+                             base::Optional<Category>());
+}
+
+void NTPSnippetsService::FetchMore(const Category& category,
+                                   FetchedMoreCallback callback) {
+  FetchSnippetsFromHostsImpl(std::set<std::string>(),
+                             /*interactive_request=*/true,
+                             /*fetch_more=*/true, callback,
+                             base::Optional<Category>(category));
+}
+
+void NTPSnippetsService::FetchSnippetsFromHostsImpl(
+    const std::set<std::string>& hosts,
+    bool interactive_request,
+    bool fetch_more,
+    FetchedMoreCallback callback,
+    base::Optional<Category> exclusive_category) {
   if (!ready())
     return;
 
-  // Empty categories are marked as loading; others are unchanged.
+  MarkEmptyCategoriesAsLoading();
+
+  // TODO(fhorschig): Refactor parameter to data object or management class.
+  snippets_fetcher_->FetchSnippetsFromHosts(
+      hosts, application_language_code_,
+      std::move(CollectIdsToExclude(fetch_more)), kMaxSnippetCount,

[Marc Treib, 2016/10/20 16:51:40]

std::move isn't needed here (it's movable anyway)

[Marc Treib, 2016/10/28 14:49:49]

Done.

+      interactive_request,
+      base::BindOnce(&NTPSnippetsService::OnFetchFinished,
+                     base::Unretained(this), fetch_more, callback),
+      std::move(exclusive_category));
+}
+
+std::set<std::string> NTPSnippetsService::CollectIdsToExclude(bool fetch_more) {
+  std::set<std::string> ids;
   for (const auto& item : categories_) {
-    Category category = item.first;
     const CategoryContent& content = item.second;
-    if (content.snippets.empty())
-      UpdateCategoryStatus(category, CategoryStatus::AVAILABLE_LOADING);
+    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());
   }
+  return ids;
+}
 
-  std::set<std::string> excluded_ids;
+void NTPSnippetsService::MarkEmptyCategoriesAsLoading() {
   for (const auto& item : categories_) {
+    Category category = item.first;
     const CategoryContent& content = item.second;
-    for (const auto& snippet : content.dismissed)
-      excluded_ids.insert(snippet->id());
+    if (content.snippets.empty())
+      UpdateCategoryStatus(category, CategoryStatus::AVAILABLE_LOADING);
   }
-  snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
-                                            excluded_ids, kMaxSnippetCount,
-                                            interactive_request);
 }
 
 void NTPSnippetsService::RescheduleFetching(bool force) {
@@ -379,7 +455,7 @@ void NTPSnippetsService::ClearCachedSuggestions(Category category) {
   database_->DeleteImages(GetSnippetIDVector(content->snippets));
   content->snippets.clear();
 
-  NotifyNewSuggestions();
+  NotifyNewSuggestions(/*fetched_more=*/false, NullCallback());
 }
 
 void NTPSnippetsService::GetDismissedSuggestionsForDebugging(
@@ -520,6 +596,8 @@ void NTPSnippetsService::OnDatabaseError() {
 }
 
 void NTPSnippetsService::OnFetchFinished(
+    bool fetched_more,
+    FetchedMoreCallback fetched_more_callback,
     NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
   if (!ready())
     return;
@@ -564,7 +642,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);
     }
   }
 
@@ -587,7 +666,7 @@ void NTPSnippetsService::OnFetchFinished(
   }
 
   // TODO(sfiera): notify only when a category changed above.
-  NotifyNewSuggestions();
+  NotifyNewSuggestions(fetched_more, fetched_more_callback);
 
   // Reschedule after a successful fetch. This resets all currently scheduled
   // fetches, to make sure the fallback interval triggers only if no wifi fetch
@@ -597,68 +676,17 @@ void NTPSnippetsService::OnFetchFinished(
     RescheduleFetching(true);
 }
 
-void NTPSnippetsService::ArchiveSnippets(Category category,
-                                         NTPSnippet::PtrVector* to_archive) {
-  CategoryContent* content = &categories_[category];
-  database_->DeleteSnippets(GetSnippetIDVector(*to_archive));
-  // Do not delete the thumbnail images as they are still handy on open NTPs.
-
-  // Archive previous snippets - move them at the beginning of the list.
-  content->archived.insert(content->archived.begin(),
-                           std::make_move_iterator(to_archive->begin()),
-                           std::make_move_iterator(to_archive->end()));
-  RemoveNullPointers(to_archive);
-
-  // If there are more archived snippets than we want to keep, delete the
-  // oldest ones by their fetch time (which are always in the back).
-  if (content->archived.size() > kMaxArchivedSnippetCount) {
-    NTPSnippet::PtrVector to_delete(
-        std::make_move_iterator(content->archived.begin() +
-                                kMaxArchivedSnippetCount),
-        std::make_move_iterator(content->archived.end()));
-    content->archived.resize(kMaxArchivedSnippetCount);
-    database_->DeleteImages(GetSnippetIDVector(to_delete));
-  }
-}
-
-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())
@@ -671,14 +699,51 @@ 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.
+  SaveSnippets(category, std::move(new_snippets), replace_snippets);
+}
+
+void NTPSnippetsService::SaveSnippets(const Category& category,

[Marc Treib, 2016/10/20 16:51:40]

So this is only called once, just above? Then I wouldn't make it a separate
method. (Also I find it a bit weird that a method called "SaveSnippets" would
archive existing ones.)

[Marc Treib, 2016/10/28 14:49:49]

Done.

+                                      NTPSnippet::PtrVector new_snippets,
+                                      bool replace_snippets) {
   database_->SaveSnippets(new_snippets);
 
-  content->snippets = std::move(new_snippets);
+  CategoryContent* content = &categories_[category];
+
+  if (replace_snippets) {
+    ArchiveSnippets(category, &content->snippets);
+    content->snippets = std::move(new_snippets);
+    return;
+  }
+
+  content->snippets.insert(content->snippets.end(),
+                           std::make_move_iterator(new_snippets.begin()),
+                           std::make_move_iterator(new_snippets.end()));
 }
 
+void NTPSnippetsService::ArchiveSnippets(Category category,

[Marc Treib, 2016/10/20 16:51:40]

Why did you move this? It makes the diff harder to read (I can't easily see if
you changed anything here), and it's now also a different order from the header.

[Marc Treib, 2016/10/28 14:49:49]

Done. (Moved back)

+                                         NTPSnippet::PtrVector* to_archive) {
+  CategoryContent* content = &categories_[category];
+  database_->DeleteSnippets(GetSnippetIDVector(*to_archive));
+  // Do not delete the thumbnail images as they are still handy on open NTPs.
+
+  // Archive previous snippets - move them at the beginning of the list.
+  content->archived.insert(content->archived.begin(),
+                           std::make_move_iterator(to_archive->begin()),
+                           std::make_move_iterator(to_archive->end()));
+  RemoveNullPointers(to_archive);
+
+  // If there are more archived snippets than we want to keep, delete the
+  // oldest ones by their fetch time (which are always in the back).
+  if (content->archived.size() > kMaxArchivedSnippetCount) {
+    NTPSnippet::PtrVector to_delete(
+        std::make_move_iterator(content->archived.begin() +
+                                kMaxArchivedSnippetCount),
+        std::make_move_iterator(content->archived.end()));
+    content->archived.resize(kMaxArchivedSnippetCount);
+    database_->DeleteImages(GetSnippetIDVector(to_delete));
+  }
+}
 void NTPSnippetsService::ClearExpiredDismissedSnippets() {
   std::vector<Category> categories_to_erase;
 
@@ -862,9 +927,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);
@@ -879,7 +941,7 @@ void NTPSnippetsService::FinishInitialization() {
 
   // Always notify here even if we got nothing from the database, because we
   // don't know how long the fetch will take or if it will even complete.
-  NotifyNewSuggestions();
+  NotifyNewSuggestions(/*fetched_more=*/false, NullCallback());
 }
 
 void NTPSnippetsService::OnSnippetsStatusChanged(
@@ -961,7 +1023,9 @@ void NTPSnippetsService::EnterState(State state) {
   RescheduleFetching(false);
 }
 
-void NTPSnippetsService::NotifyNewSuggestions() {
+void NTPSnippetsService::NotifyNewSuggestions(

[Marc Treib, 2016/10/20 16:51:40]

Hm, this now kinda does two totally different things. How about pulling the
(common) loop out into a separate ConvertSnippetsToSuggestions method, and
making two separate Notify methods?

[Marc Treib, 2016/10/28 14:49:49]

Done.

+    bool fetched_more,
+    FetchedMoreCallback fetched_more_callback) {
   for (const auto& item : categories_) {
     Category category = item.first;
     const CategoryContent& content = item.second;
@@ -986,9 +1050,14 @@ void NTPSnippetsService::NotifyNewSuggestions() {
       result.emplace_back(std::move(suggestion));
     }
 
-    DVLOG(1) << "NotifyNewSuggestions(): " << result.size()
-             << " items in category " << category;
-    observer()->OnNewSuggestions(this, category, std::move(result));
+    DVLOG(1) << "NotifyNewSuggestions(/*fetched_more=*/" << fetched_more
+             << "): " << result.size() << " items in category " << category;
+    if (fetched_more) {
+      DCHECK(!fetched_more_callback.is_null());
+      fetched_more_callback.Run(std::move(result));
+    } else {
+      observer()->OnNewSuggestions(this, category, std::move(result));
+    }
   }
 }