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 @@
       scoped_observer_(this),
       mv_source_(SUGGESTIONS_SERVICE),
       weak_ptr_factory_(this) {
   supervisor_->SetObserver(this);
 }
 
 MostVisitedSites::~MostVisitedSites() {
   supervisor_->SetObserver(nullptr);
 }
 
-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_,
         popular_sites_directory_, 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::OnBlockedSitesChanged() {
   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 10 matching lines @@
 base::FilePath MostVisitedSites::GetWhitelistLargeIconPath(const GURL& url) {
   for (const auto& whitelist : supervisor_->whitelists()) {
     if (AreURLsEquivalent(whitelist.entry_point, url))
       return whitelist.large_icon_path;
   }
   return base::FilePath();
 }
 
 void MostVisitedSites::OnMostVisitedURLsAvailable(
     const history::MostVisitedURLList& visited_list) {
-  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 (supervisor_->IsBlocked(visited.url))
       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_;
 
-  MostVisitedSites::SuggestionsPtrVector suggestions;
+  SuggestionsPtrVector suggestions;
   for (int i = 0; i < num_tiles; ++i) {
     const ChromeSuggestion& suggestion = suggestions_profile.suggestions(i);
     if (supervisor_->IsBlocked(GURL(suggestion.url())))
       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;
 
   std::set<std::string> personal_hosts;
   for (const auto& suggestion : personal_suggestions)
     personal_hosts.insert(suggestion->url.host());
 
   for (const auto& whitelist : supervisor_->whitelists()) {
     // Skip blacklisted sites.
     if (top_sites_ && top_sites_->IsBlacklisted(whitelist.entry_point))
       continue;
 
@@ skipping 15 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 (supervisor_->IsChildProfile())
-    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);
-  }
+// TODO(treib): Once we use SuggestionsVector (non-Ptr) everywhere, move this
+// into an anonymous namespace.
+// static
+void MostVisitedSites::AppendSuggestions(SuggestionsPtrVector* src,
+                                         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();
   }
 }