[NTP::Downloads] Limit number of dismissed IDs instead of pruning.

chrome/browser/ntp_snippets/download_suggestions_provider_unittest.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 "chrome/browser/ntp_snippets/download_suggestions_provider.h"
 
 #include <memory>
 
 #include "base/bind.h"
 #include "base/observer_list.h"
@@ skipping 20 matching lines @@
 using ntp_snippets::test::FakeOfflinePageModel;
 using ntp_snippets::CategoryStatus;
 using offline_pages::ClientId;
 using offline_pages::OfflinePageItem;
 using test::FakeDownloadItem;
 using testing::_;
 using testing::AllOf;
 using testing::AnyNumber;
 using testing::ElementsAre;
 using testing::IsEmpty;
+using testing::Lt;
 using testing::Mock;
 using testing::Return;
 using testing::SizeIs;
 using testing::StrictMock;
 using testing::UnorderedElementsAre;
-using testing::Lt;
 
 namespace ntp_snippets {
 // These functions are implicitly used to print out values during the tests.
 std::ostream& operator<<(std::ostream& os, const ContentSuggestion& value) {
   os << "{ url: " << value.url() << ", publish_date: " << value.publish_date()
      << "}";
   return os;
 }
 
 std::ostream& operator<<(std::ostream& os, const CategoryStatus& value) {
@@ skipping 862 matching lines @@
   IgnoreOnSuggestionInvalidated();
 
   *(downloads_manager()->mutable_items()) = CreateDummyAssetDownloads({1, 2});
   EXPECT_CALL(
       *observer(),
       OnNewSuggestions(_, downloads_category(),
                        UnorderedElementsAre(HasUrl("http://download.com/1"),
                                             HasUrl("http://download.com/2"))));
   CreateProvider(/*show_assets=*/true, /*show_offline_pages=*/false);
 }
+
+TEST_F(DownloadSuggestionsProviderTest,
+       ShouldNotPruneDismissedSuggestionsOnStartup) {
+  IgnoreOnCategoryStatusChangedToAvailable();
+  IgnoreOnSuggestionInvalidated();
+
+  // We dismiss an item to store it in the list of dismissed items.
+  *(downloads_manager()->mutable_items()) = CreateDummyAssetDownloads({1});
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, downloads_category(), _));
+  CreateProvider(/*show_assets=*/true, /*show_offline_pages=*/false);
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(1, /*is_offline_page=*/false));
+  DestroyProvider();
+
+  // We simulate current DownloadManager behaviour;
+  // The download manager has not started reading the list yet, so it is empty.
+  downloads_manager()->mutable_items()->clear();
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, downloads_category(), _));
+  CreateProvider(/*show_assets=*/true, /*show_offline_pages=*/false);
+  Mock::VerifyAndClearExpectations(observer());
+
+  // The first download is being read.
+  *(downloads_manager()->mutable_items()) = CreateDummyAssetDownloads({1});
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, downloads_category(), _))
+      .Times(0);
+  FireDownloadCreated(downloads_manager()->items()[0].get());
+  // The first download should not be reported, because it is dismissed.
+}
+
+TEST_F(DownloadSuggestionsProviderTest, ShouldStoreDismissedSuggestions) {
+  IgnoreOnCategoryStatusChangedToAvailable();
+  IgnoreOnSuggestionInvalidated();
+
+  // Dismiss items to store them in the list of dismissed items.
+  *(offline_pages_model()->mutable_items()) = CreateDummyOfflinePages({1});
+  *(downloads_manager()->mutable_items()) = CreateDummyAssetDownloads({1});
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, downloads_category(), _));
+  CreateProvider(/*show_assets=*/true, /*show_offline_pages=*/true);
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(1, /*is_offline_page=*/true));
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(1, /*is_offline_page=*/false));
+  // Destroy and create provider to simulate turning off Chrome.
+  DestroyProvider();
+
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, downloads_category(), _));
+  CreateProvider(/*show_assets=*/true, /*show_offline_pages=*/true);
+  EXPECT_THAT(GetDismissedSuggestions(),
+              UnorderedElementsAre(HasUrl("http://dummy.com/1"),
+                                   HasUrl("http://download.com/1")));
+}
+
+// TODO(vitaliii): Remove this test once the dismissed ids are pruned. See
+// crbug.com/672758.
+TEST_F(DownloadSuggestionsProviderTest, ShouldRemoveOldDismissedIdsIfTooMany) {
+  IgnoreOnCategoryStatusChangedToAvailable();
+  IgnoreOnSuggestionInvalidated();
+
+  std::vector<int> ids;
+  for (int i = 0;
+       i < DownloadSuggestionsProvider::GetMaxDismissedCountForTesting() + 1;

[Marc Treib, 2016/12/12 09:29:15]

optional: Could store this in a variable first, to make the loop a bit more
readable.

[vitaliii, 2016/12/12 15:41:58]

Done.

+       ++i) {
+    ids.push_back(i);
+  }
+
+  *(downloads_manager()->mutable_items()) = CreateDummyAssetDownloads(ids);
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, downloads_category(), _));
+  CreateProvider(/*show_assets=*/true, /*show_offline_pages=*/false);
+
+  for (int i = 0; i < static_cast<int>(ids.size()); ++i) {
+    provider()->DismissSuggestion(
+        GetDummySuggestionId(i, /*is_offline_page=*/false));
+  }
+
+  EXPECT_THAT(
+      GetDismissedSuggestions(),
+      SizeIs(DownloadSuggestionsProvider::GetMaxDismissedCountForTesting()));
+  DestroyProvider();
+  // The oldest dismissed suggestion must become undismissed now.

[Marc Treib, 2016/12/12 09:29:15]

Maybe mention that this is kind of an "anti-test"? I.e. it documents the current
behavior, but it's really not the behavior we'd want.

[vitaliii, 2016/12/12 15:41:58]

Done.

+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(_, downloads_category(),
+                               ElementsAre(HasUrl("http://download.com/0"))));
+  CreateProvider(/*show_assets=*/true, /*show_offline_pages=*/false);
+}