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..38e7daa43b6f89161ed9ca577b25baf4972accd4 100644
--- a/components/ntp_snippets/remote/ntp_snippets_service.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_service.cc
@@ -242,26 +242,46 @@ void NTPSnippetsService::FetchSnippets(bool interactive_request) {
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts,
     bool interactive_request) {
+  FetchSnippetsFromHostsImpl(hosts,
+                             interactive_request,
+                             base::MakeUnique<ReplaceExistingSnippets>(this));
+}
+
+void NTPSnippetsService::FetchMore() {
+  FetchSnippetsFromHostsImpl(std::set<std::string>(),
+                           /* interactive_request= */ true,

[Marc Treib, 2016/10/18 08:17:45]

nit: misaligned
There's a few other formatting things around here - try running "git cl format"
to automatically fix most of them.

[fhorschig, 2016/10/20 13:10:38]

Done. Thank you!

+                           base::MakeUnique<AddNewSnippets>(this));
+}
+
+void NTPSnippetsService::FetchSnippetsFromHostsImpl(
+    const std::set<std::string>& hosts,
+    bool interactive_request,
+    std::unique_ptr<NewNTPSnippetsHandlingStrategy> strategy) {
   if (!ready())
     return;
 
-  // Empty categories are marked as loading; others are unchanged.
+  MarkEmptyCategoriesAsLoading();
+
+  std::set<std::string> excluded_ids;
+  strategy->CollectIdsToExclude(&excluded_ids);
+
+  snippets_fetcher_->SetCallback(
+          base::BindOnce(&NTPSnippetsService::OnFetchFinished,
+                         base::Unretained(this),
+                         std::move(strategy)));
+
+  snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
+                                            excluded_ids, kMaxSnippetCount,
+                                            interactive_request);
+}
+
+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);
   }
-
-  std::set<std::string> excluded_ids;
-  for (const auto& item : categories_) {
-    const CategoryContent& content = item.second;
-    for (const auto& snippet : content.dismissed)
-      excluded_ids.insert(snippet->id());
-  }
-  snippets_fetcher_->FetchSnippetsFromHosts(hosts, application_language_code_,
-                                            excluded_ids, kMaxSnippetCount,
-                                            interactive_request);
 }
 
 void NTPSnippetsService::RescheduleFetching(bool force) {
@@ -520,6 +540,7 @@ void NTPSnippetsService::OnDatabaseError() {
 }
 
 void NTPSnippetsService::OnFetchFinished(
+    std::unique_ptr<NewNTPSnippetsHandlingStrategy> strategy,
     NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
   if (!ready())
     return;
@@ -564,7 +585,9 @@ void NTPSnippetsService::OnFetchFinished(
             std::min(fetched_category.snippets.size(),
                      static_cast<size_t>(kMaxSnippetCount + 1)));
       }
-      ReplaceSnippets(category, std::move(fetched_category.snippets));
+      StoreSnippets(category,
+                    std::move(fetched_category.snippets),
+                    strategy.get());
     }
   }
 
@@ -587,7 +610,7 @@ void NTPSnippetsService::OnFetchFinished(
   }
 
   // TODO(sfiera): notify only when a category changed above.
-  NotifyNewSuggestions();
+  strategy->NotifySuggestions();
 
   // Reschedule after a successful fetch. This resets all currently scheduled
   // fetches, to make sure the fallback interval triggers only if no wifi fetch
@@ -621,16 +644,8 @@ void NTPSnippetsService::ArchiveSnippets(Category category,
   }
 }
 
-void NTPSnippetsService::ReplaceSnippets(Category category,
-                                         NTPSnippet::PtrVector new_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) {
+void NTPSnippetsService::AssignExpiryDates(NTPSnippet::PtrVector* snippets) {

[Marc Treib, 2016/10/18 08:17:45]

nit: this sets both publish and expiry dates. Also, it doesn't seem to access
any members, so it could be a global helper function inside an anonymous
namespace.

[fhorschig, 2016/10/20 13:10:38]

Done.

+  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()) {
@@ -639,26 +654,44 @@ void NTPSnippetsService::ReplaceSnippets(Category category,
           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);
-    }
+void NTPSnippetsService::AddIncompleteSnippets(

[Marc Treib, 2016/10/18 08:17:45]

This has exactly the wrong name - it *removes* incomplete snippets. And same as
above: Could probably be a global helper?

[fhorschig, 2016/10/20 13:10:38]

Done.

+      NTPSnippet::PtrVector* snippets) {
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kAddIncompleteSnippets))
+    return;

