history: removes usage of MostVisitedURL::redirects from TopSites

This is the start of getting rid of the usage of MostVisitedURL::redirects
from TopSites. It turns out the redirects can be *huge*. Redirects are used
for only one thing, to optimize deleting a url. That is, when a url is
deleted storing the redirects helped to avoid unnecessarily querying history.
Given redirects can be huge, and deletion is infrequent, this optimization
isn't worth it and I'm going to remove the complexity. This patch removes
the optimization and changes TopSites to always query history after removal.

Assuming this lands, and sticks, I plan to change around the database side
and hopefully completely remove MostVisitedURL::redirects.

BUG=995906
TEST=covered by tests

Change-Id: Id0ec659e112776b89b69ad916ac5c7f6ab723f9b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1762441
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689071}

components/history/core/browser/BUILD.gn

@@ -64,8 +64,6 @@ static_library("browser") {
     "top_sites.h",
     "top_sites_backend.cc",
     "top_sites_backend.h",
-    "top_sites_cache.cc",
-    "top_sites_cache.h",
     "top_sites_database.cc",
     "top_sites_database.h",
     "top_sites_impl.cc",
@@ -205,7 +203,6 @@ source_set("unit_tests") {
     "sync/typed_url_sync_bridge_unittest.cc",
     "sync/typed_url_sync_metadata_database_unittest.cc",
     "thumbnail_database_unittest.cc",
-    "top_sites_cache_unittest.cc",
     "top_sites_database_unittest.cc",
     "top_sites_impl_unittest.cc",
     "url_database_unittest.cc",

components/history/core/browser/top_sites.h

@@ -77,10 +77,6 @@ class TopSites : public RefcountedKeyedService {
   // Clear the blacklist. Should be called from the UI thread.
   virtual void ClearBlacklistedURLs() = 0;
 
-  // Returns true if the given URL is known to the top sites service.
-  // This function also returns false if TopSites isn't loaded yet.
-  virtual bool IsKnownURL(const GURL& url) = 0;
-
   // Returns true if the top sites list is full (i.e. we already have the
   // maximum number of top sites).  This function also returns false if TopSites
   // isn't loaded yet.

components/history/core/browser/top_sites_cache.cc

-// Copyright (c) 2012 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 "base/logging.h"
-#include "components/history/core/browser/top_sites_cache.h"
-
-namespace history {
-
-TopSitesCache::CanonicalURLQuery::CanonicalURLQuery(const GURL& url) {
-  most_visited_url_.redirects.push_back(url);
-  entry_.first = &most_visited_url_;
-  entry_.second = 0u;
-}
-
-TopSitesCache::CanonicalURLQuery::~CanonicalURLQuery() {
-}
-
-TopSitesCache::TopSitesCache() {
-}
-
-TopSitesCache::~TopSitesCache() {
-}
-
-void TopSitesCache::SetTopSites(const MostVisitedURLList& top_sites) {
-  top_sites_ = top_sites;
-  GenerateCanonicalURLs();
-}
-
-const GURL& TopSitesCache::GetCanonicalURL(const GURL& url) const {
-  auto it = GetCanonicalURLsIterator(url);
-  return it == canonical_urls_.end() ? url : it->first.first->url;
-}
-
-bool TopSitesCache::IsKnownURL(const GURL& url) const {
-  return GetCanonicalURLsIterator(url) != canonical_urls_.end();
-}
-
-size_t TopSitesCache::GetURLIndex(const GURL& url) const {
-  DCHECK(IsKnownURL(url));
-  return GetCanonicalURLsIterator(url)->second;
-}
-
-size_t TopSitesCache::GetNumURLs() const {
-  return top_sites_.size();
-}
-
-void TopSitesCache::GenerateCanonicalURLs() {
-  canonical_urls_.clear();
-  for (size_t i = 0; i < top_sites_.size(); i++)
-    StoreRedirectChain(top_sites_[i].redirects, i);
-}
-
-void TopSitesCache::StoreRedirectChain(const RedirectList& redirects,
-                                       size_t destination) {
-  // |redirects| is empty if the user pinned a site and there are not enough top
-  // sites before the pinned site.
-
-  // Map all the redirected URLs to the destination.
-  for (size_t i = 0; i < redirects.size(); i++) {
-    // If this redirect is already known, don't replace it with a new one.
-    if (!IsKnownURL(redirects[i])) {
-      CanonicalURLEntry entry;
-      entry.first = &(top_sites_[destination]);
-      entry.second = i;
-      canonical_urls_[entry] = destination;
-    }
-  }
-}
-
-TopSitesCache::CanonicalURLs::const_iterator
-    TopSitesCache::GetCanonicalURLsIterator(const GURL& url) const {
-  return canonical_urls_.find(CanonicalURLQuery(url).entry());
-}
-
-const GURL& TopSitesCache::GetURLFromIterator(
-    CanonicalURLs::const_iterator it) const {
-  DCHECK(it != canonical_urls_.end());
-  return it->first.first->redirects[it->first.second];
-}
-
-}  // namespace history

components/history/core/browser/top_sites_cache.h

-// Copyright (c) 2012 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.
-
-#ifndef COMPONENTS_HISTORY_CORE_BROWSER_TOP_SITES_CACHE_H_
-#define COMPONENTS_HISTORY_CORE_BROWSER_TOP_SITES_CACHE_H_
-
-#include <stddef.h>
-
-#include <map>
-#include <utility>
-
-#include "base/macros.h"
-#include "components/history/core/browser/history_types.h"
-#include "components/history/core/browser/url_utils.h"
-#include "url/gurl.h"
-
-namespace history {
-
-// TopSitesCache caches the list of top sites for TopSites.
-class TopSitesCache {
- public:
-  TopSitesCache();
-  ~TopSitesCache();
-
-  // Set the top sites.
-  void SetTopSites(const MostVisitedURLList& top_sites);
-  const MostVisitedURLList& top_sites() const { return top_sites_; }
-
-  // Returns the canonical URL for |url|. If not found, returns |url|.
-  const GURL& GetCanonicalURL(const GURL& url) const;
-
-  // Returns true if |url| is known.
-  bool IsKnownURL(const GURL& url) const;
-
-  // Returns the index into |top_sites_| for |url|.
-  size_t GetURLIndex(const GURL& url) const;
-
-  // Returns the number of URLs in the cache.
-  size_t GetNumURLs() const;
-
- private:
-  // The entries in CanonicalURLs, see CanonicalURLs for details. The second
-  // argument gives the index of the URL into MostVisitedURLs redirects.
-  using CanonicalURLEntry = std::pair<MostVisitedURL*, size_t>;
-
-  // Comparator used for CanonicalURLs.
-  class CanonicalURLComparator {
-   public:
-    bool operator()(const CanonicalURLEntry& e1,
-                    const CanonicalURLEntry& e2) const {
-      return CanonicalURLStringCompare(e1.first->redirects[e1.second].spec(),
-                                       e2.first->redirects[e2.second].spec());
-    }
-  };
-
-  // Creates the object needed to form std::map queries into |canonical_urls_|,
-  // wrapping all required temporary data to allow inlining.
-  class CanonicalURLQuery {
-   public:
-    explicit CanonicalURLQuery(const GURL& url);
-    ~CanonicalURLQuery();
-    const CanonicalURLEntry& entry() { return entry_; }
-
-   private:
-    MostVisitedURL most_visited_url_;
-    CanonicalURLEntry entry_;
-  };
-
-  // This is used to map from redirect url to the MostVisitedURL the redirect is
-  // from. Ideally this would be map<GURL, size_t> (second param indexing into
-  // top_sites_), but this results in duplicating all redirect urls. As some
-  // sites have a lot of redirects, we instead use the MostVisitedURL* and the
-  // index of the redirect as the key, and the index into top_sites_ as the
-  // value. This way we aren't duplicating GURLs. CanonicalURLComparator
-  // enforces the ordering as if we were using GURLs.
-  using CanonicalURLs =
-      std::map<CanonicalURLEntry, size_t, CanonicalURLComparator>;
-
-  // Generates the set of canonical urls from |top_sites_|.
-  void GenerateCanonicalURLs();
-
-  // Stores a set of redirects. This is used by GenerateCanonicalURLs.
-  void StoreRedirectChain(const RedirectList& redirects, size_t destination);
-
-  // Returns the iterator into |canonical_urls_| for the |url|.
-  CanonicalURLs::const_iterator GetCanonicalURLsIterator(const GURL& url) const;
-
-  // Returns the GURL corresponding to an iterator in |canonical_urls_|.
-  const GURL& GetURLFromIterator(CanonicalURLs::const_iterator it) const;
-
-  // The list of top sites.
-  MostVisitedURLList top_sites_;
-
-  // Generated from the redirects to and from the most visited pages. See
-  // description above CanonicalURLs for details.
-  CanonicalURLs canonical_urls_;
-
-  DISALLOW_COPY_AND_ASSIGN(TopSitesCache);
-};
-
-}  // namespace history
-
-#endif  // COMPONENTS_HISTORY_CORE_BROWSER_TOP_SITES_CACHE_H_

components/history/core/browser/top_sites_cache_unittest.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 "components/history/core/browser/top_sites_cache.h"
-
-#include <stddef.h>
-
-#include <set>
-#include <string>
-#include <vector>
-
-#include "base/logging.h"
-#include "base/stl_util.h"
-#include "base/strings/string16.h"
-#include "base/strings/string_number_conversions.h"
-#include "base/strings/utf_string_conversions.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace history {
-
-namespace {
-
-class TopSitesCacheTest : public testing::Test {
- public:
-  TopSitesCacheTest() {
-  }
-
- protected:
-  // Initializes |top_sites_| on |spec|, which is a list of URL strings with
-  // optional indents: indentated URLs redirect to the last non-indented URL.
-  // Titles are assigned as "Title 1", "Title 2", etc., in the order of
-  // appearance. See |kTopSitesSpecBasic| for an example. This function does not
-  // update |cache_| so you can manipulate |top_sites_| before you update it.
-  void BuildTopSites(const char** spec, size_t size);
-
-  // Initializes |top_sites_| and |cache_| based on |spec|.
-  void InitTopSiteCache(const char** spec, size_t size);
-
-  MostVisitedURLList top_sites_;
-  TopSitesCache cache_;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TopSitesCacheTest);
-};
-
-void TopSitesCacheTest::BuildTopSites(const char** spec, size_t size) {
-  std::set<std::string> urls_seen;
-  for (size_t i = 0; i < size; ++i) {
-    const char* spec_item = spec[i];
-    while (*spec_item && *spec_item == ' ')  // Eat indent.
-      ++spec_item;
-    if (urls_seen.find(spec_item) != urls_seen.end())
-      NOTREACHED() << "Duplicate URL found: " << spec_item;
-    urls_seen.insert(spec_item);
-    if (spec_item == spec[i]) {  // No indent: add new MostVisitedURL.
-      base::string16 title(base::ASCIIToUTF16("Title ") +
-                           base::NumberToString16(top_sites_.size() + 1));
-      top_sites_.push_back(MostVisitedURL(GURL(spec_item), title));
-    }
-    ASSERT_TRUE(!top_sites_.empty());
-    // Set up redirect to canonical URL. Canonical URL redirects to itself, too.
-    top_sites_.back().redirects.push_back(GURL(spec_item));
-  }
-}
-
-void TopSitesCacheTest::InitTopSiteCache(const char** spec, size_t size) {
-  BuildTopSites(spec, size);
-  cache_.SetTopSites(top_sites_);
-}
-
-const char* kTopSitesSpecBasic[] = {
-  "http://www.google.com",
-  "  http://www.gogle.com",  // Redirects.
-  "  http://www.gooogle.com",  // Redirects.
-  "http://www.youtube.com/a/b",
-  "  http://www.youtube.com/a/b?test=1",  // Redirects.
-  "https://www.google.com/",
-  "  https://www.gogle.com",  // Redirects.
-  "http://www.example.com:3141/",
-};
-
-TEST_F(TopSitesCacheTest, GetCanonicalURL) {
-  InitTopSiteCache(kTopSitesSpecBasic, base::size(kTopSitesSpecBasic));
-  struct {
-    const char* expected;
-    const char* query;
-  } test_cases[] = {
-    // Already is canonical: redirects.
-    {"http://www.google.com/", "http://www.google.com"},
-    // Exact match with stored URL: redirects.
-    {"http://www.google.com/", "http://www.gooogle.com"},
-    // Recognizes despite trailing "/": redirects
-    {"http://www.google.com/", "http://www.gooogle.com/"},
-    // Exact match with URL with query: redirects.
-    {"http://www.youtube.com/a/b", "http://www.youtube.com/a/b?test=1"},
-    // No match with URL with query: as-is.
-    {"http://www.youtube.com/a/b?test", "http://www.youtube.com/a/b?test"},
-    // Never-seen-before URL: as-is.
-    {"http://maps.google.com/", "http://maps.google.com/"},
-    // Changing port number, does not match: as-is.
-    {"http://www.example.com:1234/", "http://www.example.com:1234"},
-    // Smart enough to know that port 80 is HTTP: redirects.
-    {"http://www.google.com/", "http://www.gooogle.com:80"},
-    // Prefix should not work: as-is.
-    {"http://www.youtube.com/a", "http://www.youtube.com/a"},
-  };
-  for (size_t i = 0; i < base::size(test_cases); ++i) {
-    std::string expected(test_cases[i].expected);
-    std::string query(test_cases[i].query);
-    EXPECT_EQ(expected, cache_.GetCanonicalURL(GURL(query)).spec())
-      << " for test_case[" << i << "]";
-  }
-}
-
-TEST_F(TopSitesCacheTest, IsKnownUrl) {
-  InitTopSiteCache(kTopSitesSpecBasic, base::size(kTopSitesSpecBasic));
-  // Matches.
-  EXPECT_TRUE(cache_.IsKnownURL(GURL("http://www.google.com")));
-  EXPECT_TRUE(cache_.IsKnownURL(GURL("http://www.gooogle.com")));
-  EXPECT_TRUE(cache_.IsKnownURL(GURL("http://www.google.com/")));
-
-  // Non-matches.
-  EXPECT_FALSE(cache_.IsKnownURL(GURL("http://www.google.com?")));
-  EXPECT_FALSE(cache_.IsKnownURL(GURL("http://www.google.net")));
-  EXPECT_FALSE(cache_.IsKnownURL(GURL("http://www.google.com/stuff")));
-  EXPECT_FALSE(cache_.IsKnownURL(GURL("https://www.gooogle.com")));
-  EXPECT_FALSE(cache_.IsKnownURL(GURL("http://www.youtube.com/a")));
-}
-
-}  // namespace
-
-}  // namespace history

components/history/core/browser/top_sites_impl.cc

@@ -7,7 +7,6 @@
 #include <stdint.h>
 #include <algorithm>
 #include <memory>
-#include <set>
 #include <utility>
 
 #include "base/bind.h"
@@ -28,12 +27,12 @@
 #include "components/history/core/browser/history_db_task.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/page_usage_data.h"
-#include "components/history/core/browser/top_sites_cache.h"
 #include "components/history/core/browser/top_sites_observer.h"
 #include "components/history/core/browser/url_utils.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
+#include "url/gurl.h"
 
 namespace history {
 namespace {
@@ -91,8 +90,6 @@ TopSitesImpl::TopSitesImpl(PrefService* pref_service,
                            const PrepopulatedPageList& prepopulated_pages,
                            const CanAddURLToHistoryFn& can_add_url_to_history)
     : backend_(nullptr),
-      cache_(std::make_unique<TopSitesCache>()),
-      thread_safe_cache_(std::make_unique<TopSitesCache>()),
       prepopulated_pages_(prepopulated_pages),
       pref_service_(pref_service),
       history_service_(history_service),
@@ -128,7 +125,7 @@ void TopSitesImpl::GetMostVisitedURLs(
           base::RetainedRef(base::ThreadTaskRunnerHandle::Get()), callback));
       return;
     }
-    filtered_urls = thread_safe_cache_->top_sites();
+    filtered_urls = thread_safe_cache_;
   }
   callback.Run(filtered_urls);
 }
