Topsites become keyedService based.

chrome/test/base/testing_profile.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 "chrome/test/base/testing_profile.h"
 
 #include "base/base_paths.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/path_service.h"
 #include "base/prefs/testing_pref_store.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/chrome_bookmark_client.h"
 #include "chrome/browser/bookmarks/chrome_bookmark_client_factory.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/favicon/chrome_favicon_client_factory.h"
 #include "chrome/browser/favicon/favicon_service.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/chrome_history_client.h"
 #include "chrome/browser/history/chrome_history_client_factory.h"
 #include "chrome/browser/history/history_backend.h"
 #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"
 #include "chrome/browser/notifications/desktop_notification_service.h"
 #include "chrome/browser/notifications/desktop_notification_service_factory.h"
 #include "chrome/browser/policy/profile_policy_connector.h"
 #include "chrome/browser/policy/profile_policy_connector_factory.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/browser/prerender/prerender_manager.h"
@@ skipping 177 matching lines @@
 }
 
 KeyedService* BuildChromeBookmarkClient(
     content::BrowserContext* context) {
   return new ChromeBookmarkClient(static_cast<Profile*>(context));
 }
 
 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,
                               sql::InitStatus status) {
   NOTREACHED();
 }
 
 KeyedService* BuildWebDataService(content::BrowserContext* context) {
   const base::FilePath& context_path = context->GetPath();
   return new WebDataServiceWrapper(
       context_path, g_browser_process->GetApplicationLocale(),
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI),
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::DB),
       sync_start_util::GetFlareForSyncableService(context_path),
       &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
 #if defined(OS_CHROMEOS)
 // Must be kept in sync with
 // ChromeBrowserMainPartsChromeos::PreEarlyInitialization.
 const char TestingProfile::kTestUserProfileDir[] = "test-user";
 #else
 const char TestingProfile::kTestUserProfileDir[] = "Default";
 #endif
@@ skipping 190 matching lines @@
 
   browser_context_dependency_manager_->CreateBrowserContextServicesForTest(
       this);
 
 #if defined(ENABLE_NOTIFICATIONS)
   // Install profile keyed service factory hooks for dummy/test services
   DesktopNotificationServiceFactory::GetInstance()->SetTestingFactory(
       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 =
         SupervisedUserSettingsServiceFactory::GetForProfile(this);
     TestingPrefStore* store = new TestingPrefStore();
     settings_service->Init(store);
     store->SetInitializationCompleted();
   }
 #endif
 
@@ skipping 25 matching lines @@
   // Any objects holding live URLFetchers should be deleted before teardown.
   TemplateURLFetcherFactory::ShutdownForProfile(this);
 
   MaybeSendDestroyedNotification();
 
   browser_context_dependency_manager_->DestroyBrowserContextServices(this);
 
   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
   // failure.
   if (resource_context_) {
     CHECK(BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE,
                                     resource_context_));
     resource_context_ = NULL;
     content::RunAllPendingInMessageLoop(BrowserThread::IO);
   }
@@ skipping 50 matching lines @@
   base::MessageLoop::current()->Run();
 
   // Make sure we don't have any event pending that could disrupt the next
   // test.
   base::MessageLoop::current()->PostTask(FROM_HERE,
                                          base::MessageLoop::QuitClosure());
   base::MessageLoop::current()->Run();
 }
 
 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) {
   if (delete_file) {
     base::FilePath path = GetPath().Append(bookmarks::kBookmarksFileName);
     base::DeleteFile(path, false);
   }
   ChromeHistoryClientFactory::GetInstance()->SetTestingFactory(
       this, BuildChromeHistoryClient);
   ChromeBookmarkClientFactory::GetInstance()->SetTestingFactory(
@@ skipping 24 matching lines @@
   run_loop.Run();
   index->set_restore_cache_observer(NULL);
   DCHECK(index->restored());
 }
 
 // TODO(phajdan.jr): Doesn't this hang if Top Sites are already loaded?
 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) {
   guest_session_ = guest;
 }
 
 base::FilePath TestingProfile::GetPath() const {
   return profile_path_;
 }
 
@@ skipping 137 matching lines @@
       this, profile_policy_connector_.get());
   CHECK_EQ(profile_policy_connector_.get(),
            policy::ProfilePolicyConnectorFactory::GetForProfile(this));
 }
 
 PrefService* TestingProfile::GetPrefs() {
   DCHECK(prefs_);
   return prefs_.get();
 }
 
-history::TopSites* TestingProfile::GetTopSites() {
-  return top_sites_.get();
-}
-
-history::TopSites* TestingProfile::GetTopSitesWithoutCreating() {
-  return top_sites_.get();
-}
-
 DownloadManagerDelegate* TestingProfile::GetDownloadManagerDelegate() {
   return NULL;
 }
 
 net::URLRequestContextGetter* TestingProfile::GetRequestContext() {
   return GetDefaultStoragePartition(this)->GetURLRequestContext();
 }
 
 net::URLRequestContextGetter* TestingProfile::CreateRequestContext(
     content::ProtocolHandlerMap* protocol_handlers,
@@ skipping 249 matching lines @@
 #if defined(ENABLE_EXTENSIONS)
                             extension_policy_,
 #endif
                             pref_service_.Pass(),
                             original_profile,
                             guest_session_,
                             supervised_user_id_,
                             policy_service_.Pass(),
                             testing_factories_);
 }