Topsites become keyedService based.

chrome/test/base/testing_profile.cc

Index: chrome/test/base/testing_profile.cc
diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc
index d42e0c7e8f8654813d4f04a6639fccd45dbff597..422379cbb6ed97845e262b635b4b264858c08646 100644
--- a/chrome/test/base/testing_profile.cc
+++ b/chrome/test/base/testing_profile.cc
@@ -27,6 +27,8 @@
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/top_sites.h"
+#include "chrome/browser/history/top_sites_impl.h"
+#include "chrome/browser/history/top_sites_factory.h"
 #include "chrome/browser/history/web_history_service_factory.h"
 #include "chrome/browser/net/pref_proxy_config_tracker.h"
 #include "chrome/browser/net/proxy_service_factory.h"
@@ -224,9 +226,10 @@ KeyedService* BuildChromeBookmarkClient(
 KeyedService* BuildChromeHistoryClient(
     content::BrowserContext* context) {
   Profile* profile = static_cast<Profile*>(context);
+  scoped_refptr<history::TopSites> top_sites =
+      TopSitesFactory::GetForProfile(profile);
   return new ChromeHistoryClient(BookmarkModelFactory::GetForProfile(profile),
-                                 profile,
-                                 profile->GetTopSites());
+                                 profile, top_sites);
 }
 
 void TestProfileErrorCallback(WebDataServiceWrapper::ErrorType error_type,
@@ -244,6 +247,14 @@ KeyedService* BuildWebDataService(content::BrowserContext* context) {
       &TestProfileErrorCallback);
 }
 
+scoped_refptr<RefcountedKeyedService> BuildTopSites(
+    content::BrowserContext* profile) {
+  history::TopSitesImpl* top_sites =
+      new history::TopSitesImpl(static_cast<Profile*>(profile));
+  top_sites->Init(profile->GetPath().Append(chrome::kTopSitesFilename));
+  return make_scoped_refptr(top_sites);
+}
+
 }  // namespace
 
 // static
@@ -454,6 +465,8 @@ void TestingProfile::Init() {
       this, CreateTestDesktopNotificationService);
 #endif
 
+  CreateTopSites();

[sdefresne, 2015/01/21 17:57:24]

This is incorrect, we do not want to unconditionally create a "TopSites".

+
 #if defined(ENABLE_SUPERVISED_USERS)
   if (!IsOffTheRecord()) {
     SupervisedUserSettingsService* settings_service =
@@ -499,8 +512,6 @@ TestingProfile::~TestingProfile() {
   if (host_content_settings_map_.get())
     host_content_settings_map_->ShutdownOnUIThread();
 
-  DestroyTopSites();

[sdefresne, 2015/01/21 17:57:24]

Here too we need to cleanup if an instance of "TopSites" has been created,
however, it is not possible to call "TopSitesFactory::GetForProfileIfExists()"
after the call to
"BrowserContextDependencyManager::DestroyBrowserContextServices()" since the
current profile would have been marked as destroyed and you'll hit "DCHECK". So
instead we need to remember whether or not a "TopSites" instance had been create
and conditionally do the cleanup.

-
   if (pref_proxy_config_tracker_.get())
     pref_proxy_config_tracker_->DetachFromPrefService();
   // Failing a post == leaks == heapcheck failure. Make that an immediate test
@@ -571,26 +582,8 @@ void TestingProfile::DestroyHistoryService() {
 }
 
 void TestingProfile::CreateTopSites() {
-  DestroyTopSites();
-  top_sites_ = history::TopSites::Create(
-      this, GetPath().Append(chrome::kTopSitesFilename));
-}
-
-void TestingProfile::SetTopSites(history::TopSites* top_sites) {
-  DestroyTopSites();
-  top_sites_ = top_sites;
-}
-
-void TestingProfile::DestroyTopSites() {
-  if (top_sites_.get()) {

[sdefresne, 2015/01/21 17:57:24]

Once the "TopSites::ShutdownOnUIThread" is correctly called, then you'll see
that tests fails due to concurrent access to the top sites database. This is
caused by improper cleanup of the previous "TopSites" instance before creating
the new one. If the "base::MessageLoop::current()->RunUntilIdle();" call is
removed, then previous "TopSites" won't have the time to cleanup and unlock the
database, and the new one will fails all access to it.

So this method need to be added back but should instead use
"TopSitesFactory::GetForProfileIfExists()" to check if an instance of "TopSites"
has been created or not and should use "TopSitesFactory::SetTestingFactory()"
with a null factory to delete the old instance.

-    top_sites_->Shutdown();
-    top_sites_ = NULL;
-    // TopSitesImpl::Shutdown schedules some tasks (from TopSitesBackend) that
-    // need to be run to properly shutdown. Run all pending tasks now. This is
-    // normally handled by browser_process shutdown.
-    if (base::MessageLoop::current())
-      base::MessageLoop::current()->RunUntilIdle();
-  }
+  // TODO Jitu
+  // TopSitesFactory::GetInstance()->SetTestingFactory(this, BuildTopSites);

[sdefresne, 2015/01/21 17:57:24]

If you comment this line, then the "BuildTopSites" function ends up not being
used which is flagged as a warning, and since warning are considered error will
cause the build to fail. Please either keep the code or remove it when
uploading, but first run the tests locally before committing to an
implementation.

 }
 
 void TestingProfile::CreateBookmarkModel(bool delete_file) {
@@ -635,9 +628,11 @@ void TestingProfile::BlockUntilTopSitesLoaded() {
   scoped_refptr<content::MessageLoopRunner> runner =
       new content::MessageLoopRunner;
   WaitTopSitesLoadedObserver observer(runner.get());
-  top_sites_->AddObserver(&observer);
+  scoped_refptr<history::TopSites> top_sites =
+      TopSitesFactory::GetForProfile(this);
+  top_sites->AddObserver(&observer);
   runner->Run();
-  top_sites_->RemoveObserver(&observer);
+  top_sites->RemoveObserver(&observer);
 }
 
 void TestingProfile::SetGuestSession(bool guest) {
@@ -795,14 +790,6 @@ PrefService* TestingProfile::GetPrefs() {
   return prefs_.get();
 }
 
-history::TopSites* TestingProfile::GetTopSites() {
-  return top_sites_.get();
-}
-
-history::TopSites* TestingProfile::GetTopSitesWithoutCreating() {
-  return top_sites_.get();
-}
-
 DownloadManagerDelegate* TestingProfile::GetDownloadManagerDelegate() {
   return NULL;
 }