Respect time range in browsing data removal for last-visited data.

components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc

Index: components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc
diff --git a/components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc b/components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc
index c99c8695a5f72b072f04afff2fa9f51bf1afc629..814b10a504acaf4fba3bc7a8c8ad512e760a7f48 100644
--- a/components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc
+++ b/components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "base/callback.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -48,32 +49,35 @@ void AddBookmarks(BookmarkModel* model,
 
 void AddBookmarksRecentOnMobile(BookmarkModel* model,
                                 int num,
-                                const base::Time& threshold_time) {
-  base::TimeDelta week = base::TimeDelta::FromDays(7);
-  base::Time recent_time = threshold_time + week;
-  std::string recent_time_string =
-      base::Int64ToString(recent_time.ToInternalValue());
-
+                                const base::Time& visit_time) {
   AddBookmarks(model, num, kBookmarkLastVisitDateOnMobileKey,
-               recent_time_string);
+               base::Int64ToString(visit_time.ToInternalValue()));
 }
 
 void AddBookmarksRecentOnDesktop(BookmarkModel* model,
                                  int num,
-                                 const base::Time& threshold_time) {
-  base::TimeDelta week = base::TimeDelta::FromDays(7);
-  base::Time recent_time = threshold_time + week;
-  std::string recent_time_string =
-      base::Int64ToString(recent_time.ToInternalValue());
-
+                                 const base::Time& visit_time) {
   AddBookmarks(model, num, kBookmarkLastVisitDateOnDesktopKey,
-               recent_time_string);
+               base::Int64ToString(visit_time.ToInternalValue()));
 }
 
 void AddBookmarksNonVisited(BookmarkModel* model, int num) {
   AddBookmarks(model, num, std::string(), std::string());
 }
 
