Refactor: avoid vectors copying in TopSitesImpl

There's no change to semantics.
- got rid of ref-counted MostVisitedThreadSafe
- made extensive use of movable vectors

BUG=1006830

Change-Id: Iba70c2955f01ceaeac8771ca93af2258829f02a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1819037Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699049}

components/history/core/browser/history_types.h

@@ -425,8 +425,6 @@ struct TopSitesDelta {
   MostVisitedURLWithRankList moved;
 };
 
-typedef base::RefCountedData<MostVisitedURLList> MostVisitedThreadSafe;
-
 // Map from origins to a count of matching URLs and the last visited time to any
 // URL under that origin.
 typedef std::map<GURL, std::pair<int, base::Time>> OriginCountAndLastVisitMap;

components/history/core/browser/top_sites_backend.cc

@@ -46,12 +46,10 @@ void TopSitesBackend::Shutdown() {
 void TopSitesBackend::GetMostVisitedSites(
     GetMostVisitedSitesCallback callback,
     base::CancelableTaskTracker* tracker) {
-  scoped_refptr<MostVisitedThreadSafe> sites = new MostVisitedThreadSafe();
-  tracker->PostTaskAndReply(
+  tracker->PostTaskAndReplyWithResult(
       db_task_runner_.get(), FROM_HERE,
-      base::BindOnce(&TopSitesBackend::GetMostVisitedSitesOnDBThread, this,
-                     sites),
-      base::BindOnce(std::move(callback), sites));
+      base::BindOnce(&TopSitesBackend::GetMostVisitedSitesOnDBThread, this),
+      std::move(callback));
 }
 
 void TopSitesBackend::UpdateTopSites(const TopSitesDelta& delta,
@@ -85,13 +83,12 @@ void TopSitesBackend::ShutdownDBOnDBThread() {
   db_.reset();
 }
 
-void TopSitesBackend::GetMostVisitedSitesOnDBThread(
-    scoped_refptr<MostVisitedThreadSafe> sites) {
+MostVisitedURLList TopSitesBackend::GetMostVisitedSitesOnDBThread() {
   DCHECK(db_task_runner_->RunsTasksInCurrentSequence());
-
-  if (db_) {
-    db_->GetSites(&(sites->data));
-  }
+  MostVisitedURLList list;
+  if (db_)
+    db_->GetSites(&list);
+  return list;
 }
 
 void TopSitesBackend::UpdateTopSitesOnDBThread(

components/history/core/browser/top_sites_backend.h

@@ -38,7 +38,7 @@ class TopSitesBackend : public base::RefCountedThreadSafe<TopSitesBackend> {
   };
 
   using GetMostVisitedSitesCallback =
-      base::OnceCallback<void(const scoped_refptr<MostVisitedThreadSafe>&)>;
+      base::OnceCallback<void(MostVisitedURLList)>;
 
   TopSitesBackend();
 
@@ -47,7 +47,7 @@ class TopSitesBackend : public base::RefCountedThreadSafe<TopSitesBackend> {
   // Schedules the db to be shutdown.
   void Shutdown();
 
-  // Fetches MostVisitedThreadSafe.
+  // Fetches MostVisitedURLList.
   void GetMostVisitedSites(GetMostVisitedSitesCallback callback,
                            base::CancelableTaskTracker* tracker);
 
@@ -70,8 +70,7 @@ class TopSitesBackend : public base::RefCountedThreadSafe<TopSitesBackend> {
   void ShutdownDBOnDBThread();
 
   // Does the work of getting the most visited sites.
-  void GetMostVisitedSitesOnDBThread(
-      scoped_refptr<MostVisitedThreadSafe> sites);
+  MostVisitedURLList GetMostVisitedSitesOnDBThread();
 
   // Updates top sites.
   void UpdateTopSitesOnDBThread(const TopSitesDelta& delta,

components/history/core/browser/top_sites_impl.cc

@@ -128,13 +128,11 @@ void TopSitesImpl::GetMostVisitedURLs(
   callback.Run(filtered_urls);
 }
 
-// Returns the index of |url| in |urls|, or -1 if not found.
-static int IndexOf(const MostVisitedURLList& urls, const GURL& url) {
-  for (size_t i = 0; i < urls.size(); i++) {
-    if (urls[i].url == url)
-      return i;
-  }
-  return -1;
+static bool Contains(const MostVisitedURLList& urls, const GURL& url) {
+  return std::find_if(urls.begin(), urls.end(),
+                      [&url](const MostVisitedURL& item) {
+                        return item.url == url;
+                      }) != urls.end();
 }
 
 void TopSitesImpl::SyncWithHistory() {
@@ -232,7 +230,7 @@ void TopSitesImpl::RegisterPrefs(PrefRegistrySimple* registry) {
 TopSitesImpl::~TopSitesImpl() = default;
 
 void TopSitesImpl::StartQueryForMostVisited() {
-  const int kDaysOfHistory = 90;
+  constexpr int kDaysOfHistory = 90;
 
   DCHECK(loaded_);
   timer_.Stop();
@@ -261,26 +259,19 @@ void TopSitesImpl::DiffMostVisited(const MostVisitedURLList& old_list,
   // When we find a match in the old set, we'll reset its index to our special
   // marker. This allows us to quickly identify the deleted ones in a later
   // pass.
-  const size_t kAlreadyFoundMarker = static_cast<size_t>(-1);
+  constexpr size_t kAlreadyFoundMarker = static_cast<size_t>(-1);
   int rank = -1;
-  for (size_t i = 0; i < new_list.size(); i++) {
+  for (const auto& new_url : new_list) {
     rank++;
-    auto found = all_old_urls.find(new_list[i].url);
+    auto found = all_old_urls.find(new_url.url);
     if (found == all_old_urls.end()) {
-      MostVisitedURLWithRank added;
-      added.url = new_list[i];
-      added.rank = rank;
-      delta->added.push_back(added);
+      delta->added.emplace_back(MostVisitedURLWithRank{new_url, rank});
     } else {
       DCHECK(found->second != kAlreadyFoundMarker)
           << "Same URL appears twice in the new list.";
       int old_rank = found->second;
-      if (old_rank != rank) {
-        MostVisitedURLWithRank moved;
-        moved.url = new_list[i];
-        moved.rank = rank;
-        delta->moved.push_back(moved);
-      }
+      if (old_rank != rank)
+        delta->moved.emplace_back(MostVisitedURLWithRank{new_url, rank});
       found->second = kAlreadyFoundMarker;
     }
   }
@@ -296,8 +287,9 @@ void TopSitesImpl::DiffMostVisited(const MostVisitedURLList& old_list,
 bool TopSitesImpl::AddPrepopulatedPages(MostVisitedURLList* urls) const {
   bool added = false;
   for (const auto& prepopulated_page : prepopulated_pages_) {
-    if (urls->size() < kTopSitesNumber &&
-        IndexOf(*urls, prepopulated_page.most_visited.url) == -1) {
+    if (urls->size() >= kTopSitesNumber)
+      break;
+    if (!Contains(*urls, prepopulated_page.most_visited.url)) {
       urls->push_back(prepopulated_page.most_visited);
       added = true;
     }
@@ -305,8 +297,8 @@ bool TopSitesImpl::AddPrepopulatedPages(MostVisitedURLList* urls) const {
   return added;
 }
 
-void TopSitesImpl::ApplyBlacklist(const MostVisitedURLList& urls,
-                                  MostVisitedURLList* out) {
+MostVisitedURLList TopSitesImpl::ApplyBlacklist(
+    const MostVisitedURLList& urls) {
   // Log the number of times ApplyBlacklist is called so we can compute the
   // average number of blacklisted items per user.
   const base::DictionaryValue* blacklist =
@@ -314,15 +306,15 @@ void TopSitesImpl::ApplyBlacklist(const MostVisitedURLList& urls,
   UMA_HISTOGRAM_BOOLEAN("TopSites.NumberOfApplyBlacklist", true);
   UMA_HISTOGRAM_COUNTS_100("TopSites.NumberOfBlacklistedItems",
       (blacklist ? blacklist->size() : 0));
-  size_t num_urls = 0;
-  for (size_t i = 0; i < urls.size(); ++i) {
-    if (!IsBlacklisted(urls[i].url)) {
-      if (num_urls >= kTopSitesNumber)
-        break;
-      num_urls++;
-      out->push_back(urls[i]);
-    }
+  MostVisitedURLList result;
+  for (const auto& url : urls) {
+    if (IsBlacklisted(url.url))
+      continue;
+    if (result.size() >= kTopSitesNumber)
+      break;
+    result.push_back(url);
   }
+  return result;
 }
 
 // static
@@ -332,11 +324,10 @@ std::string TopSitesImpl::GetURLHash(const GURL& url) {
   return base::MD5String(url.spec());
 }
 
-void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites,
+void TopSitesImpl::SetTopSites(MostVisitedURLList top_sites,
                                const CallLocation location) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  MostVisitedURLList top_sites(new_top_sites);
   AddPrepopulatedPages(&top_sites);
 
   TopSitesDelta delta;
@@ -373,7 +364,7 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites,
   // We always do the following steps (setting top sites in cache, and resetting
   // thread safe cache ...) as this method is invoked during startup at which
   // point the caches haven't been updated yet.
-  top_sites_ = top_sites;
+  top_sites_ = std::move(top_sites);
 
   ResetThreadSafeCache();
 
@@ -409,8 +400,8 @@ void TopSitesImpl::MoveStateToLoaded() {
     }
   }
 
-  for (size_t i = 0; i < pending_callbacks.size(); i++)
-    pending_callbacks[i].Run(urls);
+  for (auto& callback : pending_callbacks)
+    callback.Run(urls);
 
   if (history_service_)
     history_service_observer_.Add(history_service_);
@@ -420,9 +411,7 @@ void TopSitesImpl::MoveStateToLoaded() {
 
 void TopSitesImpl::ResetThreadSafeCache() {
   base::AutoLock lock(lock_);
-  MostVisitedURLList filtered;
-  ApplyBlacklist(top_sites_, &filtered);
-  thread_safe_cache_ = filtered;
+  thread_safe_cache_ = ApplyBlacklist(top_sites_);
 }
 
 void TopSitesImpl::ScheduleUpdateTimer() {
@@ -433,13 +422,12 @@ void TopSitesImpl::ScheduleUpdateTimer() {
                &TopSitesImpl::StartQueryForMostVisited);
 }
 
-void TopSitesImpl::OnGotMostVisitedURLs(
-    const scoped_refptr<MostVisitedThreadSafe>& sites) {
+void TopSitesImpl::OnGotMostVisitedURLs(MostVisitedURLList sites) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Set |top_sites_| directly so that SetTopSites() diffs correctly.
-  top_sites_ = sites->data;
-  SetTopSites(sites->data, CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_URLS);
+  top_sites_ = sites;
+  SetTopSites(std::move(sites), CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_URLS);
 
   MoveStateToLoaded();
 
@@ -449,7 +437,7 @@ void TopSitesImpl::OnGotMostVisitedURLs(
 }
 
 void TopSitesImpl::OnTopSitesAvailableFromHistory(MostVisitedURLList pages) {
-  SetTopSites(pages, CALL_LOCATION_FROM_OTHER_PLACES);
+  SetTopSites(std::move(pages), CALL_LOCATION_FROM_OTHER_PLACES);
 }
 
 void TopSitesImpl::OnURLsDeleted(HistoryService* history_service,

components/history/core/browser/top_sites_impl.h

@@ -125,7 +125,7 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver {
 
   // Takes |urls|, produces it's copy in |out| after removing blacklisted URLs.
   // Also ensures we respect the maximum number TopSites URLs.
-  void ApplyBlacklist(const MostVisitedURLList& urls, MostVisitedURLList* out);
+  MostVisitedURLList ApplyBlacklist(const MostVisitedURLList& urls);
 
   // Returns an MD5 hash of the URL. Hashing is required for blacklisted URLs.
   static std::string GetURLHash(const GURL& url);
@@ -133,7 +133,7 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver {
   // Updates URLs in |cache_| and the db (in the background). The URLs in
   // |new_top_sites| replace those in |cache_|. All mutations to cache_ *must*
   // go through this. Should be called from the UI thread.
-  void SetTopSites(const MostVisitedURLList& new_top_sites,
+  void SetTopSites(MostVisitedURLList new_top_sites,
                    const CallLocation location);
 
   // Returns the number of most visited results to request from history. This
@@ -153,7 +153,7 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver {
 
   // Callback from TopSites with the list of top sites. Should be called from
   // the UI thread.
-  void OnGotMostVisitedURLs(const scoped_refptr<MostVisitedThreadSafe>& sites);
+  void OnGotMostVisitedURLs(MostVisitedURLList sites);
 
   // Called when history service returns a list of top URLs.
   void OnTopSitesAvailableFromHistory(MostVisitedURLList data);
@@ -188,7 +188,7 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver {
   // The pending requests for the top sites list. Can only be non-empty at
   // startup. After we read the top sites from the DB, we'll always have a
   // cached list and be able to run callbacks immediately.
-  PendingCallbacks pending_callbacks_;
+  PendingCallbacks pending_callbacks_ GUARDED_BY(lock_);
 
   // URL List of prepopulated page.
   const PrepopulatedPageList prepopulated_pages_;

components/history/core/browser/top_sites_impl_unittest.cc

@@ -203,7 +203,7 @@ class TopSitesImplTest : public HistoryUnitTestBase {
   // Wrappers that allow private TopSites functions to be called from the
   // individual tests without making them all be friends.
   void SetTopSites(const MostVisitedURLList& new_top_sites) {
-    top_sites()->SetTopSites(new_top_sites,
+    top_sites()->SetTopSites(MostVisitedURLList(new_top_sites),
                              TopSitesImpl::CALL_LOCATION_FROM_OTHER_PLACES);
   }