FetchMore functionality backend

components/ntp_snippets/remote/ntp_snippets_fetcher.cc

Index: components/ntp_snippets/remote/ntp_snippets_fetcher.cc
diff --git a/components/ntp_snippets/remote/ntp_snippets_fetcher.cc b/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
index 24aa318c2eff0b5925b53164554f9bca0ba9bdd2..f671f738fcaffb9c95c4fac1c364f78cb61a7e53 100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
@@ -316,17 +316,14 @@ NTPSnippetsFetcher::~NTPSnippetsFetcher() {
     token_service_->RemoveObserver(this);
 }
 
-void NTPSnippetsFetcher::SetCallback(

[tschumann, 2016/10/24 06:38:25]

it's nice to see this method gone. However, i'm still not 100% sure I like the
logic of "you pass a callback into FetchSnippetsFromHosts(), and we maybe call
it back". Would it be hard to make this a proper contract and always call it
once the request finishes? Shouldn't the higher layer (which issues multiple
calls) deal with how to deal with races?

Looking at the snippet fetcher implementation, there's also some room for
refactoring. A lot of state of the NTPSnippetsFetcher is per-request. So we
could separate the snippets fetching logic from the request state (introduce an
internal request object which would also hold the callback, request parameters,
etc.)

[Marc Treib, 2016/10/24 09:47:40]

Yes, we actually discussed this, and agree it's a good idea. I'd prefer that
kind of restructuring to happen in a separate CL though - let's keep
restructurings separate from new functionality if possible.

-    const SnippetsAvailableCallback& callback) {
-  snippets_available_callback_ = callback;
-}
-
 void NTPSnippetsFetcher::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts,
     const std::string& language_code,
     const std::set<std::string>& excluded_ids,
     int count,
-    bool interactive_request) {
+    bool interactive_request,
+    SnippetsAvailableCallback callback,
+    base::Optional<Category> exclusive_category) {
   if (!DemandQuotaForRequest(interactive_request)) {
     FetchFinished(OptionalFetchedCategories(),
                   interactive_request
@@ -339,6 +336,8 @@ void NTPSnippetsFetcher::FetchSnippetsFromHosts(
   hosts_ = hosts;
   fetch_start_time_ = tick_clock_->NowTicks();
   excluded_ids_ = excluded_ids;
+  exclusive_category_ = exclusive_category;
+  snippets_available_callback_ = std::move(callback);
 
   locale_ = PosixLocaleFromBCP47Language(language_code);
   count_to_fetch_ = count;
@@ -758,6 +757,24 @@ void NTPSnippetsFetcher::OnJsonError(const std::string& error) {
       /*extra_message=*/base::StringPrintf(" (error %s)", error.c_str()));
 }
 
+void NTPSnippetsFetcher::FilterCategories(OptionalFetchedCategories* fetched) {
+  if (!fetched->has_value())
+    return;
+  if (!exclusive_category_.has_value())
+    return;
+  FetchedCategoriesVector* categories = &fetched->value();
+  Category exclusive = exclusive_category_.value();
+  auto category_it =
+      std::find_if(categories->begin(), categories->end(),
+                   [exclusive](const FetchedCategory& c) -> bool {
+                     return c.category == exclusive;
+                   });
+  if (category_it == categories->end())

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

Shouldn't we erase everything in this case?

[fhorschig, 2016/11/02 05:05:27]

Correct. Changed and tested in CL 2466863003.

+    return;
+  categories->erase(categories->begin(), category_it);
+  categories->erase(category_it + 1, categories->end());
+}
+
 void NTPSnippetsFetcher::FetchFinished(
     OptionalFetchedCategories fetched_categories,
     FetchResult result,
@@ -765,6 +782,11 @@ void NTPSnippetsFetcher::FetchFinished(
   DCHECK(result == FetchResult::SUCCESS || !fetched_categories);
   last_status_ = FetchResultToString(result) + extra_message;
 
+  // TODO(fhorschig): Filter (un)wanted categories by modifying fetch request.
+  //                  As soo as backends support the parameter, there is no
+  //                  no reason to overfetch and filter here.

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

nit: We don't align after the TODO.
wanted or unwanted?
We're not modifying any request.
Also typo: "soo" and "no no"

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

Done.

+  FilterCategories(&fetched_categories);
+
   // Don't record FetchTimes if the result indicates that a precondition
   // failed and we never actually sent a network request
   if (!IsFetchPreconditionFailed(result)) {
@@ -777,7 +799,7 @@ void NTPSnippetsFetcher::FetchFinished(
 
   DVLOG(1) << "Fetch finished: " << last_status_;
   if (!snippets_available_callback_.is_null())
-    snippets_available_callback_.Run(std::move(fetched_categories));
+    std::move(snippets_available_callback_).Run(std::move(fetched_categories));
 }
 
 bool NTPSnippetsFetcher::DemandQuotaForRequest(bool interactive_request) {