diff options
author | zmo@google.com <zmo@google.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-11-24 02:33:20 +0000 |
---|---|---|
committer | zmo@google.com <zmo@google.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-11-24 02:33:20 +0000 |
commit | 5c88980fa29b793fbce68c9d89849ed1ed13321e (patch) | |
tree | 08e5ae1eacf26b769a56197b0a70ecfc9616ad88 /chrome/browser/history | |
parent | 8cd74da71a1d6ac754b3408c41ee40a41204e726 (diff) | |
download | chromium_src-5c88980fa29b793fbce68c9d89849ed1ed13321e.zip chromium_src-5c88980fa29b793fbce68c9d89849ed1ed13321e.tar.gz chromium_src-5c88980fa29b793fbce68c9d89849ed1ed13321e.tar.bz2 |
Revert 111378 - HQP Refactoring (in Preparation for SQLite Cache)
(See crbug.com/105340 for reverting reason and how to reproduce the issue locally)
1. Move ownership of the InMemoryURLIndex from the InMemoryHistoryBackend to the HistoryService, where it truly belongs.
2. Handle (by notification) URL visits, updates and deletes. Refactor use of NOTIFICATION_HISTORY_URLS_DELETED to provide the deleted URLRow so that row ID is available.
3. Correctly handle the adding and removing of page title words when a URL change is detected.
4. Other small cleanups.
BUG=96731, 92718
TEST=Unit tests updated.
TBR=atwilson (for profile_sync_service_typed_url_unittest.cc)
Previously reviewed as: http://codereview.chromium.org/8384024/
Review URL: http://codereview.chromium.org/8451009
TBR=mrossetti@chromium.org
Review URL: http://codereview.chromium.org/8662035
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@111482 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser/history')
-rw-r--r-- | chrome/browser/history/expire_history_backend.cc | 4 | ||||
-rw-r--r-- | chrome/browser/history/history.cc | 28 | ||||
-rw-r--r-- | chrome/browser/history/history.h | 12 | ||||
-rw-r--r-- | chrome/browser/history/history_backend.cc | 4 | ||||
-rw-r--r-- | chrome/browser/history/history_backend.h | 3 | ||||
-rw-r--r-- | chrome/browser/history/history_backend_unittest.cc | 2 | ||||
-rw-r--r-- | chrome/browser/history/history_notifications.h | 15 | ||||
-rw-r--r-- | chrome/browser/history/history_unittest.cc | 4 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_history_backend.cc | 41 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_history_backend.h | 6 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_url_index.cc | 161 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_url_index.h | 62 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_url_index_unittest.cc | 152 |
13 files changed, 204 insertions, 290 deletions
diff --git a/chrome/browser/history/expire_history_backend.cc b/chrome/browser/history/expire_history_backend.cc index 75110e3..0f2ea68 100644 --- a/chrome/browser/history/expire_history_backend.cc +++ b/chrome/browser/history/expire_history_backend.cc @@ -351,10 +351,8 @@ void ExpireHistoryBackend::BroadcastDeleteNotifications( // determine if they care whether anything was deleted). URLsDeletedDetails* deleted_details = new URLsDeletedDetails; deleted_details->all_history = false; - for (size_t i = 0; i < dependencies->deleted_urls.size(); i++) { - deleted_details->rows.push_back(dependencies->deleted_urls[i]); + for (size_t i = 0; i < dependencies->deleted_urls.size(); i++) deleted_details->urls.insert(dependencies->deleted_urls[i].url()); - } delegate_->BroadcastNotifications( chrome::NOTIFICATION_HISTORY_URLS_DELETED, deleted_details); } diff --git a/chrome/browser/history/history.cc b/chrome/browser/history/history.cc index 013e175..17ba93e 100644 --- a/chrome/browser/history/history.cc +++ b/chrome/browser/history/history.cc @@ -25,7 +25,6 @@ #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" @@ -38,7 +37,6 @@ #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" @@ -46,7 +44,6 @@ #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" @@ -154,15 +151,6 @@ HistoryService::HistoryService(Profile* profile) no_db_(false), needs_top_sites_migration_(false) { DCHECK(profile_); - if (!CommandLine::ForCurrentProcess()->HasSwitch( - switches::kDisableHistoryQuickProvider)) { - in_memory_url_index_.reset( - new history::InMemoryURLIndex(profile_, history_dir_)); - std::string languages; - PrefService* prefs = profile_->GetPrefs(); - languages = prefs->GetString(prefs::kAcceptLanguages); - in_memory_url_index_->Init(languages); - } registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URLS_DELETED, content::Source<Profile>(profile_)); registrar_.Add(this, chrome::NOTIFICATION_TEMPLATE_URL_REMOVED, @@ -229,10 +217,6 @@ void HistoryService::Cleanup() { return; } - // Give the InMemoryURLIndex a chance to shutdown. - if (in_memory_url_index_.get()) - in_memory_url_index_->ShutDown(); - // Unload the backend. UnloadBackend(); @@ -259,12 +243,14 @@ history::URLDatabase* HistoryService::InMemoryDatabase() { return NULL; } -history::URLDatabase* HistoryService::HistoryDatabase() { - return history_backend_->database(); -} - history::InMemoryURLIndex* HistoryService::InMemoryIndex() { - return in_memory_url_index_.get(); + // 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) { diff --git a/chrome/browser/history/history.h b/chrome/browser/history/history.h index 82aafc1..b205dc4 100644 --- a/chrome/browser/history/history.h +++ b/chrome/browser/history/history.h @@ -143,8 +143,8 @@ class HistoryService : public CancelableRequestProvider, // TODO(brettw) this should return the InMemoryHistoryBackend. history::URLDatabase* InMemoryDatabase(); - // Returns the history database if it has been set, otherwise NULL. - history::URLDatabase* HistoryDatabase(); + // Return the quick history index. + history::InMemoryURLIndex* InMemoryIndex(); // Navigation ---------------------------------------------------------------- @@ -502,11 +502,6 @@ class HistoryService : public CancelableRequestProvider, CancelableRequestConsumerBase* consumer, const GetMostRecentKeywordSearchTermsCallback& callback); - // InMemoryURLIndex ---------------------------------------------------------- - - // Returns the quick history index. - history::InMemoryURLIndex* InMemoryIndex(); - // Bookmarks ----------------------------------------------------------------- // Notification that a URL is no longer bookmarked. @@ -850,9 +845,6 @@ class HistoryService : public CancelableRequestProvider, // on the background thread. scoped_ptr<history::InMemoryHistoryBackend> in_memory_backend_; - // The index used for quick history lookups. - scoped_ptr<history::InMemoryURLIndex> in_memory_url_index_; - // The profile, may be null when testing. Profile* profile_; diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc index 540fab6..18cc3cb 100644 --- a/chrome/browser/history/history_backend.cc +++ b/chrome/browser/history/history_backend.cc @@ -9,6 +9,7 @@ #include <set> #include <vector> +#include "base/command_line.h" #include "base/compiler_specific.h" #include "base/file_util.h" #include "base/memory/scoped_ptr.h" @@ -217,7 +218,7 @@ HistoryBackend::~HistoryBackend() { DCHECK(!scheduled_commit_) << "Deleting without cleanup"; ReleaseDBTasks(); - // Close the databases before optionally running the "destroy" task. + // First close the databases before optionally running the "destroy" task. if (db_.get()) { // Commit the long-running transaction. db_->CommitTransaction(); @@ -884,7 +885,6 @@ void HistoryBackend::SetPageTitle(const GURL& url, if (row_id && row.title() != title) { row.set_title(title); db_->UpdateURLRow(row_id, row); - row.id_ = row_id; changed_urls.push_back(row); if (row.typed_count() > 0) typed_url_changed = true; diff --git a/chrome/browser/history/history_backend.h b/chrome/browser/history/history_backend.h index 9fadc22..9600df1 100644 --- a/chrome/browser/history/history_backend.h +++ b/chrome/browser/history/history_backend.h @@ -283,9 +283,6 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, // Generic operations -------------------------------------------------------- - // Returns our database if it has been set, otherwise NULL. - URLDatabase* database() { return db_.get(); } - void ProcessDBTask(scoped_refptr<HistoryDBTaskRequest> request); virtual bool GetAllTypedURLs(std::vector<history::URLRow>* urls); diff --git a/chrome/browser/history/history_backend_unittest.cc b/chrome/browser/history/history_backend_unittest.cc index 8d95ccf..e5484bf 100644 --- a/chrome/browser/history/history_backend_unittest.cc +++ b/chrome/browser/history/history_backend_unittest.cc @@ -19,7 +19,6 @@ #include "chrome/browser/history/history_notifications.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/common/chrome_constants.h" #include "chrome/common/chrome_paths.h" #include "chrome/common/thumbnail_score.h" @@ -147,6 +146,7 @@ class HistoryBackendTest : public testing::Test { BookmarkModel bookmark_model_; + protected: bool loaded_; private: diff --git a/chrome/browser/history/history_notifications.h b/chrome/browser/history/history_notifications.h index 5e4bf17..ab9d555 100644 --- a/chrome/browser/history/history_notifications.h +++ b/chrome/browser/history/history_notifications.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2006-2008 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 NOTIFICATION_HISTORY_URL_VISITED. +// Details for HISTORY_URL_VISITED. struct URLVisitedDetails : public HistoryDetails { URLVisitedDetails(); virtual ~URLVisitedDetails(); @@ -40,7 +40,7 @@ struct URLVisitedDetails : public HistoryDetails { history::RedirectList redirects; }; -// Details for NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED. +// Details for NOTIFY_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 NOTIFICATION_HISTORY_URLS_DELETED. +// Details for NOTIFY_HISTORY_URLS_DELETED. struct URLsDeletedDetails : public HistoryDetails { URLsDeletedDetails(); virtual ~URLsDeletedDetails(); @@ -57,14 +57,9 @@ 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. (This information can be gleaned from - // |rows| but, since there are several clients that need the set, we - // pre-build it so that the clients don't have to.) + // we do not bother to list all URLs. std::set<GURL> urls; }; diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc index 587bcd3..ceb9608 100644 --- a/chrome/browser/history/history_unittest.cc +++ b/chrome/browser/history/history_unittest.cc @@ -119,8 +119,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 c0c1ca4..f1e1730 100644 --- a/chrome/browser/history/in_memory_history_backend.cc +++ b/chrome/browser/history/in_memory_history_backend.cc @@ -13,24 +13,37 @@ #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 "content/public/browser/notification_service.h" +#include "chrome/common/chrome_switches.h" +#include "content/public/browser/notification_details.h" +#include "content/public/browser/notification_source.h" namespace history { InMemoryHistoryBackend::InMemoryHistoryBackend() - : profile_(NULL) {} + : profile_(NULL) { +} -InMemoryHistoryBackend::~InMemoryHistoryBackend() {} +InMemoryHistoryBackend::~InMemoryHistoryBackend() { + if (index_.get()) + index_->ShutDown(); +} bool InMemoryHistoryBackend::Init(const FilePath& history_filename, const FilePath& history_dir, URLDatabase* db, const std::string& languages) { db_.reset(new InMemoryDatabase); - return db_->InitFromDisk(history_filename); + bool success = db_->InitFromDisk(history_filename); + if (!CommandLine::ForCurrentProcess()->HasSwitch( + switches::kDisableHistoryQuickProvider)) { + index_.reset(new InMemoryURLIndex(history_dir)); + index_->Init(db, languages); + } + return success; } void InMemoryHistoryBackend::AttachToHistoryService(Profile* profile) { @@ -118,26 +131,36 @@ void InMemoryHistoryBackend::OnTypedURLsModified( db_->UpdateURLRow(id, *i); else id = db_->AddURL(*i); + if (index_.get()) + index_->UpdateURL(id, *i); } } void InMemoryHistoryBackend::OnURLsDeleted(const URLsDeletedDetails& details) { DCHECK(db_.get()); + if (details.all_history) { // When all history is deleted, the individual URLs won't be listed. Just // create a new database to quickly clear everything out. db_.reset(new InMemoryDatabase); if (!db_->InitFromScratch()) db_.reset(); + if (index_.get()) + index_->ReloadFromHistory(db_.get(), true); return; } // Delete all matching URLs in our database. - for (std::vector<URLRow>::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()); + 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); + } } } diff --git a/chrome/browser/history/in_memory_history_backend.h b/chrome/browser/history/in_memory_history_backend.h index 11348e7..16b2907 100644 --- a/chrome/browser/history/in_memory_history_backend.h +++ b/chrome/browser/history/in_memory_history_backend.h @@ -69,6 +69,9 @@ 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); @@ -92,6 +95,9 @@ 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 dad1b75..3dbbb79 100644 --- a/chrome/browser/history/in_memory_url_index.cc +++ b/chrome/browser/history/in_memory_url_index.cc @@ -13,17 +13,20 @@ #include "base/file_util.h" #include "base/i18n/case_conversion.h" #include "base/metrics/histogram.h" +#include "base/string_util.h" #include "base/threading/thread_restrictions.h" +#include "base/time.h" #include "base/utf_string_conversions.h" #include "chrome/browser/autocomplete/autocomplete.h" -#include "chrome/browser/history/history.h" -#include "chrome/browser/history/history_notifications.h" +#include "chrome/browser/autocomplete/history_provider_util.h" #include "chrome/browser/history/url_database.h" #include "chrome/browser/profiles/profile.h" -#include "chrome/common/chrome_notification_types.h" #include "chrome/common/url_constants.h" -#include "content/public/browser/notification_service.h" +#include "googleurl/src/url_parse.h" +#include "googleurl/src/url_util.h" +#include "net/base/escape.h" #include "net/base/net_util.h" +#include "third_party/protobuf/src/google/protobuf/repeated_field.h" #include "ui/base/l10n/l10n_util.h" using google::protobuf::RepeatedField; @@ -109,20 +112,11 @@ int ScoreForValue(int value, const int* value_ranks) { return score; } -InMemoryURLIndex::InMemoryURLIndex(Profile* profile, - const FilePath& history_dir) - : profile_(profile), - history_dir_(history_dir), +InMemoryURLIndex::InMemoryURLIndex(const FilePath& history_dir) + : history_dir_(history_dir), private_data_(new URLIndexPrivateData), cached_at_shutdown_(false) { InMemoryURLIndex::InitializeSchemeWhitelist(&scheme_whitelist_); - if (profile) { - 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. @@ -135,7 +129,6 @@ InMemoryURLIndex::InMemoryURLIndex() 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. - registrar_.RemoveAll(); DCHECK(history_dir_.empty() || cached_at_shutdown_); } @@ -154,67 +147,19 @@ void InMemoryURLIndex::InitializeSchemeWhitelist( // Indexing -void InMemoryURLIndex::Init(const std::string& languages) { +bool InMemoryURLIndex::Init(URLDatabase* history_db, + const std::string& languages) { // TODO(mrossetti): Register for profile/language change notifications. languages_ = languages; - RestoreFromCache(); + return ReloadFromHistory(history_db, false); } void InMemoryURLIndex::ShutDown() { + // Write our cache. SaveToCacheFile(); cached_at_shutdown_ = true; } -void InMemoryURLIndex::Observe(int type, - const content::NotificationSource& source, - const content::NotificationDetails& details) { - switch (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: { - HistoryService* history_service = - profile_->GetHistoryService(Profile::EXPLICIT_ACCESS); - URLDatabase* history_db = history_service->HistoryDatabase(); - ReloadFromHistory(history_db); - } - 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) { - UpdateURL(details->row); -} - -void InMemoryURLIndex::OnURLsModified(const URLsModifiedDetails* details) { - for (std::vector<history::URLRow>::const_iterator row = - details->changed_urls.begin(); - row != details->changed_urls.end(); ++row) - UpdateURL(*row); -} - -void InMemoryURLIndex::OnURLsDeleted(const URLsDeletedDetails* details) { - if (details->all_history) { - ClearPrivateData(); - } else { - for (std::vector<URLRow>::const_iterator row = details->rows.begin(); - row != details->rows.end(); ++row) - DeleteURL(*row); - } -} - void InMemoryURLIndex::IndexRow(const URLRow& row) { const GURL& gurl(row.url()); @@ -308,24 +253,27 @@ void InMemoryURLIndex::RemoveRowWordsFromIndex(const URLRow& row) { } } -void InMemoryURLIndex::ReloadFromHistory(history::URLDatabase* history_db) { +bool InMemoryURLIndex::ReloadFromHistory(history::URLDatabase* history_db, + bool clear_cache) { ClearPrivateData(); if (!history_db) - return; - - base::TimeTicks beginning_time = base::TimeTicks::Now(); - // The index has to be built from scratch. - URLDatabase::URLEnumerator history_enum; - if (!history_db->InitURLEnumeratorForSignificant(&history_enum)) - return; + return false; - URLRow row; - while (history_enum.GetNextURL(&row)) - IndexRow(row); - UMA_HISTOGRAM_TIMES("History.InMemoryURLIndexingTime", - base::TimeTicks::Now() - beginning_time); - SaveToCacheFile(); + if (clear_cache || !RestoreFromCacheFile()) { + base::TimeTicks beginning_time = base::TimeTicks::Now(); + // The index has to be built from scratch. + 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); + SaveToCacheFile(); + } + return true; } void InMemoryURLIndex::ClearPrivateData() { @@ -333,14 +281,6 @@ void InMemoryURLIndex::ClearPrivateData() { search_term_cache_.clear(); } -void InMemoryURLIndex::RestoreFromCache() { - ClearPrivateData(); - if (!RestoreFromCacheFile() && profile_) { - content::Source<Profile> source(profile_); - registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED, source); - } -} - bool InMemoryURLIndex::RestoreFromCacheFile() { // 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 @@ -409,46 +349,55 @@ bool InMemoryURLIndex::SaveToCacheFile() { return true; } -void InMemoryURLIndex::UpdateURL(const URLRow& row) { +void InMemoryURLIndex::UpdateURL(URLID row_id, 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. HistoryInfoMap::iterator row_pos = - private_data_->history_info_map_.find(row.id()); + private_data_->history_info_map_.find(row_id); if (row_pos == private_data_->history_info_map_.end()) { // This new row should be indexed if it qualifies. - if (RowQualifiesAsSignificant(row, base::Time())) - IndexRow(row); + URLRow new_row(row); + new_row.set_id(row_id); + if (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& old_row = row_pos->second; - old_row.set_visit_count(row.visit_count()); - old_row.set_typed_count(row.typed_count()); - old_row.set_last_visit(row.last_visit()); + 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 (old_row.title() != row.title()) { + if (updated_row.title() != row.title()) { // Clear all words associated with this row and re-index both the // URL and title. - RemoveRowWordsFromIndex(row); - old_row.set_title(row.title()); - AddRowWordsToIndex(old_row); + RemoveRowWordsFromIndex(updated_row); + updated_row.set_title(row.title()); + AddRowWordsToIndex(updated_row); } } else { // This indexed row no longer qualifies and will be de-indexed by // clearing all words associated with this row. - RemoveRowFromIndex(row); + URLRow& removed_row = row_pos->second; + RemoveRowFromIndex(removed_row); } // This invalidates the cache. search_term_cache_.clear(); } -void InMemoryURLIndex::DeleteURL(const URLRow& row) { - RemoveRowFromIndex(row); - search_term_cache_.clear(); // Invalidate the word cache. +void InMemoryURLIndex::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. + private_data_->history_info_map_.erase(row_id); + // This invalidates the word cache. + search_term_cache_.clear(); } // Searching diff --git a/chrome/browser/history/in_memory_url_index.h b/chrome/browser/history/in_memory_url_index.h index dd6c1fe..bcbd676 100644 --- a/chrome/browser/history/in_memory_url_index.h +++ b/chrome/browser/history/in_memory_url_index.h @@ -23,12 +23,12 @@ #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 "content/public/browser/notification_observer.h" -#include "content/public/browser/notification_registrar.h" #include "sql/connection.h" #include "testing/gtest/include/gtest/gtest_prod.h" -class Profile; +namespace base { +class Time; +} namespace in_memory_url_index { class InMemoryURLIndexCacheItem; @@ -39,9 +39,6 @@ namespace history { namespace imui = in_memory_url_index; class URLDatabase; -struct URLsDeletedDetails; -struct URLsModifiedDetails; -struct URLVisitedDetails; // The URL history source. // Holds portions of the URL database in memory in an indexed form. Used to @@ -62,29 +59,33 @@ struct URLVisitedDetails; // 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 : public content::NotificationObserver { +class InMemoryURLIndex { 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. - InMemoryURLIndex(Profile* profile, const FilePath& history_dir); + explicit InMemoryURLIndex(const FilePath& history_dir); virtual ~InMemoryURLIndex(); - // Restores our index from its cache, if possible. If the cache is not - // available then we will register for the NOTIFICATION_HISTORY_LOADED - // notifications and then rebuild the index from the history database. + // Opens and indexes the URL history database. // |languages| gives a list of language encodings with which the history // URLs and omnibox searches are interpreted, i.e. when each is broken // down into words and each word is broken down into characters. - void Init(const std::string& languages); + bool Init(URLDatabase* history_db, const std::string& languages); - // Reloads the history index from the history database given in |history_db|. - void ReloadFromHistory(URLDatabase* history_db); + // Reloads the history index. Attempts to reload from the cache unless + // |clear_cache| is true. If the cache is unavailable then reload the + // index from |history_db|. + bool ReloadFromHistory(URLDatabase* history_db, bool clear_cache); // Signals that any outstanding initialization should be canceled and // flushes the cache to disk. void ShutDown(); + // Restores the index's private data from the cache file stored in the + // profile directory and returns true if successful. + bool RestoreFromCacheFile(); + // Caches the index private data and writes the cache file to the profile // directory. bool SaveToCacheFile(); @@ -104,18 +105,13 @@ class InMemoryURLIndex : public content::NotificationObserver { ScoredHistoryMatches HistoryItemsForTerms(const String16Vector& terms); // Updates or adds an history item to the index if it meets the minimum - // selection criteria. - void UpdateURL(const URLRow& row); + // '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(const URLRow& row); - - // Notification callback. - virtual void Observe(int type, - const content::NotificationSource& source, - const content::NotificationDetails& details); + void DeleteURL(URLID row_id); private: friend class AddHistoryMatch; @@ -188,16 +184,6 @@ class InMemoryURLIndex : public content::NotificationObserver { const String16Vector& lower_terms_; }; - // Initialization and Restoration -------------------------------------------- - - // Restores the index's private data from the cache, if possible, otherwise - // registers to be notified when the history database becomes available. - void RestoreFromCache(); - - // Restores the index's private data from the cache file stored in the - // profile directory and returns true if successful. - bool RestoreFromCacheFile(); - // Initializes all index data members in preparation for restoring the index // from the cache or a complete rebuild from the history database. void ClearPrivateData(); @@ -205,7 +191,7 @@ class InMemoryURLIndex : public content::NotificationObserver { // Initializes the whitelist of URL schemes. static void InitializeSchemeWhitelist(std::set<std::string>* whitelist); - // URL History Indexing ------------------------------------------------------ + // URL History indexing support functions. // Indexes one URL history item. void IndexRow(const URLRow& row); @@ -265,11 +251,6 @@ class InMemoryURLIndex : public content::NotificationObserver { // Determines if |gurl| has a whitelisted scheme and returns true if so. bool URLSchemeIsWhitelisted(const GURL& gurl) const; - // Notification handlers. - void OnURLVisited(const URLVisitedDetails* details); - void OnURLsModified(const URLsModifiedDetails* details); - void OnURLsDeleted(const URLsDeletedDetails* details); - // Utility functions supporting RestoreFromCache and SaveToCache. // Construct a file path for the cache file within the same directory where @@ -295,11 +276,6 @@ class InMemoryURLIndex : public content::NotificationObserver { bool RestoreWordIDHistoryMap(const imui::InMemoryURLIndexCacheItem& cache); bool RestoreHistoryInfoMap(const imui::InMemoryURLIndexCacheItem& cache); - content::NotificationRegistrar registrar_; - - // The profile with which we are associated. - 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. diff --git a/chrome/browser/history/in_memory_url_index_unittest.cc b/chrome/browser/history/in_memory_url_index_unittest.cc index fe1ea1c..c4420bc 100644 --- a/chrome/browser/history/in_memory_url_index_unittest.cc +++ b/chrome/browser/history/in_memory_url_index_unittest.cc @@ -139,17 +139,17 @@ URLRow InMemoryURLIndexTest::MakeURLRow(const char* url, } String16Vector InMemoryURLIndexTest::Make1Term(const char* term) const { - String16Vector terms; - terms.push_back(UTF8ToUTF16(term)); - return terms; + String16Vector original_terms; + original_terms.push_back(UTF8ToUTF16(term)); + return original_terms; } String16Vector InMemoryURLIndexTest::Make2Terms(const char* term_1, const char* term_2) const { - String16Vector terms; - terms.push_back(UTF8ToUTF16(term_1)); - terms.push_back(UTF8ToUTF16(term_2)); - return terms; + String16Vector original_terms; + original_terms.push_back(UTF8ToUTF16(term_1)); + original_terms.push_back(UTF8ToUTF16(term_2)); + return original_terms; } void InMemoryURLIndexTest::CheckTerm( @@ -215,7 +215,7 @@ void ExpandedInMemoryURLIndexTest::SetUp() { } TEST_F(InMemoryURLIndexTest, Construction) { - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); + url_index_.reset(new InMemoryURLIndex(FilePath())); EXPECT_TRUE(url_index_.get()); } @@ -227,9 +227,8 @@ TEST_F(LimitedInMemoryURLIndexTest, Initialization) { uint64 row_count = 0; while (statement.Step()) ++row_count; EXPECT_EQ(1U, row_count); - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); - url_index_->Init("en,ja,hi,zh"); - url_index_->ReloadFromHistory(this); + url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_->Init(this, "en,ja,hi,zh"); URLIndexPrivateData& private_data(*(url_index_->private_data_)); // history_info_map_ should have the same number of items as were filtered. @@ -239,9 +238,8 @@ TEST_F(LimitedInMemoryURLIndexTest, Initialization) { } TEST_F(InMemoryURLIndexTest, Retrieval) { - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); - url_index_->Init("en,ja,hi,zh"); - url_index_->ReloadFromHistory(this); + 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. @@ -273,11 +271,11 @@ TEST_F(InMemoryURLIndexTest, Retrieval) { matches[0].url_info.title()); // Search which should result in very poor result. - String16Vector terms; - terms.push_back(ASCIIToUTF16("z")); - terms.push_back(ASCIIToUTF16("y")); - terms.push_back(ASCIIToUTF16("x")); - matches = url_index_->HistoryItemsForTerms(terms); + String16Vector original_terms; + original_terms.push_back(ASCIIToUTF16("z")); + original_terms.push_back(ASCIIToUTF16("y")); + original_terms.push_back(ASCIIToUTF16("x")); + matches = url_index_->HistoryItemsForTerms(original_terms); ASSERT_EQ(1U, matches.size()); // The results should have a poor score. EXPECT_LT(matches[0].raw_score, 500); @@ -293,9 +291,8 @@ TEST_F(InMemoryURLIndexTest, Retrieval) { } TEST_F(ExpandedInMemoryURLIndexTest, ShortCircuit) { - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); - url_index_->Init("en,ja,hi,zh"); - url_index_->ReloadFromHistory(this); + url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_->Init(this, "en,ja,hi,zh"); // A search for 'w' should short-circuit and not return any matches. ScoredHistoryMatches matches = @@ -308,18 +305,18 @@ TEST_F(ExpandedInMemoryURLIndexTest, ShortCircuit) { } TEST_F(InMemoryURLIndexTest, TitleSearch) { - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); - url_index_->Init("en,ja,hi,zh"); - url_index_->ReloadFromHistory(this); + url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_->Init(this, "en,ja,hi,zh"); // Signal if someone has changed the test DB. EXPECT_EQ(27U, url_index_->private_data_->history_info_map_.size()); - String16Vector terms; + String16Vector original_terms; // Ensure title is being searched. - terms.push_back(ASCIIToUTF16("MORTGAGE")); - terms.push_back(ASCIIToUTF16("RATE")); - terms.push_back(ASCIIToUTF16("DROPS")); - ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(terms); + original_terms.push_back(ASCIIToUTF16("MORTGAGE")); + original_terms.push_back(ASCIIToUTF16("RATE")); + original_terms.push_back(ASCIIToUTF16("DROPS")); + ScoredHistoryMatches matches = + url_index_->HistoryItemsForTerms(original_terms); ASSERT_EQ(1U, matches.size()); // Verify that we got back the result we expected. @@ -332,9 +329,8 @@ TEST_F(InMemoryURLIndexTest, TitleSearch) { } TEST_F(InMemoryURLIndexTest, TitleChange) { - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); - url_index_->Init("en,ja,hi,zh"); - url_index_->ReloadFromHistory(this); + url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_->Init(this, "en,ja,hi,zh"); // Verify current title terms retrieves desired item. String16Vector original_terms; @@ -370,7 +366,7 @@ TEST_F(InMemoryURLIndexTest, TitleChange) { // Update the row. old_row.set_title(ASCIIToUTF16("Does eat oats and little lambs eat ivy")); - url_index_->UpdateURL(old_row); + url_index_->UpdateURL(expected_id, old_row); // Verify we get the row using the new terms but not the original terms. matches = url_index_->HistoryItemsForTerms(new_terms); @@ -381,9 +377,8 @@ TEST_F(InMemoryURLIndexTest, TitleChange) { } TEST_F(InMemoryURLIndexTest, NonUniqueTermCharacterSets) { - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); - url_index_->Init("en,ja,hi,zh"); - url_index_->ReloadFromHistory(this); + 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. @@ -418,9 +413,8 @@ TEST_F(InMemoryURLIndexTest, TypedCharacterCaching) { typedef InMemoryURLIndex::SearchTermCacheMap::iterator CacheIter; typedef InMemoryURLIndex::SearchTermCacheItem CacheItem; - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); - url_index_->Init("en,ja,hi,zh"); - url_index_->ReloadFromHistory(this); + url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_->Init(this, "en,ja,hi,zh"); InMemoryURLIndex::SearchTermCacheMap& cache(url_index_->search_term_cache_); @@ -432,55 +426,55 @@ TEST_F(InMemoryURLIndexTest, TypedCharacterCaching) { // Simulate typing "r" giving "r" in the simulated omnibox. The results for // 'r' will be not cached because it is only 1 character long. - String16Vector terms; + String16Vector original_terms; string16 term_r = ASCIIToUTF16("r"); - terms.push_back(term_r); - url_index_->HistoryItemsForTerms(terms); + original_terms.push_back(term_r); + url_index_->HistoryItemsForTerms(original_terms); EXPECT_EQ(0U, cache.size()); // Simulate typing "re" giving "r re" in the simulated omnibox. string16 term_re = ASCIIToUTF16("re"); - terms.push_back(term_re); + original_terms.push_back(term_re); // 're' should be cached at this point but not 'r' as it is a single // character. - ASSERT_EQ(2U, terms.size()); - url_index_->HistoryItemsForTerms(terms); + ASSERT_EQ(2U, original_terms.size()); + url_index_->HistoryItemsForTerms(original_terms); ASSERT_EQ(1U, cache.size()); CheckTerm(cache, term_re); // Simulate typing "reco" giving "r re reco" in the simulated omnibox. string16 term_reco = ASCIIToUTF16("reco"); - terms.push_back(term_reco); + original_terms.push_back(term_reco); // 're' and 'reco' should be cached at this point but not 'r' as it is a // single character. - url_index_->HistoryItemsForTerms(terms); + url_index_->HistoryItemsForTerms(original_terms); ASSERT_EQ(2U, cache.size()); CheckTerm(cache, term_re); CheckTerm(cache, term_reco); - terms.clear(); // Simulate pressing <ESC>. + original_terms.clear(); // Simulate pressing <ESC>. // Simulate typing "mort". string16 term_mort = ASCIIToUTF16("mort"); - terms.push_back(term_mort); + original_terms.push_back(term_mort); // Since we now have only one search term, the cached results for 're' and // 'reco' should be purged, giving us only 1 item in the cache (for 'mort'). - url_index_->HistoryItemsForTerms(terms); + url_index_->HistoryItemsForTerms(original_terms); ASSERT_EQ(1U, cache.size()); CheckTerm(cache, term_mort); // Simulate typing "reco" giving "mort reco" in the simulated omnibox. - terms.push_back(term_reco); - url_index_->HistoryItemsForTerms(terms); + original_terms.push_back(term_reco); + url_index_->HistoryItemsForTerms(original_terms); ASSERT_EQ(2U, cache.size()); CheckTerm(cache, term_mort); CheckTerm(cache, term_reco); // Simulate a <DELETE> by removing the 'reco' and adding back the 'rec'. - terms.resize(terms.size() - 1); + original_terms.resize(original_terms.size() - 1); string16 term_rec = ASCIIToUTF16("rec"); - terms.push_back(term_rec); - url_index_->HistoryItemsForTerms(terms); + original_terms.push_back(term_rec); + url_index_->HistoryItemsForTerms(original_terms); ASSERT_EQ(2U, cache.size()); CheckTerm(cache, term_mort); CheckTerm(cache, term_rec); @@ -522,45 +516,44 @@ TEST_F(InMemoryURLIndexTest, Scoring) { } TEST_F(InMemoryURLIndexTest, AddNewRows) { - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); - url_index_->Init("en,ja,hi,zh"); - url_index_->ReloadFromHistory(this); - String16Vector terms; + url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_->Init(this, "en,ja,hi,zh"); + String16Vector original_terms; // 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 // qualify as a quick result candidate. - terms.push_back(ASCIIToUTF16("brokeandalone")); - EXPECT_TRUE(url_index_->HistoryItemsForTerms(terms).empty()); + original_terms.push_back(ASCIIToUTF16("brokeandalone")); + EXPECT_TRUE(url_index_->HistoryItemsForTerms(original_terms).empty()); // Add a new row. URLRow new_row(GURL("http://www.brokeandaloneinmanitoba.com/"), new_row_id); new_row.set_last_visit(base::Time::Now()); - url_index_->UpdateURL(new_row); + url_index_->UpdateURL(new_row_id, new_row); // Verify that we can retrieve it. - EXPECT_EQ(1U, url_index_->HistoryItemsForTerms(terms).size()); + EXPECT_EQ(1U, url_index_->HistoryItemsForTerms(original_terms).size()); // Add it again just to be sure that is harmless. - url_index_->UpdateURL(new_row); - EXPECT_EQ(1U, url_index_->HistoryItemsForTerms(terms).size()); + url_index_->UpdateURL(new_row_id, new_row); + EXPECT_EQ(1U, url_index_->HistoryItemsForTerms(original_terms).size()); } TEST_F(InMemoryURLIndexTest, DeleteRows) { - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); - url_index_->Init("en,ja,hi,zh"); - url_index_->ReloadFromHistory(this); - String16Vector terms; + url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_->Init(this, "en,ja,hi,zh"); + String16Vector original_terms; // Make sure we actually get an existing result. - terms.push_back(ASCIIToUTF16("DrudgeReport")); - ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(terms); + original_terms.push_back(ASCIIToUTF16("DrudgeReport")); + ScoredHistoryMatches matches = + url_index_->HistoryItemsForTerms(original_terms); 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); - EXPECT_TRUE(url_index_->HistoryItemsForTerms(terms).empty()); + url_index_->DeleteURL(matches[0].url_info.id()); + EXPECT_TRUE(url_index_->HistoryItemsForTerms(original_terms).empty()); } TEST_F(InMemoryURLIndexTest, WhitelistedURLs) { @@ -636,7 +629,7 @@ TEST_F(InMemoryURLIndexTest, WhitelistedURLs) { { "xmpp:node@example.com", false }, { "xmpp://guest@example.com", false }, }; - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); + url_index_.reset(new InMemoryURLIndex(FilePath())); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(data); ++i) { GURL url(data[i].url_spec); EXPECT_EQ(data[i].expected_is_whitelisted, @@ -645,8 +638,8 @@ TEST_F(InMemoryURLIndexTest, WhitelistedURLs) { } TEST_F(InMemoryURLIndexTest, CacheFilePath) { - url_index_.reset(new InMemoryURLIndex( - NULL, FilePath(FILE_PATH_LITERAL("/flammmy/frammy/")))); + 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; @@ -664,10 +657,9 @@ TEST_F(InMemoryURLIndexTest, CacheFilePath) { TEST_F(InMemoryURLIndexTest, CacheSaveRestore) { // Save the cache to a protobuf, restore it, and compare the results. - url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); + url_index_.reset(new InMemoryURLIndex(FilePath())); InMemoryURLIndex& url_index(*(url_index_.get())); - url_index.Init("en,ja,hi,zh"); - url_index_->ReloadFromHistory(this); + url_index.Init(this, "en,ja,hi,zh"); in_memory_url_index::InMemoryURLIndexCacheItem index_cache; url_index.SavePrivateData(&index_cache); |