MostVisitedSites: Remove unused order-persisting code

chrome/browser/android/ntp/most_visited_sites.cc

 // Copyright 2013 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 "chrome/browser/android/ntp/most_visited_sites.h"
 
 #include <utility>
 
 #include "base/callback.h"
 #include "base/command_line.h"
@@ skipping 182 matching lines @@
       SupervisedUserServiceFactory::GetForProfile(profile_);
   supervised_user_service->AddObserver(this);
 }
 
 MostVisitedSites::~MostVisitedSites() {
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_);
   supervised_user_service->RemoveObserver(this);
 }
 
-void MostVisitedSites::SetMostVisitedURLsObserver(
-      MostVisitedSites::Observer* observer, int num_sites) {
+void MostVisitedSites::SetMostVisitedURLsObserver(Observer* observer,
+                                                  int num_sites) {
   DCHECK(observer);
   observer_ = observer;
   num_sites_ = num_sites;
 
   if (ShouldShowPopularSites() &&
       NeedPopularSites(prefs_, num_sites_)) {
     popular_sites_.reset(new PopularSites(
         prefs_,
         template_url_service_,
         variations_service_,
         download_context_,
         GetPopularSitesCountry(),
         GetPopularSitesVersion(),
         false,
         base::Bind(&MostVisitedSites::OnPopularSitesAvailable,
                    base::Unretained(this))));
   } else {
     received_popular_sites_ = true;
   }
 
+  // TODO(treib): Can |top_sites_| ever be null? If not, remove these checks.
   if (top_sites_) {
     // TopSites updates itself after a delay. To ensure up-to-date results,
     // force an update now.
     top_sites_->SyncWithHistory();
 
     // Register as TopSitesObserver so that we can update ourselves when the
     // TopSites changes.
     scoped_observer_.Add(top_sites_.get());
   }
 
   suggestions_subscription_ = suggestions_service_->AddCallback(
       base::Bind(&MostVisitedSites::OnSuggestionsProfileAvailable,
                  base::Unretained(this)));
 
   // Immediately build the current suggestions, getting personal suggestions
   // from the SuggestionsService's cache or, if that is empty, from TopSites.
   BuildCurrentSuggestions();
   // Also start a request for fresh suggestions.
   suggestions_service_->FetchSuggestionsData();
 }
 
-void MostVisitedSites::GetURLThumbnail(
-    const GURL& url,
-    const ThumbnailCallback& callback) {
+void MostVisitedSites::GetURLThumbnail(const GURL& url,
+                                       const ThumbnailCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  // TODO(treib): Move this to the blocking pool? Doesn't seem related to DB.
   BrowserThread::PostTaskAndReplyWithResult(
       BrowserThread::DB, FROM_HERE,
       base::Bind(&MaybeFetchLocalThumbnail, url, top_sites_),
       base::Bind(&MostVisitedSites::OnLocalThumbnailFetched,
                  weak_ptr_factory_.GetWeakPtr(), url, callback));
 }
 
 void MostVisitedSites::OnLocalThumbnailFetched(
     const GURL& url,
     const ThumbnailCallback& callback,
@@ skipping 20 matching lines @@
     }
     if (mv_source_ == SUGGESTIONS_SERVICE) {
       return suggestions_service_->GetPageThumbnail(
           url, base::Bind(&MostVisitedSites::OnObtainedThumbnail,
                           weak_ptr_factory_.GetWeakPtr(), false, callback));
     }
   }
   OnObtainedThumbnail(true, callback, url, bitmap.get());
 }
 
-void MostVisitedSites::OnObtainedThumbnail(
-    bool is_local_thumbnail,
-    const ThumbnailCallback& callback,
-    const GURL& url,
-    const SkBitmap* bitmap) {
+void MostVisitedSites::OnObtainedThumbnail(bool is_local_thumbnail,
+                                           const ThumbnailCallback& callback,
+                                           const GURL& url,
+                                           const SkBitmap* bitmap) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   callback.Run(is_local_thumbnail, bitmap);
 }
 
