Last visit dates of bookmarks - fix browsing data removal on desktop

chrome/browser/browsing_data/browsing_data_remover_unittest.cc

Index: chrome/browser/browsing_data/browsing_data_remover_unittest.cc
diff --git a/chrome/browser/browsing_data/browsing_data_remover_unittest.cc b/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
index 3adb0b589b0ba2222a2f09102bd02de8d1ceee20..e9b1505a7806820b85a796ea20ec5aa76a4368be 100644
--- a/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
@@ -64,6 +64,7 @@
 #include "components/domain_reliability/service.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h"
 #include "components/omnibox/browser/omnibox_pref_names.h"
 #include "components/os_crypt/os_crypt_mocker.h"
 #include "components/password_manager/core/browser/mock_password_store.h"
@@ -3044,3 +3045,57 @@ TEST_F(BrowsingDataRemoverTest, MultipleTasksInQuickSuccession) {
 
   EXPECT_FALSE(remover->is_removing());
 }
+
+// Test that the remover clears bookmark meta data (normally added in a tab
+// helper).
+TEST_F(BrowsingDataRemoverTest, BookmarkLastVisitDatesGetCleared) {
+  TestingProfile profile;
+  profile.CreateBookmarkModel(true);
+
+  bookmarks::BookmarkModel* bookmark_model =
+      BookmarkModelFactory::GetForBrowserContext(&profile);
+  bookmarks::test::WaitForBookmarkModelToLoad(bookmark_model);
+
+  // Create a couple of bookmarks.
+  bookmark_model->AddURL(bookmark_model->bookmark_bar_node(), 0,
+                         base::string16(),
+                         GURL("http://foo.org/desktop"));
+  bookmark_model->AddURL(bookmark_model->mobile_node(), 0,
+                         base::string16(),
+                         GURL("http://foo.org/mobile"));
+
+  // Simulate their visits.
+  ntp_snippets::UpdateBookmarkOnURLVisitedInMainFrame(
+      bookmark_model, GURL("http://foo.org/desktop"),
+      /*is_mobile_platform=*/false);
+  ntp_snippets::UpdateBookmarkOnURLVisitedInMainFrame(
+      bookmark_model, GURL("http://foo.org/mobile"),
+      /*is_mobile_platform=*/true);
+
+  // There should be some recently visited bookmarks.
+  EXPECT_FALSE(ntp_snippets::GetRecentlyVisitedBookmarks(

[tschumann, 2016/12/13 09:03:31]

nit: using EXPECT_THAT() with gmock matchers, you can make this easier to read
(and get nicer debug output):
using ::testing::IsEmpty;
using ::testing::Not;

EXPECT_THAT(ntp_snippets::GetRecentlyVisitedBookmarks(bookmark_model, 2,
base::Time::UnixEpoch(), /*consider_visits_from_desktop=*/false),
Not(IsEmpty());

(and using just IsEmpty() in the expectations at the end).

[jkrcal, 2016/12/13 10:21:15]

Done.

Well, this 3K+ lines unittest file did not have any instance of EXPECT_THAT.
Martin, are you fine with your tests going in this direction?

[msramek, 2016/12/13 10:34:39]

It's fine. I personally do find simple statements such as EXPECT_FALSE more
readable, but it's still the same testing framework, so I consider this style
deviation to be within the scope of a personal taste :)

+                   bookmark_model, 2, base::Time::UnixEpoch(),
+                   /*consider_visits_from_desktop=*/false)
+                   .empty());
+
+  // Inject the bookmark model into the remover.
+  BrowsingDataRemover* remover =
+      BrowsingDataRemoverFactory::GetForBrowserContext(&profile);
+  remover->OverrideBookmarkModelForTesting(bookmark_model);

[msramek, 2016/12/13 09:27:36]

You forgot this line, it's what makes the tests red.

[jkrcal, 2016/12/13 10:21:15]

Huh :) Sometimes CL-upload-without-local-compile hits on me ;)

+
+  BrowsingDataRemoverCompletionObserver completion_observer(remover);
+  remover->RemoveAndReply(BrowsingDataRemover::Unbounded(),
+                          BrowsingDataRemover::REMOVE_HISTORY,
+                          BrowsingDataHelper::ALL, &completion_observer);
+  completion_observer.BlockUntilCompletion();
+
+  // There should be no recently visited bookmarks.
+  EXPECT_TRUE(ntp_snippets::GetRecentlyVisitedBookmarks(
+                  bookmark_model, 2, base::Time::UnixEpoch(),
+                  /*consider_visits_from_desktop=*/false)
+                  .empty());
+  EXPECT_TRUE(ntp_snippets::GetRecentlyVisitedBookmarks(
+                  bookmark_model, 2, base::Time::UnixEpoch(),
+                  /*consider_visits_from_desktop=*/true)
+                  .empty());
+}