Eliminate sending NOTIFICATION_TOP_SITES_* from TopSites

chrome/browser/history/top_sites_impl_unittest.cc

Index: chrome/browser/history/top_sites_impl_unittest.cc
diff --git a/chrome/browser/history/top_sites_impl_unittest.cc b/chrome/browser/history/top_sites_impl_unittest.cc
index 044f3bc2eeb80839338cf3ff56497ae1b95c572f..ce50a07c312d968c5e582dc1317d73daaf26bf14 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,43 @@
 
 using content::BrowserThread;
 
+class MockTopSitesObserver : public TopSitesObserver {

[sdefresne, 2014/08/04 08:47:18]

nit: Rename MockTopSitesObserver to TestTopSitesObserver (you're not mocking
anything using the gmock framework, so the name MockTopSitesObserver is
misleading).

[nshaik, 2014/08/05 06:38:03]

Done.

+ public:
+  explicit MockTopSitesObserver(history::TopSites* top_sites);
+  virtual ~MockTopSitesObserver();
+  // TopSitesObserver:
+  virtual void TopSitesLoaded(history::TopSites* top_sites) OVERRIDE;
+  virtual void TopSitesChanged(history::TopSites* top_sites) OVERRIDE;
+
+ private:
+  history::TopSites* top_sites_;
+};
+
+MockTopSitesObserver::~MockTopSitesObserver() {
+  if (top_sites_)

[sdefresne, 2014/08/04 08:47:19]

Remove the condition, since the observer must be created with a non-NULL
pointer.

[nshaik, 2014/08/05 06:38:04]

Done.

+    top_sites_->RemoveObserver(this);
+}
+
+MockTopSitesObserver::MockTopSitesObserver(history::TopSites* top_sites)

[sdefresne, 2014/08/04 08:47:18]

Add the Profile as additional parameter to this constructor.

[nshaik, 2014/08/05 06:38:03]

Done.

+    : top_sites_(top_sites) {
+  if (top_sites_)

[sdefresne, 2014/08/04 08:47:19]

Change to DCHECK(top_sites_) as there is no point creating this observer without
a TopSites instance.

[nshaik, 2014/08/05 06:38:03]

Done.

+    top_sites_->AddObserver(this);
+}
+
+void MockTopSitesObserver::TopSitesLoaded(history::TopSites* top_sites) {
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_TOP_SITES_LOADED,
+      content::Source<Profile>(top_sites->GetProfile()),
+      content::Details<history::TopSites>(top_sites));
+}
+
+void MockTopSitesObserver::TopSitesChanged(history::TopSites* top_sites) {
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_TOP_SITES_CHANGED,
+      content::Source<Profile>(top_sites->GetProfile()),
+      content::NotificationService::NoDetails());
+}
+
 namespace history {
 
 namespace {
@@ -140,12 +181,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 +323,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 +368,21 @@ class TopSitesImplTest : public HistoryUnitTestBase {
     top_sites()->thread_safe_cache_->SetTopSites(empty);
   }
 
+  void CreateTopSitesAndObserver() {
+    if (top_sites_observer_)

[sdefresne, 2014/08/04 08:47:19]

Change to DCHECK(!top_sites_observer_) as the object should have been destroyed
by now (otherwise it may refer to a TopSites* that is dangling).

[nshaik, 2014/08/05 06:38:04]

Actually the object will not be destroyed.
profile()->CreateTopSites() destroys TopSites and not TopSitesObserver; making 
top_sites_observer_ refer to dangling TopSites.
So before CreateTopSites is called I am destroying the top_sites_observer_ in
case it is added as an observer, it will be removed.
Is there a better way to do this without changing profile()->CreateTopSites()?

On 2014/08/04 08:47:19, sdefresne wrote:

+      top_sites_observer_.reset();
+
+    profile_->CreateTopSites();
+    top_sites_observer_.reset(
+        new MockTopSitesObserver(profile_->GetTopSites()));
+  }
+
  private:
   base::MessageLoopForUI message_loop_;
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread db_thread_;
   scoped_ptr<TestingProfile> profile_;
-
+  scoped_ptr<MockTopSitesObserver> top_sites_observer_;
   // To cancel HistoryService tasks.
   base::CancelableTaskTracker history_tracker_;
 
@@ -982,7 +1033,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 +1074,7 @@ TEST_F(TopSitesImplTest, NotifyCallbacksWhenLoaded) {
   SetTopSites(pages);
 
   // Recreate top sites. It won't be loaded now.
-  profile()->CreateTopSites();
+  CreateTopSitesAndObserver();
 
   EXPECT_FALSE(IsTopSitesLoaded());
 
@@ -1068,7 +1119,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());