[Marc Treib, 2016/10/18 08:17:45]

nit: braces if either the condition or the body don't fit on one line

[fhorschig, 2016/10/20 13:10:38]

Done.

+
+  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);
   }
+}
+
+void NTPSnippetsService::StoreSnippets(
+    const Category& category,
+    NTPSnippet::PtrVector new_snippets,
+    NewNTPSnippetsHandlingStrategy* strategy) {
+  DCHECK(ready());
+  CategoryContent* content = &categories_[category];
+
+  // Remove new snippets that have been dismissed.
+  EraseMatchingSnippets(&new_snippets, content->dismissed);
+
+  AssignExpiryDates(&new_snippets);
+  AddIncompleteSnippets(&new_snippets);
 
   // Do not touch the current set of snippets if the newly fetched one is empty.
   if (new_snippets.empty())
@@ -671,12 +704,11 @@ 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);
+  strategy->SaveNewSnippets(category, std::move(new_snippets));
 }
 
 void NTPSnippetsService::ClearExpiredDismissedSnippets() {
@@ -862,9 +894,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);
@@ -1108,4 +1137,67 @@ NTPSnippetsService::CategoryContent::~CategoryContent() = default;
 NTPSnippetsService::CategoryContent& NTPSnippetsService::CategoryContent::
 operator=(CategoryContent&&) = default;
 
+////////////////////////////////////////////////////////////////////////////////
+// Snippet Handling Strategy Implementations
+
+NTPSnippetsService::
+NewNTPSnippetsHandlingStrategy::NewNTPSnippetsHandlingStrategy(
+    NTPSnippetsService* service)
+  : service_(service) {}
+
+NTPSnippetsService::ReplaceExistingSnippets::ReplaceExistingSnippets(
+    NTPSnippetsService* service)
+  : NewNTPSnippetsHandlingStrategy(service) {}
+
+void NTPSnippetsService::ReplaceExistingSnippets::CollectIdsToExclude(
+    std::set<std::string>* ids) {
+  for (const auto& item : service_->categories_) {
+    const CategoryContent& content = item.second;
+    for (const auto& snippet : content.dismissed)
+      ids->insert(snippet->id());
+  }
+}
+
+void NTPSnippetsService::ReplaceExistingSnippets::SaveNewSnippets(
+    const Category& category,
+    NTPSnippet::PtrVector new_snippets) {
+  CategoryContent* content = &service_->categories_[category];
+  service_->ArchiveSnippets(category, &content->snippets);
+  content->snippets = std::move(new_snippets);
+}
+
+void NTPSnippetsService::ReplaceExistingSnippets::NotifySuggestions() {
+  service_->NotifyNewSuggestions();
+}
+
+NTPSnippetsService::AddNewSnippets::AddNewSnippets(NTPSnippetsService* service)
+  : NewNTPSnippetsHandlingStrategy(service) {}
+
+void NTPSnippetsService::AddNewSnippets::CollectIdsToExclude(
+    std::set<std::string>* ids) {
+  for (const auto& item : service_->categories_) {
+    const CategoryContent& content = item.second;
+    for (const auto& snippet : content.dismissed)
+      ids->insert(snippet->id());
+    for (const auto& snippet : content.archived)
+      ids->insert(snippet->id());
+    for (const auto& snippet : content.snippets)
+      ids->insert(snippet->id());
+  }
+}
+
+void NTPSnippetsService::AddNewSnippets::SaveNewSnippets(
+    const Category& category,
+    NTPSnippet::PtrVector new_snippets) {
+  CategoryContent* content = &service_->categories_[category];
+  content->snippets.insert(content->snippets.end(),
+                           std::make_move_iterator(new_snippets.begin()),
+                           std::make_move_iterator(new_snippets.end()));
+}
+
+void NTPSnippetsService::AddNewSnippets::NotifySuggestions() {
+  // TODO(fhorschig): Notify additional Suggestions only.
+  service_->NotifyNewSuggestions();
+}
+
 }  // namespace ntp_snippets