[Bookmark suggestions] Clean-up in the api: switch const* to const&

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 <numeric>
 #include <set>
 #include <string>
@@ skipping 59 matching lines @@
 
 std::vector<const BookmarkNode*>::const_iterator FindMostRecentBookmark(
     const std::vector<const BookmarkNode*>& bookmarks,
     bool creation_date_fallback,
     bool consider_visits_from_desktop) {
   auto most_recent = bookmarks.end();
   base::Time most_recent_last_visited = base::Time::UnixEpoch();
 
   for (auto iter = bookmarks.begin(); iter != bookmarks.end(); ++iter) {
     base::Time last_visited;
-    if (GetLastVisitDateForNTPBookmark(*iter, creation_date_fallback,
+    if (GetLastVisitDateForNTPBookmark(**iter, creation_date_fallback,
                                        consider_visits_from_desktop,
                                        &last_visited) &&
         most_recent_last_visited <= last_visited) {
       most_recent = iter;
       most_recent_last_visited = last_visited;
     }
   }
 
   return most_recent;
 }
@@ skipping 21 matching lines @@
     bookmark_model->SetNodeMetaInfo(
         node, is_mobile_platform ? kBookmarkLastVisitDateOnMobileKey
                                  : kBookmarkLastVisitDateOnDesktopKey,
         now);
     // If the bookmark has been dismissed from NTP before, a new visit overrides
     // such a dismissal.
     bookmark_model->DeleteNodeMetaInfo(node, kBookmarkDismissedFromNTP);
   }
 }
 
