[NTPSnippets] Show all downloads on the NTP (3/3): Downloads provider.

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 "base/bind.h"
+#include "base/observer_list.h"
+#include "base/strings/string_number_conversions.h"
+#include "components/ntp_snippets/category.h"
+#include "components/ntp_snippets/category_factory.h"
+#include "components/ntp_snippets/mock_content_suggestions_provider_observer.h"
+#include "components/ntp_snippets/offline_pages/offline_pages_test_utils.h"
+#include "components/offline_pages/client_namespace_constants.h"
+#include "components/prefs/testing_pref_service.h"
+#include "content/public/test/mock_download_item.h"
+#include "content/public/test/mock_download_manager.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::DownloadItem;
+using content::MockDownloadItem;
+using content::MockDownloadManager;
+using ntp_snippets::Category;
+using ntp_snippets::CategoryFactory;
+using ntp_snippets::ContentSuggestion;
+using ntp_snippets::ContentSuggestionsProvider;
+using ntp_snippets::MockContentSuggestionsProviderObserver;
+using ntp_snippets::OfflinePageProxy;
+using ntp_snippets::test::CaptureDismissedSuggestions;
+using ntp_snippets::test::FakeOfflinePageModel;
+using offline_pages::ClientId;
+using offline_pages::OfflinePageItem;
+using testing::_;
+using testing::AnyNumber;
+using testing::ElementsAre;
+using testing::IsEmpty;
+using testing::Mock;
+using testing::Return;
+using testing::SizeIs;
+using testing::UnorderedElementsAre;
+
+namespace ntp_snippets {
+
+void PrintTo(const ContentSuggestion& value, std::ostream* os) {

[Marc Treib, 2016/10/13 12:11:26]

Is this used anywhere?

[vitaliii, 2016/10/15 18:36:31]

Yes, the testing framework uses this when printing a ContentSuggestion.
I added a comment.

[Marc Treib, 2016/10/17 10:18:40]

I think defining an "operator<<" also does the trick, and might look a bit less
"magic"? Like
ostream& operator<<(ostream& s, const ContentSuggestion& value)

[vitaliii, 2016/10/26 00:07:54]

Done.

+  *os << "{ url: " << value.url() << ", publish_date: " << value.publish_date()
+      << "}";
+}
+
+}  // namespace ntp_snippets
+
+namespace {
+
+// TODO(vitaliii): Move this and |PrintTo| above to common file and replace
+// remaining |Property(&ContentSuggestion::url, GURL("some_url"))|.
+// See crbug.com/655513.
+MATCHER_P(HasUrl, url, "") {
+  *result_listener << "expected URL: " << url
+                   << "has URL: " << arg.url().spec();
+  return arg.url().spec() == url;
+}
+
+OfflinePageItem CreateDummyOfflinePage(int id) {
+  return ntp_snippets::test::CreateDummyOfflinePageItem(
+      id, offline_pages::kAsyncNamespace);
+}
+
+std::vector<OfflinePageItem> CreateDummyOfflinePages(
+    const std::vector<int>& ids) {
+  std::vector<OfflinePageItem> result;
+  for (int id : ids) {
+    result.push_back(CreateDummyOfflinePage(id));
+  }
+  return result;
+}
+
+OfflinePageItem CreateDummyOfflinePage(int id, base::Time time) {
+  OfflinePageItem item = CreateDummyOfflinePage(id);
+  item.creation_time = time;
+  return item;
+}
+
+class FakeDownloadItem : public DownloadItem {

[Marc Treib, 2016/10/13 12:11:26]

Since this is so huge, maybe worth putting it into a separate file?

[vitaliii, 2016/10/15 18:36:31]

Done.

+ public:
+  FakeDownloadItem() {}
+  ~FakeDownloadItem() override {
+    NotifyOnDownloadRemoved();
+    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestroyed(this));
+  }
+
+  void AddObserver(Observer* observer) override {
+    observers_.AddObserver(observer);
+  }
+
+  void RemoveObserver(Observer* observer) override {
+    observers_.RemoveObserver(observer);
+  }
+
+  void NotifyOnDownloadRemoved() {
+    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadRemoved(this));
+  }
+
+  void NotifyOnDownloadUpdated() { UpdateObservers(); }
+
+  void UpdateObservers() override {
+    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadUpdated(this));
+  }
+
+  void SetId(uint32_t id) { id_ = id; }
+  uint32_t GetId() const override { return id_; }
+
+  void SetURL(GURL url) { url_ = url; }
+  const GURL& GetURL() const override { return url_; }
+
+  void SetTargetFilePath(const base::FilePath& file_path) {
+    file_path_ = file_path;
+  }
+  const base::FilePath& GetTargetFilePath() const override {
+    return file_path_;
+  }
+
+  void SetFileExternallyRemoved(bool is_file_externally_removed) {
+    is_file_externally_removed_ = is_file_externally_removed;
+  }
+  bool GetFileExternallyRemoved() const override {
+    return is_file_externally_removed_;
+  }
+
+  void SetEndTime(const base::Time& end_time) { end_time_ = end_time; }
+  base::Time GetEndTime() const override { return end_time_; }
+
+  void SetState(const DownloadState& state) { download_state_ = state; }
+  DownloadState GetState() const override { return download_state_; }
+
+  // The methods below are not supported and are not expected to be called.
+  void ValidateDangerousDownload() override { NOTREACHED(); }
+  void StealDangerousDownload(const AcquireFileCallback& callback) override {
+    NOTREACHED();
+    callback.Run(base::FilePath());
+  }
+  void Pause() override { NOTREACHED(); }
+  void Resume() override { NOTREACHED(); }
+  void Cancel(bool user_cancel) override { NOTREACHED(); }
+  void Remove() override { NOTREACHED(); }
+  void OpenDownload() override { NOTREACHED(); }
+  void ShowDownloadInShell() override { NOTREACHED(); }
+  const std::string& GetGuid() const override {
+    NOTREACHED();
+    return dummy_string;
+  }
+  content::DownloadInterruptReason GetLastReason() const override {
+    NOTREACHED();
+    return content::DownloadInterruptReason();
+  }
+  bool IsPaused() const override {
+    NOTREACHED();
+    return false;
+  }
+  bool IsTemporary() const override {
+    NOTREACHED();
+    return false;
+  }
+  bool CanResume() const override {
+    NOTREACHED();
+    return false;
+  }
+  bool IsDone() const override {
+    NOTREACHED();
+    return true;
+  }
+  const std::vector<GURL>& GetUrlChain() const override {
+    NOTREACHED();
+    return dummy_url_vector;
+  }
+  const GURL& GetOriginalUrl() const override {
+    NOTREACHED();
+    return dummy_url;
+  }
+  const GURL& GetReferrerUrl() const override {
+    NOTREACHED();
+    return dummy_url;
+  }
+  const GURL& GetSiteUrl() const override {
+    NOTREACHED();
+    return dummy_url;
+  }
+  const GURL& GetTabUrl() const override {
+    NOTREACHED();
+    return dummy_url;
+  }
+  const GURL& GetTabReferrerUrl() const override {
+    NOTREACHED();
+    return dummy_url;
+  }
+  std::string GetSuggestedFilename() const override {
+    NOTREACHED();
+    return std::string();
+  }
+  std::string GetContentDisposition() const override {
+    NOTREACHED();
+    return std::string();
+  }
+  std::string GetMimeType() const override {
+    NOTREACHED();
+    return std::string();
+  }
+  std::string GetOriginalMimeType() const override {
+    NOTREACHED();
+    return std::string();
+  }
+  std::string GetRemoteAddress() const override {
+    NOTREACHED();
+    return std::string();
+  }
+  bool HasUserGesture() const override {
+    NOTREACHED();
+    return false;
+  }
+  ui::PageTransition GetTransitionType() const override {
+    NOTREACHED();
+    return ui::PageTransition();
+  }
+  const std::string& GetLastModifiedTime() const override {
+    NOTREACHED();
+    return dummy_string;
+  }
+  const std::string& GetETag() const override {
+    NOTREACHED();
+    return dummy_string;
+  }
+  bool IsSavePackageDownload() const override {
+    NOTREACHED();
+    return false;
+  }
+  const base::FilePath& GetFullPath() const override {
+    NOTREACHED();
+    return dummy_file_path;
+  }
+  const base::FilePath& GetForcedFilePath() const override {
+    NOTREACHED();
+    return dummy_file_path;
+  }
+  base::FilePath GetFileNameToReportUser() const override {
+    NOTREACHED();
+    return base::FilePath();
+  }
+  TargetDisposition GetTargetDisposition() const override {
+    NOTREACHED();
+    return TargetDisposition();
+  }
+  const std::string& GetHash() const override {
+    NOTREACHED();
+    return dummy_string;
+  }
+  void DeleteFile(const base::Callback<void(bool)>& callback) override {
+    NOTREACHED();
+    callback.Run(false);
+  }
+  bool IsDangerous() const override {
+    NOTREACHED();
+    return false;
+  }
+  content::DownloadDangerType GetDangerType() const override {
+    NOTREACHED();
+    return content::DownloadDangerType();
+  }
+  bool TimeRemaining(base::TimeDelta* remaining) const override {
+    NOTREACHED();
+    return false;
+  }
+  int64_t CurrentSpeed() const override {
+    NOTREACHED();
+    return 1;
+  }
+  int PercentComplete() const override {
+    NOTREACHED();
+    return 1;
+  }
+  bool AllDataSaved() const override {
+    NOTREACHED();
+    return true;
+  }
+  int64_t GetTotalBytes() const override {
+    NOTREACHED();
+    return 1;
+  }
+  int64_t GetReceivedBytes() const override {
+    NOTREACHED();
+    return 1;
+  }
+  base::Time GetStartTime() const override {
+    NOTREACHED();
+    return base::Time();
+  }
+  bool CanShowInFolder() override {
+    NOTREACHED();
+    return true;
+  }
+  bool CanOpenDownload() override {
+    NOTREACHED();
+    return true;
+  }
+  bool ShouldOpenFileBasedOnExtension() override {
+    NOTREACHED();
+    return true;
+  }
+  bool GetOpenWhenComplete() const override {
+    NOTREACHED();
+    return false;
+  }
+  bool GetAutoOpened() override {
+    NOTREACHED();
+    return false;
+  }
+  bool GetOpened() const override {
+    NOTREACHED();
+    return false;
+  }
+  content::BrowserContext* GetBrowserContext() const override {
+    NOTREACHED();
+    return nullptr;
+  }
+  content::WebContents* GetWebContents() const override {
+    NOTREACHED();
+    return nullptr;
+  }
+  void OnContentCheckCompleted(
+      content::DownloadDangerType danger_type) override {
+    NOTREACHED();
+  }
+  void SetOpenWhenComplete(bool open) override { NOTREACHED(); }
+  void SetOpened(bool opened) override { NOTREACHED(); }
+  void SetDisplayName(const base::FilePath& name) override { NOTREACHED(); }
+  std::string DebugString(bool verbose) const override {
+    NOTREACHED();
+    return std::string();
+  }
+
+ private:
+  base::ObserverList<Observer> observers_;
+  uint32_t id_;
+  GURL url_;
+  base::FilePath file_path_;
+  bool is_file_externally_removed_;
+  base::Time end_time_;
+  DownloadState download_state_;
+
+  // The members bellow are to be returned by methods, which return links.

