diff options
author | mrossetti@chromium.org <mrossetti@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-11-02 00:33:13 +0000 |
---|---|---|
committer | mrossetti@chromium.org <mrossetti@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-11-02 00:33:13 +0000 |
commit | 053ec8f550db4fb9fcae72ea6de0970d9440e125 (patch) | |
tree | ca0d287b03c6c2a912684e08457d18b09ee1fc93 /chrome/browser | |
parent | bbf59b205d131b56b196fabf643158bbe788627a (diff) | |
download | chromium_src-053ec8f550db4fb9fcae72ea6de0970d9440e125.zip chromium_src-053ec8f550db4fb9fcae72ea6de0970d9440e125.tar.gz chromium_src-053ec8f550db4fb9fcae72ea6de0970d9440e125.tar.bz2 |
HQP Refactoring (in Preparation for SQLite Cache)
1. Move ownership of the InMemoryURLIndex from the InMemoryHistoryBackend to the HistoryBackend 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)
Review URL: http://codereview.chromium.org/8384024
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@108207 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser')
-rw-r--r-- | chrome/browser/autocomplete/history_provider.cc | 4 | ||||
-rw-r--r-- | chrome/browser/autocomplete/history_provider.h | 4 | ||||
-rw-r--r-- | chrome/browser/autocomplete/history_quick_provider_unittest.cc | 5 | ||||
-rw-r--r-- | chrome/browser/history/expire_history_backend.cc | 4 | ||||
-rw-r--r-- | chrome/browser/history/history.cc | 8 | ||||
-rw-r--r-- | chrome/browser/history/history_backend.cc | 19 | ||||
-rw-r--r-- | chrome/browser/history/history_backend.h | 14 | ||||
-rw-r--r-- | chrome/browser/history/history_backend_unittest.cc | 8 | ||||
-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 | 38 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_history_backend.h | 6 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_url_index.cc | 107 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_url_index.h | 32 | ||||
-rw-r--r-- | chrome/browser/history/in_memory_url_index_unittest.cc | 122 | ||||
-rw-r--r-- | chrome/browser/sync/profile_sync_service_typed_url_unittest.cc | 2 |
16 files changed, 229 insertions, 163 deletions
diff --git a/chrome/browser/autocomplete/history_provider.cc b/chrome/browser/autocomplete/history_provider.cc index b6fb001..8140aea 100644 --- a/chrome/browser/autocomplete/history_provider.cc +++ b/chrome/browser/autocomplete/history_provider.cc @@ -34,8 +34,10 @@ void HistoryProvider::DeleteMatch(const AutocompleteMatch& match) { DCHECK(history_service); DCHECK(match.destination_url.is_valid()); history_service->DeleteURL(match.destination_url); + DeleteMatchFromMatches(match); +} - // Delete the match from the current set of matches. +void HistoryProvider::DeleteMatchFromMatches(const AutocompleteMatch& match) { bool found = false; for (ACMatches::iterator i(matches_.begin()); i != matches_.end(); ++i) { if (i->destination_url == match.destination_url && i->type == match.type) { diff --git a/chrome/browser/autocomplete/history_provider.h b/chrome/browser/autocomplete/history_provider.h index 231a7b1..6cc0f80 100644 --- a/chrome/browser/autocomplete/history_provider.h +++ b/chrome/browser/autocomplete/history_provider.h @@ -54,6 +54,10 @@ class HistoryProvider : public AutocompleteProvider { // |input.prevent_inline_autocomplete()| is true, or the input text contains // trailing whitespace. bool PreventInlineAutocomplete(const AutocompleteInput& input); + + // Finds and removes the match from the current collection of matches and + // backing data. + void DeleteMatchFromMatches(const AutocompleteMatch& match); }; #endif // CHROME_BROWSER_AUTOCOMPLETE_HISTORY_PROVIDER_H_ diff --git a/chrome/browser/autocomplete/history_quick_provider_unittest.cc b/chrome/browser/autocomplete/history_quick_provider_unittest.cc index 76a17dc..1325ea6 100644 --- a/chrome/browser/autocomplete/history_quick_provider_unittest.cc +++ b/chrome/browser/autocomplete/history_quick_provider_unittest.cc @@ -144,7 +144,7 @@ void HistoryQuickProviderTest::FillData() { ASSERT_TRUE(db != NULL); history::InMemoryURLIndex* index = - new history::InMemoryURLIndex(FilePath()); + new history::InMemoryURLIndex(profile_.get(), FilePath()); PrefService* prefs = profile_->GetPrefs(); std::string languages(prefs->GetString(prefs::kAcceptLanguages)); index->Init(db, languages); @@ -159,7 +159,8 @@ void HistoryQuickProviderTest::FillData() { 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); + url_info.set_id(i); + index->UpdateURL(url_info); } provider_->set_index(index); diff --git a/chrome/browser/history/expire_history_backend.cc b/chrome/browser/history/expire_history_backend.cc index 0f2ea68..75110e3 100644 --- a/chrome/browser/history/expire_history_backend.cc +++ b/chrome/browser/history/expire_history_backend.cc @@ -351,8 +351,10 @@ 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++) + for (size_t i = 0; i < dependencies->deleted_urls.size(); i++) { + deleted_details->rows.push_back(dependencies->deleted_urls[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 583e106..89a0da6 100644 --- a/chrome/browser/history/history.cc +++ b/chrome/browser/history/history.cc @@ -37,6 +37,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" @@ -248,8 +249,8 @@ history::InMemoryURLIndex* HistoryService::InMemoryIndex() { // 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(); + if (history_backend_.get()) + return history_backend_->InMemoryIndex(); return NULL; } @@ -800,7 +801,8 @@ void HistoryService::LoadBackendIfNecessary() { ++current_backend_id_; scoped_refptr<HistoryBackend> backend( - new HistoryBackend(history_dir_, + new HistoryBackend(profile_, + history_dir_, current_backend_id_, new BackendDelegate(this, profile_), bookmark_service_)); diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc index d16c6c6..d2dc967 100644 --- a/chrome/browser/history/history_backend.cc +++ b/chrome/browser/history/history_backend.cc @@ -23,10 +23,13 @@ #include "chrome/browser/history/history_notifications.h" #include "chrome/browser/history/history_publisher.h" #include "chrome/browser/history/in_memory_history_backend.h" +#include "chrome/browser/history/in_memory_url_index.h" #include "chrome/browser/history/page_usage_data.h" #include "chrome/browser/history/top_sites.h" +#include "chrome/browser/profiles/profile.h" #include "chrome/common/chrome_constants.h" #include "chrome/common/chrome_notification_types.h" +#include "chrome/common/chrome_switches.h" #include "chrome/common/url_constants.h" #include "content/browser/cancelable_request.h" #include "content/browser/download/download_persistent_store_info.h" @@ -199,7 +202,8 @@ class HistoryBackend::URLQuerier { // HistoryBackend -------------------------------------------------------------- -HistoryBackend::HistoryBackend(const FilePath& history_dir, +HistoryBackend::HistoryBackend(Profile* profile, + const FilePath& history_dir, int id, Delegate* delegate, BookmarkService* bookmark_service) @@ -212,13 +216,20 @@ HistoryBackend::HistoryBackend(const FilePath& history_dir, backend_destroy_task_(NULL), segment_queried_(false), bookmark_service_(bookmark_service) { + if (!CommandLine::ForCurrentProcess()->HasSwitch( + switches::kDisableHistoryQuickProvider)) + in_memory_url_index_.reset(new InMemoryURLIndex(profile, history_dir_)); } HistoryBackend::~HistoryBackend() { DCHECK(!scheduled_commit_) << "Deleting without cleanup"; ReleaseDBTasks(); - // First close the databases before optionally running the "destroy" task. + // Give the InMemoryURLIndex a chance to shutdown. + if (in_memory_url_index_.get()) + in_memory_url_index_->ShutDown(); + + // Close the databases before optionally running the "destroy" task. if (db_.get()) { // Commit the long-running transaction. db_->CommitTransaction(); @@ -651,6 +662,9 @@ void HistoryBackend::InitImpl(const std::string& languages) { archived_db_.reset(); } + if (in_memory_url_index_.get()) + in_memory_url_index_->Init(db_.get(), languages); + // Tell the expiration module about all the nice databases we made. This must // happen before db_->Init() is called since the callback ForceArchiveHistory // may need to expire stuff. @@ -885,6 +899,7 @@ 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 057b0f5..73423f2 100644 --- a/chrome/browser/history/history_backend.h +++ b/chrome/browser/history/history_backend.h @@ -27,6 +27,7 @@ class BookmarkService; struct DownloadPersistentStoreInfo; +class Profile; class TestingProfile; struct ThumbnailScore; @@ -104,7 +105,8 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, // may be NULL. // // This constructor is fast and does no I/O, so can be called at any time. - HistoryBackend(const FilePath& history_dir, + HistoryBackend(Profile* profile, + const FilePath& history_dir, int id, Delegate* delegate, BookmarkService* bookmark_service); @@ -257,6 +259,13 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, const base::Time remove_end); void RemoveDownloads(const base::Time remove_end); + // InMemoryURLIndex ---------------------------------------------------------- + + // Returns the quick history index. + history::InMemoryURLIndex* InMemoryIndex() const { + return in_memory_url_index_.get(); + } + // Segment usage ------------------------------------------------------------- void QuerySegmentUsage(scoped_refptr<QuerySegmentUsageRequest> request, @@ -574,6 +583,9 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>, // created. scoped_ptr<TextDatabaseManager> text_database_; + // The index used for quick history lookups. + scoped_ptr<history::InMemoryURLIndex> in_memory_url_index_; + // Manages expiration between the various databases. ExpireHistoryBackend expirer_; diff --git a/chrome/browser/history/history_backend_unittest.cc b/chrome/browser/history/history_backend_unittest.cc index 9840e09..c1bc731 100644 --- a/chrome/browser/history/history_backend_unittest.cc +++ b/chrome/browser/history/history_backend_unittest.cc @@ -19,6 +19,7 @@ #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" @@ -146,7 +147,6 @@ class HistoryBackendTest : public testing::Test { BookmarkModel bookmark_model_; - protected: bool loaded_; private: @@ -157,7 +157,8 @@ class HistoryBackendTest : public testing::Test { if (!file_util::CreateNewTempDirectory(FILE_PATH_LITERAL("BackendTest"), &test_dir_)) return; - backend_ = new HistoryBackend(test_dir_, + backend_ = new HistoryBackend(NULL, + test_dir_, 0, new HistoryBackendTestDelegate(this), &bookmark_model_); @@ -955,7 +956,8 @@ TEST_F(HistoryBackendTest, MigrationVisitSource) { FilePath new_history_file = new_history_path.Append(chrome::kHistoryFilename); ASSERT_TRUE(file_util::CopyFile(old_history_path, new_history_file)); - backend_ = new HistoryBackend(new_history_path, + backend_ = new HistoryBackend(NULL, + new_history_path, 0, new HistoryBackendTestDelegate(this), &bookmark_model_); diff --git a/chrome/browser/history/history_notifications.h b/chrome/browser/history/history_notifications.h index ab9d555..5e4bf17 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) 2011 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 that 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_unittest.cc b/chrome/browser/history/history_unittest.cc index ceb9608..1bc6586 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(NULL, 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 f1e1730..c325459 100644 --- a/chrome/browser/history/in_memory_history_backend.cc +++ b/chrome/browser/history/in_memory_history_backend.cc @@ -13,13 +13,10 @@ #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" +#include "content/public/browser/notification_service.h" namespace history { @@ -27,23 +24,14 @@ 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) { 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,36 +119,26 @@ 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::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 (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()); } } diff --git a/chrome/browser/history/in_memory_history_backend.h b/chrome/browser/history/in_memory_history_backend.h index 5eb8c18e..aaacf4c 100644 --- a/chrome/browser/history/in_memory_history_backend.h +++ b/chrome/browser/history/in_memory_history_backend.h @@ -70,9 +70,6 @@ class InMemoryHistoryBackend : public content::NotificationObserver { const content::NotificationSource& source, const content::NotificationDetails& details); - // Return the quick history index. - history::InMemoryURLIndex* InMemoryIndex() const { return index_.get(); } - private: FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, DeleteAll); @@ -96,9 +93,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 b92a062..bce02ac 100644 --- a/chrome/browser/history/in_memory_url_index.cc +++ b/chrome/browser/history/in_memory_url_index.cc @@ -13,20 +13,15 @@ #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/autocomplete/history_provider_util.h" +#include "chrome/browser/history/history_notifications.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 "googleurl/src/url_parse.h" -#include "googleurl/src/url_util.h" -#include "net/base/escape.h" +#include "content/public/browser/notification_service.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; @@ -112,11 +107,19 @@ int ScoreForValue(int value, const int* value_ranks) { return score; } -InMemoryURLIndex::InMemoryURLIndex(const FilePath& history_dir) +InMemoryURLIndex::InMemoryURLIndex(Profile* profile, + 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. @@ -155,11 +158,54 @@ bool InMemoryURLIndex::Init(URLDatabase* history_db, } void InMemoryURLIndex::ShutDown() { - // Write our cache. + registrar_.RemoveAll(); 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; + 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()); @@ -349,55 +395,46 @@ bool InMemoryURLIndex::SaveToCacheFile() { return true; } -void InMemoryURLIndex::UpdateURL(URLID row_id, const URLRow& row) { +void InMemoryURLIndex::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. 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. - URLRow new_row(row); - new_row.set_id(row_id); - if (RowQualifiesAsSignificant(new_row, base::Time())) - IndexRow(new_row); + if (RowQualifiesAsSignificant(row, base::Time())) + IndexRow(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()); + 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()); // 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()) { + if (old_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); + RemoveRowWordsFromIndex(row); + old_row.set_title(row.title()); + AddRowWordsToIndex(old_row); } } 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); } // This invalidates the cache. search_term_cache_.clear(); } -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(); +void InMemoryURLIndex::DeleteURL(const URLRow& row) { + RemoveRowFromIndex(row); + search_term_cache_.clear(); // Invalidate the word cache. } // Searching diff --git a/chrome/browser/history/in_memory_url_index.h b/chrome/browser/history/in_memory_url_index.h index dfae2d4..d9ecc36 100644 --- a/chrome/browser/history/in_memory_url_index.h +++ b/chrome/browser/history/in_memory_url_index.h @@ -23,15 +23,13 @@ #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; } @@ -41,6 +39,9 @@ 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 @@ -61,12 +62,13 @@ 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); + explicit InMemoryURLIndex(Profile* profile, + const FilePath& history_dir); virtual ~InMemoryURLIndex(); // Opens and indexes the URL history database. @@ -107,13 +109,18 @@ class InMemoryURLIndex { ScoredHistoryMatches HistoryItemsForTerms(const String16Vector& terms); // Updates or adds an history item to the index if it meets the minimum - // 'quick' criteria. - void UpdateURL(URLID row_id, const URLRow& row); + // selection criteria. + void UpdateURL(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); + void DeleteURL(const URLRow& row); + + // Notification callback. + virtual void Observe(int type, + const content::NotificationSource& source, + const content::NotificationDetails& details); private: friend class AddHistoryMatch; @@ -253,6 +260,11 @@ class InMemoryURLIndex { // 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 @@ -278,6 +290,8 @@ class InMemoryURLIndex { bool RestoreWordIDHistoryMap(const imui::InMemoryURLIndexCacheItem& cache); bool RestoreHistoryInfoMap(const imui::InMemoryURLIndexCacheItem& cache); + content::NotificationRegistrar registrar_; + // 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 c4420bc..01aaa4a 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 original_terms; - original_terms.push_back(UTF8ToUTF16(term)); - return original_terms; + String16Vector terms; + terms.push_back(UTF8ToUTF16(term)); + return terms; } String16Vector InMemoryURLIndexTest::Make2Terms(const char* term_1, const char* term_2) const { - String16Vector original_terms; - original_terms.push_back(UTF8ToUTF16(term_1)); - original_terms.push_back(UTF8ToUTF16(term_2)); - return original_terms; + String16Vector terms; + terms.push_back(UTF8ToUTF16(term_1)); + terms.push_back(UTF8ToUTF16(term_2)); + return terms; } void InMemoryURLIndexTest::CheckTerm( @@ -215,7 +215,7 @@ void ExpandedInMemoryURLIndexTest::SetUp() { } TEST_F(InMemoryURLIndexTest, Construction) { - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); EXPECT_TRUE(url_index_.get()); } @@ -227,7 +227,7 @@ TEST_F(LimitedInMemoryURLIndexTest, Initialization) { uint64 row_count = 0; while (statement.Step()) ++row_count; EXPECT_EQ(1U, row_count); - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); url_index_->Init(this, "en,ja,hi,zh"); URLIndexPrivateData& private_data(*(url_index_->private_data_)); @@ -238,7 +238,7 @@ TEST_F(LimitedInMemoryURLIndexTest, Initialization) { } TEST_F(InMemoryURLIndexTest, Retrieval) { - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); url_index_->Init(this, "en,ja,hi,zh"); // The term will be lowercased by the search. @@ -271,11 +271,11 @@ TEST_F(InMemoryURLIndexTest, Retrieval) { matches[0].url_info.title()); // Search which should result in very poor result. - 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); + String16Vector terms; + terms.push_back(ASCIIToUTF16("z")); + terms.push_back(ASCIIToUTF16("y")); + terms.push_back(ASCIIToUTF16("x")); + matches = url_index_->HistoryItemsForTerms(terms); ASSERT_EQ(1U, matches.size()); // The results should have a poor score. EXPECT_LT(matches[0].raw_score, 500); @@ -291,7 +291,7 @@ TEST_F(InMemoryURLIndexTest, Retrieval) { } TEST_F(ExpandedInMemoryURLIndexTest, ShortCircuit) { - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); url_index_->Init(this, "en,ja,hi,zh"); // A search for 'w' should short-circuit and not return any matches. @@ -305,18 +305,17 @@ TEST_F(ExpandedInMemoryURLIndexTest, ShortCircuit) { } TEST_F(InMemoryURLIndexTest, TitleSearch) { - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, 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 original_terms; + String16Vector terms; // Ensure title is being searched. - 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); + terms.push_back(ASCIIToUTF16("MORTGAGE")); + terms.push_back(ASCIIToUTF16("RATE")); + terms.push_back(ASCIIToUTF16("DROPS")); + ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(terms); ASSERT_EQ(1U, matches.size()); // Verify that we got back the result we expected. @@ -329,7 +328,7 @@ TEST_F(InMemoryURLIndexTest, TitleSearch) { } TEST_F(InMemoryURLIndexTest, TitleChange) { - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); url_index_->Init(this, "en,ja,hi,zh"); // Verify current title terms retrieves desired item. @@ -366,7 +365,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); + url_index_->UpdateURL(old_row); // Verify we get the row using the new terms but not the original terms. matches = url_index_->HistoryItemsForTerms(new_terms); @@ -377,7 +376,7 @@ TEST_F(InMemoryURLIndexTest, TitleChange) { } TEST_F(InMemoryURLIndexTest, NonUniqueTermCharacterSets) { - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); url_index_->Init(this, "en,ja,hi,zh"); // The presence of duplicate characters should succeed. Exercise by cycling @@ -413,7 +412,7 @@ TEST_F(InMemoryURLIndexTest, TypedCharacterCaching) { typedef InMemoryURLIndex::SearchTermCacheMap::iterator CacheIter; typedef InMemoryURLIndex::SearchTermCacheItem CacheItem; - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); url_index_->Init(this, "en,ja,hi,zh"); InMemoryURLIndex::SearchTermCacheMap& cache(url_index_->search_term_cache_); @@ -426,55 +425,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 original_terms; + String16Vector terms; string16 term_r = ASCIIToUTF16("r"); - original_terms.push_back(term_r); - url_index_->HistoryItemsForTerms(original_terms); + terms.push_back(term_r); + url_index_->HistoryItemsForTerms(terms); EXPECT_EQ(0U, cache.size()); // Simulate typing "re" giving "r re" in the simulated omnibox. string16 term_re = ASCIIToUTF16("re"); - original_terms.push_back(term_re); + terms.push_back(term_re); // 're' should be cached at this point but not 'r' as it is a single // character. - ASSERT_EQ(2U, original_terms.size()); - url_index_->HistoryItemsForTerms(original_terms); + ASSERT_EQ(2U, terms.size()); + url_index_->HistoryItemsForTerms(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"); - original_terms.push_back(term_reco); + 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(original_terms); + url_index_->HistoryItemsForTerms(terms); ASSERT_EQ(2U, cache.size()); CheckTerm(cache, term_re); CheckTerm(cache, term_reco); - original_terms.clear(); // Simulate pressing <ESC>. + terms.clear(); // Simulate pressing <ESC>. // Simulate typing "mort". string16 term_mort = ASCIIToUTF16("mort"); - original_terms.push_back(term_mort); + 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(original_terms); + url_index_->HistoryItemsForTerms(terms); ASSERT_EQ(1U, cache.size()); CheckTerm(cache, term_mort); // Simulate typing "reco" giving "mort reco" in the simulated omnibox. - original_terms.push_back(term_reco); - url_index_->HistoryItemsForTerms(original_terms); + terms.push_back(term_reco); + url_index_->HistoryItemsForTerms(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'. - original_terms.resize(original_terms.size() - 1); + terms.resize(terms.size() - 1); string16 term_rec = ASCIIToUTF16("rec"); - original_terms.push_back(term_rec); - url_index_->HistoryItemsForTerms(original_terms); + terms.push_back(term_rec); + url_index_->HistoryItemsForTerms(terms); ASSERT_EQ(2U, cache.size()); CheckTerm(cache, term_mort); CheckTerm(cache, term_rec); @@ -516,44 +515,43 @@ TEST_F(InMemoryURLIndexTest, Scoring) { } TEST_F(InMemoryURLIndexTest, AddNewRows) { - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); url_index_->Init(this, "en,ja,hi,zh"); - String16Vector original_terms; + String16Vector 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. - original_terms.push_back(ASCIIToUTF16("brokeandalone")); - EXPECT_TRUE(url_index_->HistoryItemsForTerms(original_terms).empty()); + terms.push_back(ASCIIToUTF16("brokeandalone")); + EXPECT_TRUE(url_index_->HistoryItemsForTerms(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_id, new_row); + url_index_->UpdateURL(new_row); // Verify that we can retrieve it. - EXPECT_EQ(1U, url_index_->HistoryItemsForTerms(original_terms).size()); + EXPECT_EQ(1U, url_index_->HistoryItemsForTerms(terms).size()); // Add it again just to be sure that is harmless. - url_index_->UpdateURL(new_row_id, new_row); - EXPECT_EQ(1U, url_index_->HistoryItemsForTerms(original_terms).size()); + url_index_->UpdateURL(new_row); + EXPECT_EQ(1U, url_index_->HistoryItemsForTerms(terms).size()); } TEST_F(InMemoryURLIndexTest, DeleteRows) { - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); url_index_->Init(this, "en,ja,hi,zh"); - String16Vector original_terms; + String16Vector terms; // Make sure we actually get an existing result. - original_terms.push_back(ASCIIToUTF16("DrudgeReport")); - ScoredHistoryMatches matches = - url_index_->HistoryItemsForTerms(original_terms); + terms.push_back(ASCIIToUTF16("DrudgeReport")); + ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(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.id()); - EXPECT_TRUE(url_index_->HistoryItemsForTerms(original_terms).empty()); + url_index_->DeleteURL(matches[0].url_info); + EXPECT_TRUE(url_index_->HistoryItemsForTerms(terms).empty()); } TEST_F(InMemoryURLIndexTest, WhitelistedURLs) { @@ -629,7 +627,7 @@ TEST_F(InMemoryURLIndexTest, WhitelistedURLs) { { "xmpp:node@example.com", false }, { "xmpp://guest@example.com", false }, }; - url_index_.reset(new InMemoryURLIndex(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(data); ++i) { GURL url(data[i].url_spec); EXPECT_EQ(data[i].expected_is_whitelisted, @@ -638,8 +636,8 @@ TEST_F(InMemoryURLIndexTest, WhitelistedURLs) { } TEST_F(InMemoryURLIndexTest, CacheFilePath) { - url_index_.reset(new InMemoryURLIndex(FilePath(FILE_PATH_LITERAL( - "/flammmy/frammy/")))); + url_index_.reset(new InMemoryURLIndex( + NULL, FilePath(FILE_PATH_LITERAL("/flammmy/frammy/")))); FilePath full_file_path; url_index_->GetCacheFilePath(&full_file_path); std::vector<FilePath::StringType> expected_parts; @@ -657,7 +655,7 @@ 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(FilePath())); + url_index_.reset(new InMemoryURLIndex(NULL, FilePath())); InMemoryURLIndex& url_index(*(url_index_.get())); url_index.Init(this, "en,ja,hi,zh"); in_memory_url_index::InMemoryURLIndexCacheItem index_cache; diff --git a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc index bda65ee..a248ccf 100644 --- a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc @@ -80,7 +80,7 @@ using testing::WithArgs; class HistoryBackendMock : public HistoryBackend { public: - HistoryBackendMock() : HistoryBackend(FilePath(), 0, NULL, NULL) {} + HistoryBackendMock() : HistoryBackend(NULL, FilePath(), 0, NULL, NULL) {} MOCK_METHOD1(GetAllTypedURLs, bool(std::vector<history::URLRow>* entries)); MOCK_METHOD3(GetMostRecentVisitsForURL, bool(history::URLID id, int max_visits, |