Fix a timing assumption that is broken by decoupling History and Bookmarks.

Previously, the HistoryQuickProvider unit tests were relying on the fact that the InMemoryURLIndex used by the HistoryService is initialized by the time Bookmarks are done loading. Once that assumption is invalidated, the tests would populate the index only to have it cleared once the HistoryService initialization completed.

This CL adds a method to TestingProfile that allows one to block until the index is populated.

R=sky
BUG=144050

Review URL: https://chromiumcodereview.appspot.com/10887019

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@154019 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 110 insertions, 27 deletions

diff --git a/chrome/browser/autocomplete/extension_app_provider_unittest.cc b/chrome/browser/autocomplete/extension_app_provider_unittest.cc
index a818628..ba9cf09 100644
--- a/chrome/browser/autocomplete/extension_app_provider_unittest.cc
+++ b/chrome/browser/autocomplete/extension_app_provider_unittest.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/url_database.h"
 #include "chrome/test/base/testing_profile.h"
+#include "content/public/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 class ExtensionAppProviderTest : public testing::Test {
@@ -21,7 +22,9 @@ class ExtensionAppProviderTest : public testing::Test {
     const GURL output[3];
   };
 
-  ExtensionAppProviderTest() : history_service_(NULL) { }
+  ExtensionAppProviderTest()
+      : ui_thread_(content::BrowserThread::UI, &message_loop_),
+        history_service_(NULL) { }
   virtual ~ExtensionAppProviderTest() { }
 
   virtual void SetUp() OVERRIDE;
