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

components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc

Index: components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc
diff --git a/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc b/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc
index 63be8ad9b54fc9af227915dc6566f586d30ddb79..9b5919056416e40388dfe883f16aedc57108e7d7 100644
--- a/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc
+++ b/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc
@@ -42,8 +42,8 @@ std::string FormatLastVisitDate(const base::Time& date) {
 }
 
 bool ExtractLastVisitDate(const BookmarkNode& node,
-                 const std::string& meta_info_key,
-                 base::Time* out) {
+                          const std::string& meta_info_key,
+                          base::Time* out) {
   std::string last_visit_date_string;
   if (!node.GetMetaInfo(meta_info_key, &last_visit_date_string)) {
     return false;
@@ -269,7 +269,21 @@ std::vector<const BookmarkNode*> GetDismissedBookmarksForDebugging(
   return result;
 }
 
-void RemoveAllLastVisitDates(bookmarks::BookmarkModel* bookmark_model) {
+namespace {
+
+void ClearNTPMetadata(bookmarks::BookmarkModel* model,
+                      const BookmarkNode* node) {
+  model->DeleteNodeMetaInfo(node, kBookmarkLastVisitDateOnMobileKey);
+  model->DeleteNodeMetaInfo(node, kBookmarkLastVisitDateOnDesktopKey);
+  model->DeleteNodeMetaInfo(node, kBookmarkDismissedFromNTP);
+}
+
+}  // namespace
+
+void RemoveLastVisitedDatesBetween(const base::Time& begin,
+                                   const base::Time& end,
+                                   base::Callback<bool(const GURL& url)> filter,

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

Why do you accept |filter| when you not use it? Why not call the filter with
|url_and_title.url|?

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

Good point! Will add this functionality. (Sending my responses now to get
Martin's input on the other comment).

[tschumann, 2017/01/04 15:08:44]

Done.
+Martin: Can you double-check I got the filter semantics right? (couldn't find a
clear documentation if filtering out means out of the history or out of the
removal process).
Somebody remind me that I shouldn't go grocery shopping today [milk / bread
rolls...] xD

[msramek, 2017/01/04 15:16:26]

Yeah, sorry for that. I guess we really should have used the word 'predicate'
primarily... url should be deleted iff filter(url), so you got the semantics
right.

+                                   bookmarks::BookmarkModel* bookmark_model) {
   // Get all the bookmark URLs.
   std::vector<BookmarkModel::URLAndTitle> bookmark_urls;
   bookmark_model->GetBookmarks(&bookmark_urls);
@@ -280,10 +294,17 @@ void RemoveAllLastVisitDates(bookmarks::BookmarkModel* bookmark_model) {
     bookmark_model->GetNodesByURL(url_and_title.url, &bookmarks_for_url);
 
     for (const BookmarkNode* bookmark : bookmarks_for_url) {
-      bookmark_model->DeleteNodeMetaInfo(bookmark,
-                                         kBookmarkLastVisitDateOnMobileKey);
-      bookmark_model->DeleteNodeMetaInfo(bookmark,
-                                         kBookmarkLastVisitDateOnDesktopKey);
+      base::Time last_visit_time;
+      if (ExtractLastVisitDate(*bookmark, kBookmarkLastVisitDateOnMobileKey,
+                               &last_visit_time) &&
+          begin <= last_visit_time && last_visit_time <= end) {
+        ClearNTPMetadata(bookmark_model, bookmark);
+      }
+      if (ExtractLastVisitDate(*bookmark, kBookmarkLastVisitDateOnDesktopKey,
+                               &last_visit_time) &&
+          begin <= last_visit_time && last_visit_time <= end) {
+        ClearNTPMetadata(bookmark_model, bookmark);
+      }
     }
   }
 }