-void MostVisitedSites::AddOrRemoveBlacklistedUrl(
-    const GURL& url, bool add_url) {
+void MostVisitedSites::AddOrRemoveBlacklistedUrl(const GURL& url,
+                                                 bool add_url) {
   // Always blacklist in the local TopSites.
   if (top_sites_) {
     if (add_url)
       top_sites_->AddBlacklistedURL(url);
     else
       top_sites_->RemoveBlacklistedURL(url);
   }
 
   // Only blacklist in the server-side suggestions service if it's active.
   if (mv_source_ == SUGGESTIONS_SERVICE) {
@@ skipping 39 matching lines @@
   LogHistogramEvent(histogram, tile_type, NUM_TILE_TYPES);
 }
 
 void MostVisitedSites::OnURLFilterChanged() {
   BuildCurrentSuggestions();
 }
 
 // static
 void MostVisitedSites::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
+  // TODO(treib): Remove this, it's unused. Do we need migration code to clean
+  // up existing entries?
   registry->RegisterListPref(ntp_tiles::prefs::kNTPSuggestionsURL);
+  // TODO(treib): Remove this. It's only used to determine if we need
+  // PopularSites at all. Find a way to do that without prefs, or failing that,
+  // replace this list pref by a simple bool.
   registry->RegisterListPref(ntp_tiles::prefs::kNTPSuggestionsIsPersonal);
 }
 
 void MostVisitedSites::BuildCurrentSuggestions() {
   // Get the current suggestions from cache. If the cache is empty, this will
   // fall back to TopSites.
   OnSuggestionsProfileAvailable(
       suggestions_service_->GetSuggestionsDataFromCache());
 }
 
@@ skipping 17 matching lines @@
   }
   return base::FilePath();
 }
 
 void MostVisitedSites::OnMostVisitedURLsAvailable(
     const history::MostVisitedURLList& visited_list) {
   SupervisedUserURLFilter* url_filter =
       SupervisedUserServiceFactory::GetForProfile(profile_)
           ->GetURLFilterForUIThread();
 
-  MostVisitedSites::SuggestionsPtrVector suggestions;
+  SuggestionsPtrVector suggestions;
   size_t num_tiles =
       std::min(visited_list.size(), static_cast<size_t>(num_sites_));
   for (size_t i = 0; i < num_tiles; ++i) {
     const history::MostVisitedURL& visited = visited_list[i];
     if (visited.url.is_empty()) {
       num_tiles = i;
       break;  // This is the signal that there are no more real visited sites.
     }
     if (url_filter->GetFilteringBehaviorForURL(visited.url) ==
         SupervisedUserURLFilter::FilteringBehavior::BLOCK) {
       continue;
     }
 
     std::unique_ptr<Suggestion> suggestion(new Suggestion());
     suggestion->title = visited.title;
     suggestion->url = visited.url;
     suggestion->source = TOP_SITES;
     suggestion->whitelist_icon_path = GetWhitelistLargeIconPath(visited.url);
 
     suggestions.push_back(std::move(suggestion));
   }
 
   received_most_visited_sites_ = true;
   mv_source_ = TOP_SITES;
-  SaveNewNTPSuggestions(&suggestions);
+  SaveNewSuggestions(&suggestions);
   NotifyMostVisitedURLsObserver();
 }
 
 void MostVisitedSites::OnSuggestionsProfileAvailable(
     const SuggestionsProfile& suggestions_profile) {
   int num_tiles = suggestions_profile.suggestions_size();
   // With no server suggestions, fall back to local TopSites.
   if (num_tiles == 0) {
     InitiateTopSitesQuery();
     return;
   }
   if (num_sites_ < num_tiles)
     num_tiles = num_sites_;
 
   SupervisedUserURLFilter* url_filter =
       SupervisedUserServiceFactory::GetForProfile(profile_)
           ->GetURLFilterForUIThread();
-  MostVisitedSites::SuggestionsPtrVector suggestions;
+  SuggestionsPtrVector suggestions;
   for (int i = 0; i < num_tiles; ++i) {
     const ChromeSuggestion& suggestion = suggestions_profile.suggestions(i);
     if (url_filter->GetFilteringBehaviorForURL(GURL(suggestion.url())) ==
         SupervisedUserURLFilter::FilteringBehavior::BLOCK) {
       continue;
     }
 
     std::unique_ptr<Suggestion> generated_suggestion(new Suggestion());
     generated_suggestion->title = base::UTF8ToUTF16(suggestion.title());
     generated_suggestion->url = GURL(suggestion.url());
     generated_suggestion->source = SUGGESTIONS_SERVICE;
     generated_suggestion->whitelist_icon_path = GetWhitelistLargeIconPath(
         GURL(suggestion.url()));
     if (suggestion.providers_size() > 0)
       generated_suggestion->provider_index = suggestion.providers(0);
 
     suggestions.push_back(std::move(generated_suggestion));
   }
 
   received_most_visited_sites_ = true;
   mv_source_ = SUGGESTIONS_SERVICE;
-  SaveNewNTPSuggestions(&suggestions);
+  SaveNewSuggestions(&suggestions);
   NotifyMostVisitedURLsObserver();
 }
 
 MostVisitedSites::SuggestionsPtrVector
 MostVisitedSites::CreateWhitelistEntryPointSuggestions(
-    const MostVisitedSites::SuggestionsPtrVector& personal_suggestions) {
+    const SuggestionsPtrVector& personal_suggestions) {
   size_t num_personal_suggestions = personal_suggestions.size();
   DCHECK_LE(num_personal_suggestions, static_cast<size_t>(num_sites_));
 
   size_t num_whitelist_suggestions = num_sites_ - num_personal_suggestions;
-  MostVisitedSites::SuggestionsPtrVector whitelist_suggestions;
+  SuggestionsPtrVector whitelist_suggestions;
 
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_);
   SupervisedUserURLFilter* url_filter =
       supervised_user_service->GetURLFilterForUIThread();
 
   std::set<std::string> personal_hosts;
   for (const auto& suggestion : personal_suggestions)
     personal_hosts.insert(suggestion->url.host());
 