-bool GetLastVisitDateForNTPBookmark(const BookmarkNode* node,
+bool GetLastVisitDateForNTPBookmark(const BookmarkNode& node,
                                     bool creation_date_fallback,
                                     bool consider_visits_from_desktop,
                                     base::Time* out) {
-  if (!node || IsDismissedFromNTPForBookmark(node)) {
+  if (IsDismissedFromNTPForBookmark(node)) {
     return false;
   }
 
   bool got_mobile_date =
-      ExtractLastVisitDate(*node, kBookmarkLastVisitDateOnMobileKey, out);
+      ExtractLastVisitDate(node, kBookmarkLastVisitDateOnMobileKey, out);
 
   if (consider_visits_from_desktop) {
     // Consider the later visit from these two platform groups.
     base::Time last_visit_desktop;
-    if (ExtractLastVisitDate(*node, kBookmarkLastVisitDateOnDesktopKey,
+    if (ExtractLastVisitDate(node, kBookmarkLastVisitDateOnDesktopKey,
                              &last_visit_desktop)) {
       if (!got_mobile_date) {
         *out = last_visit_desktop;
       } else {
         *out = std::max(*out, last_visit_desktop);
       }
       return true;
     }
   }
 
   if (!got_mobile_date && creation_date_fallback) {
-    *out = node->date_added();
+    *out = node.date_added();
     return true;
   }
 
   return got_mobile_date;
 }
 
 void MarkBookmarksDismissed(BookmarkModel* bookmark_model, const GURL& url) {
   std::vector<const BookmarkNode*> nodes;
   bookmark_model->GetNodesByURL(url, &nodes);
   for (const BookmarkNode* node : nodes) {
     bookmark_model->SetNodeMetaInfo(node, kBookmarkDismissedFromNTP, "1");
   }
 }
 
-bool IsDismissedFromNTPForBookmark(const BookmarkNode* node) {
-  if (!node) {
-    return false;
-  }
-
+bool IsDismissedFromNTPForBookmark(const BookmarkNode& node) {
   std::string dismissed_from_ntp;
   bool result =
-      node->GetMetaInfo(kBookmarkDismissedFromNTP, &dismissed_from_ntp);
+      node.GetMetaInfo(kBookmarkDismissedFromNTP, &dismissed_from_ntp);
   DCHECK(!result || dismissed_from_ntp == "1");
   return result;
 }
 
 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
@@ skipping 10 matching lines @@
     BookmarkModel* bookmark_model,
     int min_count,
     int max_count,
     const base::Time& min_visit_time,
     bool creation_date_fallback,
     bool consider_visits_from_desktop) {
   // Get all the bookmark URLs.
   std::vector<BookmarkModel::URLAndTitle> bookmark_urls;
   bookmark_model->GetBookmarks(&bookmark_urls);
 
   std::vector<RecentBookmark> bookmarks;

[tschumann, 2016/11/25 13:47:43]

nit: unrelated to your change, but consider moving these two variables down to
line 228 where they get used first. this reduces the scope of the variable and
gives much better context when reading the code.

[jkrcal, 2016/11/25 14:05:36]

Both these variables should outlive the for-cycle starting at line 201. I am
surprised that
 - it would work what you suggest (wouldn't the variables be local to the
cycle?) and
 - you would consider it more readable.
Are you sure?

[tschumann, 2016/11/25 15:38:14]

ahh... i see... the nesting tricked me in the diff view. Someday we should
probably split up that method, but it's not today ;-)

[Marc Treib, 2016/11/25 16:23:47]

Semi-related: I just sent out a CL which significantly shortens this method,
https://codereview.chromium.org/2533443004/ :)

   int recently_visited_count = 0;
   // Find for each bookmark the most recently visited BookmarkNode and find out
   // whether it is visited since |min_visit_time|.
   for (const BookmarkModel::URLAndTitle& url_and_title : bookmark_urls) {
     // Skip URLs that are blacklisted.
     if (IsBlacklisted(url_and_title.url)) {
       continue;
     }
 
     // Get all bookmarks for the given URL.
     std::vector<const BookmarkNode*> bookmarks_for_url;
     bookmark_model->GetNodesByURL(url_and_title.url, &bookmarks_for_url);
     DCHECK(!bookmarks_for_url.empty());
 
     // Find the most recently visited node for the given URL.
     auto most_recent =
         FindMostRecentBookmark(bookmarks_for_url, creation_date_fallback,
                                consider_visits_from_desktop);
     if (most_recent == bookmarks_for_url.end()) {
       continue;
     }
 
     // Extract the last visit of the node to use later for sorting.
     base::Time last_visit;
-    if (!GetLastVisitDateForNTPBookmark(*most_recent, creation_date_fallback,
+    if (!GetLastVisitDateForNTPBookmark(**most_recent, creation_date_fallback,
                                         consider_visits_from_desktop,
                                         &last_visit)) {
       continue;
     }
 
     // Has it been _visited_ recently enough (not considering creation date)?
     base::Time last_real_visit;
     if (GetLastVisitDateForNTPBookmark(
-            *most_recent, /*creation_date_fallback=*/false,
+            **most_recent, /*creation_date_fallback=*/false,
             consider_visits_from_desktop, &last_real_visit) &&
         min_visit_time < last_real_visit) {
       recently_visited_count++;
       bookmarks.push_back(
           {*most_recent, last_visit, /*visited_recently=*/true});
     } else {
       bookmarks.push_back(
           {*most_recent, last_visit, /*visited_recently=*/false});
     }
   }
@@ skipping 49 matching lines @@
   // Remove the bookmark URLs which have at least one non-dismissed bookmark.
   bookmarks.erase(
       std::remove_if(
           bookmarks.begin(), bookmarks.end(),
           [&bookmark_model](const BookmarkModel::URLAndTitle& bookmark) {
             std::vector<const BookmarkNode*> bookmarks_for_url;
             bookmark_model->GetNodesByURL(bookmark.url, &bookmarks_for_url);
             DCHECK(!bookmarks_for_url.empty());
 
             for (const BookmarkNode* node : bookmarks_for_url) {
-              if (!IsDismissedFromNTPForBookmark(node)) {
+              if (!IsDismissedFromNTPForBookmark(*node)) {
                 return true;
               }
             }
             return false;
           }),
       bookmarks.end());
 
   // Insert into |result|.
   std::vector<const BookmarkNode*> result;
   for (const BookmarkModel::URLAndTitle& bookmark : bookmarks) {
@@ skipping 16 matching lines @@
     for (const BookmarkNode* bookmark : bookmarks_for_url) {
       bookmark_model->DeleteNodeMetaInfo(bookmark,
                                          kBookmarkLastVisitDateOnMobileKey);
       bookmark_model->DeleteNodeMetaInfo(bookmark,
                                          kBookmarkLastVisitDateOnDesktopKey);
     }
   }
 }
 
 }  // namespace ntp_snippets