diff options
author | mrossetti@chromium.org <mrossetti@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-02-07 04:12:46 +0000 |
---|---|---|
committer | mrossetti@chromium.org <mrossetti@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-02-07 04:12:46 +0000 |
commit | 91f4b017e820a0cb0dc20a7ee076384e32559c02 (patch) | |
tree | 927a7931c739b17851dfdc59f1386bd5f4deb5af | |
parent | 50b82260d18b63c6678ea8bb8f90410cffb4b0b4 (diff) | |
download | chromium_src-91f4b017e820a0cb0dc20a7ee076384e32559c02.zip chromium_src-91f4b017e820a0cb0dc20a7ee076384e32559c02.tar.gz chromium_src-91f4b017e820a0cb0dc20a7ee076384e32559c02.tar.bz2 |
Move Ownership of IMUI to HistoryService.
This change moves the InMemoryURLIndex (IMUI) from being owned by the InMemoryHistoryBackend to being owned by the HistoryService. The only dependency the IMUI has on the HistoryBackend is the need to access the history database for the one-time need to build the IMUI from scratch. Divorcing the IMUI from the HistoryBackend also pointed out that updates to the IMUI index related to URL visits and history deletes should be handled by notification, and this was implemented in this CL.
NOTE: Portions previously reviewed as: http://codereview.chromium.org/9030031/. That change involved both 1) moving the ownership of the IMUI from the InMemoryHistoryBackend, and 2) performing the caching file operations on the FILE thread. I've split this up into two CLs with this CL dealing with the move of ownership.
BUG=83659
TEST=Updated unit tests.
Review URL: https://chromiumcodereview.appspot.com/9316109
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@120707 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | chrome/browser/autocomplete/history_quick_provider_unittest.cc | 32 | ||||
-rw-r--r-- | chrome/browser/history/history.cc | 28 | ||||
-rw-r--r-- | chrome/browser/history/history.h | 13 | ||||
-rw-r--r-- | chrome/browser/history/history_backend.cc | 3 | ||||
-rw-r--r-- | chrome/browser/history/history_database.h | 1 | ||||
-rw-r--r-- | chrome/browser/history/history_notifications.h | 15 | ||||
-rw-r--r-- | chrome/browser/history/history_types.h | 3 | ||||
-rw-r--r-- | chrome/browser/history/history_unittest.cc | 4 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_history_backend.cc | 38 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_history_backend.h | 21 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_url_index.cc | 198 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_url_index.h | 139 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_url_index_types.h | 3 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_url_index_unittest.cc | 223 | ||||
-rw-r--r-- | chrome/browser/history/url_index_private_data.cc | 135 | ||||
-rw-r--r-- | chrome/browser/history/url_index_private_data.h | 42 |
16 files changed, 597 insertions, 301 deletions
diff --git a/chrome/browser/autocomplete/history_quick_provider_unittest.cc b/chrome/browser/autocomplete/history_quick_provider_unittest.cc index bec93c5..f6892e9 100644 --- a/chrome/browser/autocomplete/history_quick_provider_unittest.cc +++ b/chrome/browser/autocomplete/history_quick_provider_unittest.cc @@ -18,6 +18,7 @@ #include "chrome/browser/history/history.h" #include "chrome/browser/history/in_memory_url_index.h" #include "chrome/browser/history/url_database.h" +#include "chrome/browser/history/url_index_private_data.h" #include "chrome/browser/prefs/pref_service.h" #include "chrome/common/pref_names.h" #include "chrome/test/base/testing_browser_process.h" @@ -99,10 +100,7 @@ class HistoryQuickProviderTest : public testing::Test, }; void SetUp(); - - void TearDown() { - provider_ = NULL; - } + void TearDown(); virtual void GetTestData(size_t* data_count, TestURLInfo** test_data); @@ -117,6 +115,9 @@ class HistoryQuickProviderTest : public testing::Test, bool can_inline_top_result, string16 expected_fill_into_edit); + // Pass-through functions to simplify our friendship with URLIndexPrivateData. + bool UpdateURL(const history::URLRow& row); + MessageLoopForUI message_loop_; content::TestBrowserThread ui_thread_; content::TestBrowserThread file_thread_; @@ -141,6 +142,18 @@ void HistoryQuickProviderTest::SetUp() { FillData(); } +void HistoryQuickProviderTest::TearDown() { + provider_ = NULL; +} + +bool HistoryQuickProviderTest::UpdateURL(const history::URLRow& row) { + history::InMemoryURLIndex* index = provider_->GetIndex(); + DCHECK(index); + history::URLIndexPrivateData* private_data = index->private_data(); + DCHECK(private_data); + return private_data->UpdateURL(row); +} + void HistoryQuickProviderTest::OnProviderUpdate(bool updated_matches) { MessageLoop::current()->Quit(); } @@ -156,12 +169,6 @@ void HistoryQuickProviderTest::GetTestData(size_t* data_count, void HistoryQuickProviderTest::FillData() { history::URLDatabase* db = history_service_->InMemoryDatabase(); ASSERT_TRUE(db != NULL); - - history::InMemoryURLIndex* index = - new history::InMemoryURLIndex(FilePath()); - PrefService* prefs = profile_->GetPrefs(); - std::string languages(prefs->GetString(prefs::kAcceptLanguages)); - index->Init(db, languages); size_t data_count = 0; TestURLInfo* test_data = NULL; GetTestData(&data_count, &test_data); @@ -171,15 +178,14 @@ void HistoryQuickProviderTest::FillData() { Time visit_time = Time::Now() - TimeDelta::FromDays(cur.days_from_now); history::URLRow url_info(current_url); + url_info.set_id(i + 5000); url_info.set_title(UTF8ToUTF16(cur.title)); url_info.set_visit_count(cur.visit_count); url_info.set_typed_count(cur.typed_count); url_info.set_last_visit(visit_time); url_info.set_hidden(false); - index->UpdateURL(i, url_info); + UpdateURL(url_info); } - - provider_->set_index(index); } HistoryQuickProviderTest::SetShouldContain::SetShouldContain( diff --git a/chrome/browser/history/history.cc b/chrome/browser/history/history.cc index 7ef5edd..8bdb754 100644 --- a/chrome/browser/history/history.cc +++ b/chrome/browser/history/history.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -25,6 +25,7 @@ #include "chrome/browser/history/history.h" #include "base/callback.h" +#include "base/command_line.h" #include "base/memory/ref_counted.h" #include "base/message_loop.h" #include "base/path_service.h" @@ -37,6 +38,7 @@ #include "chrome/browser/history/history_types.h" #include "chrome/browser/history/in_memory_database.h" #include "chrome/browser/history/in_memory_history_backend.h" +#include "chrome/browser/history/in_memory_url_index.h" #include "chrome/browser/history/top_sites.h" #include "chrome/browser/prefs/pref_service.h" #include "chrome/browser/profiles/profile.h" @@ -44,6 +46,7 @@ #include "chrome/browser/visitedlink/visitedlink_master.h" #include "chrome/common/chrome_constants.h" #include "chrome/common/chrome_notification_types.h" +#include "chrome/common/chrome_switches.h" #include "chrome/common/pref_names.h" #include "chrome/common/thumbnail_score.h" #include "chrome/common/url_constants.h" @@ -181,6 +184,10 @@ void HistoryService::UnloadBackend() { // Get rid of the in-memory backend. in_memory_backend_.reset(); + // Give the InMemoryURLIndex a chance to shutdown. + if (in_memory_url_index_.get()) + in_memory_url_index_->ShutDown(); + // The backend's destructor must run on the history thread since it is not // threadsafe. So this thread must not be the last thread holding a reference // to the backend, or a crash could happen. @@ -243,16 +250,6 @@ history::URLDatabase* HistoryService::InMemoryDatabase() { return NULL; } -history::InMemoryURLIndex* HistoryService::InMemoryIndex() { - // NOTE: See comments in BackendLoaded() as to why we call - // LoadBackendIfNecessary() here even though it won't affect the return value - // for this call. - LoadBackendIfNecessary(); - if (in_memory_backend_.get()) - return in_memory_backend_->InMemoryIndex(); - return NULL; -} - void HistoryService::SetSegmentPresentationIndex(int64 segment_id, int index) { ScheduleAndForget(PRIORITY_UI, &HistoryBackend::SetSegmentPresentationIndex, @@ -690,6 +687,15 @@ bool HistoryService::Init(const FilePath& history_dir, bookmark_service_ = bookmark_service; no_db_ = no_db; + if (profile_ && !CommandLine::ForCurrentProcess()->HasSwitch( + switches::kDisableHistoryQuickProvider)) { + std::string languages = + profile_->GetPrefs()->GetString(prefs::kAcceptLanguages); + in_memory_url_index_.reset( + new history::InMemoryURLIndex(profile_, history_dir_, languages)); + in_memory_url_index_->Init(); + } + // Create the history backend. LoadBackendIfNecessary(); return true; diff --git a/chrome/browser/history/history.h b/chrome/browser/history/history.h index 71e4df9..fc66691 100644 --- a/chrome/browser/history/history.h +++ b/chrome/browser/history/history.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -43,6 +43,7 @@ class Time; namespace history { class InMemoryHistoryBackend; class InMemoryURLIndex; +class InMemoryURLIndexTest; class HistoryAddPageArgs; class HistoryBackend; class HistoryDatabase; @@ -67,7 +68,7 @@ class HistoryDBTask : public base::RefCountedThreadSafe<HistoryDBTask> { virtual bool RunOnDBThread(history::HistoryBackend* backend, history::HistoryDatabase* db) = 0; - // Invoked on the main thread once RunOnDBThread has returned false. This is + // Invoked on the main thread once RunOnDBThread has returned true. This is // only invoked if the request was not canceled and returned true from // RunOnDBThread. virtual void DoneRunOnMainThread() = 0; @@ -143,7 +144,9 @@ class HistoryService : public CancelableRequestProvider, history::URLDatabase* InMemoryDatabase(); // Return the quick history index. - history::InMemoryURLIndex* InMemoryIndex(); + history::InMemoryURLIndex* InMemoryIndex() const { + return in_memory_url_index_.get(); + } // Navigation ---------------------------------------------------------------- @@ -590,6 +593,7 @@ class HistoryService : public CancelableRequestProvider, friend class HistoryOperation; friend class HistoryURLProvider; friend class HistoryURLProviderTest; + friend class history::InMemoryURLIndexTest; template<typename Info, typename Callback> friend class DownloadRequest; friend class PageUsageRequest; friend class RedirectRequest; @@ -865,6 +869,9 @@ class HistoryService : public CancelableRequestProvider, // True if needs top site migration. bool needs_top_sites_migration_; + // The index used for quick history lookups. + scoped_ptr<history::InMemoryURLIndex> in_memory_url_index_; + DISALLOW_COPY_AND_ASSIGN(HistoryService); }; diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc index d79fa6a..4c78a8e 100644 --- a/chrome/browser/history/history_backend.cc +++ b/chrome/browser/history/history_backend.cc @@ -10,7 +10,6 @@ #include <vector> #include "base/bind.h" -#include "base/command_line.h" #include "base/compiler_specific.h" #include "base/file_util.h" #include "base/memory/scoped_ptr.h" @@ -580,7 +579,7 @@ void HistoryBackend::InitImpl(const std::string& languages) { // Fill the in-memory database and send it back to the history service on the // main thread. InMemoryHistoryBackend* mem_backend = new InMemoryHistoryBackend; - if (mem_backend->Init(history_name, history_dir_, db_.get(), languages)) + if (mem_backend->Init(history_name, db_.get())) delegate_->SetInMemoryBackend(id_, mem_backend); // Takes ownership of // pointer. else diff --git a/chrome/browser/history/history_database.h b/chrome/browser/history/history_database.h index 9a25949..1f88efc 100644 --- a/chrome/browser/history/history_database.h +++ b/chrome/browser/history/history_database.h @@ -141,6 +141,7 @@ class HistoryDatabase : public DownloadDatabase, virtual void UpdateEarlyExpirationThreshold(base::Time threshold); private: + friend class InMemoryURLIndexTest; FRIEND_TEST_ALL_PREFIXES(IconMappingMigrationTest, TestIconMappingMigration); // Overridden from URLDatabase: diff --git a/chrome/browser/history/history_notifications.h b/chrome/browser/history/history_notifications.h index ab9d555..e5ea48b 100644 --- a/chrome/browser/history/history_notifications.h +++ b/chrome/browser/history/history_notifications.h @@ -1,4 +1,4 @@ -// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. +// 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. @@ -25,7 +25,7 @@ struct HistoryDetails { virtual ~HistoryDetails() {} }; -// Details for HISTORY_URL_VISITED. +// Details for NOTIFICATION_HISTORY_URL_VISITED. struct URLVisitedDetails : public HistoryDetails { URLVisitedDetails(); virtual ~URLVisitedDetails(); @@ -40,7 +40,7 @@ struct URLVisitedDetails : public HistoryDetails { history::RedirectList redirects; }; -// Details for NOTIFY_HISTORY_TYPED_URLS_MODIFIED. +// Details for NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED. struct URLsModifiedDetails : public HistoryDetails { URLsModifiedDetails(); virtual ~URLsModifiedDetails(); @@ -49,7 +49,7 @@ struct URLsModifiedDetails : public HistoryDetails { std::vector<URLRow> changed_urls; }; -// Details for NOTIFY_HISTORY_URLS_DELETED. +// Details for NOTIFICATION_HISTORY_URLS_DELETED. struct URLsDeletedDetails : public HistoryDetails { URLsDeletedDetails(); virtual ~URLsDeletedDetails(); @@ -57,9 +57,14 @@ struct URLsDeletedDetails : public HistoryDetails { // Set when all history was deleted. False means just a subset was deleted. bool all_history; + // The URLRows which have been deleted. + std::vector<URLRow> rows; + // The list of unique URLs affected. This is valid only when a subset of // history is deleted. When all of it is deleted, this will be empty, since - // we do not bother to list all URLs. + // we do not bother to list all URLs. (This information can be gleaned from + // |rows| but, since there are several clients who need the set, we pre-build + // it so that the clients don't have to.) std::set<GURL> urls; }; diff --git a/chrome/browser/history/history_types.h b/chrome/browser/history/history_types.h index df626a7..823a836 100644 --- a/chrome/browser/history/history_types.h +++ b/chrome/browser/history/history_types.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -175,6 +175,7 @@ class URLRow { // We support the implicit copy constuctor and operator=. }; +typedef std::vector<URLRow> URLRows; // The enumeration of all possible sources of visits is listed below. // The source will be propagated along with a URL or a visit item diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc index 66a8dbf..ddd743c 100644 --- a/chrome/browser/history/history_unittest.cc +++ b/chrome/browser/history/history_unittest.cc @@ -114,8 +114,8 @@ class HistoryTest : public testing::Test { // Creates the HistoryBackend and HistoryDatabase on the current thread, // assigning the values to backend_ and db_. void CreateBackendAndDatabase() { - backend_ = - new HistoryBackend(history_dir_, 0, new BackendDelegate(this), NULL); + backend_ = new HistoryBackend(history_dir_, 0, new BackendDelegate(this), + NULL); backend_->Init(std::string(), false); db_ = backend_->db_.get(); DCHECK(in_mem_backend_.get()) << "Mem backend should have been set by " diff --git a/chrome/browser/history/in_memory_history_backend.cc b/chrome/browser/history/in_memory_history_backend.cc index 41b843d..d191c99 100644 --- a/chrome/browser/history/in_memory_history_backend.cc +++ b/chrome/browser/history/in_memory_history_backend.cc @@ -13,11 +13,9 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/history/history_notifications.h" #include "chrome/browser/history/in_memory_database.h" -#include "chrome/browser/history/in_memory_url_index.h" #include "chrome/browser/history/url_database.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/chrome_notification_types.h" -#include "chrome/common/chrome_switches.h" #include "content/public/browser/notification_details.h" #include "content/public/browser/notification_source.h" @@ -27,23 +25,12 @@ InMemoryHistoryBackend::InMemoryHistoryBackend() : profile_(NULL) { } -InMemoryHistoryBackend::~InMemoryHistoryBackend() { - if (index_.get()) - index_->ShutDown(); -} +InMemoryHistoryBackend::~InMemoryHistoryBackend() {} bool InMemoryHistoryBackend::Init(const FilePath& history_filename, - const FilePath& history_dir, - URLDatabase* db, - const std::string& languages) { + URLDatabase* db) { db_.reset(new InMemoryDatabase); - bool success = db_->InitFromDisk(history_filename); - if (!CommandLine::ForCurrentProcess()->HasSwitch( - switches::kDisableHistoryQuickProvider)) { - index_.reset(new InMemoryURLIndex(history_dir)); - index_->Init(db, languages); - } - return success; + return db_->InitFromDisk(history_filename); } void InMemoryHistoryBackend::AttachToHistoryService(Profile* profile) { @@ -131,8 +118,6 @@ void InMemoryHistoryBackend::OnTypedURLsModified( db_->UpdateURLRow(id, *i); else id = db_->AddURL(*i); - if (index_.get()) - index_->UpdateURL(id, *i); } } @@ -145,22 +130,15 @@ void InMemoryHistoryBackend::OnURLsDeleted(const URLsDeletedDetails& details) { db_.reset(new InMemoryDatabase); if (!db_->InitFromScratch()) db_.reset(); - if (index_.get()) - index_->ReloadFromHistory(db_.get()); return; } // Delete all matching URLs in our database. - for (std::set<GURL>::const_iterator i = details.urls.begin(); - i != details.urls.end(); ++i) { - URLID id = db_->GetRowForURL(*i, NULL); - if (id) { - // We typically won't have most of them since we only have a subset of - // history, so ignore errors. - db_->DeleteURLRow(id); - if (index_.get()) - index_->DeleteURL(id); - } + for (URLRows::const_iterator row = details.rows.begin(); + row != details.rows.end(); ++row) { + // We typically won't have most of them since we only have a subset of + // history, so ignore errors. + db_->DeleteURLRow(row->id()); } } diff --git a/chrome/browser/history/in_memory_history_backend.h b/chrome/browser/history/in_memory_history_backend.h index 16b2907..d51c5db 100644 --- a/chrome/browser/history/in_memory_history_backend.h +++ b/chrome/browser/history/in_memory_history_backend.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -41,16 +41,9 @@ class InMemoryHistoryBackend : public content::NotificationObserver { virtual ~InMemoryHistoryBackend(); // Initializes the backend from the history database pointed to by the - // full path in |history_filename|. |history_dir| is the path to the - // directory containing the history database and is also used - // as the directory where the InMemoryURLIndex's cache is kept. |db| is - // used for building the InMemoryURLIndex. |languages| gives the - // preferred user languages with which URLs and page titles are - // interpreted while decomposing into words and characters during indexing. - bool Init(const FilePath& history_filename, - const FilePath& history_dir, - URLDatabase* db, - const std::string& languages); + // full path in |history_filename|. |db| is used for setting up the + // InMemoryDatabase. + bool Init(const FilePath& history_filename, URLDatabase* db); // Does initialization work when this object is attached to the history // system on the main thread. The argument is the profile with which the @@ -69,9 +62,6 @@ class InMemoryHistoryBackend : public content::NotificationObserver { const content::NotificationSource& source, const content::NotificationDetails& details) OVERRIDE; - // Return the quick history index. - history::InMemoryURLIndex* InMemoryIndex() const { return index_.get(); } - private: FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, DeleteAll); @@ -95,9 +85,6 @@ class InMemoryHistoryBackend : public content::NotificationObserver { // initialization. Profile* profile_; - // The index used for quick history lookups. - scoped_ptr<history::InMemoryURLIndex> index_; - DISALLOW_COPY_AND_ASSIGN(InMemoryHistoryBackend); }; diff --git a/chrome/browser/history/in_memory_url_index.cc b/chrome/browser/history/in_memory_url_index.cc index 9e29191..d6495c8 100644 --- a/chrome/browser/history/in_memory_url_index.cc +++ b/chrome/browser/history/in_memory_url_index.cc @@ -5,56 +5,85 @@ #include "chrome/browser/history/in_memory_url_index.h" #include "base/utf_string_conversions.h" +#include "chrome/browser/history/history_notifications.h" #include "chrome/browser/history/url_database.h" +#include "chrome/browser/history/url_index_private_data.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/common/chrome_notification_types.h" +#include "content/public/browser/notification_details.h" +#include "content/public/browser/notification_source.h" using in_memory_url_index::InMemoryURLIndexCacheItem; namespace history { -InMemoryURLIndex::InMemoryURLIndex(const FilePath& history_dir) - : history_dir_(history_dir), +InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask:: + RebuildPrivateDataFromHistoryDBTask(InMemoryURLIndex* index) + : index_(index), + succeeded_(false) {} + +InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask:: + ~RebuildPrivateDataFromHistoryDBTask() {} + +bool InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::RunOnDBThread( + HistoryBackend* backend, + HistoryDatabase* db) { + data_.reset(URLIndexPrivateData::RebuildFromHistory(db)); + succeeded_ = data_.get() && !data_->history_info_map_.empty(); + if (!succeeded_) + data_.reset(); + return true; +} + +void InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask:: + DoneRunOnMainThread() { + if (succeeded_) + index_->DoneRebuidingPrivateDataFromHistoryDB(data_.release()); +} + +InMemoryURLIndex::InMemoryURLIndex(Profile* profile, + const FilePath& history_dir, + const std::string& languages) + : profile_(profile), + history_dir_(history_dir), private_data_(new URLIndexPrivateData), - cached_at_shutdown_(false) { + shutdown_(false), + needs_to_be_cached_(false) { + private_data_->set_languages(languages); + if (profile) { + // TODO(mrossetti): Register for language change notifications. + content::Source<Profile> source(profile); + registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URL_VISITED, source); + registrar_.Add(this, chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED, + source); + registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URLS_DELETED, source); + } } // Called only by unit tests. InMemoryURLIndex::InMemoryURLIndex() - : private_data_(new URLIndexPrivateData), - cached_at_shutdown_(false) { + : profile_(NULL), + private_data_(new URLIndexPrivateData), + shutdown_(false), + needs_to_be_cached_(false) { } InMemoryURLIndex::~InMemoryURLIndex() { // If there was a history directory (which there won't be for some unit tests) // then insure that the cache has already been saved. - DCHECK(history_dir_.empty() || cached_at_shutdown_); -} - -bool InMemoryURLIndex::Init(URLDatabase* history_db, - const std::string& languages) { - // TODO(mrossetti): Register for profile/language change notifications. - private_data_->set_languages(languages); - FilePath cache_file_path; - return (GetCacheFilePath(&cache_file_path) && - private_data_->RestoreFromFile(cache_file_path)) || - ReloadFromHistory(history_db); + DCHECK(history_dir_.empty() || !needs_to_be_cached_); } -bool InMemoryURLIndex::ReloadFromHistory(history::URLDatabase* history_db) { - if (!private_data_->ReloadFromHistory(history_db)) - return false; - FilePath cache_file_path; - if (GetCacheFilePath(&cache_file_path)) - private_data_->SaveToFile(cache_file_path); - return true; +void InMemoryURLIndex::Init() { + RestoreFromCacheFile(); } void InMemoryURLIndex::ShutDown() { - // Write our cache. - FilePath cache_file_path; - if (!GetCacheFilePath(&cache_file_path)) - return; - private_data_->SaveToFile(cache_file_path); - cached_at_shutdown_ = true; + registrar_.RemoveAll(); + cache_reader_consumer_.CancelAllRequests(); + shutdown_ = true; + SaveToCacheFile(); + needs_to_be_cached_ = false; } void InMemoryURLIndex::ClearPrivateData() { @@ -68,19 +97,120 @@ bool InMemoryURLIndex::GetCacheFilePath(FilePath* file_path) { return true; } -// Querying and Updating ------------------------------------------------------- +// Querying -------------------------------------------------------------------- ScoredHistoryMatches InMemoryURLIndex::HistoryItemsForTerms( const string16& term_string) { return private_data_->HistoryItemsForTerms(term_string); } -void InMemoryURLIndex::UpdateURL(URLID row_id, const URLRow& row) { - private_data_->UpdateURL(row_id, row); +// Updating -------------------------------------------------------------------- + +void InMemoryURLIndex::Observe(int notification_type, + const content::NotificationSource& source, + const content::NotificationDetails& details) { + switch (notification_type) { + case chrome::NOTIFICATION_HISTORY_URL_VISITED: + OnURLVisited(content::Details<URLVisitedDetails>(details).ptr()); + break; + case chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED: + OnURLsModified( + content::Details<history::URLsModifiedDetails>(details).ptr()); + break; + case chrome::NOTIFICATION_HISTORY_URLS_DELETED: + OnURLsDeleted( + content::Details<history::URLsDeletedDetails>(details).ptr()); + break; + case chrome::NOTIFICATION_HISTORY_LOADED: { + registrar_.Remove(this, chrome::NOTIFICATION_HISTORY_LOADED, + content::Source<Profile>(profile_)); + ScheduleRebuildFromHistory(); + break; + } + default: + // For simplicity, the unit tests send us all notifications, even when + // we haven't registered for them, so don't assert here. + break; + } +} + +void InMemoryURLIndex::OnURLVisited(const URLVisitedDetails* details) { + needs_to_be_cached_ |= private_data_->UpdateURL(details->row); +} + +void InMemoryURLIndex::OnURLsModified(const URLsModifiedDetails* details) { + for (URLRows::const_iterator row = details->changed_urls.begin(); + row != details->changed_urls.end(); ++row) + needs_to_be_cached_ |= private_data_->UpdateURL(*row); +} + +void InMemoryURLIndex::OnURLsDeleted(const URLsDeletedDetails* details) { + if (details->all_history) { + ClearPrivateData(); + needs_to_be_cached_ = true; + } else { + for (URLRows::const_iterator row = details->rows.begin(); + row != details->rows.end(); ++row) + needs_to_be_cached_ |= private_data_->DeleteURL(row->url()); + } +} + +// Restoring from Cache -------------------------------------------------------- + +void InMemoryURLIndex::RestoreFromCacheFile() { + FilePath path; + if (GetCacheFilePath(&path) && !shutdown_) + DoRestoreFromCacheFile(path); +} + +void InMemoryURLIndex::DoRestoreFromCacheFile(const FilePath& path) { + if (private_data_->RestoreFromFile(path)) + return; + + // When unable to restore from the cache file we must rebuild from the + // history database. + HistoryService* service = profile_->GetHistoryServiceWithoutCreating(); + if (service && service->backend_loaded()) + ScheduleRebuildFromHistory(); + // We must wait to rebuild until the history backend has been loaded. + registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED, + content::Source<Profile>(profile_)); +} + +// Restoring from the History DB ----------------------------------------------- + +void InMemoryURLIndex::ScheduleRebuildFromHistory() { + HistoryService* service = + profile_->GetHistoryService(Profile::EXPLICIT_ACCESS); + service->ScheduleDBTask( + new InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask(this), + &cache_reader_consumer_); +} + +void InMemoryURLIndex::DoneRebuidingPrivateDataFromHistoryDB( + URLIndexPrivateData* data) { + scoped_ptr<URLIndexPrivateData> private_data(data); + private_data_.swap(private_data); + // Cache the newly rebuilt index. + FilePath cache_file_path; + if (GetCacheFilePath(&cache_file_path)) + private_data_->SaveToFile(cache_file_path); +} + +void InMemoryURLIndex::RebuildFromHistory(HistoryDatabase* history_db) { + private_data_.reset(URLIndexPrivateData::RebuildFromHistory(history_db)); +} + +// Saving to Cache ------------------------------------------------------------- + +void InMemoryURLIndex::SaveToCacheFile() { + FilePath path; + if (GetCacheFilePath(&path)) + DoSaveToCacheFile(path); } -void InMemoryURLIndex::DeleteURL(URLID row_id) { - private_data_->DeleteURL(row_id); +void InMemoryURLIndex::DoSaveToCacheFile(const FilePath& path) { + private_data_->SaveToFile(path); } } // namespace history diff --git a/chrome/browser/history/in_memory_url_index.h b/chrome/browser/history/in_memory_url_index.h index 456ff41..665f7de 100644 --- a/chrome/browser/history/in_memory_url_index.h +++ b/chrome/browser/history/in_memory_url_index.h @@ -20,12 +20,18 @@ #include "base/string16.h" #include "chrome/browser/autocomplete/autocomplete_match.h" #include "chrome/browser/autocomplete/history_provider_util.h" +#include "chrome/browser/cancelable_request.h" +#include "chrome/browser/history/history.h" #include "chrome/browser/history/history_types.h" #include "chrome/browser/history/in_memory_url_index_types.h" #include "chrome/browser/history/in_memory_url_index_cache.pb.h" -#include "chrome/browser/history/url_index_private_data.h" +#include "content/public/browser/notification_observer.h" +#include "content/public/browser/notification_registrar.h" #include "sql/connection.h" +class HistoryQuickProviderTest; +class Profile; + namespace base { class Time; } @@ -38,7 +44,11 @@ namespace history { namespace imui = in_memory_url_index; -class URLDatabase; +class HistoryDatabase; +class URLIndexPrivateData; +struct URLVisitedDetails; +struct URLsModifiedDetails; +struct URLsDeletedDetails; // The URL history source. // Holds portions of the URL database in memory in an indexed form. Used to @@ -59,66 +69,116 @@ class URLDatabase; // will eliminate such words except in the case where a single character // is being searched on and which character occurs as the second char16 of a // multi-char16 instance. -class InMemoryURLIndex { +class InMemoryURLIndex : public content::NotificationObserver { public: - // |history_dir| is a path to the directory containing the history database - // within the profile wherein the cache and transaction journals will be - // stored. - explicit InMemoryURLIndex(const FilePath& history_dir); + // |profile|, which may be NULL during unit testing, is used to register for + // history changes. |history_dir| is a path to the directory containing the + // history database within the profile wherein the cache and transaction + // journals will be stored. |languages| gives a list of language encodings by + // which URLs and omnibox searches are broken down into words and characters. + InMemoryURLIndex(Profile* profile, + const FilePath& history_dir, + const std::string& languages); virtual ~InMemoryURLIndex(); - // Opens and indexes the URL history database. If the index private data - // cannot be restored from its cache file then it is rebuilt from the - // |history_db|. |languages| gives a list of language encodings by which URLs - // and omnibox searches are broken down into words and characters. - bool Init(URLDatabase* history_db, const std::string& languages); + // Opens and prepares the index of historical URL visits. If the index private + // data cannot be restored from its cache file then it is rebuilt from the + // history database. + void Init(); // Signals that any outstanding initialization should be canceled and // flushes the cache to disk. void ShutDown(); - // Reloads the history index from |history_db| ignoring any cache file that - // may be available, clears the cache and saves the cache after reloading. - bool ReloadFromHistory(history::URLDatabase* history_db); - // Scans the history index and returns a vector with all scored, matching // history items. This entry point simply forwards the call on to the // URLIndexPrivateData class. For a complete description of this function // refer to that class. ScoredHistoryMatches HistoryItemsForTerms(const string16& term_string); - // Updates or adds an history item to the index if it meets the minimum - // 'quick' criteria. - void UpdateURL(URLID row_id, const URLRow& row); - - // Deletes indexing data for an history item. The item may not have actually - // been indexed (which is the case if it did not previously meet minimum - // 'quick' criteria). - void DeleteURL(URLID row_id); - private: + friend class ::HistoryQuickProviderTest; friend class InMemoryURLIndexTest; - FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, CacheFilePath); - FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, CacheSaveRestore); - FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, HugeResultSet); - FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, TitleSearch); - FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, TypedCharacterCaching); - FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, WhitelistedURLs); FRIEND_TEST_ALL_PREFIXES(LimitedInMemoryURLIndexTest, Initialization); + FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexCacheTest, CacheFilePath); // Creating one of me without a history path is not allowed (tests excepted). InMemoryURLIndex(); + // HistoryDBTask used to rebuild our private data from the history database. + class RebuildPrivateDataFromHistoryDBTask : public HistoryDBTask { + public: + explicit RebuildPrivateDataFromHistoryDBTask(InMemoryURLIndex* index); + virtual ~RebuildPrivateDataFromHistoryDBTask(); + + virtual bool RunOnDBThread(HistoryBackend* backend, + history::HistoryDatabase* db) OVERRIDE; + virtual void DoneRunOnMainThread() OVERRIDE; + + private: + InMemoryURLIndex* index_; // Call back to this index at completion. + bool succeeded_; // Indicates if the rebuild was successful. + scoped_ptr<URLIndexPrivateData> data_; // The rebuilt private data. + + DISALLOW_COPY_AND_ASSIGN(RebuildPrivateDataFromHistoryDBTask); + }; + // Initializes all index data members in preparation for restoring the index // from the cache or a complete rebuild from the history database. void ClearPrivateData(); - // Construct a file path for the cache file within the same directory where + // Constructs a file path for the cache file within the same directory where // the history database is kept and saves that path to |file_path|. Returns // true if |file_path| can be successfully constructed. (This function // provided as a hook for unit testing.) bool GetCacheFilePath(FilePath* file_path); + // Restores the index's private data from the cache file stored in the + // profile directory. + void RestoreFromCacheFile(); + + // Restores private_data_ from the given |path|. Runs on the UI thread. + // Provided for unit testing so that a test cache file can be used. + void DoRestoreFromCacheFile(const FilePath& path); + + // Schedules a history task to rebuild our private data from the history + // database. + void ScheduleRebuildFromHistory(); + + // Callback used by RebuildPrivateDataFromHistoryDBTask to signal completion + // or rebuilding our private data from the history database. |data| points to + // a new instance of the private data just rebuilt. This callback is only + // called upon a successful restore from the history database. + void DoneRebuidingPrivateDataFromHistoryDB(URLIndexPrivateData* data); + + // Rebuilds the history index from the history database in |history_db|. + // Used for unit testing only. + void RebuildFromHistory(HistoryDatabase* history_db); + + // Caches the index private data and writes the cache file to the profile + // directory. + void SaveToCacheFile(); + + // Saves private_data_ to the given |path|. Runs on the UI thread. + // Provided for unit testing so that a test cache file can be used. + void DoSaveToCacheFile(const FilePath& path); + + // Handles notifications of history changes. + virtual void Observe(int notification_type, + const content::NotificationSource& source, + const content::NotificationDetails& details) OVERRIDE; + + // Notification handlers. + void OnURLVisited(const URLVisitedDetails* details); + void OnURLsModified(const URLsModifiedDetails* details); + void OnURLsDeleted(const URLsDeletedDetails* details); + + // Returns a pointer to our private data. For unit testing only. + URLIndexPrivateData* private_data() { return private_data_.get(); } + + // The profile, may be null when testing. + Profile* profile_; + // Directory where cache file resides. This is, except when unit testing, // the same directory in which the profile's history database is found. It // should never be empty. @@ -127,12 +187,19 @@ class InMemoryURLIndex { // The index's durable private data. scoped_ptr<URLIndexPrivateData> private_data_; - // Set to true at shutdown when the cache has been written to disk. Used - // as a temporary safety check to insure that the cache is saved before - // the index has been destructed. + // Set to true once the shutdown process has begun. + bool shutdown_; + + CancelableRequestConsumer cache_reader_consumer_; + content::NotificationRegistrar registrar_; + + // 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 + // index has been destructed. // TODO(mrossetti): Eliminate once the transition to SQLite has been done. // http://crbug.com/83659 - bool cached_at_shutdown_; + bool needs_to_be_cached_; DISALLOW_COPY_AND_ASSIGN(InMemoryURLIndex); }; diff --git a/chrome/browser/history/in_memory_url_index_types.h b/chrome/browser/history/in_memory_url_index_types.h index 4881ae5..d13f913 100644 --- a/chrome/browser/history/in_memory_url_index_types.h +++ b/chrome/browser/history/in_memory_url_index_types.h @@ -139,9 +139,6 @@ typedef std::map<HistoryID, WordIDSet> HistoryIDWordMap; // A map from history_id to the history's URL and title. typedef std::map<HistoryID, URLRow> HistoryInfoMap; -// TODO(rohitrao): Probably replace this with QueryResults. -typedef std::vector<history::URLRow> URLRowVector; - } // namespace history #endif // CHROME_BROWSER_HISTORY_IN_MEMORY_URL_INDEX_TYPES_H_ diff --git a/chrome/browser/history/in_memory_url_index_unittest.cc b/chrome/browser/history/in_memory_url_index_unittest.cc index 6722354..24f202b 100644 --- a/chrome/browser/history/in_memory_url_index_unittest.cc +++ b/chrome/browser/history/in_memory_url_index_unittest.cc @@ -6,15 +6,21 @@ #include "base/file_path.h" #include "base/file_util.h" +#include "base/message_loop.h" #include "base/path_service.h" -#include "base/string16.h" #include "base/string_util.h" +#include "base/string16.h" #include "base/utf_string_conversions.h" #include "chrome/browser/autocomplete/autocomplete.h" -#include "chrome/browser/history/in_memory_database.h" -#include "chrome/browser/history/in_memory_url_index.h" +#include "chrome/browser/history/history.h" +#include "chrome/browser/history/history_backend.h" +#include "chrome/browser/history/history_database.h" #include "chrome/browser/history/in_memory_url_index_types.h" +#include "chrome/browser/history/in_memory_url_index.h" +#include "chrome/browser/history/url_index_private_data.h" #include "chrome/common/chrome_paths.h" +#include "chrome/test/base/testing_profile.h" +#include "content/test/test_browser_thread.h" #include "sql/transaction.h" #include "testing/gtest/include/gtest/gtest.h" @@ -31,10 +37,9 @@ namespace history { -class InMemoryURLIndexTest : public testing::Test, - public InMemoryDatabase { +class InMemoryURLIndexTest : public testing::Test { public: - InMemoryURLIndexTest() { InitFromScratch(); } + InMemoryURLIndexTest(); protected: // Test setup. @@ -62,10 +67,70 @@ class InMemoryURLIndexTest : public testing::Test, void CheckTerm(const URLIndexPrivateData::SearchTermCacheMap& cache, string16 term) const; + // Pass-through function to simplify our friendship with HistoryService. + sql::Connection& GetDB(); + + // Pass-through functions to simplify our friendship with InMemoryURLIndex. + URLIndexPrivateData* GetPrivateData() const; + bool GetCacheFilePath(FilePath* file_path) const; + void ClearHistoryDir() const; + + // Pass-through functions to simplify our friendship with URLIndexPrivateData. + bool UpdateURL(const URLRow& row); + bool DeleteURL(const GURL& url); + + MessageLoopForUI message_loop_; + content::TestBrowserThread ui_thread_; + content::TestBrowserThread file_thread_; + TestingProfile profile_; + scoped_ptr<InMemoryURLIndex> url_index_; + HistoryDatabase* history_database_; }; +InMemoryURLIndexTest::InMemoryURLIndexTest() + : ui_thread_(content::BrowserThread::UI, &message_loop_), + file_thread_(content::BrowserThread::FILE, &message_loop_) { +} + +sql::Connection& InMemoryURLIndexTest::GetDB() { + return history_database_->GetDB(); +} + +URLIndexPrivateData* InMemoryURLIndexTest::GetPrivateData() const { + DCHECK(url_index_->private_data_.get()); + return url_index_->private_data_.get(); +} + +bool InMemoryURLIndexTest::GetCacheFilePath(FilePath* file_path) const { + DCHECK(file_path); + return url_index_->GetCacheFilePath(file_path); +} + +void InMemoryURLIndexTest::ClearHistoryDir() const { + url_index_->history_dir_.clear(); +} + +bool InMemoryURLIndexTest::UpdateURL(const URLRow& row) { + return url_index_->private_data_->UpdateURL(row); +} + +bool InMemoryURLIndexTest::DeleteURL(const GURL& url) { + return url_index_->private_data_->DeleteURL(url); +} + void InMemoryURLIndexTest::SetUp() { + // We cannot access the database until the backend has been loaded. + profile_.CreateHistoryService(true, false); + profile_.CreateBookmarkModel(true); + profile_.BlockUntilBookmarkModelLoaded(); + profile_.BlockUntilHistoryProcessesPendingRequests(); + HistoryService* history_service = + profile_.GetHistoryService(Profile::EXPLICIT_ACCESS); + ASSERT_TRUE(history_service); + HistoryBackend* backend = history_service->history_backend_.get(); + history_database_ = backend->db(); + // Create and populate a working copy of the URL history database. FilePath history_proto_path; PathService::Get(chrome::DIR_TEST_DATA, &history_proto_path); @@ -79,6 +144,7 @@ void InMemoryURLIndexTest::SetUp() { char sql_cmd_line[kCommandBufferMaxSize]; sql::Connection& db(GetDB()); + ASSERT_TRUE(db.is_open()); { sql::Transaction transaction(&db); transaction.Begin(); @@ -110,15 +176,20 @@ void InMemoryURLIndexTest::SetUp() { transaction.Begin(); while (statement.Step()) { URLRow row; - FillURLRow(statement, &row); + history_database_->FillURLRow(statement, &row); base::Time last_visit = time_right_now; for (int64 i = row.last_visit().ToInternalValue(); i > 0; --i) last_visit -= day_delta; row.set_last_visit(last_visit); - UpdateURLRow(row.id(), row); + history_database_->UpdateURLRow(row.id(), row); } transaction.Commit(); } + + url_index_.reset( + new InMemoryURLIndex(&profile_, FilePath(), "en,ja,hi,zh")); + url_index_->Init(); + url_index_->RebuildFromHistory(history_database_); } FilePath::StringType InMemoryURLIndexTest::TestDBName() const { @@ -195,21 +266,19 @@ FilePath::StringType LimitedInMemoryURLIndexTest::TestDBName() const { return FILE_PATH_LITERAL("url_history_provider_test_limited.db.txt"); } -TEST_F(InMemoryURLIndexTest, Construction) { - url_index_.reset(new InMemoryURLIndex(FilePath())); - EXPECT_TRUE(url_index_.get()); -} - TEST_F(LimitedInMemoryURLIndexTest, Initialization) { // Verify that the database contains the expected number of items, which // is the pre-filtered count, i.e. all of the items. - sql::Statement statement(GetDB().GetUniqueStatement("SELECT * FROM urls;")); + sql::Connection& db(GetDB()); + sql::Statement statement(db.GetUniqueStatement("SELECT * FROM urls;")); ASSERT_TRUE(statement.is_valid()); uint64 row_count = 0; while (statement.Step()) ++row_count; EXPECT_EQ(1U, row_count); - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); + url_index_.reset( + new InMemoryURLIndex(&profile_, FilePath(), "en,ja,hi,zh")); + url_index_->Init(); + url_index_->RebuildFromHistory(history_database_); URLIndexPrivateData& private_data(*(url_index_->private_data_)); // history_info_map_ should have the same number of items as were filtered. @@ -219,10 +288,6 @@ TEST_F(LimitedInMemoryURLIndexTest, Initialization) { } TEST_F(InMemoryURLIndexTest, Retrieval) { - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); - // The term will be lowercased by the search. - // See if a very specific term gives a single result. ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(ASCIIToUTF16("DrudgeReport")); @@ -292,9 +357,6 @@ TEST_F(InMemoryURLIndexTest, Retrieval) { } TEST_F(InMemoryURLIndexTest, URLPrefixMatching) { - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); - // "drudgere" - found, can inline ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(ASCIIToUTF16("drudgere")); @@ -355,9 +417,6 @@ TEST_F(InMemoryURLIndexTest, URLPrefixMatching) { } TEST_F(InMemoryURLIndexTest, ProperStringMatching) { - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); - // Search for the following with the expected results: // "atdmt view" - found // "atdmt.view" - not found @@ -372,21 +431,18 @@ TEST_F(InMemoryURLIndexTest, ProperStringMatching) { } TEST_F(InMemoryURLIndexTest, HugeResultSet) { - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); - // Create a huge set of qualifying history items. for (URLID row_id = 5000; row_id < 6000; ++row_id) { URLRow new_row(GURL("http://www.brokeandaloneinmanitoba.com/"), row_id); new_row.set_last_visit(base::Time::Now()); - url_index_->UpdateURL(row_id, new_row); + EXPECT_TRUE(UpdateURL(new_row)); } ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(ASCIIToUTF16("b")); + URLIndexPrivateData& private_data(*GetPrivateData()); ASSERT_EQ(AutocompleteProvider::kMaxMatches, matches.size()); // There are 7 matches already in the database. - URLIndexPrivateData& private_data(*(url_index_->private_data_.get())); ASSERT_EQ(1008U, private_data.pre_filter_item_count_); ASSERT_EQ(500U, private_data.post_filter_item_count_); ASSERT_EQ(AutocompleteProvider::kMaxMatches, @@ -394,10 +450,8 @@ TEST_F(InMemoryURLIndexTest, HugeResultSet) { } TEST_F(InMemoryURLIndexTest, TitleSearch) { - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); // Signal if someone has changed the test DB. - EXPECT_EQ(28U, url_index_->private_data_->history_info_map_.size()); + EXPECT_EQ(28U, GetPrivateData()->history_info_map_.size()); // Ensure title is being searched. ScoredHistoryMatches matches = @@ -414,9 +468,6 @@ TEST_F(InMemoryURLIndexTest, TitleSearch) { } TEST_F(InMemoryURLIndexTest, TitleChange) { - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); - // Verify current title terms retrieves desired item. string16 original_terms = ASCIIToUTF16("lebronomics could high taxes influence"); @@ -441,7 +492,7 @@ TEST_F(InMemoryURLIndexTest, TitleChange) { // Update the row. old_row.set_title(ASCIIToUTF16("Does eat oats and little lambs eat ivy")); - url_index_->UpdateURL(expected_id, old_row); + EXPECT_TRUE(UpdateURL(old_row)); // Verify we get the row using the new terms but not the original terms. matches = url_index_->HistoryItemsForTerms(new_terms); @@ -452,9 +503,6 @@ TEST_F(InMemoryURLIndexTest, TitleChange) { } TEST_F(InMemoryURLIndexTest, NonUniqueTermCharacterSets) { - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); - // The presence of duplicate characters should succeed. Exercise by cycling // through a string with several duplicate characters. ScoredHistoryMatches matches = @@ -488,11 +536,8 @@ TEST_F(InMemoryURLIndexTest, TypedCharacterCaching) { typedef URLIndexPrivateData::SearchTermCacheMap::iterator CacheIter; typedef URLIndexPrivateData::SearchTermCacheItem CacheItem; - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); - URLIndexPrivateData::SearchTermCacheMap& cache( - url_index_->private_data_->search_term_cache_); + GetPrivateData()->search_term_cache_); // The cache should be empty at this point. EXPECT_EQ(0U, cache.size()); @@ -582,9 +627,6 @@ TEST_F(InMemoryURLIndexTest, Scoring) { } TEST_F(InMemoryURLIndexTest, AddNewRows) { - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); - // Verify that the row we're going to add does not already exist. URLID new_row_id = 87654321; // Newly created URLRows get a last_visit time of 'right now' so it should @@ -593,32 +635,39 @@ TEST_F(InMemoryURLIndexTest, AddNewRows) { ASCIIToUTF16("brokeandalone")).empty()); // Add a new row. - URLRow new_row(GURL("http://www.brokeandaloneinmanitoba.com/"), new_row_id); + URLRow new_row(GURL("http://www.brokeandaloneinmanitoba.com/"), new_row_id++); new_row.set_last_visit(base::Time::Now()); - url_index_->UpdateURL(new_row_id, new_row); + EXPECT_TRUE(UpdateURL(new_row)); // Verify that we can retrieve it. EXPECT_EQ(1U, url_index_->HistoryItemsForTerms( ASCIIToUTF16("brokeandalone")).size()); - // Add it again just to be sure that is harmless. - url_index_->UpdateURL(new_row_id, new_row); + // Add it again just to be sure that is harmless and that it does not update + // the index. + EXPECT_FALSE(UpdateURL(new_row)); EXPECT_EQ(1U, url_index_->HistoryItemsForTerms( ASCIIToUTF16("brokeandalone")).size()); + + // Make up an URL that does not qualify and try to add it. + URLRow unqualified_row(GURL("http://www.brokeandaloneinmanitoba.com/"), + new_row_id++); + EXPECT_FALSE(UpdateURL(new_row)); } TEST_F(InMemoryURLIndexTest, DeleteRows) { - url_index_.reset(new InMemoryURLIndex(FilePath())); - url_index_->Init(this, "en,ja,hi,zh"); - ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(ASCIIToUTF16("DrudgeReport")); ASSERT_EQ(1U, matches.size()); // Determine the row id for that result, delete that id, then search again. - url_index_->DeleteURL(matches[0].url_info.id()); + EXPECT_TRUE(DeleteURL(matches[0].url_info.url())); EXPECT_TRUE(url_index_->HistoryItemsForTerms( ASCIIToUTF16("DrudgeReport")).empty()); + + // Make up an URL that does not exist in the database and delete it. + GURL url("http://www.hokeypokey.com/putyourrightfootin.html"); + EXPECT_FALSE(DeleteURL(url)); } TEST_F(InMemoryURLIndexTest, WhitelistedURLs) { @@ -694,8 +743,8 @@ TEST_F(InMemoryURLIndexTest, WhitelistedURLs) { { "xmpp:node@example.com", false }, { "xmpp://guest@example.com", false }, }; - url_index_.reset(new InMemoryURLIndex(FilePath())); - URLIndexPrivateData& private_data(*(url_index_->private_data_.get())); + + URLIndexPrivateData& private_data(*GetPrivateData()); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(data); ++i) { GURL url(data[i].url_spec); EXPECT_EQ(data[i].expected_is_whitelisted, @@ -703,31 +752,10 @@ TEST_F(InMemoryURLIndexTest, WhitelistedURLs) { } } -TEST_F(InMemoryURLIndexTest, CacheFilePath) { - url_index_.reset(new InMemoryURLIndex(FilePath(FILE_PATH_LITERAL( - "/flammmy/frammy/")))); - FilePath full_file_path; - url_index_->GetCacheFilePath(&full_file_path); - std::vector<FilePath::StringType> expected_parts; - FilePath(FILE_PATH_LITERAL("/flammmy/frammy/History Provider Cache")). - GetComponents(&expected_parts); - std::vector<FilePath::StringType> actual_parts; - full_file_path.GetComponents(&actual_parts); - ASSERT_EQ(expected_parts.size(), actual_parts.size()); - size_t count = expected_parts.size(); - for (size_t i = 0; i < count; ++i) - EXPECT_EQ(expected_parts[i], actual_parts[i]); - // Must clear the history_dir_ to satisfy the dtor's DCHECK. - url_index_->history_dir_.clear(); -} - TEST_F(InMemoryURLIndexTest, CacheSaveRestore) { // Save the cache to a protobuf, restore it, and compare the results. - url_index_.reset(new InMemoryURLIndex(FilePath())); - InMemoryURLIndex& url_index(*(url_index_.get())); - url_index.Init(this, "en,ja,hi,zh"); in_memory_url_index::InMemoryURLIndexCacheItem index_cache; - URLIndexPrivateData& private_data(*(url_index_->private_data_.get())); + URLIndexPrivateData& private_data(*GetPrivateData()); private_data.SavePrivateData(&index_cache); // Capture our private data so we can later compare for equality. @@ -797,4 +825,39 @@ TEST_F(InMemoryURLIndexTest, CacheSaveRestore) { } } +class InMemoryURLIndexCacheTest : public testing::Test { + public: + InMemoryURLIndexCacheTest() {} + + protected: + virtual void SetUp() OVERRIDE; + + ScopedTempDir temp_dir_; + scoped_ptr<InMemoryURLIndex> url_index_; +}; + +void InMemoryURLIndexCacheTest::SetUp() { + ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); + FilePath path(temp_dir_.path()); + url_index_.reset( + new InMemoryURLIndex(NULL, path, "en,ja,hi,zh")); +} + +TEST_F(InMemoryURLIndexCacheTest, CacheFilePath) { + FilePath expectedPath = + temp_dir_.path().Append(FILE_PATH_LITERAL("History Provider Cache")); + std::vector<FilePath::StringType> expected_parts; + expectedPath.GetComponents(&expected_parts); + FilePath full_file_path; + ASSERT_TRUE(url_index_->GetCacheFilePath(&full_file_path)); + std::vector<FilePath::StringType> actual_parts; + full_file_path.GetComponents(&actual_parts); + ASSERT_EQ(expected_parts.size(), actual_parts.size()); + size_t count = expected_parts.size(); + for (size_t i = 0; i < count; ++i) + EXPECT_EQ(expected_parts[i], actual_parts[i]); + // Must clear the history_dir_ to satisfy the dtor's DCHECK. + url_index_->history_dir_.clear(); +} + } // namespace history diff --git a/chrome/browser/history/url_index_private_data.cc b/chrome/browser/history/url_index_private_data.cc index bc18583..e7a72f6 100644 --- a/chrome/browser/history/url_index_private_data.cc +++ b/chrome/browser/history/url_index_private_data.cc @@ -17,7 +17,7 @@ #include "base/threading/thread_restrictions.h" #include "base/utf_string_conversions.h" #include "chrome/browser/autocomplete/autocomplete.h" -#include "chrome/browser/history/url_database.h" +#include "chrome/browser/history/history_database.h" #include "chrome/common/url_constants.h" #include "net/base/net_util.h" #include "third_party/protobuf/src/google/protobuf/repeated_field.h" @@ -135,12 +135,12 @@ void URLIndexPrivateData::Clear() { // Cache Updating -------------------------------------------------------------- -void URLIndexPrivateData::IndexRow(const URLRow& row) { +bool URLIndexPrivateData::IndexRow(const URLRow& row) { const GURL& gurl(row.url()); // Index only URLs with a whitelisted scheme. if (!URLIndexPrivateData::URLSchemeIsWhitelisted(gurl)) - return; + return false; URLID row_id = row.id(); // Strip out username and password before saving and indexing. @@ -162,7 +162,7 @@ void URLIndexPrivateData::IndexRow(const URLRow& row) { // Index the words contained in the URL and title of the row. AddRowWordsToIndex(new_row); - return; + return true; } void URLIndexPrivateData::AddRowWordsToIndex(const URLRow& row) { @@ -295,53 +295,78 @@ void URLIndexPrivateData::AddToHistoryIDWordMap(HistoryID history_id, } } -void URLIndexPrivateData::UpdateURL(URLID row_id, const URLRow& row) { +bool URLIndexPrivateData::UpdateURL(const URLRow& row) { // The row may or may not already be in our index. If it is not already // indexed and it qualifies then it gets indexed. If it is already // indexed and still qualifies then it gets updated, otherwise it // is deleted from the index. + bool row_was_updated = false; + URLID row_id = row.id(); HistoryInfoMap::iterator row_pos = history_info_map_.find(row_id); if (row_pos == history_info_map_.end()) { // This new row should be indexed if it qualifies. URLRow new_row(row); new_row.set_id(row_id); - if (RowQualifiesAsSignificant(new_row, base::Time())) - IndexRow(new_row); + row_was_updated = + RowQualifiesAsSignificant(new_row, base::Time()) && IndexRow(new_row); } else if (RowQualifiesAsSignificant(row, base::Time())) { // This indexed row still qualifies and will be re-indexed. // The url won't have changed but the title, visit count, etc. // might have changed. - URLRow& updated_row = row_pos->second; - updated_row.set_visit_count(row.visit_count()); - updated_row.set_typed_count(row.typed_count()); - updated_row.set_last_visit(row.last_visit()); - // While the URL is guaranteed to remain stable, the title may have changed. - // If so, then we need to update the index with the changed words. - if (updated_row.title() != row.title()) { - // Clear all words associated with this row and re-index both the - // URL and title. - RemoveRowWordsFromIndex(updated_row); - updated_row.set_title(row.title()); - AddRowWordsToIndex(updated_row); + URLRow& row_to_update = row_pos->second; + bool title_updated = row_to_update.title() != row.title(); + if (row_to_update.visit_count() != row.visit_count() || + row_to_update.typed_count() != row.typed_count() || + row_to_update.last_visit() != row.last_visit() || title_updated) { + row_to_update.set_visit_count(row.visit_count()); + row_to_update.set_typed_count(row.typed_count()); + row_to_update.set_last_visit(row.last_visit()); + // While the URL is guaranteed to remain stable, the title may have + // changed. If so, then update the index with the changed words. + if (title_updated) { + // Clear all words associated with this row and re-index both the + // URL and title. + RemoveRowWordsFromIndex(row_to_update); + row_to_update.set_title(row.title()); + AddRowWordsToIndex(row_to_update); + } + row_was_updated = true; } } else { // This indexed row no longer qualifies and will be de-indexed by // clearing all words associated with this row. - URLRow& removed_row = row_pos->second; - RemoveRowFromIndex(removed_row); + RemoveRowFromIndex(row); + row_was_updated = true; } - // This invalidates the cache. - search_term_cache_.clear(); + if (row_was_updated) + search_term_cache_.clear(); // This invalidates the cache. + return row_was_updated; } -void URLIndexPrivateData::DeleteURL(URLID row_id) { - // Note that this does not remove any reference to this row from the - // word_id_history_map_. That map will continue to contain (and return) - // hits against this row until that map is rebuilt, but since the - // history_info_map_ no longer references the row no erroneous results - // will propagate to the user. - history_info_map_.erase(row_id); - search_term_cache_.clear(); // This invalidates the word cache. +// Helper functor for DeleteURL. +class HistoryInfoMapItemHasURL { + public: + explicit HistoryInfoMapItemHasURL(const GURL& url): url_(url) {} + + bool operator()(const std::pair<const HistoryID, URLRow>& item) { + return item.second.url() == url_; + } + + private: + const GURL& url_; +}; + +bool URLIndexPrivateData::DeleteURL(const GURL& url) { + // Find the matching entry in the history_info_map_. + HistoryInfoMap::iterator pos = std::find_if( + history_info_map_.begin(), + history_info_map_.end(), + HistoryInfoMapItemHasURL(url)); + if (pos == history_info_map_.end()) + return false; + RemoveRowFromIndex(pos->second); + search_term_cache_.clear(); // This invalidates the cache. + return true; } bool URLIndexPrivateData::URLSchemeIsWhitelisted(const GURL& gurl) const { @@ -975,32 +1000,18 @@ void URLIndexPrivateData::SaveHistoryInfoMap( // Cache Restoring ------------------------------------------------------------- -bool URLIndexPrivateData::ReloadFromHistory(history::URLDatabase* history_db) { - Clear(); - - if (!history_db) - return false; - - base::TimeTicks beginning_time = base::TimeTicks::Now(); - URLDatabase::URLEnumerator history_enum; - if (!history_db->InitURLEnumeratorForSignificant(&history_enum)) - return false; - URLRow row; - while (history_enum.GetNextURL(&row)) - IndexRow(row); - UMA_HISTOGRAM_TIMES("History.InMemoryURLIndexingTime", - base::TimeTicks::Now() - beginning_time); - return true; -} - bool URLIndexPrivateData::RestoreFromFile(const FilePath& file_path) { // TODO(mrossetti): Figure out how to determine if the cache is up-to-date. // That is: ensure that the database has not been modified since the cache // was last saved. DB file modification date is inadequate. There are no // SQLite table checksums automatically stored. + Clear(); // Start with a clean slate. + // FIXME(mrossetti): Move File IO to another thread. base::ThreadRestrictions::ScopedAllowIO allow_io; base::TimeTicks beginning_time = base::TimeTicks::Now(); + if (!file_util::PathExists(file_path)) + return false; std::string data; // If there is no cache file then simply give up. This will cause us to // attempt to rebuild from the history database. @@ -1029,6 +1040,32 @@ bool URLIndexPrivateData::RestoreFromFile(const FilePath& file_path) { return true; } +// static +URLIndexPrivateData* URLIndexPrivateData::RebuildFromHistory( + HistoryDatabase* history_db) { + if (!history_db) + return NULL; + + base::TimeTicks beginning_time = base::TimeTicks::Now(); + + scoped_ptr<URLIndexPrivateData> rebuilt_data(new URLIndexPrivateData); + URLDatabase::URLEnumerator history_enum; + if (!history_db->InitURLEnumeratorForSignificant(&history_enum)) + return NULL; + for (URLRow row; history_enum.GetNextURL(&row); ) + rebuilt_data->IndexRow(row); + + UMA_HISTOGRAM_TIMES("History.InMemoryURLIndexingTime", + base::TimeTicks::Now() - beginning_time); + UMA_HISTOGRAM_COUNTS("History.InMemoryURLHistoryItems", + rebuilt_data->history_id_word_map_.size()); + UMA_HISTOGRAM_COUNTS_10000("History.InMemoryURLWords", + rebuilt_data->word_map_.size()); + UMA_HISTOGRAM_COUNTS_10000("History.InMemoryURLChars", + rebuilt_data->char_word_map_.size()); + return rebuilt_data.release(); +} + bool URLIndexPrivateData::RestorePrivateData( const InMemoryURLIndexCacheItem& cache) { return RestoreWordList(cache) && RestoreWordMap(cache) && diff --git a/chrome/browser/history/url_index_private_data.h b/chrome/browser/history/url_index_private_data.h index c928ded..debb199 100644 --- a/chrome/browser/history/url_index_private_data.h +++ b/chrome/browser/history/url_index_private_data.h @@ -11,6 +11,8 @@ #include "chrome/browser/history/in_memory_url_index_types.h" #include "chrome/browser/history/in_memory_url_index_cache.pb.h" +class HistoryQuickProviderTest; + namespace in_memory_url_index { class InMemoryURLIndexCacheItem; } @@ -19,6 +21,8 @@ namespace history { namespace imui = in_memory_url_index; +class HistoryDatabase; + // A structure describing the InMemoryURLIndex's internal data and providing for // restoring, rebuilding and updating that internal data. class URLIndexPrivateData { @@ -27,8 +31,9 @@ class URLIndexPrivateData { ~URLIndexPrivateData(); private: - friend class InMemoryURLIndex; friend class AddHistoryMatch; + friend class ::HistoryQuickProviderTest; + friend class InMemoryURLIndex; friend class InMemoryURLIndexTest; FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, CacheSaveRestore); FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, HugeResultSet); @@ -130,13 +135,15 @@ class URLIndexPrivateData { // profile directory and returns true if successful. bool RestoreFromFile(const FilePath& file_path); + // Constructs a new object by rebuilding its contents from the history + // database in |history_db|. Returns the new URLIndexPrivateData which on + // success will contain the rebuilt data but upon failure will be empty. + static URLIndexPrivateData* RebuildFromHistory(HistoryDatabase* history_db); + // Caches the index private data and writes the cache file to the profile // directory. bool SaveToFile(const FilePath& file_path); - // Reloads the history index from |history_db|. - bool ReloadFromHistory(URLDatabase* history_db); - // Initializes all index data members in preparation for restoring the index // from the cache or a complete rebuild from the history database. void Clear(); @@ -153,17 +160,22 @@ class URLIndexPrivateData { // URL History indexing support functions. - // Indexes one URL history item. - void IndexRow(const URLRow& row); - - // Updates or adds an history item to the index if it meets the minimum - // 'quick' criteria. - void UpdateURL(URLID row_id, const URLRow& row); - - // Deletes indexing data for an history item. The item may not have actually - // been indexed (which is the case if it did not previously meet minimum - // 'quick' criteria). - void DeleteURL(URLID row_id); + // Indexes one URL history item as described by |row|. Returns true if the + // row was actually indexed. + bool IndexRow(const URLRow& row); + + // Adds the history item in |row| to the index if it does not already already + // exist and it meets the minimum 'quick' criteria. If the row already exists + // in the index then the index will be updated if the row still meets the + // criteria, otherwise the row will be removed from the index. Returns true + // if the index was actually updated. + bool UpdateURL(const URLRow& row); + + // Deletes indexing data for the history item with the URL given in |url|. + // The item may not have actually been indexed, which is the case if it did + // not previously meet minimum 'quick' criteria. Returns true if the index + // was actually updated. + bool DeleteURL(const GURL& url); // Parses and indexes the words in the URL and page title of |row|. void AddRowWordsToIndex(const URLRow& row); |