diff options
author | mrossetti@chromium.org <mrossetti@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-10-19 13:05:19 +0000 |
---|---|---|
committer | mrossetti@chromium.org <mrossetti@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-10-19 13:05:19 +0000 |
commit | bbf3c9d2ef091f6c109d9ab93c2bd2e477806395 (patch) | |
tree | 632e748a461fea09bfa256ddc73ce5c1294dc533 | |
parent | 610ca838d7afb9c551bb186888e2ebf25ca913e7 (diff) | |
download | chromium_src-bbf3c9d2ef091f6c109d9ab93c2bd2e477806395.zip chromium_src-bbf3c9d2ef091f6c109d9ab93c2bd2e477806395.tar.gz chromium_src-bbf3c9d2ef091f6c109d9ab93c2bd2e477806395.tar.bz2 |
Minor Changes Seeking Stability.
Short-circuit saving the InMemoryURLCache earlier if there is no history directory (helps with unit tests).
Check in ~InMemoryURLIndex to insure the cache has been saved before dtor called. (This will signal if the dtor is called before ShutDown for some reason.)
BUG=95876
TEST=Unit tests continue to succeed.
Review URL: http://codereview.chromium.org/8344002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@106271 0039d316-1c4b-4281-b951-d872f2087c98
4 files changed, 35 insertions, 23 deletions
diff --git a/chrome/browser/autocomplete/history_quick_provider_unittest.cc b/chrome/browser/autocomplete/history_quick_provider_unittest.cc index f936860..df3ac82 100644 --- a/chrome/browser/autocomplete/history_quick_provider_unittest.cc +++ b/chrome/browser/autocomplete/history_quick_provider_unittest.cc @@ -161,8 +161,7 @@ void HistoryQuickProviderTest::FillData() { history::SOURCE_BROWSED); } - history::InMemoryURLIndex* index = - new history::InMemoryURLIndex(FilePath(FILE_PATH_LITERAL("/dummy"))); + history::InMemoryURLIndex* index = new history::InMemoryURLIndex(FilePath()); PrefService* prefs = profile_->GetPrefs(); std::string languages(prefs->GetString(prefs::kAcceptLanguages)); index->Init(db, languages); diff --git a/chrome/browser/history/in_memory_url_index.cc b/chrome/browser/history/in_memory_url_index.cc index c557acb..6388686 100644 --- a/chrome/browser/history/in_memory_url_index.cc +++ b/chrome/browser/history/in_memory_url_index.cc @@ -137,17 +137,23 @@ int ScoreForValue(int value, const int* value_ranks) { InMemoryURLIndex::InMemoryURLIndex(const FilePath& history_dir) : history_dir_(history_dir), - history_item_count_(0) { + history_item_count_(0), + cached_at_shutdown_(false) { InMemoryURLIndex::InitializeSchemeWhitelist(&scheme_whitelist_); } // Called only by unit tests. InMemoryURLIndex::InMemoryURLIndex() - : history_item_count_(0) { + : history_item_count_(0), + cached_at_shutdown_(false) { InMemoryURLIndex::InitializeSchemeWhitelist(&scheme_whitelist_); } -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. + DCHECK(history_dir_.empty() || cached_at_shutdown_); +} // static void InMemoryURLIndex::InitializeSchemeWhitelist( @@ -174,6 +180,7 @@ bool InMemoryURLIndex::Init(history::URLDatabase* history_db, void InMemoryURLIndex::ShutDown() { // Write our cache. SaveToCacheFile(); + cached_at_shutdown_ = true; } bool InMemoryURLIndex::IndexRow(const URLRow& row) { @@ -292,6 +299,10 @@ bool InMemoryURLIndex::RestoreFromCacheFile() { bool InMemoryURLIndex::SaveToCacheFile() { // TODO(mrossetti): Move File IO to another thread. base::ThreadRestrictions::ScopedAllowIO allow_io; + FilePath file_path; + if (!GetCacheFilePath(&file_path)) + return false; + base::TimeTicks beginning_time = base::TimeTicks::Now(); InMemoryURLIndexCacheItem index_cache; SavePrivateData(&index_cache); @@ -301,12 +312,6 @@ bool InMemoryURLIndex::SaveToCacheFile() { return false; } - // TODO(mrossetti): Write the cache to a file then swap it for the old cache, - // if any, and delete the old cache. - FilePath file_path; - if (!GetCacheFilePath(&file_path)) - return false; - int size = data.size(); if (file_util::WriteFile(file_path, data.c_str(), size) != size) { LOG(WARNING) << "Failed to write " << file_path.value(); diff --git a/chrome/browser/history/in_memory_url_index.h b/chrome/browser/history/in_memory_url_index.h index be31abe..5de5604 100644 --- a/chrome/browser/history/in_memory_url_index.h +++ b/chrome/browser/history/in_memory_url_index.h @@ -420,6 +420,13 @@ class InMemoryURLIndex { // Only URLs with a whitelisted scheme are indexed. std::set<std::string> scheme_whitelist_; + // 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. + // TODO(mrossetti): Eliminate once the transition to SQLite has been done. + // http://crbug.com/83659 + bool cached_at_shutdown_; + DISALLOW_COPY_AND_ASSIGN(InMemoryURLIndex); }; diff --git a/chrome/browser/history/in_memory_url_index_unittest.cc b/chrome/browser/history/in_memory_url_index_unittest.cc index de8e4e0c9..62b708d 100644 --- a/chrome/browser/history/in_memory_url_index_unittest.cc +++ b/chrome/browser/history/in_memory_url_index_unittest.cc @@ -203,7 +203,7 @@ void ExpandedInMemoryURLIndexTest::SetUp() { } TEST_F(InMemoryURLIndexTest, Construction) { - url_index_.reset(new InMemoryURLIndex(FilePath(FILE_PATH_LITERAL("/dummy")))); + url_index_.reset(new InMemoryURLIndex(FilePath())); EXPECT_TRUE(url_index_.get()); } @@ -215,7 +215,7 @@ TEST_F(LimitedInMemoryURLIndexTest, Initialization) { uint64 row_count = 0; while (statement.Step()) ++row_count; EXPECT_EQ(1U, row_count); - url_index_.reset(new InMemoryURLIndex); + url_index_.reset(new InMemoryURLIndex(FilePath())); url_index_->Init(this, "en,ja,hi,zh"); EXPECT_EQ(1, url_index_->history_item_count_); @@ -226,7 +226,7 @@ TEST_F(LimitedInMemoryURLIndexTest, Initialization) { } TEST_F(InMemoryURLIndexTest, Retrieval) { - url_index_.reset(new InMemoryURLIndex(FilePath(FILE_PATH_LITERAL("/dummy")))); + url_index_.reset(new InMemoryURLIndex(FilePath())); url_index_->Init(this, "en,ja,hi,zh"); // The term will be lowercased by the search. @@ -279,7 +279,7 @@ TEST_F(InMemoryURLIndexTest, Retrieval) { } TEST_F(ExpandedInMemoryURLIndexTest, ShortCircuit) { - url_index_.reset(new InMemoryURLIndex(FilePath(FILE_PATH_LITERAL("/dummy")))); + 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. @@ -293,7 +293,7 @@ TEST_F(ExpandedInMemoryURLIndexTest, ShortCircuit) { } TEST_F(InMemoryURLIndexTest, TitleSearch) { - url_index_.reset(new InMemoryURLIndex()); + 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_->history_info_map_.size()); @@ -316,7 +316,7 @@ TEST_F(InMemoryURLIndexTest, TitleSearch) { } TEST_F(InMemoryURLIndexTest, NonUniqueTermCharacterSets) { - url_index_.reset(new InMemoryURLIndex()); + url_index_.reset(new InMemoryURLIndex(FilePath())); url_index_->Init(this, "en,ja,hi,zh"); // The presence of duplicate characters should succeed. Exercise by cycling @@ -447,7 +447,7 @@ TEST_F(InMemoryURLIndexTest, TypedCharacterCaching) { typedef InMemoryURLIndex::SearchTermCacheMap::iterator CacheIter; typedef InMemoryURLIndex::SearchTermCacheItem CacheItem; - url_index_.reset(new InMemoryURLIndex(FilePath(FILE_PATH_LITERAL("/dummy")))); + url_index_.reset(new InMemoryURLIndex(FilePath())); url_index_->Init(this, "en,ja,hi,zh"); InMemoryURLIndex::SearchTermCacheMap& cache(url_index_->search_term_cache_); @@ -550,7 +550,7 @@ TEST_F(InMemoryURLIndexTest, Scoring) { } TEST_F(InMemoryURLIndexTest, AddNewRows) { - url_index_.reset(new InMemoryURLIndex(FilePath(FILE_PATH_LITERAL("/dummy")))); + url_index_.reset(new InMemoryURLIndex(FilePath())); url_index_->Init(this, "en,ja,hi,zh"); InMemoryURLIndex::String16Vector terms; @@ -575,7 +575,7 @@ TEST_F(InMemoryURLIndexTest, AddNewRows) { } TEST_F(InMemoryURLIndexTest, DeleteRows) { - url_index_.reset(new InMemoryURLIndex(FilePath(FILE_PATH_LITERAL("/dummy")))); + url_index_.reset(new InMemoryURLIndex(FilePath())); url_index_->Init(this, "en,ja,hi,zh"); InMemoryURLIndex::String16Vector terms; @@ -662,8 +662,7 @@ TEST_F(InMemoryURLIndexTest, WhitelistedURLs) { { "xmpp:node@example.com", false }, { "xmpp://guest@example.com", false }, }; - url_index_.reset(new InMemoryURLIndex(FilePath(FILE_PATH_LITERAL( - "/flammmy/frammy/")))); + 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, @@ -685,11 +684,13 @@ TEST_F(InMemoryURLIndexTest, CacheFilePath) { 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(FILE_PATH_LITERAL("/dummy")))); + 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; |