@@ -31,6 +34,7 @@ class ExtensionAppProviderTest : public testing::Test {
 
  protected:
   MessageLoopForUI message_loop_;
+  content::TestBrowserThread ui_thread_;
   scoped_refptr<ExtensionAppProvider> app_provider_;
   scoped_ptr<TestingProfile> profile_;
   HistoryService* history_service_;
diff --git a/chrome/browser/autocomplete/history_quick_provider_unittest.cc b/chrome/browser/autocomplete/history_quick_provider_unittest.cc
index ff225b3..5a03bc4 100644
--- a/chrome/browser/autocomplete/history_quick_provider_unittest.cc
+++ b/chrome/browser/autocomplete/history_quick_provider_unittest.cc
@@ -137,6 +137,7 @@ void HistoryQuickProviderTest::SetUp() {
   profile_->CreateHistoryService(true, false);
   profile_->CreateBookmarkModel(true);
   profile_->BlockUntilBookmarkModelLoaded();
+  profile_->BlockUntilHistoryIndexIsRefreshed();
   history_service_ =
       HistoryServiceFactory::GetForProfile(profile_.get(),
                                            Profile::EXPLICIT_ACCESS);
diff --git a/chrome/browser/autocomplete/search_provider_unittest.cc b/chrome/browser/autocomplete/search_provider_unittest.cc
index 8a45714..3e8fff8 100644
--- a/chrome/browser/autocomplete/search_provider_unittest.cc
+++ b/chrome/browser/autocomplete/search_provider_unittest.cc
@@ -48,6 +48,7 @@ class SearchProviderTest : public testing::Test,
         term1_(UTF8ToUTF16("term1")),
         keyword_t_url_(NULL),
         keyword_term_(UTF8ToUTF16("keyword")),
+        ui_thread_(BrowserThread::UI, &message_loop_),
         io_thread_(BrowserThread::IO),
         quit_when_done_(false) {
     io_thread_.Start();
@@ -104,6 +105,7 @@ class SearchProviderTest : public testing::Test,
   GURL keyword_url_;
 
   MessageLoopForUI message_loop_;
+  content::TestBrowserThread ui_thread_;
   content::TestBrowserThread io_thread_;
 
   // URLFetcherFactory implementation registered.
diff --git a/chrome/browser/history/in_memory_url_index.cc b/chrome/browser/history/in_memory_url_index.cc
index ace4275..78955ac 100644
--- a/chrome/browser/history/in_memory_url_index.cc
+++ b/chrome/browser/history/in_memory_url_index.cc
@@ -98,6 +98,7 @@ InMemoryURLIndex::InMemoryURLIndex(Profile* profile,
       restore_cache_observer_(NULL),
       save_cache_observer_(NULL),
       shutdown_(false),
+      restored_(false),
       needs_to_be_cached_(false) {
   InitializeSchemeWhitelist(&scheme_whitelist_);
   if (profile) {
@@ -117,6 +118,7 @@ InMemoryURLIndex::InMemoryURLIndex()
       restore_cache_observer_(NULL),
       save_cache_observer_(NULL),
       shutdown_(false),
+      restored_(false),
       needs_to_be_cached_(false) {
   InitializeSchemeWhitelist(&scheme_whitelist_);
 }
@@ -217,9 +219,16 @@ void InMemoryURLIndex::OnURLsDeleted(const URLsDeletedDetails* details) {
 // Restoring from Cache --------------------------------------------------------
 
 void InMemoryURLIndex::PostRestoreFromCacheFileTask() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
   FilePath path;
-  if (!GetCacheFilePath(&path) || shutdown_)
+  if (!GetCacheFilePath(&path) || shutdown_) {
+    restored_ = true;
+    if (restore_cache_observer_)
+      restore_cache_observer_->OnCacheRestoreFinished(false);
     return;
+  }
+
   scoped_refptr<URLIndexPrivateData> restored_private_data =
       new URLIndexPrivateData;
   content::BrowserThread::PostTaskAndReply(
@@ -234,6 +243,7 @@ void InMemoryURLIndex::OnCacheLoadDone(
     scoped_refptr<URLIndexPrivateData> private_data) {
   if (private_data.get() && !private_data->Empty()) {
     private_data_ = private_data;
+    restored_ = true;
     if (restore_cache_observer_)
       restore_cache_observer_->OnCacheRestoreFinished(true);
   } else if (profile_) {
@@ -280,6 +290,7 @@ void InMemoryURLIndex::DoneRebuidingPrivateDataFromHistoryDB(
     // There is no need to do anything with the cache file as it was deleted
     // when the rebuild from the history operation was kicked off.
   }
+  restored_ = true;
   if (restore_cache_observer_)
     restore_cache_observer_->OnCacheRestoreFinished(succeeded);
 }
diff --git a/chrome/browser/history/in_memory_url_index.h b/chrome/browser/history/in_memory_url_index.h
index 0c7724ac..7bff098 100644
--- a/chrome/browser/history/in_memory_url_index.h
+++ b/chrome/browser/history/in_memory_url_index.h
@@ -130,6 +130,11 @@ class InMemoryURLIndex : public content::NotificationObserver,
     save_cache_observer_ = save_cache_observer;
   }
 
+  // Indicates that the index restoration is complete.
+  bool restored() const {
+    return restored_;
+  }
+
  private:
   friend class ::HistoryQuickProviderTest;
   friend class InMemoryURLIndexTest;
@@ -266,6 +271,9 @@ class InMemoryURLIndex : public content::NotificationObserver,
   // Set to true once the shutdown process has begun.
   bool shutdown_;
 
+  // Set to true once the index restoration is complete.
+  bool restored_;
+
   // Set to true when changes to the index have been made and the index needs
   // to be cached. Set to false when the index has been cached. Used as a
   // temporary safety check to insure that the cache is saved before the
diff --git a/chrome/browser/history/in_memory_url_index_unittest.cc b/chrome/browser/history/in_memory_url_index_unittest.cc
index ddc7825..eebc027 100644
--- a/chrome/browser/history/in_memory_url_index_unittest.cc
+++ b/chrome/browser/history/in_memory_url_index_unittest.cc
@@ -24,6 +24,7 @@
 #include "chrome/browser/history/url_index_private_data.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/test/base/history_index_restore_observer.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
@@ -70,28 +71,6 @@ void CacheFileSaverObserver::OnCacheSaveFinished(bool succeeded) {
   loop_->Quit();
 }
 
-// Observer class so the unit tests can wait while the cache is being restored.
-class CacheFileReaderObserver : public InMemoryURLIndex::RestoreCacheObserver {
- public:
-  explicit CacheFileReaderObserver(MessageLoop* loop);
-  virtual void OnCacheRestoreFinished(bool succeeded) OVERRIDE;
-
-  MessageLoop* loop_;
-  bool succeeded_;
-  DISALLOW_COPY_AND_ASSIGN(CacheFileReaderObserver);
-};
-
-CacheFileReaderObserver::CacheFileReaderObserver(MessageLoop* loop)
-    : loop_(loop),
-      succeeded_(false) {
-  DCHECK(loop);
-}
-
-void CacheFileReaderObserver::OnCacheRestoreFinished(bool succeeded) {
-  succeeded_ = succeeded;
-  loop_->Quit();
-}
-
 // -----------------------------------------------------------------------------
 
 class InMemoryURLIndexTest : public testing::Test {
@@ -1101,11 +1080,12 @@ TEST_F(InMemoryURLIndexTest, CacheSaveRestore) {
   EXPECT_TRUE(private_data.history_info_map_.empty());
   EXPECT_TRUE(private_data.word_starts_map_.empty());
 
-  CacheFileReaderObserver read_observer(&message_loop_);
-  url_index_->set_restore_cache_observer(&read_observer);
+  HistoryIndexRestoreObserver restore_observer(
+      base::Bind(&MessageLoop::Quit, base::Unretained(&message_loop_)));
+  url_index_->set_restore_cache_observer(&restore_observer);
   PostRestoreFromCacheFileTask();
   message_loop_.Run();
-  EXPECT_TRUE(read_observer.succeeded_);
+  EXPECT_TRUE(restore_observer.succeeded());
 
   URLIndexPrivateData& new_data(*GetPrivateData());
 
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index a4e229da..cc344cb 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -225,6 +225,8 @@
         'test/base/chrome_render_view_host_test_harness.h',
         'test/base/chrome_test_suite.cc',
         'test/base/chrome_test_suite.h',
+        'test/base/history_index_restore_observer.cc',
+        'test/base/history_index_restore_observer.h',
         'test/base/in_process_browser_test.cc',
         'test/base/in_process_browser_test.h',
         'test/base/javascript_test_observer.cc',
diff --git a/chrome/test/base/history_index_restore_observer.cc b/chrome/test/base/history_index_restore_observer.cc
new file mode 100644
index 0000000..cd54866
--- /dev/null
+++ b/chrome/test/base/history_index_restore_observer.cc
@@ -0,0 +1,18 @@
+// 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/history_index_restore_observer.h"
+
+HistoryIndexRestoreObserver::HistoryIndexRestoreObserver(
+    const base::Closure& task)
+    : task_(task),
+      succeeded_(false) {
+}
+
+HistoryIndexRestoreObserver::~HistoryIndexRestoreObserver() {}
+
+void HistoryIndexRestoreObserver::OnCacheRestoreFinished(bool success) {
+  succeeded_ = success;
+  task_.Run();
+}
diff --git a/chrome/test/base/history_index_restore_observer.h b/chrome/test/base/history_index_restore_observer.h
new file mode 100644
index 0000000..3f03377
--- /dev/null
+++ b/chrome/test/base/history_index_restore_observer.h
@@ -0,0 +1,34 @@
+// 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.
+
+#ifndef CHROME_TEST_BASE_HISTORY_INDEX_RESTORE_OBSERVER_H_
+#define CHROME_TEST_BASE_HISTORY_INDEX_RESTORE_OBSERVER_H_
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/compiler_specific.h"
+#include "chrome/browser/history/in_memory_url_index.h"
+
+// HistoryIndexRestoreObserver is used when blocking until the InMemoryURLIndex
+// finishes restoring. As soon as the InMemoryURLIndex finishes restoring the
+// provided Closure is invoked.
+class HistoryIndexRestoreObserver
+    : public history::InMemoryURLIndex::RestoreCacheObserver {
+ public:
+  explicit HistoryIndexRestoreObserver(const base::Closure& task);
+  virtual ~HistoryIndexRestoreObserver();
+
+  bool succeeded() const { return succeeded_; }
+
+  // RestoreCacheObserver implementation.
+  virtual void OnCacheRestoreFinished(bool success) OVERRIDE;
+
+ private:
+  base::Closure task_;
+  bool succeeded_;
+
+  DISALLOW_COPY_AND_ASSIGN(HistoryIndexRestoreObserver);
+};
+
+#endif  // CHROME_TEST_BASE_HISTORY_INDEX_RESTORE_OBSERVER_H_
diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc
index 5b05335..e2568a1 100644
--- a/chrome/test/base/testing_profile.cc
+++ b/chrome/test/base/testing_profile.cc
@@ -50,6 +50,7 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/bookmark_load_observer.h"
+#include "chrome/test/base/history_index_restore_observer.h"
 #include "chrome/test/base/testing_pref_service.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/browser_thread.h"
@@ -438,6 +439,24 @@ void TestingProfile::BlockUntilBookmarkModelLoaded() {
   DCHECK(bookmark_model->IsLoaded());
 }
 
+void TestingProfile::BlockUntilHistoryIndexIsRefreshed() {
+  // Only get the history service if it actually exists since the caller of the
+  // test should explicitly call CreateHistoryService to build it.
+  HistoryService* history_service =
+      HistoryServiceFactory::GetForProfileWithoutCreating(this);
+  DCHECK(history_service);
+  history::InMemoryURLIndex* index = history_service->InMemoryIndex();
+  if (!index || index->restored())
+    return;
+  base::RunLoop run_loop;
+  HistoryIndexRestoreObserver observer(
+      content::GetQuitTaskForRunLoop(&run_loop));
+  index->set_restore_cache_observer(&observer);
+  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() {
   content::WindowedNotificationObserver top_sites_loaded_observer(
diff --git a/chrome/test/base/testing_profile.h b/chrome/test/base/testing_profile.h
index b218326f..0476899 100644
--- a/chrome/test/base/testing_profile.h
+++ b/chrome/test/base/testing_profile.h
@@ -167,6 +167,10 @@ class TestingProfile : public Profile {
   // CreateBookmarkModel.
   void BlockUntilBookmarkModelLoaded();
 
+  // Blocks until the HistoryService finishes restoring its in-memory cache.
+  // This is NOT invoked from CreateHistoryService.
+  void BlockUntilHistoryIndexIsRefreshed();
+
   // Blocks until TopSites finishes loading.
   void BlockUntilTopSitesLoaded();