@@ -197,12 +194,8 @@ void TopSitesImpl::ClearBlacklistedURLs() {
   NotifyTopSitesChanged(TopSitesObserver::ChangeReason::BLACKLIST);
 }
 
-bool TopSitesImpl::IsKnownURL(const GURL& url) {
-  return loaded_ && cache_->IsKnownURL(url);
-}
-
 bool TopSitesImpl::IsFull() {
-  return loaded_ && cache_->GetNumURLs() >= kTopSitesNumber;
+  return loaded_ && top_sites_.size() >= kTopSitesNumber;
 }
 
 PrepopulatedPageList TopSitesImpl::GetPrepopulatedPages() {
@@ -349,7 +342,7 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites,
   AddPrepopulatedPages(&top_sites);
 
   TopSitesDelta delta;
-  DiffMostVisited(cache_->top_sites(), top_sites, &delta);
+  DiffMostVisited(top_sites_, top_sites, &delta);
 
   TopSitesBackend::RecordHistogram record_or_not =
       TopSitesBackend::RECORD_HISTOGRAM_NO;
@@ -377,12 +370,12 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites,
   // If there is no url change in top sites, check if the titles have changes.
   // Notify observers if there's a change in titles.
   if (!should_notify_observers)
-    should_notify_observers = DoTitlesDiffer(cache_->top_sites(), top_sites);
+    should_notify_observers = DoTitlesDiffer(top_sites_, 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.
-  cache_->SetTopSites(top_sites);
+  top_sites_ = top_sites;
 
   ResetThreadSafeCache();
 
@@ -413,7 +406,7 @@ void TopSitesImpl::MoveStateToLoaded() {
     // Now that we're loaded we can service the queued up callbacks. Copy them
     // here and service them outside the lock.
     if (!pending_callbacks_.empty()) {
-      urls = thread_safe_cache_->top_sites();
+      urls = thread_safe_cache_;
       pending_callbacks.swap(pending_callbacks_);
     }
   }
@@ -429,9 +422,9 @@ void TopSitesImpl::MoveStateToLoaded() {
 
 void TopSitesImpl::ResetThreadSafeCache() {
   base::AutoLock lock(lock_);
-  MostVisitedURLList cached;
-  ApplyBlacklist(cache_->top_sites(), &cached);
-  thread_safe_cache_->SetTopSites(cached);
+  MostVisitedURLList filtered;
+  ApplyBlacklist(top_sites_, &filtered);
+  thread_safe_cache_ = filtered;
 }
 
 void TopSitesImpl::ScheduleUpdateTimer() {
@@ -446,9 +439,8 @@ void TopSitesImpl::OnGotMostVisitedURLs(
     const scoped_refptr<MostVisitedThreadSafe>& sites) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // Set the top sites directly in the cache so that SetTopSites diffs
-  // correctly.
-  cache_->SetTopSites(sites->data);
+  // Set |top_sites_| directly so that SetTopSites() diffs correctly.
+  top_sites_ = sites->data;
   SetTopSites(sites->data, CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_URLS);
 
   MoveStateToLoaded();
@@ -470,22 +462,6 @@ void TopSitesImpl::OnURLsDeleted(HistoryService* history_service,
   if (deletion_info.IsAllHistory()) {
     SetTopSites(MostVisitedURLList(), CALL_LOCATION_FROM_OTHER_PLACES);
     backend_->ResetDatabase();
-  } else {
-    std::set<size_t> indices_to_delete;  // Indices into top_sites_.
-    for (const auto& row : deletion_info.deleted_rows()) {
-      if (cache_->IsKnownURL(row.url()))
-        indices_to_delete.insert(cache_->GetURLIndex(row.url()));
-    }
-
-    if (indices_to_delete.empty())
-      return;
-
-    MostVisitedURLList new_top_sites(cache_->top_sites());
-    for (auto i = indices_to_delete.rbegin(); i != indices_to_delete.rend();
-         i++) {
-      new_top_sites.erase(new_top_sites.begin() + *i);
-    }
-    SetTopSites(new_top_sites, CALL_LOCATION_FROM_OTHER_PLACES);
   }
   StartQueryForMostVisited();
 }

components/history/core/browser/top_sites_impl.h

@@ -7,9 +7,6 @@
 
 #include <stddef.h>
 
-#include <list>
-#include <memory>
-#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -27,8 +24,6 @@
 #include "components/history/core/browser/history_types.h"
 #include "components/history/core/browser/top_sites.h"
 #include "components/history/core/browser/top_sites_backend.h"
-#include "third_party/skia/include/core/SkColor.h"
-#include "url/gurl.h"
 
 class PrefRegistrySimple;
 class PrefService;
@@ -40,7 +35,6 @@ class FilePath;
 namespace history {
 
 class HistoryService;
-class TopSitesCache;
 class TopSitesImplTest;
 
 // This class allows requests for most visited urls on any thread. All other
@@ -72,7 +66,6 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver {
   void RemoveBlacklistedURL(const GURL& url) override;
   bool IsBlacklisted(const GURL& url) override;
   void ClearBlacklistedURLs() override;
-  bool IsKnownURL(const GURL& url) override;
   bool IsFull() override;
   PrepopulatedPageList GetPrepopulatedPages() override;
   bool loaded() const override;
@@ -174,16 +167,16 @@ class TopSitesImpl : public TopSites, public HistoryServiceObserver {
 
   scoped_refptr<TopSitesBackend> backend_;
 
+  // Lock used to access |thread_safe_cache_|.
+  mutable base::Lock lock_;
+
   // The top sites data.
-  std::unique_ptr<TopSitesCache> cache_;
+  MostVisitedURLList top_sites_;
 
   // Copy of the top sites data that may be accessed on any thread (assuming
   // you hold |lock_|). The data in |thread_safe_cache_| has blacklisted urls
-  // applied (|cache_| does not).
-  std::unique_ptr<TopSitesCache> thread_safe_cache_;
-
-  // Lock used to access |thread_safe_cache_|.
-  mutable base::Lock lock_;
+  // applied (|top_sites_| does not).
+  MostVisitedURLList thread_safe_cache_ GUARDED_BY(lock_);
 
   // Task tracker for history and backend requests.
   base::CancelableTaskTracker cancelable_task_tracker_;

components/history/core/browser/top_sites_impl_unittest.cc

@@ -23,7 +23,6 @@
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/history_types.h"
 #include "components/history/core/browser/top_sites.h"
-#include "components/history/core/browser/top_sites_cache.h"
 #include "components/history/core/browser/top_sites_observer.h"
 #include "components/history/core/browser/visit_delegate.h"
 #include "components/history/core/test/history_service_test_util.h"
@@ -203,10 +202,6 @@ class TopSitesImplTest : public HistoryUnitTestBase {
 
   // Wrappers that allow private TopSites functions to be called from the
   // individual tests without making them all be friends.
-  GURL GetCanonicalURL(const GURL& url) {
-    return top_sites()->cache_->GetCanonicalURL(url);
-  }
-
   void SetTopSites(const MostVisitedURLList& new_top_sites) {
     top_sites()->SetTopSites(new_top_sites,
                              TopSitesImpl::CALL_LOCATION_FROM_OTHER_PLACES);
@@ -222,8 +217,7 @@ class TopSitesImplTest : public HistoryUnitTestBase {
 
   void EmptyThreadSafeCache() {
     base::AutoLock lock(top_sites()->lock_);
-    MostVisitedURLList empty;
-    top_sites()->thread_safe_cache_->SetTopSites(empty);
+    top_sites()->thread_safe_cache_.clear();
   }
 
   void ResetTopSites() {
@@ -308,37 +302,6 @@ void AppendMostVisitedURLwithTitle(const GURL& url,
   list->push_back(mv);
 }
 
-// Tests GetCanonicalURL.
-TEST_F(TopSitesImplTest, GetCanonicalURL) {
-  // Have two chains:
-  //   google.com -> www.google.com
-  //   news.google.com (no redirects)
-  GURL news("http://news.google.com/");
-  GURL source("http://google.com/");
-  GURL dest("http://www.google.com/");
-
-  std::vector<MostVisitedURL> most_visited;
-  AppendMostVisitedURLWithRedirect(source, dest, &most_visited);
-  AppendMostVisitedURL(news, &most_visited);
-  SetTopSites(most_visited);
-
-  // Random URLs not in the database are returned unchanged.
-  GURL result = GetCanonicalURL(GURL("http://fark.com/"));
-  EXPECT_EQ(GURL("http://fark.com/"), result);
-
-  // Easy case, there are no redirects and the exact URL is stored.
-  result = GetCanonicalURL(news);
-  EXPECT_EQ(news, result);
-
-  // The URL in question is the source URL in a redirect list.
-  result = GetCanonicalURL(source);
-  EXPECT_EQ(dest, result);
-
-  // The URL in question is the destination of a redirect.
-  result = GetCanonicalURL(dest);
-  EXPECT_EQ(dest, result);
-}
-
 class MockTopSitesObserver : public TopSitesObserver {
  public:
   MockTopSitesObserver() {}
@@ -634,6 +597,9 @@ TEST_F(TopSitesImplTest, DeleteNotifications) {
 
   // Wait for history to process the deletion.
   WaitForHistory();
+  // The deletion called back to TopSitesImpl (on the main thread), which
+  // triggers a history query. Wait for that to complete.
+  WaitForHistory();
 
   {
     TopSitesQuerier querier;
@@ -660,6 +626,9 @@ TEST_F(TopSitesImplTest, DeleteNotifications) {
 
   // Wait for history to process the deletion.
   WaitForHistory();
+  // The deletion called back to TopSitesImpl (on the main thread), which
+  // triggers a history query. Wait for that to complete.
+  WaitForHistory();
 
   {
     TopSitesQuerier querier;

components/omnibox/browser/zero_suggest_provider_unittest.cc

@@ -52,9 +52,6 @@ class FakeEmptyTopSites : public history::TopSites {
     return false;
   }
   void ClearBlacklistedURLs() override {}
-  bool IsKnownURL(const GURL& url) override {
-    return false;
-  }
   bool IsFull() override { return false; }
   bool loaded() const override {
     return false;