[Marc Treib, 2016/10/13 12:11:26]

s/bellow/below/
s/links/pointers/ ? I assume that's what you mean?

[vitaliii, 2016/10/15 18:36:31]

First - yes.
Second - no, I meant by reference.

+  std::string dummy_string;
+  std::vector<GURL> dummy_url_vector;
+  GURL dummy_url;
+  base::FilePath dummy_file_path;
+};
+
+std::unique_ptr<FakeDownloadItem> CreateDummyAssetDownload(int id) {

[Marc Treib, 2016/10/13 12:11:26]

nit: Please move this down to the other CreateDummyAssetDownload* functions. (or
move those here)

[vitaliii, 2016/10/15 18:36:31]

Done.

+  std::unique_ptr<FakeDownloadItem> item = base::MakeUnique<FakeDownloadItem>();
+  item->SetId(id);
+  std::string id_string = base::IntToString(id);
+  item->SetTargetFilePath(
+      base::FilePath::FromUTF8Unsafe("folder/file" + id_string + ".mhtml"));
+  item->SetURL(GURL("http://dummy_file.com/" + id_string));
+  item->SetEndTime(base::Time::Now());
+  item->SetFileExternallyRemoved(false);
+  item->SetState(DownloadItem::DownloadState::COMPLETE);
+  return item;
+}
+
+class ObservedMockDownloadManager : public MockDownloadManager {
+ public:
+  ObservedMockDownloadManager() {}
+  ~ObservedMockDownloadManager() override {
+    FOR_EACH_OBSERVER(Observer, observers_, ManagerGoingDown(this));
+  }
+
+  // Observer accessors.
+  void AddObserver(Observer* observer) override {
+    observers_.AddObserver(observer);
+  }
+
+  void RemoveObserver(Observer* observer) override {
+    observers_.RemoveObserver(observer);
+  }
+
+  void NotifyOnDownloadCreated(DownloadItem* item) {
+    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, item));
+  }
+
+  std::vector<std::unique_ptr<FakeDownloadItem>>* mutable_items() {
+    return &items_;
+  }
+
+  const std::vector<std::unique_ptr<FakeDownloadItem>>& items() const {
+    return items_;
+  }
+
+  void GetAllDownloads(std::vector<DownloadItem*>* all_downloads) override {
+    all_downloads->clear();
+    for (const auto& item : items_)
+      all_downloads->push_back(item.get());
+  }
+
+ private:
+  base::ObserverList<Observer> observers_;
+  std::vector<std::unique_ptr<FakeDownloadItem>> items_;
+};
+
+std::unique_ptr<FakeDownloadItem> CreateDummyAssetDownload(
+    int id,
+    const base::Time end_time) {
+  std::unique_ptr<FakeDownloadItem> item = CreateDummyAssetDownload(id);
+  item->SetEndTime(end_time);
+  return item;
+}
+
+std::vector<std::unique_ptr<FakeDownloadItem>> CreateDummyAssetDownloads(
+    const std::vector<int>& ids) {
+  std::vector<std::unique_ptr<FakeDownloadItem>> result;
+  for (int id : ids) {
+    result.push_back(CreateDummyAssetDownload(id));
+  }
+  return result;
+}
+
+}  // namespace
+
+class DownloadSuggestionsProviderTest : public testing::Test {
+ public:
+  DownloadSuggestionsProviderTest()
+      : pref_service_(new TestingPrefServiceSimple()) {
+    DownloadSuggestionsProvider::RegisterProfilePrefs(
+        pref_service()->registry());
+
+    scoped_refptr<OfflinePageProxy> proxy(
+        new OfflinePageProxy(&offline_pages_model_));
+
+    provider_.reset(
+        new DownloadSuggestionsProvider(&observer_, &category_factory_, proxy,
+                                        &downloads_manager_, pref_service(),
+                                        /*download_manager_ui_enabled=*/false));
+  }
+
+  Category downloads_category() {
+    return category_factory_.FromKnownCategory(
+        ntp_snippets::KnownCategories::DOWNLOADS);
+  }
+
+  void FireOfflinePageModelChanged(const std::vector<OfflinePageItem>& items) {
+    provider_->OfflinePageModelChanged(items);
+  }
+
+  void FireOfflinePageDeleted(const OfflinePageItem& item) {
+    provider_->OfflinePageDeleted(item.offline_id, item.client_id);
+  }
+
+  void FireOnDownloadCreated(DownloadItem* item) {
+    downloads_manager_.NotifyOnDownloadCreated(item);
+  }
+
+  void FireDownloadRemoved(FakeDownloadItem* item) {

[Marc Treib, 2016/10/13 12:11:26]

nit: either FireOnDownloadRemoved (add "On"), or remove the "On" in
FireOnDownloadCreated and FireOnDownloadsCreated

[vitaliii, 2016/10/15 18:36:30]

Done.

+    item->NotifyOnDownloadRemoved();
+  }
+
+  void FireOnDownloadsCreated(
+      const std::vector<std::unique_ptr<FakeDownloadItem>>& items) {
+    for (const auto& item : items)
+      FireOnDownloadCreated(item.get());
+  }
+
+  ContentSuggestion::ID GetDummySuggestionId(int id, bool is_offline_page) {
+    return ContentSuggestion::ID(
+        downloads_category(),
+        (is_offline_page ? "O" : "D") + base::IntToString(id));
+  }
+
+  std::set<std::string> ReadDismissedIDsFromPrefs(bool for_offline_pages) {
+    return provider_->ReadDismissedIDsFromPrefs(for_offline_pages);
+  }
+
+  ContentSuggestionsProvider* provider() { return provider_.get(); }
+  ObservedMockDownloadManager* downloads_manager() {
+    return &downloads_manager_;
+  }
+  FakeOfflinePageModel* offline_pages_model() { return &offline_pages_model_; }
+  MockContentSuggestionsProviderObserver* observer() { return &observer_; }
+  TestingPrefServiceSimple* pref_service() { return pref_service_.get(); }
+
+ private:
+  ObservedMockDownloadManager downloads_manager_;
+  FakeOfflinePageModel offline_pages_model_;
+  MockContentSuggestionsProviderObserver observer_;
+  CategoryFactory category_factory_;
+  std::unique_ptr<TestingPrefServiceSimple> pref_service_;
+  // Last so that the dependencies are deleted after the provider.
+  std::unique_ptr<DownloadSuggestionsProvider> provider_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadSuggestionsProviderTest);
+};
+
+TEST_F(DownloadSuggestionsProviderTest,
+       ShouldConvertOfflinePagesToSuggestions) {
+  std::vector<OfflinePageItem> offline_pages = CreateDummyOfflinePages({1, 2});
+
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, downloads_category(),
+                                            UnorderedElementsAre(
+                                                HasUrl("http://dummy.com/1"),
+                                                HasUrl("http://dummy.com/2"))));
+  FireOfflinePageModelChanged(offline_pages);
+}
+
+TEST_F(DownloadSuggestionsProviderTest,
+       ShouldConvertDownloadItemsToSuggestions) {
+  std::vector<std::unique_ptr<FakeDownloadItem>> asset_downloads =
+      CreateDummyAssetDownloads({1, 2});
+
+  FireOnDownloadCreated(asset_downloads[0].get());

[Marc Treib, 2016/10/13 12:11:26]

Shouldn't the observer already get a call from this?
Probably best to make the observer a StrictMock, so that we make sure we don't
get any extra unexpected calls. (I think right now the tests will produce a lot
of "uninteresting mock function call" warnings?!)

[vitaliii, 2016/10/15 18:36:31]

Yes, it should.
I use a StrictMock now.

+
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, downloads_category(),
+                  UnorderedElementsAre(HasUrl("file:///folder/file1.mhtml"),
+                                       HasUrl("file:///folder/file2.mhtml"))));
+
+  FireOnDownloadCreated(asset_downloads[1].get());
+}
+
+TEST_F(DownloadSuggestionsProviderTest, ShouldMixInBothSources) {
+  std::vector<std::unique_ptr<FakeDownloadItem>> asset_downloads =
+      CreateDummyAssetDownloads({1, 2});
+  std::vector<OfflinePageItem> offline_pages = CreateDummyOfflinePages({1, 2});
+
+  FireOnDownloadCreated(asset_downloads[0].get());
+  FireOnDownloadCreated(asset_downloads[1].get());
+
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, downloads_category(),
+                  UnorderedElementsAre(HasUrl("file:///folder/file1.mhtml"),
+                                       HasUrl("file:///folder/file2.mhtml"),
+                                       HasUrl("http://dummy.com/1"),
+                                       HasUrl("http://dummy.com/2"))));
+
+  FireOfflinePageModelChanged(offline_pages);
+}
+
+TEST_F(DownloadSuggestionsProviderTest, ShouldSortSuggestions) {
+  base::Time now = base::Time::Now();
+  base::Time yesterday = now - base::TimeDelta::FromDays(1);
+  base::Time tomorrow = now + base::TimeDelta::FromDays(1);
+  base::Time next_week = now + base::TimeDelta::FromDays(7);
+
+  std::vector<OfflinePageItem> offline_pages;
+  offline_pages.push_back(CreateDummyOfflinePage(1, yesterday));
+  offline_pages.push_back(CreateDummyOfflinePage(2, tomorrow));
+  std::vector<std::unique_ptr<FakeDownloadItem>> asset_downloads;
+  asset_downloads.push_back(CreateDummyAssetDownload(3, next_week));
+  asset_downloads.push_back(CreateDummyAssetDownload(4, now));
+
+  FireOnDownloadCreated(asset_downloads[0].get());
+  FireOnDownloadCreated(asset_downloads[1].get());
+
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(_, downloads_category(),
+                               ElementsAre(HasUrl("file:///folder/file3.mhtml"),
+                                           HasUrl("http://dummy.com/2"),
+                                           HasUrl("file:///folder/file4.mhtml"),
+                                           HasUrl("http://dummy.com/1"))));
+
+  FireOfflinePageModelChanged(offline_pages);
+}
+
+TEST_F(DownloadSuggestionsProviderTest, ShouldDismiss) {
+  // OfflinePageModel is initialised here because
+  // |GetDismissedSuggestionsForDebugging| may need to call |GetAllPages|
+  *(offline_pages_model()->mutable_items()) = CreateDummyOfflinePages({1, 2});
+  FireOfflinePageModelChanged(offline_pages_model()->items());
+
+  *(downloads_manager()->mutable_items()) = CreateDummyAssetDownloads({3, 4});
+  FireOnDownloadsCreated(downloads_manager()->items());
+
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(1, /*is_offline_page=*/true));
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(3, /*is_offline_page=*/false));
+  Mock::VerifyAndClearExpectations(observer());
+
+  // The dismissed suggestions should not be in the reported suggestions.
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, downloads_category(),
+                  UnorderedElementsAre(HasUrl("http://dummy.com/2"),
+                                       HasUrl("file:///folder/file4.mhtml"))));
+  FireOfflinePageModelChanged(offline_pages_model()->items());
+  Mock::VerifyAndClearExpectations(observer());
+
+  // Instead they should be reported as dismissed.
+  std::vector<ContentSuggestion> dismissed_suggestions;
+  provider()->GetDismissedSuggestionsForDebugging(
+      downloads_category(),
+      base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
+  EXPECT_THAT(dismissed_suggestions,
+              UnorderedElementsAre(HasUrl("http://dummy.com/1"),
+                                   HasUrl("file:///folder/file3.mhtml")));
+
+  provider()->ClearDismissedSuggestionsForDebugging(downloads_category());
+
+  // No more dismissed suggestions.
+  dismissed_suggestions.clear();
+  provider()->GetDismissedSuggestionsForDebugging(
+      downloads_category(),
+      base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
+  EXPECT_THAT(dismissed_suggestions, IsEmpty());
+
+  // And they should be reported now.
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(_, downloads_category(), SizeIs(4)));
+  FireOfflinePageModelChanged(offline_pages_model()->items());
+  Mock::VerifyAndClearExpectations(observer());

[Marc Treib, 2016/10/13 12:11:26]

Not needed

[vitaliii, 2016/10/15 18:36:31]

Done.

+}
+
+TEST_F(DownloadSuggestionsProviderTest,
+       ShouldNotDismissOtherTypeWithTheSameID) {
+  *(offline_pages_model()->mutable_items()) = CreateDummyOfflinePages({1, 2});
+  FireOfflinePageModelChanged(offline_pages_model()->items());
+
+  *(downloads_manager()->mutable_items()) = CreateDummyAssetDownloads({1, 2});
+  FireOnDownloadsCreated(downloads_manager()->items());
+
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(1, /*is_offline_page=*/true));
+  Mock::VerifyAndClearExpectations(observer());