@@ skipping 22 matching lines @@
     whitelist_suggestions.push_back(std::move(suggestion));
     if (whitelist_suggestions.size() >= num_whitelist_suggestions)
       break;
   }
 
   return whitelist_suggestions;
 }
 
 MostVisitedSites::SuggestionsPtrVector
 MostVisitedSites::CreatePopularSitesSuggestions(
-    const MostVisitedSites::SuggestionsPtrVector& personal_suggestions,
-    const MostVisitedSites::SuggestionsPtrVector& whitelist_suggestions) {
+    const SuggestionsPtrVector& personal_suggestions,
+    const SuggestionsPtrVector& whitelist_suggestions) {
   // For child accounts popular sites suggestions will not be added.
   if (is_child_profile_)
-    return MostVisitedSites::SuggestionsPtrVector();
+    return SuggestionsPtrVector();
 
   size_t num_suggestions =
       personal_suggestions.size() + whitelist_suggestions.size();
   DCHECK_LE(num_suggestions, static_cast<size_t>(num_sites_));
 
   // Collect non-blacklisted popular suggestions, skipping those already present
   // in the personal suggestions.
   size_t num_popular_sites_suggestions = num_sites_ - num_suggestions;
-  MostVisitedSites::SuggestionsPtrVector popular_sites_suggestions;
+  SuggestionsPtrVector popular_sites_suggestions;
 
   if (num_popular_sites_suggestions > 0 && popular_sites_) {
     std::set<std::string> hosts;
     for (const auto& suggestion : personal_suggestions)
       hosts.insert(suggestion->url.host());
     for (const auto& suggestion : whitelist_suggestions)
       hosts.insert(suggestion->url.host());
     for (const PopularSites::Site& popular_site : popular_sites_->sites()) {
       // Skip blacklisted sites.
       if (top_sites_ && top_sites_->IsBlacklisted(popular_site.url))
         continue;
       std::string host = popular_site.url.host();
       // Skip suggestions already present in personal or whitelists.
       if (hosts.find(host) != hosts.end())
         continue;
 
       std::unique_ptr<Suggestion> suggestion(new Suggestion());
       suggestion->title = popular_site.title;
       suggestion->url = GURL(popular_site.url);
       suggestion->source = POPULAR;
 
       popular_sites_suggestions.push_back(std::move(suggestion));
       if (popular_sites_suggestions.size() >= num_popular_sites_suggestions)
         break;
     }
   }
   return popular_sites_suggestions;
 }
 
