Eliminate sending NOTIFICATION_TOP_SITES_* from TopSites

Add new interface TopSitesObserver.
- add TopSitesLoaded method to TopSitesObserver
- add TopSitesChanged method to TopSitesObserver
- Make ChromeHistoryClient a TopSitesObserver.
- Invoke the observer methods and ChromeHistoryClient will send the notification

BUG=373329
TEST=unit_tests --gest_filter=TopSites*,ExpireHistoryTest*

Review URL: https://codereview.chromium.org/441623002

Cr-Commit-Position: refs/heads/master@{#295398}

12 files changed, 208 insertions, 32 deletions

diff --git a/chrome/browser/history/chrome_history_client.cc b/chrome/browser/history/chrome_history_client.cc
index de8da22..ea979be 100644
--- a/chrome/browser/history/chrome_history_client.cc
+++ b/chrome/browser/history/chrome_history_client.cc
@@ -5,15 +5,30 @@
 #include "chrome/browser/history/chrome_history_client.h"
 
 #include "base/logging.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/history/history_notifications.h"
+#include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/ui/profile_error_dialog.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "content/public/browser/notification_service.h"
 
-ChromeHistoryClient::ChromeHistoryClient(BookmarkModel* bookmark_model)
-    : bookmark_model_(bookmark_model) {
+ChromeHistoryClient::ChromeHistoryClient(BookmarkModel* bookmark_model,
+                                         Profile* profile,
+                                         history::TopSites* top_sites)
+    : bookmark_model_(bookmark_model),
+      profile_(profile),
+      top_sites_(top_sites) {
   DCHECK(bookmark_model_);
+  if (top_sites_)
+    top_sites_->AddObserver(this);
+}
+
+ChromeHistoryClient::~ChromeHistoryClient() {
+  if (top_sites_)
+    top_sites_->RemoveObserver(this);
 }
 
 void ChromeHistoryClient::BlockUntilBookmarksLoaded() {
@@ -65,3 +80,17 @@ void ChromeHistoryClient::Shutdown() {
   // sees an incorrect view of bookmarks, but it's better than a deadlock.
   bookmark_model_->Shutdown();
 }
+
+void ChromeHistoryClient::TopSitesLoaded(history::TopSites* top_sites) {
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_TOP_SITES_LOADED,
+      content::Source<Profile>(profile_),
+      content::Details<history::TopSites>(top_sites));
+}
+
+void ChromeHistoryClient::TopSitesChanged(history::TopSites* top_sites) {
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_TOP_SITES_CHANGED,
+      content::Source<history::TopSites>(top_sites),
+      content::NotificationService::NoDetails());
+}
diff --git a/chrome/browser/history/chrome_history_client.h b/chrome/browser/history/chrome_history_client.h
index 1a02506..92f9186 100644
--- a/chrome/browser/history/chrome_history_client.h
+++ b/chrome/browser/history/chrome_history_client.h
@@ -7,14 +7,24 @@
 
 #include "base/macros.h"
 #include "components/history/core/browser/history_client.h"
+#include "components/history/core/browser/top_sites_observer.h"
 
 class BookmarkModel;
+class Profile;
+
+namespace history {
+class TopSites;
+}
 
 // This class implements history::HistoryClient to abstract operations that
 // depend on Chrome environment.
-class ChromeHistoryClient : public history::HistoryClient {
+class ChromeHistoryClient : public history::HistoryClient,
+                            public history::TopSitesObserver {
  public:
-  explicit ChromeHistoryClient(BookmarkModel* bookmark_model);
+  explicit ChromeHistoryClient(BookmarkModel* bookmark_model,
+                               Profile* profile,
+                               history::TopSites* top_sites);
+  virtual ~ChromeHistoryClient();
 
   // history::HistoryClient:
   virtual void BlockUntilBookmarksLoaded() OVERRIDE;
@@ -27,9 +37,25 @@ class ChromeHistoryClient : public history::HistoryClient {
   // KeyedService:
   virtual void Shutdown() OVERRIDE;
 
+  // TopSitesObserver:
+  virtual void TopSitesLoaded(history::TopSites* top_sites) OVERRIDE;
+  virtual void TopSitesChanged(history::TopSites* top_sites) OVERRIDE;
+
  private:
   // The BookmarkModel, this should outlive ChromeHistoryClient.
   BookmarkModel* bookmark_model_;
+  Profile* profile_;
+  // The TopSites object is owned by the Profile (see
+  // chrome/browser/profiles/profile_impl.h)
+  // and lazily constructed by the  getter.
+  // ChromeHistoryClient is a KeyedService linked to the Profile lifetime by the
+  // ChromeHistoryClientFactory (which is a BrowserContextKeyedServiceFactory).
+  // Before the Profile is destroyed, all the KeyedService Shutdown methods are
+  // called, and the Profile is fully constructed before any of the KeyedService
+  // can  be constructed. The TopSites does not use the HistoryService nor the
+  // HistoryClient during construction (it uses it later, but supports getting
+  // an NULL  pointer).
+  history::TopSites* top_sites_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeHistoryClient);
 };
diff --git a/chrome/browser/history/chrome_history_client_factory.cc b/chrome/browser/history/chrome_history_client_factory.cc
index b56a898..f612a18 100644
--- a/chrome/browser/history/chrome_history_client_factory.cc
+++ b/chrome/browser/history/chrome_history_client_factory.cc
@@ -35,8 +35,10 @@ ChromeHistoryClientFactory::~ChromeHistoryClientFactory() {
 
 KeyedService* ChromeHistoryClientFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  return new ChromeHistoryClient(
-      BookmarkModelFactory::GetForProfile(static_cast<Profile*>(context)));
+  Profile* profile = static_cast<Profile*>(context);
+  return new ChromeHistoryClient(BookmarkModelFactory::GetForProfile(profile),
+                                 profile,
+                                 profile->GetTopSites());
 }
 
 content::BrowserContext* ChromeHistoryClientFactory::GetBrowserContextToUse(
diff --git a/chrome/browser/history/top_sites.cc b/chrome/browser/history/top_sites.cc
index b37271e..b06bf64 100644
--- a/chrome/browser/history/top_sites.cc
+++ b/chrome/browser/history/top_sites.cc
@@ -37,4 +37,26 @@ TopSites* TopSites::Create(Profile* profile, const base::FilePath& db_name) {
   return top_sites_impl;
 }
 
+TopSites::TopSites() {
+}
+
+TopSites::~TopSites() {
+}
+
+void TopSites::AddObserver(TopSitesObserver* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void TopSites::RemoveObserver(TopSitesObserver* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
+void TopSites::NotifyTopSitesLoaded() {
+  FOR_EACH_OBSERVER(TopSitesObserver, observer_list_, TopSitesLoaded(this));
+}
+
+void TopSites::NotifyTopSitesChanged() {
+  FOR_EACH_OBSERVER(TopSitesObserver, observer_list_, TopSitesChanged(this));
+}
+
 }  // namespace history
diff --git a/chrome/browser/history/top_sites.h b/chrome/browser/history/top_sites.h
index 47ed7de..d8f9c29 100644
--- a/chrome/browser/history/top_sites.h
+++ b/chrome/browser/history/top_sites.h
@@ -11,6 +11,7 @@
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_types.h"
+#include "components/history/core/browser/top_sites_observer.h"
 #include "components/history/core/common/thumbnail_score.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/image/image.h"
@@ -38,7 +39,7 @@ class TopSites
     : public base::RefCountedThreadSafe<TopSites>,
       public content::NotificationObserver {
  public:
-  TopSites() {}
+  TopSites();
 
   // Initializes TopSites.
   static TopSites* Create(Profile* profile, const base::FilePath& db_name);
@@ -167,10 +168,20 @@ class TopSites
     // The best color to highlight the page (should roughly match favicon).
     SkColor color;
   };
+
+  // Add Observer to the list.
+  void AddObserver(TopSitesObserver* observer);
+
+  // Remove Observer from the list.
+  void RemoveObserver(TopSitesObserver* observer);
+
  protected:
-  virtual ~TopSites() {}
+  void NotifyTopSitesLoaded();
+  void NotifyTopSitesChanged();
+  virtual ~TopSites();
 
  private:
+  ObserverList<TopSitesObserver> observer_list_;
   friend class base::RefCountedThreadSafe<TopSites>;
 };
 
diff --git a/chrome/browser/history/top_sites_impl.cc b/chrome/browser/history/top_sites_impl.cc
index d14e387..fe5ce18 100644
--- a/chrome/browser/history/top_sites_impl.cc
+++ b/chrome/browser/history/top_sites_impl.cc
@@ -874,10 +874,7 @@ void TopSitesImpl::MoveStateToLoaded() {
   for (size_t i = 0; i < pending_callbacks.size(); i++)
     pending_callbacks[i].Run(filtered_urls_all, filtered_urls_nonforced);
 
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_TOP_SITES_LOADED,
-      content::Source<Profile>(profile_),
-      content::Details<TopSites>(this));
+  NotifyTopSitesLoaded();
 }
 
 void TopSitesImpl::ResetThreadSafeCache() {
@@ -892,13 +889,6 @@ void TopSitesImpl::ResetThreadSafeImageCache() {
   thread_safe_cache_->SetThumbnails(cache_->images());
 }
 
-void TopSitesImpl::NotifyTopSitesChanged() {
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_TOP_SITES_CHANGED,
-      content::Source<TopSites>(this),
-      content::NotificationService::NoDetails());
-}
-
 void TopSitesImpl::RestartQueryForTopSitesTimer(base::TimeDelta delta) {
   if (timer_.IsRunning() && ((timer_start_time_ + timer_.GetCurrentDelay()) <
                              (base::TimeTicks::Now() + delta))) {
diff --git a/chrome/browser/history/top_sites_impl.h b/chrome/browser/history/top_sites_impl.h
index d1a7c91e..23702e0 100644
--- a/chrome/browser/history/top_sites_impl.h
+++ b/chrome/browser/history/top_sites_impl.h
@@ -205,8 +205,6 @@ class TopSitesImpl : public TopSites {
 
   void ResetThreadSafeImageCache();
 
-  void NotifyTopSitesChanged();
-
   // Stops and starts timer with a delay of |delta|.
   void RestartQueryForTopSitesTimer(base::TimeDelta delta);
 
diff --git a/chrome/browser/history/top_sites_impl_unittest.cc b/chrome/browser/history/top_sites_impl_unittest.cc
index 044f3bc..9cd07aa 100644
--- a/chrome/browser/history/top_sites_impl_unittest.cc
+++ b/chrome/browser/history/top_sites_impl_unittest.cc
@@ -8,15 +8,19 @@
 #include "base/path_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/cancelable_task_tracker.h"
+#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/history/history_db_task.h"
+#include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/history/history_unittest_base.h"
+#include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/history/top_sites_cache.h"
 #include "chrome/browser/history/top_sites_impl.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -25,6 +29,44 @@
 
 using content::BrowserThread;
 
+class TestTopSitesObserver : public history::TopSitesObserver {
+ public:
+  explicit TestTopSitesObserver(Profile* profile, history::TopSites* top_sites);
+  virtual ~TestTopSitesObserver();
+  // TopSitesObserver:
+  virtual void TopSitesLoaded(history::TopSites* top_sites) OVERRIDE;
+  virtual void TopSitesChanged(history::TopSites* top_sites) OVERRIDE;
+
+ private:
+  Profile* profile_;
+  history::TopSites* top_sites_;
+};
+
+TestTopSitesObserver::~TestTopSitesObserver() {
+  top_sites_->RemoveObserver(this);
+}
+
+TestTopSitesObserver::TestTopSitesObserver(Profile* profile,
+                                           history::TopSites* top_sites)
+    : profile_(profile), top_sites_(top_sites) {
+  DCHECK(top_sites_);
+  top_sites_->AddObserver(this);
+}
+
+void TestTopSitesObserver::TopSitesLoaded(history::TopSites* top_sites) {
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_TOP_SITES_LOADED,
+      content::Source<Profile>(profile_),
+      content::Details<history::TopSites>(top_sites));
+}
+
+void TestTopSitesObserver::TopSitesChanged(history::TopSites* top_sites) {
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_TOP_SITES_CHANGED,
+      content::Source<Profile>(profile_),
+      content::NotificationService::NoDetails());
+}
+
 namespace history {
 
 namespace {
@@ -140,12 +182,13 @@ class TopSitesImplTest : public HistoryUnitTestBase {
     profile_.reset(new TestingProfile);
     if (CreateHistoryAndTopSites()) {
       ASSERT_TRUE(profile_->CreateHistoryService(false, false));
-      profile_->CreateTopSites();
+      CreateTopSitesAndObserver();
       profile_->BlockUntilTopSitesLoaded();
     }
   }
 
   virtual void TearDown() {
+    top_sites_observer_.reset();
     profile_.reset();
   }
 
@@ -281,7 +324,7 @@ class TopSitesImplTest : public HistoryUnitTestBase {
   // Recreates top sites. This forces top sites to reread from the db.
   void RecreateTopSitesAndBlock() {
     // Recreate TopSites and wait for it to load.
-    profile()->CreateTopSites();
+    CreateTopSitesAndObserver();
     // As history already loaded we have to fake this call.
     profile()->BlockUntilTopSitesLoaded();
   }
@@ -326,12 +369,21 @@ class TopSitesImplTest : public HistoryUnitTestBase {
     top_sites()->thread_safe_cache_->SetTopSites(empty);
   }
 
+  void CreateTopSitesAndObserver() {
+    if (top_sites_observer_)
+      top_sites_observer_.reset();
+
+    profile_->CreateTopSites();
+    top_sites_observer_.reset(
+        new TestTopSitesObserver(profile_.get(), profile_->GetTopSites()));
+  }
+
  private:
   base::MessageLoopForUI message_loop_;
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread db_thread_;
   scoped_ptr<TestingProfile> profile_;
-
+  scoped_ptr<TestTopSitesObserver> top_sites_observer_;
   // To cancel HistoryService tasks.
   base::CancelableTaskTracker history_tracker_;
 
@@ -982,7 +1034,7 @@ TEST_F(TopSitesImplTest, GetUpdateDelay) {
 // has loaded.
 TEST_F(TopSitesImplTest, NotifyCallbacksWhenLoaded) {
   // Recreate top sites. It won't be loaded now.
-  profile()->CreateTopSites();
+  CreateTopSitesAndObserver();
 
   EXPECT_FALSE(IsTopSitesLoaded());
 
@@ -1023,7 +1075,7 @@ TEST_F(TopSitesImplTest, NotifyCallbacksWhenLoaded) {
   SetTopSites(pages);
 
   // Recreate top sites. It won't be loaded now.
-  profile()->CreateTopSites();
+  CreateTopSitesAndObserver();
 
   EXPECT_FALSE(IsTopSitesLoaded());
 
@@ -1068,7 +1120,7 @@ TEST_F(TopSitesImplTest, NotifyCallbacksWhenLoaded) {
 // Makes sure canceled requests are not notified.
 TEST_F(TopSitesImplTest, CancelingRequestsForTopSites) {
   // Recreate top sites. It won't be loaded now.
-  profile()->CreateTopSites();
+  CreateTopSitesAndObserver();
 
   EXPECT_FALSE(IsTopSitesLoaded());
 
diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc
index e652fde..ca36ace 100644
--- a/chrome/test/base/testing_profile.cc
+++ b/chrome/test/base/testing_profile.cc
@@ -54,6 +54,7 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/common/bookmark_constants.h"
+#include "components/history/core/browser/top_sites_observer.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/policy/core/common/policy_service.h"
 #include "components/user_prefs/user_prefs.h"
@@ -106,6 +107,21 @@ using testing::Return;
 
 namespace {
 
+// Used to make sure TopSites has finished loading
+class WaitTopSitesLoadedObserver : public history::TopSitesObserver {
+ public:
+  explicit WaitTopSitesLoadedObserver(content::MessageLoopRunner* runner)
+      : runner_(runner) {}
+  virtual void TopSitesLoaded(history::TopSites* top_sites) OVERRIDE {
+    runner_->Quit();
+  }
+  virtual void TopSitesChanged(history::TopSites* top_sites) OVERRIDE {}
+
+ private:
+  // weak
+  content::MessageLoopRunner* runner_;
+};
+
 // Task used to make sure history has finished processing a request. Intended
 // for use with BlockUntilHistoryProcessesPendingRequests.
 
@@ -543,7 +559,9 @@ static KeyedService* BuildChromeBookmarkClient(
 static KeyedService* BuildChromeHistoryClient(
     content::BrowserContext* context) {
   Profile* profile = static_cast<Profile*>(context);
-  return new ChromeHistoryClient(BookmarkModelFactory::GetForProfile(profile));
+  return new ChromeHistoryClient(BookmarkModelFactory::GetForProfile(profile),
+                                 profile,
+                                 profile->GetTopSites());
 }
 
 void TestingProfile::CreateBookmarkModel(bool delete_file) {
@@ -589,10 +607,12 @@ void TestingProfile::BlockUntilHistoryIndexIsRefreshed() {
 
 // TODO(phajdan.jr): Doesn't this hang if Top Sites are already loaded?
 void TestingProfile::BlockUntilTopSitesLoaded() {
-  content::WindowedNotificationObserver top_sites_loaded_observer(
-      chrome::NOTIFICATION_TOP_SITES_LOADED,
-      content::NotificationService::AllSources());
-  top_sites_loaded_observer.Wait();
+  scoped_refptr<content::MessageLoopRunner> runner =
+      new content::MessageLoopRunner;
+  WaitTopSitesLoadedObserver observer(runner.get());
+  top_sites_->AddObserver(&observer);
+  runner->Run();
+  top_sites_->RemoveObserver(&observer);
 }
 
 void TestingProfile::SetGuestSession(bool guest) {
diff --git a/components/history.gypi b/components/history.gypi
index 868d9fe..e413d34 100644
--- a/components/history.gypi
+++ b/components/history.gypi
@@ -36,6 +36,7 @@
         'history/core/browser/keyword_search_term.h',
         'history/core/browser/page_usage_data.cc',
         'history/core/browser/page_usage_data.h',
+        'history/core/browser/top_sites_observer.h',
         'history/core/browser/url_database.cc',
         'history/core/browser/url_database.h',
         'history/core/browser/url_row.cc',
diff --git a/components/history/core/browser/BUILD.gn b/components/history/core/browser/BUILD.gn
index ed2e9a9..d619649 100644
--- a/components/history/core/browser/BUILD.gn
+++ b/components/history/core/browser/BUILD.gn
@@ -17,6 +17,7 @@ static_library("browser") {
     "keyword_search_term.h",
     "page_usage_data.cc",
     "page_usage_data.h",
+    "top_sites_observer.h",
     "url_database.cc",
     "url_database.h",
     "url_row.cc",
diff --git a/components/history/core/browser/top_sites_observer.h b/components/history/core/browser/top_sites_observer.h
new file mode 100644
index 0000000..3dde7f2
--- /dev/null
+++ b/components/history/core/browser/top_sites_observer.h
@@ -0,0 +1,24 @@
+// Copyright 2014 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_OBSERVER_H_
+#define COMPONENTS_HISTORY_CORE_BROWSER_TOP_SITES_OBSERVER_H_
+
+namespace history {
+class TopSites;
+
+// Interface for observing notifications from TopSites.
+class TopSitesObserver {
+ public:
+  // Is called when TopSites finishes loading.
+  virtual void TopSitesLoaded(history::TopSites* top_sites) = 0;
+
+  // Is called when either one of the most visited urls
+  // changed, or one of the images changes.
+  virtual void TopSitesChanged(history::TopSites* top_sites) = 0;
+};
+
+}  // namespace history
+
+#endif  // COMPONENTS_HISTORY_CORE_BROWSER_TOP_SITES_OBSERVER_H_