[Marc Treib, 2016/10/13 12:11:26]

nit: This and the EXPECT_CALL above aren't really necessary here

[vitaliii, 2016/10/15 18:36:31]

Done.

+
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, downloads_category(),
+                  UnorderedElementsAre(HasUrl("http://dummy.com/2"),
+                                       HasUrl("file:///folder/file1.mhtml"),
+                                       HasUrl("file:///folder/file2.mhtml"))));
+  FireOfflinePageModelChanged(offline_pages_model()->items());
+}
+
+TEST_F(DownloadSuggestionsProviderTest, ShouldReplaceDismissedItemWithNewData) {
+  *(downloads_manager()->mutable_items()) =
+      CreateDummyAssetDownloads({1, 2, 3, 4, 5, 6});

[Marc Treib, 2016/10/13 12:11:26]

Took me a while to see why you need 6 :D
Maybe add a comment that this is one more than kMaxSuggestions? Or expose that
number, and programmatically add one more than that?

[vitaliii, 2016/10/15 18:36:31]

Done.

+  FireOnDownloadsCreated(downloads_manager()->items());
+
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, _, _)).Times(0);
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(1, /*is_offline_page=*/true));

[Marc Treib, 2016/10/13 12:11:26]

Wait, this dismisses an offline page - does the test pass?! Then something is
seriously wrong.

