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

components/ntp_snippets/bookmarks/bookmark_last_visit_utils_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 "components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h"
 
 #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"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "components/bookmarks/test/test_bookmark_client.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ skipping 22 matching lines @@
         model->AddURL(model->bookmark_bar_node(), 0, title, url);
 
     if (!meta_key.empty()) {
       model->SetNodeMetaInfo(node, meta_key, meta_value);
     }
   }
 }
 
 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 {
  public:
   GetRecentlyVisitedBookmarksTest() {
     base::TimeDelta week = base::TimeDelta::FromDays(7);
     threshold_time_ = base::Time::UnixEpoch() + 52 * week;
   }
 
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(GetRecentlyVisitedBookmarksTest);
 };
 
 TEST_F(GetRecentlyVisitedBookmarksTest, ShouldNotReturnMissing) {
   const int number_of_bookmarks = 3;
   std::unique_ptr<BookmarkModel> model =
       bookmarks::TestBookmarkClient::CreateModel();
   AddBookmarksNonVisited(model.get(), number_of_bookmarks);
 
   std::vector<const bookmarks::BookmarkNode*> result =
       GetRecentlyVisitedBookmarks(model.get(), number_of_bookmarks,
                                   threshold_time(),
                                   /*consider_visits_from_desktop=*/false);
   EXPECT_THAT(result, IsEmpty());
 }
 
 TEST_F(GetRecentlyVisitedBookmarksTest, ShouldNotConsiderDesktopVisits) {
   const int number_of_bookmarks = 3;
   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,
                                   threshold_time(),
                                   /*consider_visits_from_desktop=*/false);
   EXPECT_THAT(result, IsEmpty());
 }
 
 TEST_F(GetRecentlyVisitedBookmarksTest, ShouldConsiderDesktopVisits) {
   const int number_of_bookmarks = 3;
   const int number_of_recent_desktop = 2;
   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);
 
   std::vector<const bookmarks::BookmarkNode*> result =
       GetRecentlyVisitedBookmarks(model.get(), number_of_bookmarks,
                                   threshold_time(),
                                   /*consider_visits_from_desktop=*/true);
   EXPECT_THAT(result, SizeIs(number_of_recent_desktop));
 }
 
 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 =
       GetRecentlyVisitedBookmarks(model.get(), max_count, threshold_time(),
                                   /*consider_visits_from_desktop=*/false);
   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