FetchMore functionality backend

components/ntp_snippets/remote/ntp_snippets_fetcher.cc

 // Copyright 2016 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_fetcher.h"
 
 #include <cstdlib>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
@@ skipping 298 matching lines @@
         << "Unknown value for " << kPersonalizationName << ": "
         << personalization;
   }
 }
 
 NTPSnippetsFetcher::~NTPSnippetsFetcher() {
   if (waiting_for_refresh_token_)
     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
                       ? FetchResult::INTERACTIVE_QUOTA_ERROR
                       : FetchResult::NON_INTERACTIVE_QUOTA_ERROR,
                   /*extra_message=*/std::string());
     return;
   }
 
   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;
 
   bool use_authentication = UsesAuthentication();
   interactive_request_ = interactive_request;
 
   if (use_authentication && signin_manager_->IsAuthenticated()) {
     // Signed-in: get OAuth token --> fetch snippets.
     oauth_token_retried_ = false;
@@ skipping 399 matching lines @@
 }
 
 void NTPSnippetsFetcher::OnJsonError(const std::string& error) {
   LOG(WARNING) << "Received invalid JSON (" << error
                << "): " << last_fetch_json_;
   FetchFinished(
       OptionalFetchedCategories(), FetchResult::JSON_PARSE_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,
     const std::string& extra_message) {
   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)) {
     UMA_HISTOGRAM_TIMES("NewTabPage.Snippets.FetchTime",
                         tick_clock_->NowTicks() - fetch_start_time_);
   }
   UMA_HISTOGRAM_ENUMERATION("NewTabPage.Snippets.FetchResult",
                             static_cast<int>(result),
                             static_cast<int>(FetchResult::RESULT_MAX));
 
   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) {
   switch (user_classifier_->GetUserClass()) {
     case UserClassifier::UserClass::RARE_NTP_USER:
       return request_throttler_rare_ntp_user_.DemandQuotaForRequest(
           interactive_request);
     case UserClassifier::UserClass::ACTIVE_NTP_USER:
       return request_throttler_active_ntp_user_.DemandQuotaForRequest(
           interactive_request);
     case UserClassifier::UserClass::ACTIVE_SUGGESTIONS_CONSUMER:
       return request_throttler_active_suggestions_consumer_
           .DemandQuotaForRequest(interactive_request);
   }
   NOTREACHED();
   return false;
 }
 
 }  // namespace ntp_snippets