[vitaliii, 2016/10/15 18:36:30]

{2, 3, 4, 5, 6} were shown by the provider. 
I changed |CreateDummyAssetDownloads| to set up items' time, such that the first
items will be shown first.

+  Mock::VerifyAndClearExpectations(observer());

[Marc Treib, 2016/10/13 12:11:26]

Also here. Generally, it's better to avoid VerifyAndClearExpectations when
possible.

[vitaliii, 2016/10/15 18:36:30]

Done. Why?

[Marc Treib, 2016/10/17 10:18:40]

It's a hint that the test is testing more than one thing, and should probably be
split up. (Tim feels more strongly about this than I do)

[vitaliii, 2016/10/26 00:07:54]

I see, good point!

+
+  // The provider is not notified about the 6th item for the second time,
+  // however, it must report it.
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, downloads_category(),
+                  UnorderedElementsAre(HasUrl("file:///folder/file2.mhtml"),
+                                       HasUrl("file:///folder/file3.mhtml"),
+                                       HasUrl("file:///folder/file4.mhtml"),
+                                       HasUrl("file:///folder/file5.mhtml"),
+                                       HasUrl("file:///folder/file6.mhtml"))));

[Marc Treib, 2016/10/13 12:11:26]

Maybe also verify before dismissing that items 1-5 get reported? It might
randomly happen that you'd get 2-6 even before dismissing, since none of them
have a date.

