Removes a data-dependent DCHECK().

This check actually triggered on the wrong condition (hence I also renamed a few things to reduce the confusion).
There was a valid point for the check, though: we should only fill the histogram when we don't get too many snippets.
Once we completely moved to the Zine cloud backend, this histogram should go away anyways.

Review-Url: https://codereview.chromium.org/2387293009
Cr-Commit-Position: refs/heads/master@{#423749}

components/ntp_snippets/remote/ntp_snippets_fetcher.cc

@@ -137,8 +137,9 @@ bool AddSnippetsFromListValue(bool content_suggestions_api,
                               NTPSnippet::PtrVector* snippets) {
   for (const auto& value : list) {
     const base::DictionaryValue* dict = nullptr;
-    if (!value->GetAsDictionary(&dict))
+    if (!value->GetAsDictionary(&dict)) {
       return false;
+    }
 
     std::unique_ptr<NTPSnippet> snippet;
     if (content_suggestions_api) {
@@ -146,8 +147,9 @@ bool AddSnippetsFromListValue(bool content_suggestions_api,
     } else {
       snippet = NTPSnippet::CreateFromChromeReaderDictionary(*dict);
     }
-    if (!snippet)
+    if (!snippet) {
       return false;
+    }
 
     snippets->push_back(std::move(snippet));
   }
@@ -238,7 +240,7 @@ void NTPSnippetsFetcher::FetchSnippetsFromHosts(
     int count,
     bool interactive_request) {
   if (!request_throttler_.DemandQuotaForRequest(interactive_request)) {
-    FetchFinished(OptionalSnippets(),
+    FetchFinished(OptionalFetchedCategories(),
                   interactive_request
                       ? FetchResult::INTERACTIVE_QUOTA_ERROR
                       : FetchResult::NON_INTERACTIVE_QUOTA_ERROR,
@@ -484,7 +486,7 @@ void NTPSnippetsFetcher::OnGetTokenFailure(
 
   DLOG(ERROR) << "Unable to get token: " << error.ToString();
   FetchFinished(
-      OptionalSnippets(), FetchResult::OAUTH_TOKEN_ERROR,
+      OptionalFetchedCategories(), FetchResult::OAUTH_TOKEN_ERROR,
       /*extra_message=*/base::StringPrintf(" (%s)", error.ToString().c_str()));
 }
 
@@ -514,7 +516,8 @@ void NTPSnippetsFetcher::OnURLFetchComplete(const URLFetcher* source) {
       status.is_success() ? source->GetResponseCode() : status.error());
 
   if (!status.is_success()) {
-    FetchFinished(OptionalSnippets(), FetchResult::URL_REQUEST_STATUS_ERROR,
+    FetchFinished(OptionalFetchedCategories(),
+                  FetchResult::URL_REQUEST_STATUS_ERROR,
                   /*extra_message=*/base::StringPrintf(" %d", status.error()));
   } else if (source->GetResponseCode() != net::HTTP_OK) {
     // TODO(jkrcal): https://crbug.com/609084
@@ -523,7 +526,7 @@ void NTPSnippetsFetcher::OnURLFetchComplete(const URLFetcher* source) {
     // fetch a new auth token). We should extract that into a common class
     // instead of adding it to every single class that uses auth tokens.
     FetchFinished(
-        OptionalSnippets(), FetchResult::HTTP_ERROR,
+        OptionalFetchedCategories(), FetchResult::HTTP_ERROR,
         /*extra_message=*/base::StringPrintf(" %d", source->GetResponseCode()));
   } else {
     bool stores_result_to_string =
@@ -598,11 +601,12 @@ bool NTPSnippetsFetcher::JsonToSnippets(const base::Value& parsed,
 void NTPSnippetsFetcher::OnJsonParsed(std::unique_ptr<base::Value> parsed) {
   FetchedCategoriesVector categories;
   if (JsonToSnippets(*parsed, &categories)) {
-    FetchFinished(OptionalSnippets(std::move(categories)), FetchResult::SUCCESS,
+    FetchFinished(OptionalFetchedCategories(std::move(categories)),
+                  FetchResult::SUCCESS,
                   /*extra_message=*/std::string());
   } else {
     LOG(WARNING) << "Received invalid snippets: " << last_fetch_json_;
-    FetchFinished(OptionalSnippets(),
+    FetchFinished(OptionalFetchedCategories(),
                   FetchResult::INVALID_SNIPPET_CONTENT_ERROR,
                   /*extra_message=*/std::string());
   }
@@ -612,14 +616,15 @@ void NTPSnippetsFetcher::OnJsonError(const std::string& error) {
   LOG(WARNING) << "Received invalid JSON (" << error
                << "): " << last_fetch_json_;
   FetchFinished(
-      OptionalSnippets(), FetchResult::JSON_PARSE_ERROR,
+      OptionalFetchedCategories(), FetchResult::JSON_PARSE_ERROR,
       /*extra_message=*/base::StringPrintf(" (error %s)", error.c_str()));
 }
 
-void NTPSnippetsFetcher::FetchFinished(OptionalSnippets snippets,
-                                       FetchResult result,
-                                       const std::string& extra_message) {
-  DCHECK(result == FetchResult::SUCCESS || !snippets);
+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;
 
   // Don't record FetchTimes if the result indicates that a precondition
@@ -634,7 +639,7 @@ void NTPSnippetsFetcher::FetchFinished(OptionalSnippets snippets,
 
   DVLOG(1) << "Fetch finished: " << last_status_;
   if (!snippets_available_callback_.is_null())
-    snippets_available_callback_.Run(std::move(snippets));
+    snippets_available_callback_.Run(std::move(fetched_categories));
 }
 
 }  // namespace ntp_snippets

components/ntp_snippets/remote/ntp_snippets_fetcher.h

@@ -54,13 +54,13 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
     FetchedCategory& operator=(FetchedCategory&&);  // = default, in .cc
   };
   using FetchedCategoriesVector = std::vector<FetchedCategory>;
-  using OptionalSnippets = base::Optional<FetchedCategoriesVector>;
+  using OptionalFetchedCategories = base::Optional<FetchedCategoriesVector>;
 
   // |snippets| contains parsed snippets if a fetch succeeded. If problems
   // occur, |snippets| contains no value (no actual vector in base::Optional).
   // Error details can be retrieved using last_status().
   using SnippetsAvailableCallback =
-      base::Callback<void(OptionalSnippets snippets)>;
+      base::Callback<void(OptionalFetchedCategories fetched_categories)>;
 
   // Enumeration listing all possible outcomes for fetch attempts. Used for UMA
   // histograms, so do not change existing values. Insert new values at the end,
@@ -195,7 +195,7 @@ class NTPSnippetsFetcher : public OAuth2TokenService::Consumer,
                       FetchedCategoriesVector* categories);
   void OnJsonParsed(std::unique_ptr<base::Value> parsed);
   void OnJsonError(const std::string& error);
-  void FetchFinished(OptionalSnippets snippets,
+  void FetchFinished(OptionalFetchedCategories fetched_categories,
                      FetchResult result,
                      const std::string& extra_message);
 

components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc

@@ -63,16 +63,17 @@ MATCHER(HasValue, "") {
 }
 
 MATCHER(IsEmptyArticleList, "is an empty list of articles") {
-  NTPSnippetsFetcher::OptionalSnippets& snippets = *arg;
-  return snippets && snippets->size() == 1 &&
-         snippets->begin()->snippets.empty();
+  NTPSnippetsFetcher::OptionalFetchedCategories& fetched_categories = *arg;
+  return fetched_categories && fetched_categories->size() == 1 &&
+         fetched_categories->begin()->snippets.empty();
 }
 
 MATCHER_P(IsSingleArticle, url, "is a list with the single article %(url)s") {
-  NTPSnippetsFetcher::OptionalSnippets& snippets = *arg;
-  return snippets && snippets->size() == 1 &&
-         snippets->begin()->snippets.size() == 1 &&
-         snippets->begin()->snippets[0]->best_source().url.spec() == url;
+  NTPSnippetsFetcher::OptionalFetchedCategories& fetched_categories = *arg;
+  return fetched_categories && fetched_categories->size() == 1 &&
+         fetched_categories->begin()->snippets.size() == 1 &&
+         fetched_categories->begin()->snippets[0]->best_source().url.spec() ==
+             url;
 }
 
 MATCHER_P(EqualsJSON, json, "equals JSON") {
@@ -97,11 +98,14 @@ MATCHER_P(EqualsJSON, json, "equals JSON") {
 class MockSnippetsAvailableCallback {
  public:
   // Workaround for gMock's lack of support for movable arguments.
-  void WrappedRun(NTPSnippetsFetcher::OptionalSnippets snippets) {
-    Run(&snippets);
+  void WrappedRun(
+      NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
+    Run(&fetched_categories);
   }
 
-  MOCK_METHOD1(Run, void(NTPSnippetsFetcher::OptionalSnippets* snippets));
+  MOCK_METHOD1(
+      Run,
+      void(NTPSnippetsFetcher::OptionalFetchedCategories* fetched_categories));
 };
 
 // Factory for FakeURLFetcher objects that always generate errors.
@@ -528,18 +532,18 @@ TEST_F(NTPSnippetsContentSuggestionsFetcherTest, ServerCategories) {
       "}]}";
   SetFakeResponse(/*response_data=*/kJsonStr, net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
-  NTPSnippetsFetcher::OptionalSnippets snippets;
+  NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories;
   EXPECT_CALL(mock_callback(), Run(_))
-      .WillOnce(WithArg<0>(MovePointeeTo(&snippets)));
+      .WillOnce(WithArg<0>(MovePointeeTo(&fetched_categories)));
   snippets_fetcher().FetchSnippetsFromHosts(test_hosts(), test_lang(),
                                             test_excluded(),
                                             /*count=*/1,
                                             /*interactive_request=*/true);
   FastForwardUntilNoTasksRemain();
 
-  ASSERT_TRUE(snippets);
-  ASSERT_THAT(snippets->size(), Eq(2u));
-  for (const auto& category : *snippets) {
+  ASSERT_TRUE(fetched_categories);
+  ASSERT_THAT(fetched_categories->size(), Eq(2u));
+  for (const auto& category : *fetched_categories) {
     const auto& articles = category.snippets;
     switch (category.category.id()) {
       case static_cast<int>(KnownCategories::ARTICLES):
@@ -765,11 +769,11 @@ TEST_F(NTPSnippetsFetcherTest, ShouldCancelOngoingFetch) {
 
 ::std::ostream& operator<<(
     ::std::ostream& os,
-    const NTPSnippetsFetcher::OptionalSnippets& snippets) {
-  if (snippets) {
+    const NTPSnippetsFetcher::OptionalFetchedCategories& fetched_categories) {
+  if (fetched_categories) {
     // Matchers above aren't any more precise than this, so this is sufficient
     // for test-failure diagnostics.
-    return os << "list with " << snippets->size() << " elements";
+    return os << "list with " << fetched_categories->size() << " elements";
   }
   return os << "null";
 }

components/ntp_snippets/remote/ntp_snippets_service.cc

@@ -499,7 +499,7 @@ void NTPSnippetsService::OnDatabaseError() {
 }
 
 void NTPSnippetsService::OnFetchFinished(
-    NTPSnippetsFetcher::OptionalSnippets snippets) {
+    NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories) {
   if (!ready())
     return;
 
@@ -513,10 +513,11 @@ void NTPSnippetsService::OnFetchFinished(
 
   // If snippets were fetched successfully, update our |categories_| from each
   // category provided by the server.
-  if (snippets) {
+  if (fetched_categories) {
     // TODO(jkrcal): A bit hard to understand with so many variables called
     // "*categor*". Isn't here some room for simplification?
-    for (NTPSnippetsFetcher::FetchedCategory& fetched_category : *snippets) {
+    for (NTPSnippetsFetcher::FetchedCategory& fetched_category :
+         *fetched_categories) {
       Category category = fetched_category.category;
 
       // TODO(sfiera): Avoid hard-coding articles category checks in so many
@@ -528,13 +529,13 @@ void NTPSnippetsService::OnFetchFinished(
       }
       categories_[category].provided_by_server = true;
 
-      DCHECK_LE(snippets->size(), static_cast<size_t>(kMaxSnippetCount));
-      // TODO(sfiera): histograms for server categories.
-      // Sparse histogram used because the number of snippets is small (bound by
-      // kMaxSnippetCount).
+      // TODO(tschumann): Remove this histogram once we only talk to the content
+      // suggestions cloud backend.
       if (category == articles_category_) {
-        UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticlesFetched",
-                                    fetched_category.snippets.size());
+        UMA_HISTOGRAM_SPARSE_SLOWLY(
+            "NewTabPage.Snippets.NumArticlesFetched",
+            std::min(fetched_category.snippets.size(),
+                     static_cast<size_t>(kMaxSnippetCount + 1)));
       }
       ReplaceSnippets(category, std::move(fetched_category.snippets));
     }
@@ -561,7 +562,7 @@ void NTPSnippetsService::OnFetchFinished(
   // fetches, to make sure the fallback interval triggers only if no wifi fetch
   // succeeded, and also that we don't do a background fetch immediately after
   // a user-initiated one.
-  if (snippets)
+  if (fetched_categories)
     RescheduleFetching(true);
 }
 

components/ntp_snippets/remote/ntp_snippets_service.h

@@ -206,7 +206,8 @@ class NTPSnippetsService final : public ContentSuggestionsProvider,
   void OnDatabaseError();
 
   // Callback for the NTPSnippetsFetcher.
-  void OnFetchFinished(NTPSnippetsFetcher::OptionalSnippets snippets);
+  void OnFetchFinished(
+      NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories);
 
   // Moves all snippets from |to_archive| into the archive of the |category|.
   // It also deletes the snippets from the DB and keeps the archive reasonably

components/ntp_snippets/remote/ntp_snippets_service_unittest.cc

@@ -1172,4 +1172,19 @@ TEST_F(NTPSnippetsServiceTest, ShouldClearOrphanedImagesOnRestart) {
   EXPECT_TRUE(FetchImage(service.get(), MakeArticleID(kSnippetUrl)).IsEmpty());
 }
 
+TEST_F(NTPSnippetsServiceTest, ShouldHandleMoreThanMaxSnippetsInResponse) {
+  auto service = MakeSnippetsService();
+
+  std::vector<std::string> suggestions;
+  for (int i = 0 ; i < service->GetMaxSnippetCountForTesting() + 1; ++i) {
+    suggestions.push_back(GetSnippetWithUrl(
+        base::StringPrintf("http://localhost/snippet-id-%d", i)));
+  }
+  LoadFromJSONString(service.get(), GetTestJson(suggestions));
+  // TODO(tschumann): We should probably trim out any additional results and
+  // only serve the MaxSnippetCount items.
+  EXPECT_THAT(service->GetSnippetsForTesting(articles_category()),
+              SizeIs(service->GetMaxSnippetCountForTesting() + 1));
+}
+
 }  // namespace ntp_snippets