Make bookmark suggestions dismissable

components/ntp_snippets/bookmarks/bookmark_last_visit_utils.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 <algorithm>
 #include <string>
 #include <utility>
 
 #include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "url/gurl.h"
 
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
 
 namespace ntp_snippets {
 
 namespace {
 
 const char kBookmarkLastVisitDateKey[] = "last_visited";
+const char kBookmarkDismissedFromNTP[] = "dismissed_from_ntp";
 
 base::Time ParseLastVisitDate(const std::string& date_string) {
   int64_t date = 0;
   if (!base::StringToInt64(date_string, &date))
     return base::Time::UnixEpoch();
   return base::Time::FromInternalValue(date);
 }
 
 std::string FormatLastVisitDate(const base::Time& date) {
   return base::Int64ToString(date.ToInternalValue());
@@ skipping 28 matching lines @@
   node->GetMetaInfo(kBookmarkLastVisitDateKey, &last_visit_date_string);
 
   if (last_visit_date_string.empty()) {
     // Use creation date if no last visit info present.
     return node->date_added();
   }
 
   return ParseLastVisitDate(last_visit_date_string);
 }
 
+void MarkBookmarksDismissed(BookmarkModel* bookmark_model, const GURL& url) {
+  std::vector<const BookmarkNode*> nodes;
+  bookmark_model->GetNodesByURL(url, &nodes);
+  for (const BookmarkNode* node : nodes) {

[Marc Treib, 2016/08/08 09:10:46]

nit: braces around single-line bodies or not?
Either way is fine with me, but try to stay consistent within each file.

[Philipp Keck, 2016/08/08 09:42:25]

Done.

+    bookmark_model->SetNodeMetaInfo(node, kBookmarkDismissedFromNTP, "1");
+  }
+}
+
+bool GetDismissedFromNTPForBookmark(const BookmarkNode* node) {
+  if (!node)
+    return false;
+
+  std::string dismissed_from_ntp;
+  return node->GetMetaInfo(kBookmarkDismissedFromNTP, &dismissed_from_ntp) &&
+         dismissed_from_ntp == "1";

[Marc Treib, 2016/08/08 09:10:46]

If the meta info exists at all, it should never have any other value than "1",
right? Maybe DCHECK that?

[Philipp Keck, 2016/08/08 09:42:25]

Done.

+}
+
+void MarkAllBookmarksUndismissed(BookmarkModel* bookmark_model) {
+  // Get all the bookmark URLs.
+  std::vector<BookmarkModel::URLAndTitle> bookmarks;
+  bookmark_model->GetBookmarks(&bookmarks);
+
+  // Remove dismissed flag from all bookmarks
+  for (const BookmarkModel::URLAndTitle& bookmark : bookmarks) {
+    std::vector<const BookmarkNode*> nodes;
+    bookmark_model->GetNodesByURL(bookmark.url, &nodes);
+    for (const BookmarkNode* node : nodes) {
+      bookmark_model->DeleteNodeMetaInfo(node, kBookmarkDismissedFromNTP);
+    }
+  }
+}
+
 std::vector<const BookmarkNode*> GetRecentlyVisitedBookmarks(
     BookmarkModel* bookmark_model,
     int max_count,
     const base::Time& min_visit_time) {
-  // Get all the bookmarks.
+  // Get all the bookmark URLs.
   std::vector<BookmarkModel::URLAndTitle> bookmarks;
   bookmark_model->GetBookmarks(&bookmarks);
 
-  // Remove bookmarks that have not been visited after |min_visit_time|.
+  // Remove the bookmark URLs whose bookmarks are all dismissed or whose most
+  // recent visit is older than |min_visit_time|.
   bookmarks.erase(
       std::remove_if(bookmarks.begin(), bookmarks.end(),
                      [&bookmark_model, &min_visit_time](
                          const BookmarkModel::URLAndTitle& bookmark) {
                        // Get all bookmarks for the given URL.
                        std::vector<const BookmarkNode*> bookmarks_for_url;
                        bookmark_model->GetNodesByURL(bookmark.url,
                                                      &bookmarks_for_url);
 
-                       // Return false if there is a more recent visit time.
+                       // Check if there is any recent and/or non-dismissed one.
+                       bool has_recent = false;
+                       bool has_non_dismissed = false;
                        for (const auto* node : bookmarks_for_url) {
                          if (GetLastVisitDateForBookmark(node) > min_visit_time)
-                           return false;
+                           has_recent = true;
+                         if (!GetDismissedFromNTPForBookmark(node))
+                           has_non_dismissed = true;
                        }
-                       return true;
+                       return has_recent && has_non_dismissed;

[Marc Treib, 2016/08/08 09:10:46]

This is the wrong way around - you want to remove the ones that *don't* have a
recent visit or that *are* dismissed.
Also I'm not quite sure about the "has_non_dismissed" - what should happen if
some, but not all of the nodes are dismissed?

[Philipp Keck, 2016/08/08 09:42:25]

Right. I changed it around, trying to get rid of all the confusing double
negations.
If not all are dismissed, the following must have happened:
1. suggestion appears
2. user dismisses
3. all bookmarks for the URL are marked dismissed
4. the user adds (or syncs in) a new bookmark for the same URL.

I talked to Patrick about that case and we want to show a new suggestion for
that URL. So it will reappear because not all are dismissed, and that's the
intended behavior.

                      }),
       bookmarks.end());
 
   // Sort the remaining bookmarks by date.
   std::sort(bookmarks.begin(), bookmarks.end(),
             [&bookmark_model](const BookmarkModel::URLAndTitle& a,
                               const BookmarkModel::URLAndTitle& b) {
               return CompareBookmarksByLastVisitDate(
                   bookmark_model->GetMostRecentlyAddedUserNodeForURL(a.url),
                   bookmark_model->GetMostRecentlyAddedUserNodeForURL(b.url));
             });
 
   // Insert the first |max_count| items from |bookmarks| into |result|.
   std::vector<const BookmarkNode*> result;
   for (const BookmarkModel::URLAndTitle& bookmark : bookmarks) {
     result.push_back(
         bookmark_model->GetMostRecentlyAddedUserNodeForURL(bookmark.url));
     if (result.size() >= static_cast<size_t>(max_count))
       break;
   }
   return result;
 }
 
 }  // namespace ntp_snippets