[vitaliii, 2016/10/15 18:36:31]

Done.

+  FireOfflinePageModelChanged(offline_pages_model()->items());
+}
+
+TEST_F(DownloadSuggestionsProviderTest,
+       ShouldInvalidateWhenUnderlyingItemDeleted) {
+  *(offline_pages_model()->mutable_items()) = CreateDummyOfflinePages({1, 2});
+  FireOfflinePageModelChanged(offline_pages_model()->items());
+
+  *(downloads_manager()->mutable_items()) = CreateDummyAssetDownloads({3, 4});
+  FireOnDownloadsCreated(downloads_manager()->items());
+
+  EXPECT_CALL(*observer(),
+              OnSuggestionInvalidated(
+                  _, GetDummySuggestionId(1, /*is_offline_page=*/true)));
+  FireOfflinePageDeleted(offline_pages_model()->items()[0]);
+  EXPECT_CALL(*observer(),
+              OnSuggestionInvalidated(
+                  _, GetDummySuggestionId(3, /*is_offline_page=*/false)));
+  FireDownloadRemoved(downloads_manager()->items()[0].get());
+}
+
+TEST_F(DownloadSuggestionsProviderTest, ShouldReplaceRemovedItemWithNewData) {
+  *(downloads_manager()->mutable_items()) =
+      CreateDummyAssetDownloads({1, 2, 3, 4, 5, 6});
+  FireOnDownloadsCreated(downloads_manager()->items());

[Marc Treib, 2016/10/13 12:11:26]

Also here: Verify that actually items 1-5 get reported?

[vitaliii, 2016/10/15 18:36:31]

Done.

+
+  EXPECT_CALL(*observer(),
+              OnSuggestionInvalidated(
+                  _, GetDummySuggestionId(1, /*is_offline_page=*/false)));
+  // |OnDownloadRemoved| notification is done in |DownloadItem|'s destructor.
+  downloads_manager()->mutable_items()->erase(
+      downloads_manager()->mutable_items()->begin());
+
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, downloads_category(),
+                  UnorderedElementsAre(HasUrl("file:///folder/file2.mhtml"),
+                                       HasUrl("file:///folder/file3.mhtml"),
+                                       HasUrl("file:///folder/file4.mhtml"),
+                                       HasUrl("file:///folder/file5.mhtml"),
+                                       HasUrl("file:///folder/file6.mhtml"))));
+  FireOfflinePageModelChanged(offline_pages_model()->items());
+}
+
+TEST_F(DownloadSuggestionsProviderTest, ShouldPruneOfflinePagesDismissedIDs) {
+  *(offline_pages_model()->mutable_items()) =
+      CreateDummyOfflinePages({1, 2, 3});
+  FireOfflinePageModelChanged(offline_pages_model()->items());
+
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(1, /*is_offline_page=*/true));
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(2, /*is_offline_page=*/true));
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(3, /*is_offline_page=*/true));
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(/*for_offline_pages=*/true), SizeIs(3));