+const BookmarkNode* AddSingleBookmark(BookmarkModel* model,
+                                      const std::string& url,
+                                      const std::string& last_visit_key,
+                                      const base::Time& visit_time) {
+  base::string16 title =
+      base::ASCIIToUTF16(base::StringPrintf("title-%s", url.c_str()));
+  const BookmarkNode* node =
+      model->AddURL(model->bookmark_bar_node(), 0, title, GURL(url));
+  model->SetNodeMetaInfo(node, last_visit_key,
+                         base::Int64ToString(visit_time.ToInternalValue()));
+  return node;
+}
+
 }  // namespace
 
 class GetRecentlyVisitedBookmarksTest : public testing::Test {
@@ -85,6 +89,10 @@ class GetRecentlyVisitedBookmarksTest : public testing::Test {
 
   const base::Time& threshold_time() const { return threshold_time_; }
 
+  base::Time GetRecentTime() const {
+    return threshold_time_ + base::TimeDelta::FromDays(7);
+  }
+
  private:
   base::Time threshold_time_;
 
@@ -109,7 +117,7 @@ TEST_F(GetRecentlyVisitedBookmarksTest, ShouldNotConsiderDesktopVisits) {
   std::unique_ptr<BookmarkModel> model =
       bookmarks::TestBookmarkClient::CreateModel();
   AddBookmarksRecentOnDesktop(model.get(), number_of_bookmarks,
-                              threshold_time());
+                              GetRecentTime());
 
   std::vector<const bookmarks::BookmarkNode*> result =
       GetRecentlyVisitedBookmarks(model.get(), number_of_bookmarks,
@@ -124,7 +132,7 @@ TEST_F(GetRecentlyVisitedBookmarksTest, ShouldConsiderDesktopVisits) {
   std::unique_ptr<BookmarkModel> model =
       bookmarks::TestBookmarkClient::CreateModel();
   AddBookmarksRecentOnDesktop(model.get(), number_of_recent_desktop,
-                              threshold_time());
+                              GetRecentTime());
   AddBookmarksNonVisited(model.get(),
                          number_of_bookmarks - number_of_recent_desktop);
 
@@ -139,8 +147,7 @@ TEST_F(GetRecentlyVisitedBookmarksTest, ShouldReturnNotMoreThanMaxCount) {
   const int number_of_bookmarks = 3;
   std::unique_ptr<BookmarkModel> model =
       bookmarks::TestBookmarkClient::CreateModel();
-  AddBookmarksRecentOnMobile(model.get(), number_of_bookmarks,
-                             threshold_time());
+  AddBookmarksRecentOnMobile(model.get(), number_of_bookmarks, GetRecentTime());
 
   const int max_count = number_of_bookmarks - 1;
   std::vector<const bookmarks::BookmarkNode*> result =
@@ -149,4 +156,129 @@ TEST_F(GetRecentlyVisitedBookmarksTest, ShouldReturnNotMoreThanMaxCount) {
   EXPECT_THAT(result, SizeIs(max_count));
 }
 
+TEST(RemoveLastVisitedDatesBetween, ShouldRemoveTimestampsWithinTimeRange) {
+  const base::Time delete_begin =
+      base::Time::Now() - base::TimeDelta::FromDays(2);
+  const base::Time delete_end = base::Time::Max();
+
+  std::unique_ptr<BookmarkModel> model =
+      bookmarks::TestBookmarkClient::CreateModel();
+  AddSingleBookmark(model.get(), "http://url-1.com",
+                    kBookmarkLastVisitDateOnMobileKey,
+                    delete_begin + base::TimeDelta::FromSeconds(1));
+  AddSingleBookmark(model.get(), "http://url-1.com",
+                    kBookmarkLastVisitDateOnDesktopKey,
+                    delete_begin + base::TimeDelta::FromSeconds(1));
+  ASSERT_THAT(
+      GetRecentlyVisitedBookmarks(model.get(), 20, base::Time(),
+                                  /*consider_visits_from_desktop=*/true),
+      SizeIs(1));
+
+  base::Callback<bool(const GURL& url)> filter;
+  RemoveLastVisitedDatesBetween(delete_begin, delete_end, filter, model.get());
+
+  EXPECT_THAT(
+      GetRecentlyVisitedBookmarks(model.get(), 20, base::Time(),
+                                  /*consider_visits_from_desktop=*/true),
+      IsEmpty());
+  // Verify that the bookmark model nodes themselve still exist.
+  std::vector<const BookmarkNode*> remaining_nodes;
+  model->GetNodesByURL(GURL("http://url-1.com"), &remaining_nodes);
+  EXPECT_THAT(remaining_nodes, SizeIs(2));
+}
+
+TEST(RemoveLastVisitedDatesBetween,
+     ShouldAlsoRemoveAnyTimestampFromOtherDevices) {
+  const base::Time delete_begin =
+      base::Time::Now() - base::TimeDelta::FromDays(2);
+  const base::Time delete_end = base::Time::Max();
+
+  std::unique_ptr<BookmarkModel> model =
+      bookmarks::TestBookmarkClient::CreateModel();
+  // Create a bookmark with last visited times from both, mobile and desktop.
+  // The mobile one is within the deletion interval, the desktop one outside.
+  // Both should get removed.

[jkrcal, 2017/01/04 12:14:22]

This is more a question to the implementation: are you sure both should get
removed? This way, you remove more than desired, in my opinion.

[tschumann, 2017/01/04 13:32:32]

This was my understanding of Dominic's comment:
https://bugs.chromium.org/p/chromium/issues/detail?id=674178#c3

+msramek What is your take on this?

[jkrcal, 2017/01/04 14:09:25]

Interesting :) 

My interpretation of Dominic's comment is "also delete visits from the other
platform (if they satisfy the time range)" and not "also delete visits from the
other platform irrespective of the time".

[msramek, 2017/01/04 14:19:07]

I agree with Jan. Definitely take timestamps into account where applicable.

(...this reminds me of the joke about the programmer buying milk and bread rolls
:) )

+  const BookmarkNode* node = AddSingleBookmark(
+      model.get(), "http://url-1.com", kBookmarkLastVisitDateOnMobileKey,
+      delete_begin + base::TimeDelta::FromSeconds(1));
+  model->SetNodeMetaInfo(
+      node, kBookmarkLastVisitDateOnDesktopKey,
+      base::Int64ToString(
+          (delete_begin - base::TimeDelta::FromSeconds(1)).ToInternalValue()));
+  ASSERT_THAT(
+      GetRecentlyVisitedBookmarks(model.get(), 20, base::Time(),
+                                  /*consider_visits_from_desktop=*/true),
+      SizeIs(1));
+
+  base::Callback<bool(const GURL& url)> filter;
+  RemoveLastVisitedDatesBetween(delete_begin, delete_end, filter, model.get());
+
+  EXPECT_THAT(
+      GetRecentlyVisitedBookmarks(model.get(), 20, base::Time(),
+                                  /*consider_visits_from_desktop=*/true),
+      IsEmpty());
+}
+
+TEST(RemoveLastVisitedDatesBetween,
+     ShouldKeepSeparateBookmarksForDifferentDeviceTypes) {
+  // This test verifies that if we see two bookmark nodes for the same URL,
+  // where one is only visited from mobile devices and the other one only from
+  // desktop devices, clearing one should not affect the other one.
+  const base::Time delete_begin =
+      base::Time::Now() - base::TimeDelta::FromDays(2);
+  const base::Time delete_end = base::Time::Max();
+
+  std::unique_ptr<BookmarkModel> model =
+      bookmarks::TestBookmarkClient::CreateModel();
+  AddSingleBookmark(
+      model.get(), "http://url-1.com", kBookmarkLastVisitDateOnMobileKey,
+      delete_begin + base::TimeDelta::FromSeconds(1));
+  AddSingleBookmark(model.get(), "http://url-1.com",
+                    kBookmarkLastVisitDateOnDesktopKey,
+                    delete_begin - base::TimeDelta::FromSeconds(1));
+  ASSERT_THAT(
+      GetRecentlyVisitedBookmarks(model.get(), 20, base::Time(),
+                                  /*consider_visits_from_desktop=*/true),
+      SizeIs(1));
+
+  base::Callback<bool(const GURL& url)> filter;
+  RemoveLastVisitedDatesBetween(delete_begin, delete_end, filter, model.get());
+
+  EXPECT_THAT(
+      GetRecentlyVisitedBookmarks(model.get(), 20, base::Time(),
+                                  /*consider_visits_from_desktop=*/false),
+      IsEmpty());
+  EXPECT_THAT(
+      GetRecentlyVisitedBookmarks(model.get(), 20, base::Time(),
+                                  /*consider_visits_from_desktop=*/true),
+      SizeIs(1));
+}
+
+TEST(RemoveLastVisitedDatesBetween, ShouldNotRemoveTimestampsOutsideTimeRange) {
+  const base::Time delete_begin =
+      base::Time::Now() - base::TimeDelta::FromDays(2);
+  const base::Time delete_end = delete_begin + base::TimeDelta::FromDays(5);
+
+  std::unique_ptr<BookmarkModel> model =
+      bookmarks::TestBookmarkClient::CreateModel();
+  AddSingleBookmark(model.get(), "http://url-1.com",
+                    kBookmarkLastVisitDateOnMobileKey,
+                    delete_begin - base::TimeDelta::FromSeconds(1));
+  AddSingleBookmark(model.get(), "http://url-2.com",
+                    kBookmarkLastVisitDateOnDesktopKey,
+                    delete_end + base::TimeDelta::FromSeconds(1));
+  ASSERT_THAT(
+      GetRecentlyVisitedBookmarks(model.get(), 20, base::Time(),
+                                  /*consider_visits_from_desktop=*/true),
+      SizeIs(2));
+
+  base::Callback<bool(const GURL& url)> filter;
+  RemoveLastVisitedDatesBetween(delete_begin, delete_end, filter, model.get());
+
+  EXPECT_THAT(
+      GetRecentlyVisitedBookmarks(model.get(), 20, base::Time(),
+                                  /*consider_visits_from_desktop=*/true),
+      SizeIs(2));
+}
+
 }  // namespace ntp_snippets