-void MostVisitedSites::SaveNewNTPSuggestions(
-    MostVisitedSites::SuggestionsPtrVector* personal_suggestions) {
-  MostVisitedSites::SuggestionsPtrVector whitelist_suggestions =
+void MostVisitedSites::SaveNewSuggestions(
+    SuggestionsPtrVector* personal_suggestions) {
+  SuggestionsPtrVector whitelist_suggestions =
       CreateWhitelistEntryPointSuggestions(*personal_suggestions);
-  MostVisitedSites::SuggestionsPtrVector popular_sites_suggestions =
+  SuggestionsPtrVector popular_sites_suggestions =
       CreatePopularSitesSuggestions(*personal_suggestions,
                                     whitelist_suggestions);
+
   size_t num_actual_tiles = personal_suggestions->size() +
                             whitelist_suggestions.size() +
                             popular_sites_suggestions.size();
-  std::vector<std::string> old_sites_url;
-  std::vector<bool> old_sites_is_personal;
-  // TODO(treib): We used to call |GetPreviousNTPSites| here to populate
-  // |old_sites_url| and |old_sites_is_personal|, but that caused problems
-  // (crbug.com/585391). Either figure out a way to fix them and re-enable,
-  // or properly remove the order-persisting code. crbug.com/601734
-  MostVisitedSites::SuggestionsPtrVector merged_suggestions = MergeSuggestions(
-      personal_suggestions, &whitelist_suggestions, &popular_sites_suggestions,
-      old_sites_url, old_sites_is_personal);
+  DCHECK_LE(num_actual_tiles, static_cast<size_t>(num_sites_));
+
+  SuggestionsPtrVector merged_suggestions = MergeSuggestions(
+      personal_suggestions, &whitelist_suggestions, &popular_sites_suggestions);
   DCHECK_EQ(num_actual_tiles, merged_suggestions.size());
+
   current_suggestions_.resize(merged_suggestions.size());
   for (size_t i = 0; i < merged_suggestions.size(); ++i)
     std::swap(*merged_suggestions[i], current_suggestions_[i]);
+
   if (received_popular_sites_)
-    SaveCurrentNTPSites();
+    SaveCurrentSuggestionsToPrefs();
 }
 
 // static
 MostVisitedSites::SuggestionsPtrVector MostVisitedSites::MergeSuggestions(
-    MostVisitedSites::SuggestionsPtrVector* personal_suggestions,
-    MostVisitedSites::SuggestionsPtrVector* whitelist_suggestions,
-    MostVisitedSites::SuggestionsPtrVector* popular_suggestions,
-    const std::vector<std::string>& old_sites_url,
-    const std::vector<bool>& old_sites_is_personal) {
+    SuggestionsPtrVector* personal_suggestions,
+    SuggestionsPtrVector* whitelist_suggestions,
+    SuggestionsPtrVector* popular_suggestions) {
   size_t num_personal_suggestions = personal_suggestions->size();
   size_t num_whitelist_suggestions = whitelist_suggestions->size();
   size_t num_popular_suggestions = popular_suggestions->size();
   size_t num_tiles = num_popular_suggestions + num_whitelist_suggestions +
                      num_personal_suggestions;
-  MostVisitedSites::SuggestionsPtrVector merged_suggestions;
-  merged_suggestions.resize(num_tiles);
+  SuggestionsPtrVector merged_suggestions;
+  AppendSuggestions(personal_suggestions, &merged_suggestions);
+  AppendSuggestions(whitelist_suggestions, &merged_suggestions);
+  AppendSuggestions(popular_suggestions, &merged_suggestions);
+  DCHECK_EQ(num_tiles, merged_suggestions.size());
 
-  size_t num_old_tiles = old_sites_url.size();
-  DCHECK_LE(num_old_tiles, num_tiles);
-  DCHECK_EQ(num_old_tiles, old_sites_is_personal.size());
-  std::vector<std::string> old_sites_host;
-  old_sites_host.reserve(num_old_tiles);
-  // Only populate the hosts for popular suggestions as only they can be
-  // replaced by host. Personal suggestions require an exact url match to be
-  // replaced.
-  for (size_t i = 0; i < num_old_tiles; ++i) {
-    old_sites_host.push_back(old_sites_is_personal[i]
-                                 ? std::string()
-                                 : GURL(old_sites_url[i]).host());
-  }
-
-  // Insert personal suggestions if they existed previously.
-  std::vector<size_t> new_personal_suggestions = InsertMatchingSuggestions(
-      personal_suggestions, &merged_suggestions, old_sites_url, old_sites_host);
-  // Insert whitelist suggestions if they existed previously.
-  std::vector<size_t> new_whitelist_suggestions =
-      InsertMatchingSuggestions(whitelist_suggestions, &merged_suggestions,
-                                old_sites_url, old_sites_host);
-  // Insert popular suggestions if they existed previously.
-  std::vector<size_t> new_popular_suggestions = InsertMatchingSuggestions(
-      popular_suggestions, &merged_suggestions, old_sites_url, old_sites_host);
-  // Insert leftover personal suggestions.
-  size_t filled_so_far = InsertAllSuggestions(
-      0, new_personal_suggestions, personal_suggestions, &merged_suggestions);
-  // Insert leftover whitelist suggestions.
-  filled_so_far =
-      InsertAllSuggestions(filled_so_far, new_whitelist_suggestions,
-                           whitelist_suggestions, &merged_suggestions);
-  // Insert leftover popular suggestions.
-  InsertAllSuggestions(filled_so_far, new_popular_suggestions,
-                       popular_suggestions, &merged_suggestions);
   return merged_suggestions;
 }
 
-void MostVisitedSites::GetPreviousNTPSites(
-    size_t num_tiles,
-    std::vector<std::string>* old_sites_url,
-    std::vector<bool>* old_sites_is_personal) const {
-  const base::ListValue* url_list = prefs_->GetList(
-      ntp_tiles::prefs::kNTPSuggestionsURL);
-  const base::ListValue* source_list =
-      prefs_->GetList(ntp_tiles::prefs::kNTPSuggestionsIsPersonal);
-  DCHECK_EQ(url_list->GetSize(), source_list->GetSize());
-  if (url_list->GetSize() < num_tiles)
-    num_tiles = url_list->GetSize();
-  if (num_tiles == 0) {
-    // No fallback required as Personal suggestions take precedence anyway.
-    return;
-  }
-  old_sites_url->reserve(num_tiles);
-  old_sites_is_personal->reserve(num_tiles);
-  for (size_t i = 0; i < num_tiles; ++i) {
-    std::string url_string;
-    bool success = url_list->GetString(i, &url_string);
-    DCHECK(success);
-    old_sites_url->push_back(url_string);
-    bool is_personal;
-    success = source_list->GetBoolean(i, &is_personal);
-    DCHECK(success);
-    old_sites_is_personal->push_back(is_personal);
-  }
+// static
+void MostVisitedSites::AppendSuggestions(SuggestionsPtrVector* src,

[Bernhard Bauer, 2016/05/18 10:13:31]

Can we move this to an anonymous namespace? Heck, we could even templatize it
and move it to stl_util.h or something.

[Marc Treib, 2016/05/18 10:47:59]

Moving it into stl_util.h isn't worth it IMO.

We can't directly move it into an anonymous namespace, since
SuggestionsPtrVector is private in MostVisitedSites. Of course, I could either
make it public, or use the underlying type directly, but.. bleargh.
Given that we're planning to switch from SuggestionsPtrVector to
SuggestionsVector (which is public) anyway, can we leave it like this for now,
and move it into an anonymous namespace after the switch to non-Ptr?

[Bernhard Bauer, 2016/05/18 10:59:28]

Ok, that's the next thing I'd have suggested 😉 Add a TODO for that?

[Marc Treib, 2016/05/18 11:31:15]

There is already a TODO in the header for the switch to SuggestionsVector. I've
also added one here, to move this into an anonymous namespace.

+                                         SuggestionsPtrVector* dst) {
+  dst->insert(dst->end(),
+              std::make_move_iterator(src->begin()),
+              std::make_move_iterator(src->end()));
 }
 
-void MostVisitedSites::SaveCurrentNTPSites() {
+void MostVisitedSites::SaveCurrentSuggestionsToPrefs() {
   base::ListValue url_list;
   base::ListValue source_list;
   for (const auto& suggestion : current_suggestions_) {
     url_list.AppendString(suggestion.url.spec());
-    source_list.AppendBoolean(suggestion.source != MostVisitedSites::POPULAR);
+    source_list.AppendBoolean(suggestion.source != POPULAR);
   }
   prefs_->Set(ntp_tiles::prefs::kNTPSuggestionsIsPersonal, source_list);
   prefs_->Set(ntp_tiles::prefs::kNTPSuggestionsURL, url_list);
 }
 
-// static
-std::vector<size_t> MostVisitedSites::InsertMatchingSuggestions(
-    MostVisitedSites::SuggestionsPtrVector* src_suggestions,
-    MostVisitedSites::SuggestionsPtrVector* dst_suggestions,
-    const std::vector<std::string>& match_urls,
-    const std::vector<std::string>& match_hosts) {
-  std::vector<size_t> unmatched_suggestions;
-  size_t num_src_suggestions = src_suggestions->size();
-  size_t num_matchers = match_urls.size();
-  for (size_t i = 0; i < num_src_suggestions; ++i) {
-    size_t position;
-    for (position = 0; position < num_matchers; ++position) {
-      if ((*dst_suggestions)[position] != nullptr)
-        continue;
-      if (match_urls[position] == (*src_suggestions)[i]->url.spec())
-        break;
-      // match_hosts is only populated for suggestions which can be replaced by
-      // host matching like popular suggestions.
-      if (match_hosts[position] == (*src_suggestions)[i]->url.host())
-        break;
-    }
-    if (position == num_matchers) {
-      unmatched_suggestions.push_back(i);
-    } else {
-      // A move is required as the source and destination containers own the
-      // elements.
-      std::swap((*dst_suggestions)[position], (*src_suggestions)[i]);
-    }
-  }
-  return unmatched_suggestions;
-}
-
-// static
-size_t MostVisitedSites::InsertAllSuggestions(
-    size_t start_position,
-    const std::vector<size_t>& insert_positions,
-    std::vector<std::unique_ptr<Suggestion>>* src_suggestions,
-    std::vector<std::unique_ptr<Suggestion>>* dst_suggestions) {
-  size_t num_inserts = insert_positions.size();
-  size_t num_dests = dst_suggestions->size();
-
-  size_t src_pos = 0;
-  size_t i = start_position;
-  for (; i < num_dests && src_pos < num_inserts; ++i) {
-    if ((*dst_suggestions)[i] != nullptr)
-      continue;
-    size_t src = insert_positions[src_pos++];
-    std::swap((*dst_suggestions)[i], (*src_suggestions)[src]);
-  }
-  // Return destination positions filled so far which becomes the start_position
-  // for future runs.
-  return i;
-}
-
 void MostVisitedSites::NotifyMostVisitedURLsObserver() {
-  size_t num_suggestions = current_suggestions_.size();
   if (received_most_visited_sites_ && received_popular_sites_ &&
       !recorded_uma_) {
     RecordImpressionUMAMetrics();
-    UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.NumberOfTiles", num_suggestions);
+    UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.NumberOfTiles",
+                                current_suggestions_.size());
     recorded_uma_ = true;
   }
 
   if (!observer_)
     return;
 
   observer_->OnMostVisitedURLsAvailable(current_suggestions_);
 }
 
 void MostVisitedSites::OnPopularSitesAvailable(bool success) {
@@ skipping 23 matching lines @@
 
 void MostVisitedSites::TopSitesLoaded(TopSites* top_sites) {}
 
 void MostVisitedSites::TopSitesChanged(TopSites* top_sites,
                                        ChangeReason change_reason) {
   if (mv_source_ == TOP_SITES) {
     // The displayed suggestions are invalidated.
     InitiateTopSitesQuery();
   }
 }