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

components/ntp_snippets/bookmarks/bookmark_suggestions_provider.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_suggestions_provider.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 191 matching lines @@
 
 void BookmarkSuggestionsProvider::GetDismissedSuggestionsForDebugging(
     Category category,
     const DismissedSuggestionsCallback& callback) {
   DCHECK_EQ(category, provided_category_);
   std::vector<const BookmarkNode*> bookmarks =
       GetDismissedBookmarksForDebugging(bookmark_model_);
 
   std::vector<ContentSuggestion> suggestions;
   for (const BookmarkNode* bookmark : bookmarks) {
-    ConvertBookmark(bookmark, &suggestions);
+    ConvertBookmark(*bookmark, &suggestions);
   }
   callback.Run(std::move(suggestions));
 }
 
 void BookmarkSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
     Category category) {
   DCHECK_EQ(category, provided_category_);
   if (!bookmark_model_->loaded()) {
     return;
   }
   MarkAllBookmarksUndismissed(bookmark_model_);
 }
 
 void BookmarkSuggestionsProvider::BookmarkModelLoaded(
     bookmarks::BookmarkModel* model,
     bool ids_reassigned) {
   DCHECK_EQ(bookmark_model_, model);
   if (fetch_requested_) {
     fetch_requested_ = false;
     FetchBookmarksInternal();
   }
 }
 
 void BookmarkSuggestionsProvider::OnWillChangeBookmarkMetaInfo(
     BookmarkModel* model,
     const BookmarkNode* node) {
   // Store the last visit date of the node that is about to change.
-  if (!GetLastVisitDateForNTPBookmark(node, creation_date_fallback_,
+  if (!GetLastVisitDateForNTPBookmark(*node, creation_date_fallback_,

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

is |node| guaranteed to be non-null here and in BookmarkMetaInfoChanged()?

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

I am not sure. I see no info on this in BookmarkModelObserver.
sky: 
 - What is the reason for passing the bookmark nodes as const* and not const&?
 - Are they guaranteed to be non-null?

[sky, 2016/11/28 16:25:09]

They will never be null. I agree a const BookmarkNode& would be better here, and
in many other methods in BookmarkModelObserver.

                                       consider_bookmark_visits_from_desktop_,
                                       &node_to_change_last_visit_date_)) {
     node_to_change_last_visit_date_ = base::Time::UnixEpoch();
   }
 }
 
 void BookmarkSuggestionsProvider::BookmarkMetaInfoChanged(
     BookmarkModel* model,
     const BookmarkNode* node) {
   base::Time time;
-  if (!GetLastVisitDateForNTPBookmark(node, creation_date_fallback_,
+  if (!GetLastVisitDateForNTPBookmark(*node, creation_date_fallback_,
                                       consider_bookmark_visits_from_desktop_,
                                       &time)) {
     // Error in loading the last visit date after the change. This happens when
     // the bookmark just got dismissed. We must not update the suggestion in
     // such a case.
     return;
   }
 
   if (time == node_to_change_last_visit_date_ ||
       time < end_of_list_last_visit_date_) {
     // The last visit date has not changed or the change is irrelevant.
     return;
   }
 
   // Otherwise, we should update the suggestions.
   FetchBookmarks();
 }
 
 void BookmarkSuggestionsProvider::BookmarkNodeRemoved(
       bookmarks::BookmarkModel* model,
       const bookmarks::BookmarkNode* parent,
       int old_index,
       const bookmarks::BookmarkNode* node,
       const std::set<GURL>& no_longer_bookmarked) {
   base::Time time;
-  if (GetLastVisitDateForNTPBookmark(node, creation_date_fallback_,
+  if (GetLastVisitDateForNTPBookmark(*node, creation_date_fallback_,
                                      consider_bookmark_visits_from_desktop_,
                                      &time) &&
       time < end_of_list_last_visit_date_) {
     // We know the node is too old to influence the list.
     return;
   }
 
   // Some node from our list got deleted, we should update the suggestions.
   FetchBookmarks();
 }
 
 void BookmarkSuggestionsProvider::BookmarkNodeAdded(
     bookmarks::BookmarkModel* model,
     const bookmarks::BookmarkNode* parent,
     int index) {
   base::Time time;
   if (!GetLastVisitDateForNTPBookmark(
-          parent->GetChild(index), creation_date_fallback_,
+          *parent->GetChild(index), creation_date_fallback_,
           consider_bookmark_visits_from_desktop_, &time) ||
       time < end_of_list_last_visit_date_) {
     // The new node has no last visited info or is too old to get into the list.
     return;
   }
 
   // Some relevant node got created (e.g. by sync), we should update the list.
   FetchBookmarks();
 }
 
 void BookmarkSuggestionsProvider::ConvertBookmark(
-    const BookmarkNode* bookmark,
+    const BookmarkNode& bookmark,
     std::vector<ContentSuggestion>* suggestions) {
   base::Time publish_date;
   if (!GetLastVisitDateForNTPBookmark(bookmark, creation_date_fallback_,
                                       consider_bookmark_visits_from_desktop_,
                                       &publish_date)) {
     return;
   }
 
-  ContentSuggestion suggestion(provided_category_, bookmark->url().spec(),
-                               bookmark->url());
-  suggestion.set_title(bookmark->GetTitle());
+  ContentSuggestion suggestion(provided_category_, bookmark.url().spec(),
+                               bookmark.url());
+  suggestion.set_title(bookmark.GetTitle());
   suggestion.set_snippet_text(base::string16());
   suggestion.set_publish_date(publish_date);
-  suggestion.set_publisher_name(base::UTF8ToUTF16(bookmark->url().host()));
+  suggestion.set_publisher_name(base::UTF8ToUTF16(bookmark.url().host()));
 
   suggestions->emplace_back(std::move(suggestion));
 }
 
 void BookmarkSuggestionsProvider::FetchBookmarksInternal() {
   DCHECK(bookmark_model_->loaded());
 
   NotifyStatusChanged(CategoryStatus::AVAILABLE);
 
   base::Time threshold_time = GetThresholdTime();
   std::vector<const BookmarkNode*> bookmarks = GetRecentlyVisitedBookmarks(
       bookmark_model_, GetMinCount(), GetMaxCount(), threshold_time,
       creation_date_fallback_, consider_bookmark_visits_from_desktop_);
 
   std::vector<ContentSuggestion> suggestions;
   for (const BookmarkNode* bookmark : bookmarks) {
-    ConvertBookmark(bookmark, &suggestions);
+    ConvertBookmark(*bookmark, &suggestions);
   }
 
   if (suggestions.empty()) {
     end_of_list_last_visit_date_ = threshold_time;
   } else {
     end_of_list_last_visit_date_ = suggestions.back().publish_date();
   }
 
   observer()->OnNewSuggestions(this, provided_category_,
                                std::move(suggestions));
@@ skipping 10 matching lines @@
 void BookmarkSuggestionsProvider::NotifyStatusChanged(
     CategoryStatus new_status) {
   if (category_status_ == new_status) {
     return;
   }
   category_status_ = new_status;
   observer()->OnCategoryStatusChanged(this, provided_category_, new_status);
 }
 
 }  // namespace ntp_snippets