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

Previously, last_visit_date metadata has was removed by
BookmarkSuggestionProvider via ContentSuggestionService::ClearHistory()
which only works on Android. This CL makes the removal platform-
independent. It aims at simplicity to make is easier to merge to M56
(and does not fix the pre-existing TODO).

The data is collected outside of BookmarkSuggestionProvider (by a tab
helper). Previously, the tab helper was only instantiated on Android.
Since M56 the tab helper is created and we thus collect the data on all
desktop platforms as well. For this reason, we also need to fix the
browsing data removal.

BUG=673268

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

chrome/browser/browsing_data/browsing_data_remover.cc

@@ -16,6 +16,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
 #include "chrome/browser/autofill/personal_data_manager_factory.h"
+#include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browsing_data/browsing_data_filter_builder.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
@@ -51,6 +52,7 @@
 #include "chrome/common/url_constants.h"
 #include "components/autofill/core/browser/personal_data_manager.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
+#include "components/bookmarks/browser/bookmark_model.h"
 #include "components/browsing_data/content/storage_partition_http_cache_data_remover.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
@@ -62,6 +64,7 @@
 #include "components/history/core/browser/history_service.h"
 #include "components/nacl/browser/nacl_browser.h"
 #include "components/nacl/browser/pnacl_host.h"
+#include "components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h"
 #include "components/ntp_snippets/content_suggestions_service.h"
 #include "components/omnibox/browser/omnibox_pref_names.h"
 #include "components/password_manager/core/browser/password_store.h"
@@ -528,6 +531,7 @@ void BrowsingDataRemover::RemoveImpl(
           &history_task_tracker_);
     }
 
+    // Currently, ContentSuggestionService instance exists only on Android.
     ntp_snippets::ContentSuggestionsService* content_suggestions_service =
         ContentSuggestionsServiceFactory::GetForProfileIfExists(profile_);
     if (content_suggestions_service) {
@@ -535,6 +539,14 @@ void BrowsingDataRemover::RemoveImpl(
                                                 filter);
     }
 
+    // Remove the last visit dates meta-data from the bookmark model.
+    // TODO(vitaliii): Do not remove all dates, but only the ones matched by the
+    // time range and the filter.
+    bookmarks::BookmarkModel* bookmark_model =
+        BookmarkModelFactory::GetForBrowserContext(profile_);
+    if (bookmark_model)
+      ntp_snippets::RemoveAllLastVisitDates(bookmark_model);
+
 #if BUILDFLAG(ENABLE_EXTENSIONS)
     // The extension activity log contains details of which websites extensions
     // were active on. It therefore indirectly stores details of websites a

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"
@@ -131,10 +132,12 @@ using domain_reliability::DomainReliabilityServiceFactory;
 using testing::_;
 using testing::ByRef;
 using testing::Invoke;
+using testing::IsEmpty;
 using testing::Matcher;
 using testing::MakeMatcher;
 using testing::MatcherInterface;
 using testing::MatchResultListener;
+using testing::Not;
 using testing::Return;
 using testing::WithArgs;
 
@@ -3044,3 +3047,56 @@ 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_THAT(ntp_snippets::GetRecentlyVisitedBookmarks(
+                  bookmark_model, 2, base::Time::UnixEpoch(),
+                  /*consider_visits_from_desktop=*/false),
+              Not(IsEmpty()));
+
+  // Inject the bookmark model into the remover.
+  BrowsingDataRemover* remover =
+      BrowsingDataRemoverFactory::GetForBrowserContext(&profile);
+
+  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_THAT(ntp_snippets::GetRecentlyVisitedBookmarks(
+                  bookmark_model, 2, base::Time::UnixEpoch(),
+                  /*consider_visits_from_desktop=*/false),
+              IsEmpty());
+  EXPECT_THAT(ntp_snippets::GetRecentlyVisitedBookmarks(
+                  bookmark_model, 2, base::Time::UnixEpoch(),
+                  /*consider_visits_from_desktop=*/true),
+              IsEmpty());
+}

components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc

@@ -150,9 +150,8 @@ void BookmarkSuggestionsProvider::ClearHistory(
     base::Time begin,
     base::Time end,
     const base::Callback<bool(const GURL& url)>& filter) {
-  // TODO(vitaliii): Do not remove all dates, but only the ones matched by the
-  // time range and the filter.
-  RemoveAllLastVisitDates(bookmark_model_);
+  // The last visit dates are not "owned" by the bookmark suggestion provider so
+  // it is cleared directly from browsing_data_remover.cc.
   ClearDismissedSuggestionsForDebugging(provided_category_);
   FetchBookmarks();
   // Temporarily enter an "explicitly disabled" state, so that any open UIs