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

chrome/browser/browsing_data/browsing_data_remover_unittest.cc

 // Copyright (c) 2012 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/browsing_data/browsing_data_remover.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <list>
@@ skipping 46 matching lines @@
 #include "components/bookmarks/test/bookmark_test_helpers.h"
 #include "components/browsing_data/core/browsing_data_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/domain_reliability/clear_mode.h"
 #include "components/domain_reliability/monitor.h"
 #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"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
 #include "components/password_manager/core/browser/password_store_consumer.h"
 #include "components/prefs/testing_pref_service.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/cookie_store_factory.h"
 #include "content/public/browser/dom_storage_context.h"
 #include "content/public/browser/local_storage_usage_info.h"
@@ skipping 2960 matching lines @@
   EXPECT_TRUE(remover->is_removing());
 
   // Add one more deletion and wait for it.
   BlockUntilBrowsingDataRemoved(
       browsing_data::ALL_TIME,
       BrowsingDataRemover::REMOVE_COOKIES,
       BrowsingDataHelper::UNPROTECTED_WEB);
 
   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());
+}