[Marc Treib, 2016/10/13 12:11:26]

IMO the prefs are an implementation detail that doesn't really need to get
tested. You could test the behavior instead, using GetDismissedSuggestions?

[vitaliii, 2016/10/15 18:36:30]

I use GetDismissedSuggestions now.
However, this makes it a bit trickier, because GetDismissedSuggestions fetches
all data.

+
+  // Prune on getting all offline pages.
+  *(offline_pages_model()->mutable_items()) = CreateDummyOfflinePages({2, 3});
+  FireOfflinePageModelChanged(offline_pages_model()->items());
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(/*for_offline_pages=*/true), SizeIs(2));
+
+  // Prune when offline page is deleted.
+  FireOfflinePageDeleted(offline_pages_model()->items()[0]);
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(/*for_offline_pages=*/true), SizeIs(1));
+
+  // Clear when explicitly requested.
+  provider()->ClearDismissedSuggestionsForDebugging(downloads_category());
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(/*for_offline_pages=*/true), SizeIs(0));
+}
+
+TEST_F(DownloadSuggestionsProviderTest, ShouldPruneAssetDownloadsDismissedIDs) {
+  *(downloads_manager()->mutable_items()) =
+      CreateDummyAssetDownloads({4, 5, 6, 7, 8, 9});
+  FireOnDownloadsCreated(downloads_manager()->items());
+
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(4, /*is_offline_page=*/false));
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(5, /*is_offline_page=*/false));
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(/*for_offline_pages=*/false),
+              SizeIs(2));
+
+  // Prune when item is deleted.
+  FireDownloadRemoved(downloads_manager()->items()[0].get());
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(/*for_offline_pages=*/false),
+              SizeIs(1));
+
+  // Clear when explicitly requested.
+  provider()->ClearDismissedSuggestionsForDebugging(downloads_category());
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(/*for_offline_pages=*/false),
+              SizeIs(0));
+
+  // Simulate that we miss |OnDownloadRemoved|. The pruning should occur when
+  // all pages are fetched.
+  std::unique_ptr<FakeDownloadItem> missed_item = CreateDummyAssetDownload(100);
+  FireOnDownloadCreated(missed_item.get());
+
+  provider()->DismissSuggestion(
+      GetDummySuggestionId(100, /*is_offline_page=*/false));
+  FireOfflinePageModelChanged(offline_pages_model()->items());
+  // After OfflinePage model change all download items request must have
+  // occurred. Note that |missed_item| is not in |downloads_manager| and so the
+  // provider does not receive it.
+  EXPECT_THAT(ReadDismissedIDsFromPrefs(/*for_offline_pages=*/false),
+              SizeIs(0));
+}