Eliminate sending NOTIFICATION_TOP_SITES_* from TopSites

chrome/browser/history/top_sites_impl_unittest.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/bind.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #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"
 #include "ui/gfx/codec/jpeg_codec.h"
 #include "url/gurl.h"
 
 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 {
 
 // Used by WaitForHistory, see it for details.
 class WaitForHistoryTask : public HistoryDBTask {
  public:
   WaitForHistoryTask() {}
 
   virtual bool RunOnDBThread(HistoryBackend* backend,
@@ skipping 95 matching lines @@
  public:
   TopSitesImplTest()
       : ui_thread_(BrowserThread::UI, &message_loop_),
         db_thread_(BrowserThread::DB, &message_loop_) {
   }
 
   virtual void SetUp() {
     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();
   }
 
   // Returns true if history and top sites should be created in SetUp.
   virtual bool CreateHistoryAndTopSites() {
     return true;
   }
 
   // Gets the thumbnail for |url| from TopSites.
   SkBitmap GetThumbnail(const GURL& url) {
@@ skipping 115 matching lines @@
   bool ThumbnailEqualsBytes(const gfx::Image& image,
                             base::RefCountedMemory* bytes) {
     scoped_refptr<base::RefCountedBytes> encoded_image;
     TopSitesImpl::EncodeBitmap(image, &encoded_image);
     return ThumbnailsAreEqual(encoded_image.get(), bytes);
   }
 
   // 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();
   }
 
   // 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);
   }
 
@@ skipping 24 matching lines @@
   bool AddPrepopulatedPages(MostVisitedURLList* urls) {
     return top_sites()->AddPrepopulatedPages(urls, 0u);
   }
 
   void EmptyThreadSafeCache() {
     base::AutoLock lock(top_sites()->lock_);
     MostVisitedURLList empty;
     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_;
 
   // To cancel TopSitesBackend tasks.
   base::CancelableTaskTracker top_sites_tracker_;
 
   DISALLOW_COPY_AND_ASSIGN(TopSitesImplTest);
 };  // Class TopSitesImplTest
 
 // Helper function for appending a URL to a vector of "most visited" URLs,
@@ skipping 630 matching lines @@
   EXPECT_EQ(52, GetUpdateDelay().InMinutes());
 
   SetLastNumUrlsChanged(20);
   EXPECT_EQ(1, GetUpdateDelay().InMinutes());
 }
 
 // Verifies that callbacks are notified correctly if requested before top sites
 // has loaded.
 TEST_F(TopSitesImplTest, NotifyCallbacksWhenLoaded) {
   // Recreate top sites. It won't be loaded now.
-  profile()->CreateTopSites();
+  CreateTopSitesAndObserver();
 
   EXPECT_FALSE(IsTopSitesLoaded());
 
   TopSitesQuerier querier1;
   TopSitesQuerier querier2;
   TopSitesQuerier querier3;
 
   // Starts the queries.
   querier1.QueryTopSites(top_sites(), false);
   querier2.QueryTopSites(top_sites(), false);
@@ skipping 20 matching lines @@
   MostVisitedURL url;
   url.url = GURL("http://1.com/");
   url.redirects.push_back(url.url);
   pages.push_back(url);
   url.url = GURL("http://2.com/");
   url.redirects.push_back(url.url);
   pages.push_back(url);
   SetTopSites(pages);
 
   // Recreate top sites. It won't be loaded now.
-  profile()->CreateTopSites();
+  CreateTopSitesAndObserver();
 
   EXPECT_FALSE(IsTopSitesLoaded());
 
   TopSitesQuerier querier4;
 
   // Query again.
   querier4.QueryTopSites(top_sites(), false);
 
   // We shouldn't have gotten a callback.
   EXPECT_EQ(0, querier4.number_of_callbacks());
@@ skipping 24 matching lines @@
   ASSERT_EQ(3u + GetPrepopulatePages().size(), querier5.urls().size());
   EXPECT_EQ("http://1.com/", querier5.urls()[0].url.spec());
   EXPECT_EQ("http://2.com/", querier5.urls()[1].url.spec());
   EXPECT_EQ("http://3.com/", querier5.urls()[2].url.spec());
   ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier5, 3));
 }
 
 // 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());
 
   TopSitesQuerier querier1;
   TopSitesQuerier querier2;
 
   // Starts the queries.
   querier1.QueryTopSites(top_sites(), false);
   querier2.QueryTopSites(top_sites(), false);
 
@@ skipping 485 matching lines @@
   ASSERT_EQ(orig_thumbnail.getSize(), thumbnail.getSize());
   orig_thumbnail.lockPixels();
   thumbnail.lockPixels();
   EXPECT_EQ(0, memcmp(orig_thumbnail.getPixels(), thumbnail.getPixels(),
                       orig_thumbnail.getSize()));
   thumbnail.unlockPixels();
   orig_thumbnail.unlockPixels();
 }
 
 }  // namespace history