diff options
Diffstat (limited to 'chrome/browser/history')
-rw-r--r-- | chrome/browser/history/expire_history_backend_unittest.cc | 20 | ||||
-rw-r--r-- | chrome/browser/history/history_backend.cc | 4 | ||||
-rw-r--r-- | chrome/browser/history/history_backend_unittest.cc | 8 | ||||
-rw-r--r-- | chrome/browser/history/history_database.cc | 13 | ||||
-rw-r--r-- | chrome/browser/history/history_types.h | 11 | ||||
-rw-r--r-- | chrome/browser/history/history_unittest.cc | 5 | ||||
-rw-r--r-- | chrome/browser/history/thumbnail_database.cc | 18 | ||||
-rw-r--r-- | chrome/browser/history/thumbnail_database_unittest.cc | 20 | ||||
-rw-r--r-- | chrome/browser/history/top_sites.cc | 180 | ||||
-rw-r--r-- | chrome/browser/history/top_sites.h | 49 | ||||
-rw-r--r-- | chrome/browser/history/top_sites_database.cc | 15 | ||||
-rw-r--r-- | chrome/browser/history/top_sites_database.h | 21 | ||||
-rw-r--r-- | chrome/browser/history/top_sites_unittest.cc | 64 |
13 files changed, 287 insertions, 141 deletions
diff --git a/chrome/browser/history/expire_history_backend_unittest.cc b/chrome/browser/history/expire_history_backend_unittest.cc index ca822bc..95b391f 100644 --- a/chrome/browser/history/expire_history_backend_unittest.cc +++ b/chrome/browser/history/expire_history_backend_unittest.cc @@ -16,8 +16,10 @@ #include "chrome/browser/history/history_notifications.h" #include "chrome/browser/history/text_database_manager.h" #include "chrome/browser/history/thumbnail_database.h" +#include "chrome/browser/history/top_sites.h" #include "chrome/common/notification_service.h" #include "chrome/common/thumbnail_score.h" +#include "chrome/test/testing_profile.h" #include "chrome/tools/profiles/thumbnail-inl.h" #include "gfx/codec/jpeg_codec.h" #include "testing/gtest/include/gtest/gtest.h" @@ -90,6 +92,8 @@ class ExpireHistoryTest : public testing::Test, scoped_ptr<ArchivedDatabase> archived_db_; scoped_ptr<ThumbnailDatabase> thumb_db_; scoped_ptr<TextDatabaseManager> text_db_; + TestingProfile profile_; + scoped_refptr<TopSites> top_sites_; // Time at the beginning of the test, so everybody agrees what "now" is. const Time now_; @@ -134,6 +138,7 @@ class ExpireHistoryTest : public testing::Test, expirer_.SetDatabases(main_db_.get(), archived_db_.get(), thumb_db_.get(), text_db_.get()); + top_sites_ = profile_.GetTopSites(); } void TearDown() { @@ -145,6 +150,7 @@ class ExpireHistoryTest : public testing::Test, archived_db_.reset(); thumb_db_.reset(); text_db_.reset(); + TopSites::DeleteTopSites(top_sites_); file_util::Delete(dir_, true); } @@ -211,9 +217,9 @@ void ExpireHistoryTest::AddExampleData(URLID url_ids[3], Time visit_times[4]) { Time time; GURL gurl; - thumb_db_->SetPageThumbnail(gurl, url_ids[0], *thumbnail, score, time); - thumb_db_->SetPageThumbnail(gurl, url_ids[1], *thumbnail, score, time); - thumb_db_->SetPageThumbnail(gurl, url_ids[2], *thumbnail, score, time); + top_sites_->SetPageThumbnail(url_row1.url(), *thumbnail, score); + top_sites_->SetPageThumbnail(url_row2.url(), *thumbnail, score); + top_sites_->SetPageThumbnail(url_row3.url(), *thumbnail, score); // Four visits. VisitRow visit_row1; @@ -271,8 +277,12 @@ bool ExpireHistoryTest::HasFavIcon(FavIconID favicon_id) { } bool ExpireHistoryTest::HasThumbnail(URLID url_id) { - std::vector<unsigned char> temp_data; - return thumb_db_->GetPageThumbnail(url_id, &temp_data); + URLRow info; + if (!main_db_->GetURLRow(url_id, &info)) + return false; + GURL url = info.url(); + RefCountedBytes *data; + return top_sites_->GetPageThumbnail(url, &data); } int ExpireHistoryTest::CountTextMatchesForURL(const GURL& url) { diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc index 8e1c5fb..1c58f74 100644 --- a/chrome/browser/history/history_backend.cc +++ b/chrome/browser/history/history_backend.cc @@ -581,7 +581,7 @@ void HistoryBackend::InitImpl() { // Thumbnail database. thumbnail_db_.reset(new ThumbnailDatabase()); - if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kTopSites)) { + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) { if (!db_->needs_version_18_migration()) { // No convertion needed - use new filename right away. thumbnail_name = GetFaviconsFileName(); @@ -598,7 +598,7 @@ void HistoryBackend::InitImpl() { thumbnail_db_.reset(); } - if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kTopSites)) { + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) { if (db_->needs_version_18_migration()) { LOG(INFO) << "Starting TopSites migration"; delegate_->StartTopSitesMigration(); diff --git a/chrome/browser/history/history_backend_unittest.cc b/chrome/browser/history/history_backend_unittest.cc index 24cc1cd..780038d 100644 --- a/chrome/browser/history/history_backend_unittest.cc +++ b/chrome/browser/history/history_backend_unittest.cc @@ -4,6 +4,7 @@ #include "base/file_path.h" #include "base/file_util.h" +#include "base/command_line.h" #include "base/path_service.h" #include "base/ref_counted.h" #include "base/scoped_ptr.h" @@ -13,6 +14,7 @@ #include "chrome/browser/history/history_notifications.h" #include "chrome/browser/history/in_memory_history_backend.h" #include "chrome/browser/history/in_memory_database.h" +#include "chrome/common/chrome_switches.h" #include "chrome/common/notification_service.h" #include "chrome/common/thumbnail_score.h" #include "chrome/tools/profiles/thumbnail-inl.h" @@ -402,6 +404,9 @@ TEST_F(HistoryBackendTest, URLsNoLongerBookmarked) { TEST_F(HistoryBackendTest, GetPageThumbnailAfterRedirects) { ASSERT_TRUE(backend_.get()); + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) + return; + const char* base_url = "http://mail"; const char* thumbnail_url = "http://mail.google.com"; @@ -598,6 +603,9 @@ TEST_F(HistoryBackendTest, StripUsernamePasswordTest) { } TEST_F(HistoryBackendTest, DeleteThumbnailsDatabaseTest) { + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) + return; + EXPECT_TRUE(backend_->thumbnail_db_->NeedsMigrationToTopSites()); backend_->delegate_->StartTopSitesMigration(); EXPECT_FALSE(backend_->thumbnail_db_->NeedsMigrationToTopSites()); diff --git a/chrome/browser/history/history_database.cc b/chrome/browser/history/history_database.cc index 26bb81a..7869fcc 100644 --- a/chrome/browser/history/history_database.cc +++ b/chrome/browser/history/history_database.cc @@ -134,14 +134,11 @@ void HistoryDatabase::BeginExclusiveMode() { // static int HistoryDatabase::GetCurrentVersion() { - // Temporary solution while TopSites is behind a flag. If there is - // no flag, we are still using the Thumbnails file, i.e. we are at - // version 17. - if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kTopSites)) { + // If we don't use TopSites, we are one version number behind. + if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) + return 17; // Last pre-TopSites version. + else return kCurrentVersionNumber; - } else { - return kCurrentVersionNumber - 1; - } } void HistoryDatabase::BeginTransaction() { @@ -286,7 +283,7 @@ sql::InitStatus HistoryDatabase::EnsureCurrentVersion( if (cur_version == 17) needs_version_18_migration_ = true; - if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kTopSites) && + if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites) && cur_version == 18) { // Set DB version back to pre-top sites. cur_version = 17; diff --git a/chrome/browser/history/history_types.h b/chrome/browser/history/history_types.h index 0c283b1..21e8067 100644 --- a/chrome/browser/history/history_types.h +++ b/chrome/browser/history/history_types.h @@ -13,12 +13,14 @@ #include <vector> #include "base/basictypes.h" +#include "base/ref_counted_memory.h" #include "base/stack_container.h" #include "base/string16.h" #include "base/time.h" #include "chrome/browser/history/snippet.h" #include "chrome/common/page_transition_types.h" #include "chrome/common/ref_counted_util.h" +#include "chrome/common/thumbnail_score.h" #include "googleurl/src/gurl.h" namespace history { @@ -527,6 +529,15 @@ struct MostVisitedURL { } }; +// Used by TopSites to store the thumbnails. +struct Images { + scoped_refptr<RefCountedBytes> thumbnail; + ThumbnailScore thumbnail_score; + + // TODO(brettw): this will eventually store the favicon. + // scoped_refptr<RefCountedBytes> favicon; +}; + typedef std::vector<MostVisitedURL> MostVisitedURLList; // Used for intermediate history result operations. diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc index c8db05a..063414f 100644 --- a/chrome/browser/history/history_unittest.cc +++ b/chrome/browser/history/history_unittest.cc @@ -24,6 +24,7 @@ #include "app/sql/statement.h" #include "base/basictypes.h" #include "base/callback.h" +#include "base/command_line.h" #include "base/file_path.h" #include "base/file_util.h" #include "base/message_loop.h" @@ -41,6 +42,7 @@ #include "chrome/browser/history/in_memory_history_backend.h" #include "chrome/browser/history/page_usage_data.h" #include "chrome/common/chrome_paths.h" +#include "chrome/common/chrome_switches.h" #include "chrome/common/notification_service.h" #include "chrome/common/thumbnail_score.h" #include "chrome/tools/profiles/thumbnail-inl.h" @@ -685,6 +687,9 @@ TEST_F(HistoryTest, Segments) { // This just tests history system -> thumbnail database integration, the actual // thumbnail tests are in its own file. TEST_F(HistoryTest, Thumbnails) { + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) + return; // TopSitesTest replaces this. + scoped_refptr<HistoryService> history(new HistoryService); history_service_ = history; ASSERT_TRUE(history->Init(history_dir_, NULL)); diff --git a/chrome/browser/history/thumbnail_database.cc b/chrome/browser/history/thumbnail_database.cc index 8bf203d..979accf 100644 --- a/chrome/browser/history/thumbnail_database.cc +++ b/chrome/browser/history/thumbnail_database.cc @@ -130,7 +130,7 @@ sql::InitStatus ThumbnailDatabase::OpenDatabase(sql::Connection* db, bool ThumbnailDatabase::InitThumbnailTable() { if (!db_.DoesTableExist("thumbnails")) { - if (CommandLine::ForCurrentProcess()-> HasSwitch(switches::kTopSites)) { + if (!CommandLine::ForCurrentProcess()-> HasSwitch(switches::kNoTopSites)) { use_top_sites_ = true; return true; } @@ -231,8 +231,10 @@ void ThumbnailDatabase::SetPageThumbnail( const SkBitmap& thumbnail, const ThumbnailScore& score, base::Time time) { - if (use_top_sites_) + if (use_top_sites_) { + LOG(WARNING) << "Use TopSites instead."; return; // Not possible after migration to TopSites. + } if (!thumbnail.isNull()) { bool add_thumbnail = true; @@ -286,8 +288,10 @@ void ThumbnailDatabase::SetPageThumbnail( bool ThumbnailDatabase::GetPageThumbnail(URLID id, std::vector<unsigned char>* data) { - if (use_top_sites_) + if (use_top_sites_) { + LOG(WARNING) << "Use TopSites instead."; return false; // Not possible after migration to TopSites. + } sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE, "SELECT data FROM thumbnails WHERE url_id=?")); @@ -303,8 +307,10 @@ bool ThumbnailDatabase::GetPageThumbnail(URLID id, } bool ThumbnailDatabase::DeleteThumbnail(URLID id) { - if (use_top_sites_) + if (use_top_sites_) { + LOG(WARNING) << "Use TopSites instead."; return true; // Not possible after migration to TopSites. + } sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE, "DELETE FROM thumbnails WHERE url_id = ?")); @@ -317,8 +323,10 @@ bool ThumbnailDatabase::DeleteThumbnail(URLID id) { bool ThumbnailDatabase::ThumbnailScoreForId(URLID id, ThumbnailScore* score) { - if (use_top_sites_) + if (use_top_sites_) { + LOG(WARNING) << "Use TopSites instead."; return false; // Not possible after migration to TopSites. + } // Fetch the current thumbnail's information to make sure we // aren't replacing a good thumbnail with one that's worse. diff --git a/chrome/browser/history/thumbnail_database_unittest.cc b/chrome/browser/history/thumbnail_database_unittest.cc index 4d2c2bf..24c990b 100644 --- a/chrome/browser/history/thumbnail_database_unittest.cc +++ b/chrome/browser/history/thumbnail_database_unittest.cc @@ -6,6 +6,7 @@ #include <vector> #include "base/basictypes.h" +#include "base/command_line.h" #include "base/file_path.h" #include "base/file_util.h" #include "base/path_service.h" @@ -13,6 +14,7 @@ #include "base/scoped_temp_dir.h" #include "chrome/browser/history/thumbnail_database.h" #include "chrome/common/chrome_paths.h" +#include "chrome/common/chrome_switches.h" #include "chrome/common/thumbnail_score.h" #include "chrome/tools/profiles/thumbnail-inl.h" #include "gfx/codec/jpeg_codec.h" @@ -70,6 +72,9 @@ class ThumbnailDatabaseTest : public testing::Test { }; TEST_F(ThumbnailDatabaseTest, AddDelete) { + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) + return; // TopSitesTest replaces this. + ThumbnailDatabase db; ASSERT_EQ(sql::INIT_OK, db.Init(file_name_, NULL)); @@ -112,6 +117,9 @@ TEST_F(ThumbnailDatabaseTest, AddDelete) { } TEST_F(ThumbnailDatabaseTest, UseLessBoringThumbnails) { + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) + return; // TopSitesTest replaces this. + ThumbnailDatabase db; Time now = Time::Now(); ASSERT_EQ(sql::INIT_OK, db.Init(file_name_, NULL)); @@ -146,6 +154,9 @@ TEST_F(ThumbnailDatabaseTest, UseLessBoringThumbnails) { } TEST_F(ThumbnailDatabaseTest, UseAtTopThumbnails) { + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) + return; // TopSitesTest replaces this. + ThumbnailDatabase db; Time now = Time::Now(); ASSERT_EQ(sql::INIT_OK, db.Init(file_name_, NULL)); @@ -217,6 +228,9 @@ TEST_F(ThumbnailDatabaseTest, UseAtTopThumbnails) { } TEST_F(ThumbnailDatabaseTest, ThumbnailTimeDegradation) { + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) + return; // TopSitesTest replaces this. + ThumbnailDatabase db; const Time kNow = Time::Now(); const Time kThreeHoursAgo = kNow - TimeDelta::FromHours(4); @@ -261,6 +275,9 @@ TEST_F(ThumbnailDatabaseTest, NeverAcceptTotallyBoringThumbnail) { // should replace a thumbnail with another because of reasons other // than straight up boringness score, still reject because the // thumbnail is totally boring. + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) + return; // TopSitesTest replaces this. + ThumbnailDatabase db; Time now = Time::Now(); ASSERT_EQ(sql::INIT_OK, db.Init(file_name_, NULL)); @@ -332,6 +349,9 @@ TEST_F(ThumbnailDatabaseTest, NeverAcceptTotallyBoringThumbnail) { } TEST_F(ThumbnailDatabaseTest, NeedsMigrationToTopSites) { + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoTopSites)) + return; // TopSitesTest replaces this. + ThumbnailDatabase db; ASSERT_EQ(sql::INIT_OK, db.Init(file_name_, NULL)); db.BeginTransaction(); diff --git a/chrome/browser/history/top_sites.cc b/chrome/browser/history/top_sites.cc index 7d8c66a..eb0fb74 100644 --- a/chrome/browser/history/top_sites.cc +++ b/chrome/browser/history/top_sites.cc @@ -54,10 +54,15 @@ TopSites::TopSites(Profile* profile) : profile_(profile), waiting_for_results_(true), blacklist_(NULL), pinned_urls_(NULL) { - registrar_.Add(this, NotificationType::HISTORY_URLS_DELETED, - Source<Profile>(profile_)); - registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED, - NotificationService::AllSources()); + if (!profile_) + return; + + if (NotificationService::current()) { + registrar_.Add(this, NotificationType::HISTORY_URLS_DELETED, + Source<Profile>(profile_)); + registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED, + NotificationService::AllSources()); + } blacklist_ = profile_->GetPrefs()-> GetMutableDictionary(prefs::kNTPMostVisitedURLsBlacklist); @@ -90,16 +95,14 @@ void TopSites::ReadDatabase() { std::map<GURL, Images> thumbnails; DCHECK(db_.get()); - { - AutoLock lock(lock_); - MostVisitedURLList top_urls; - db_->GetPageThumbnails(&top_urls, &thumbnails); - MostVisitedURLList copy(top_urls); // StoreMostVisited destroys the list. - StoreMostVisited(&top_urls); - if (AddPrepopulatedPages(©)) - UpdateMostVisited(copy); - } // Lock is released here. + MostVisitedURLList top_urls; + db_->GetPageThumbnails(&top_urls, &thumbnails); + MostVisitedURLList copy(top_urls); // StoreMostVisited destroys the list. + StoreMostVisited(&top_urls); + if (AddPrepopulatedPages(©)) + UpdateMostVisited(copy); + AutoLock lock(lock_); for (size_t i = 0; i < top_sites_.size(); i++) { GURL url = top_sites_[i].url; Images thumbnail = thumbnails[url]; @@ -145,12 +148,14 @@ bool TopSites::SetPageThumbnail(const GURL& url, return true; } - return SetPageThumbnail(url, thumbnail_data, score); + AutoLock lock(lock_); + return SetPageThumbnailEncoded(url, thumbnail_data, score); } -bool TopSites::SetPageThumbnail(const GURL& url, - const RefCountedBytes* thumbnail, - const ThumbnailScore& score) { +bool TopSites::SetPageThumbnailEncoded(const GURL& url, + const RefCountedBytes* thumbnail, + const ThumbnailScore& score) { + lock_.AssertAcquired(); if (!SetPageThumbnailNoDB(url, thumbnail, score)) return false; @@ -171,7 +176,7 @@ bool TopSites::SetPageThumbnail(const GURL& url, void TopSites::WriteThumbnailToDB(const MostVisitedURL& url, int url_rank, - const TopSites::Images& thumbnail) { + const Images& thumbnail) { DCHECK(db_.get()); DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB)); db_->SetPageThumbnail(url, url_rank, thumbnail); @@ -181,7 +186,7 @@ void TopSites::WriteThumbnailToDB(const MostVisitedURL& url, bool TopSites::SetPageThumbnailNoDB(const GURL& url, const RefCountedBytes* thumbnail_data, const ThumbnailScore& score) { - AutoLock lock(lock_); + lock_.AssertAcquired(); std::map<GURL, size_t>::iterator found = canonical_urls_.find(url); if (found == canonical_urls_.end()) { @@ -237,15 +242,21 @@ void TopSites::GetMostVisitedURLs(CancelableRequestConsumer* consumer, return; MostVisitedURLList filtered_urls; - ApplyBlacklistAndPinnedURLs(top_sites_, &filtered_urls); - + { + AutoLock lock(lock_); + ApplyBlacklistAndPinnedURLs(top_sites_, &filtered_urls); + } request->ForwardResult(GetTopSitesCallback::TupleType(filtered_urls)); } bool TopSites::GetPageThumbnail(const GURL& url, RefCountedBytes** data) const { + AutoLock lock(lock_); std::map<GURL, Images>::const_iterator found = top_images_.find(url); - if (found == top_images_.end()) - return false; // No thumbnail for this URL. + if (found == top_images_.end()) { + found = temp_thumbnails_map_.find(url); + if (found == temp_thumbnails_map_.end()) + return false; // No thumbnail for this URL. + } Images image = found->second; *data = image.thumbnail.get(); @@ -262,8 +273,11 @@ static int IndexOf(const MostVisitedURLList& urls, const GURL& url) { int TopSites::GetIndexForChromeStore(const MostVisitedURLList& urls) { GURL store_url = MostVisitedHandler::GetChromeStoreURLWithLocale(); - if (IsBlacklisted(store_url)) - return -1; + { + AutoLock lock(lock_); + if (IsBlacklisted(store_url)) + return -1; + } if (IndexOf(urls, store_url) != -1) return -1; // It's already there, no need to add. @@ -290,6 +304,9 @@ bool TopSites::AddChromeStore(MostVisitedURLList* urls) { if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableApps)) return false; + if (!profile_) + return false; + ExtensionsService* service = profile_->GetExtensionsService(); if (!service || service->HasApps()) return false; @@ -371,6 +388,7 @@ void TopSites::MigratePinnedURLs() { void TopSites::ApplyBlacklistAndPinnedURLs(const MostVisitedURLList& urls, MostVisitedURLList* out) { MostVisitedURLList urls_copy; + lock_.AssertAcquired(); for (size_t i = 0; i < urls.size(); i++) { if (!IsBlacklisted(urls[i].url)) urls_copy.push_back(urls[i]); @@ -417,10 +435,12 @@ void TopSites::ApplyBlacklistAndPinnedURLs(const MostVisitedURLList& urls, } std::wstring TopSites::GetURLString(const GURL& url) { + lock_.AssertAcquired(); return ASCIIToWide(GetCanonicalURL(url).spec()); } std::wstring TopSites::GetURLHash(const GURL& url) { + lock_.AssertAcquired(); return ASCIIToWide(MD5String(GetCanonicalURL(url).spec())); } @@ -430,27 +450,33 @@ void TopSites::UpdateMostVisited(MostVisitedURLList most_visited) { std::vector<size_t> added; // Indices into most_visited. std::vector<size_t> deleted; // Indices into top_sites_. std::vector<size_t> moved; // Indices into most_visited. + DiffMostVisited(top_sites_, most_visited, &added, &deleted, &moved); // #added == #deleted; #added + #moved = total. last_num_urls_changed_ = added.size() + moved.size(); - // Process the diff: delete from images and disk, add to disk. - // Delete all the thumbnails associated with URLs that were deleted. - for (size_t i = 0; i < deleted.size(); i++) { - const MostVisitedURL& deleted_url = top_sites_[deleted[i]]; - std::map<GURL, Images>::iterator found = - top_images_.find(deleted_url.url); - if (found != top_images_.end()) - top_images_.erase(found); + { + AutoLock lock(lock_); - // Delete from disk. - if (db_.get()) - db_->RemoveURL(deleted_url); + // Process the diff: delete from images and disk, add to disk. + // Delete all the thumbnails associated with URLs that were deleted. + for (size_t i = 0; i < deleted.size(); i++) { + const MostVisitedURL& deleted_url = top_sites_[deleted[i]]; + std::map<GURL, Images>::iterator found = + top_images_.find(deleted_url.url); + if (found != top_images_.end()) + top_images_.erase(found); + } } + // Write the updates to the DB. if (db_.get()) { - // Write both added and moved urls. + for (size_t i = 0; i < deleted.size(); i++) { + const MostVisitedURL& deleted_url = top_sites_[deleted[i]]; + if (db_.get()) + db_->RemoveURL(deleted_url); + } for (size_t i = 0; i < added.size(); i++) { const MostVisitedURL& added_url = most_visited[added[i]]; db_->SetPageThumbnail(added_url, added[i], Images()); @@ -484,6 +510,9 @@ void TopSites::UpdateMostVisited(MostVisitedURLList most_visited) { void TopSites::OnMigrationDone() { migration_in_progress_ = false; + if (!profile_) + return; + HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS); // |hs| may be null during unit tests. if (!hs) @@ -521,6 +550,9 @@ void TopSites::StartQueryForThumbnail(size_t index) { return; } + if (!profile_) + return; + HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS); // |hs| may be null during unit tests. if (!hs) @@ -533,6 +565,7 @@ void TopSites::StartQueryForThumbnail(size_t index) { } void TopSites::GenerateCanonicalURLs() { + lock_.AssertAcquired(); canonical_urls_.clear(); for (size_t i = 0; i < top_sites_.size(); i++) { const MostVisitedURL& mv = top_sites_[i]; @@ -542,8 +575,9 @@ void TopSites::GenerateCanonicalURLs() { void TopSites::StoreMostVisited(MostVisitedURLList* most_visited) { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB)); - // Take ownership of the most visited data. + AutoLock lock(lock_); top_sites_.clear(); + // Take ownership of the most visited data. top_sites_.swap(*most_visited); waiting_for_results_ = false; @@ -560,8 +594,8 @@ void TopSites::StoreMostVisited(MostVisitedURLList* most_visited) { // when we add a temp thumbnail, redirect chain is not known. // This is slow, but temp_thumbnails_map_ should have very few URLs. if (canonical_url == GetCanonicalURL(it->first)) { - SetPageThumbnail(mv.url, it->second.thumbnail, - it->second.thumbnail_score); + SetPageThumbnailEncoded(mv.url, it->second.thumbnail, + it->second.thumbnail_score); temp_thumbnails_map_.erase(it); break; } @@ -584,6 +618,7 @@ void TopSites::StoreRedirectChain(const RedirectList& redirects, } GURL TopSites::GetCanonicalURL(const GURL& url) const { + lock_.AssertAcquired(); std::map<GURL, size_t>::const_iterator found = canonical_urls_.find(url); if (found == canonical_urls_.end()) return url; // Unknown URL - return unchanged. @@ -652,6 +687,9 @@ void TopSites::StartQueryForMostVisited() { &cancelable_consumer_, NewCallback(this, &TopSites::OnTopSitesAvailable)); } else { + if (!profile_) + return; + HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS); // |hs| may be null during unit tests. if (hs) { @@ -673,17 +711,20 @@ void TopSites::StartMigration() { } void TopSites::AddBlacklistedURL(const GURL& url) { - RemovePinnedURL(url); + AutoLock lock(lock_); + RemovePinnedURLLocked(url); Value* dummy = Value::CreateNullValue(); blacklist_->SetWithoutPathExpansion(GetURLHash(url), dummy); } bool TopSites::IsBlacklisted(const GURL& url) { + lock_.AssertAcquired(); bool result = blacklist_->HasKey(GetURLHash(url)); return result; } void TopSites::RemoveBlacklistedURL(const GURL& url) { + AutoLock lock(lock_); blacklist_->RemoveWithoutPathExpansion(GetURLHash(url), NULL); } @@ -702,22 +743,22 @@ void TopSites::AddPinnedURL(const GURL& url, size_t pinned_index) { } Value* index = Value::CreateIntegerValue(pinned_index); + AutoLock lock(lock_); pinned_urls_->SetWithoutPathExpansion(GetURLString(url), index); } void TopSites::RemovePinnedURL(const GURL& url) { - pinned_urls_->RemoveWithoutPathExpansion(GetURLString(url), NULL); + AutoLock lock(lock_); + RemovePinnedURLLocked(url); } -bool TopSites::GetIndexOfPinnedURL(const GURL& url, size_t* index) { - int tmp; - bool result = pinned_urls_->GetIntegerWithoutPathExpansion( - GetURLString(url), &tmp); - *index = static_cast<size_t>(tmp); - return result; +void TopSites::RemovePinnedURLLocked(const GURL& url) { + lock_.AssertAcquired(); + pinned_urls_->RemoveWithoutPathExpansion(GetURLString(url), NULL); } bool TopSites::IsURLPinned(const GURL& url) { + AutoLock lock(lock_); int tmp; bool result = pinned_urls_->GetIntegerWithoutPathExpansion( GetURLString(url), &tmp); @@ -738,6 +779,24 @@ bool TopSites::GetPinnedURLAtIndex(size_t index, GURL* url) { return false; } +// static +void TopSites::DeleteTopSites(scoped_refptr<TopSites>& ptr) { + if (!ptr.get() || !MessageLoop::current()) + return; + if (ChromeThread::IsWellKnownThread(ChromeThread::UI)) { + ptr = NULL; + } else { + // Need to roll our own UI thread. + ChromeThread ui_loop(ChromeThread::UI, MessageLoop::current()); + ptr = NULL; + MessageLoop::current()->RunAllPending(); + } +} + +void TopSites::ClearProfile() { + profile_ = NULL; +} + base::TimeDelta TopSites::GetUpdateDelay() { if (top_sites_.size() == 0) return base::TimeDelta::FromSeconds(30); @@ -758,7 +817,10 @@ void TopSites::OnTopSitesAvailable( if (!pending_callbacks_.empty()) { MostVisitedURLList filtered_urls; - ApplyBlacklistAndPinnedURLs(pages, &filtered_urls); + { + AutoLock lock(lock_); + ApplyBlacklistAndPinnedURLs(pages, &filtered_urls); + } PendingCallbackSet::iterator i; for (i = pending_callbacks_.begin(); @@ -777,6 +839,9 @@ void TopSites::OnThumbnailAvailable(CancelableRequestProvider::Handle handle, if (mock_history_service_) { index = handle; } else { + if (!profile_) + return; + HistoryService* hs = profile_ ->GetHistoryService(Profile::EXPLICIT_ACCESS); index = cancelable_consumer_.GetClientData(hs, handle); } @@ -787,7 +852,8 @@ void TopSites::OnThumbnailAvailable(CancelableRequestProvider::Handle handle, if (thumbnail.get() && thumbnail->size()) { const MostVisitedURL& url = top_sites_[index]; - SetPageThumbnail(url.url, thumbnail, ThumbnailScore()); + AutoLock lock(lock_); + SetPageThumbnailEncoded(url.url, thumbnail, ThumbnailScore()); } if (migration_in_progress_ && migration_pending_urls_.empty() && @@ -803,6 +869,7 @@ void TopSites::Observe(NotificationType type, const NotificationSource& source, const NotificationDetails& details) { if (type == NotificationType::HISTORY_URLS_DELETED) { + AutoLock lock(lock_); Details<history::URLsDeletedDetails> deleted_details(details); if (deleted_details->all_history) { top_sites_.clear(); @@ -821,7 +888,7 @@ void TopSites::Observe(NotificationType type, for (std::set<size_t>::reverse_iterator i = indices_to_delete.rbegin(); i != indices_to_delete.rend(); i++) { size_t index = *i; - RemovePinnedURL(top_sites_[index].url); + RemovePinnedURLLocked(top_sites_[index].url); top_sites_.erase(top_sites_.begin() + index); } } @@ -830,11 +897,14 @@ void TopSites::Observe(NotificationType type, StartQueryForMostVisited(); } else if (type == NotificationType::NAV_ENTRY_COMMITTED) { if (top_sites_.size() < kTopSitesNumber) { - const NavigationController::LoadCommittedDetails& load_details = - *Details<NavigationController::LoadCommittedDetails>(details).ptr(); - GURL url = load_details.entry->url(); + NavigationController::LoadCommittedDetails* load_details = + Details<NavigationController::LoadCommittedDetails>(details).ptr(); + if (!load_details) + return; + GURL url = load_details->entry->url(); if (canonical_urls_.find(url) == canonical_urls_.end() && - HistoryService::CanAddURL(url)) { + HistoryService::CanAddURL(url)) { + AutoLock lock(lock_); // Add this page to the known pages in case the thumbnail comes // in before we get the results. MostVisitedURL mv; diff --git a/chrome/browser/history/top_sites.h b/chrome/browser/history/top_sites.h index 09d0af0..b460afb 100644 --- a/chrome/browser/history/top_sites.h +++ b/chrome/browser/history/top_sites.h @@ -18,6 +18,7 @@ #include "base/ref_counted.h" #include "base/ref_counted_memory.h" #include "chrome/browser/cancelable_request.h" +#include "chrome/browser/chrome_thread.h" #include "chrome/browser/history/history_types.h" #include "chrome/browser/history/history.h" #include "chrome/browser/history/page_usage_data.h" @@ -47,9 +48,11 @@ typedef std::vector<MostVisitedURL> MostVisitedURLList; // new tab page requests on the I/O thread without proxying to the UI thread is // a nontrivial performance win, especially when the browser is starting and // the UI thread is busy. -class TopSites : public NotificationObserver, - public base::RefCountedThreadSafe<TopSites>, - public CancelableRequestProvider { +class TopSites : + public base::RefCountedThreadSafe<TopSites, + ChromeThread::DeleteOnUIThread>, + public NotificationObserver, + public CancelableRequestProvider { public: explicit TopSites(Profile* profile); @@ -68,14 +71,6 @@ class TopSites : public NotificationObserver, size_t index) = 0; }; - struct Images { - scoped_refptr<RefCountedBytes> thumbnail; - ThumbnailScore thumbnail_score; - - // TODO(brettw): this will eventually store the favicon. - // scoped_refptr<RefCountedBytes> favicon; - }; - // Initializes TopSites. void Init(const FilePath& db_name); @@ -108,9 +103,6 @@ class TopSites : public NotificationObserver, // Add a URL to the blacklist. void AddBlacklistedURL(const GURL& url); - // Returns true if the URL is blacklisted. - bool IsBlacklisted(const GURL& url); - // Removes a URL from the blacklist. void RemoveBlacklistedURL(const GURL& url); @@ -122,9 +114,6 @@ class TopSites : public NotificationObserver, // Pin a URL at |index|. void AddPinnedURL(const GURL& url, size_t index); - // Get the index of a URL. Returns true if |url| is pinned. - bool GetIndexOfPinnedURL(const GURL& url, size_t* index); - // Returns true if a URL is pinned. bool IsURLPinned(const GURL& url); @@ -135,8 +124,18 @@ class TopSites : public NotificationObserver, // is a URL pinned at |index|. bool GetPinnedURLAtIndex(size_t index, GURL* out); + // TopSites must be deleted on a UI thread. This happens + // automatically in a real browser, but in unit_tests we don't have + // a real UI thread. Use this function to delete a TopSites object. + static void DeleteTopSites(scoped_refptr<TopSites>& ptr); + + // Sets the profile pointer to NULL. This is for the case where + // TopSites outlives the profile, since TopSites is refcounted. + void ClearProfile(); + private: - friend class base::RefCountedThreadSafe<TopSites>; + friend struct ChromeThread::DeleteOnThread<ChromeThread::UI>; + friend class DeleteTask<TopSites>; friend class TopSitesTest; FRIEND_TEST_ALL_PREFIXES(TopSitesTest, GetMostVisited); FRIEND_TEST_ALL_PREFIXES(TopSitesTest, RealDatabase); @@ -165,9 +164,9 @@ class TopSites : public NotificationObserver, // A version of SetPageThumbnail that takes RefCountedBytes as // returned by HistoryService. - bool SetPageThumbnail(const GURL& url, - const RefCountedBytes* thumbnail, - const ThumbnailScore& score); + bool SetPageThumbnailEncoded(const GURL& url, + const RefCountedBytes* thumbnail, + const ThumbnailScore& score); // Query history service for the list of available thumbnails. void StartQueryForMostVisited(); @@ -229,6 +228,12 @@ class TopSites : public NotificationObserver, const NotificationSource& source, const NotificationDetails& details); + // Returns true if the URL is blacklisted. + bool IsBlacklisted(const GURL& url); + + // A variant of RemovePinnedURL that must be called within a lock. + void RemovePinnedURLLocked(const GURL& url); + // Returns the delay until the next update of history is needed. // Uses num_urls_changed base::TimeDelta GetUpdateDelay(); @@ -243,7 +248,7 @@ class TopSites : public NotificationObserver, // Write a thumbnail to database. void WriteThumbnailToDB(const MostVisitedURL& url, int url_rank, - const TopSites::Images& thumbnail); + const Images& thumbnail); // Updates the top sites list and writes the difference to disk. void UpdateMostVisited(MostVisitedURLList most_visited); diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc index 99f0bb4..96da3f9 100644 --- a/chrome/browser/history/top_sites_database.cc +++ b/chrome/browser/history/top_sites_database.cc @@ -5,6 +5,7 @@ #include "app/sql/transaction.h" #include "base/string_util.h" #include "chrome/browser/diagnostics/sqlite_diagnostics.h" +#include "chrome/browser/history/history_types.h" #include "chrome/browser/history/top_sites.h" #include "chrome/browser/history/top_sites_database.h" @@ -48,7 +49,7 @@ bool TopSitesDatabaseImpl::InitThumbnailTable() { void TopSitesDatabaseImpl::GetPageThumbnails(MostVisitedURLList* urls, std::map<GURL, - TopSites::Images>* thumbnails) { + Images>* thumbnails) { sql::Statement statement(db_.GetCachedStatement( SQL_FROM_HERE, "SELECT url, url_rank, title, thumbnail, redirects, " @@ -75,7 +76,7 @@ void TopSitesDatabaseImpl::GetPageThumbnails(MostVisitedURLList* urls, std::vector<unsigned char> data; statement.ColumnBlobAsVector(3, &data); - TopSites::Images thumbnail; + Images thumbnail; thumbnail.thumbnail = RefCountedBytes::TakeVector(&data); thumbnail.thumbnail_score.boring_score = statement.ColumnDouble(5); thumbnail.thumbnail_score.good_clipping = statement.ColumnBool(6); @@ -106,7 +107,7 @@ void TopSitesDatabaseImpl::SetRedirects(const std::string& redirects, void TopSitesDatabaseImpl::SetPageThumbnail(const MostVisitedURL& url, int new_rank, - const TopSites::Images& thumbnail) { + const Images& thumbnail) { sql::Transaction transaction(&db_); transaction.Begin(); @@ -122,7 +123,7 @@ void TopSitesDatabaseImpl::SetPageThumbnail(const MostVisitedURL& url, } void TopSitesDatabaseImpl::UpdatePageThumbnail( - const MostVisitedURL& url, const TopSites::Images& thumbnail) { + const MostVisitedURL& url, const Images& thumbnail) { sql::Statement statement(db_.GetCachedStatement( SQL_FROM_HERE, "UPDATE thumbnails SET " @@ -150,7 +151,7 @@ void TopSitesDatabaseImpl::UpdatePageThumbnail( void TopSitesDatabaseImpl::AddPageThumbnail(const MostVisitedURL& url, int new_rank, - const TopSites::Images& thumbnail) { + const Images& thumbnail) { int count = GetRowCount(); sql::Statement statement(db_.GetCachedStatement( @@ -194,7 +195,7 @@ void TopSitesDatabaseImpl::UpdatePageRankNoTransaction( const MostVisitedURL& url, int new_rank) { int prev_rank = GetURLRank(url); if (prev_rank == -1) { - NOTREACHED() << "Updating rank of an unknown URL: " << url.url.spec(); + LOG(WARNING) << "Updating rank of an unknown URL: " << url.url.spec(); return; } @@ -236,7 +237,7 @@ void TopSitesDatabaseImpl::UpdatePageRankNoTransaction( } bool TopSitesDatabaseImpl::GetPageThumbnail(const GURL& url, - TopSites::Images* thumbnail) { + Images* thumbnail) { sql::Statement statement(db_.GetCachedStatement( SQL_FROM_HERE, "SELECT thumbnail, boring_score, good_clipping, at_top, last_updated " diff --git a/chrome/browser/history/top_sites_database.h b/chrome/browser/history/top_sites_database.h index 18ec201..11188a7 100644 --- a/chrome/browser/history/top_sites_database.h +++ b/chrome/browser/history/top_sites_database.h @@ -12,13 +12,12 @@ #include "app/sql/connection.h" #include "base/ref_counted.h" -#include "chrome/browser/history/history_types.h" #include "chrome/browser/history/url_database.h" // For DBCloseScoper. class FilePath; class RefCountedMemory; class SkBitmap; -class TopSites; +class Images; namespace base { class Time; @@ -38,7 +37,7 @@ class TopSitesDatabase { // Returns a list of all URLs currently in the table. virtual void GetPageThumbnails(MostVisitedURLList* urls, std::map<GURL, - TopSites::Images>* thumbnails) = 0; + Images>* thumbnails) = 0; // Set a thumbnail for a URL. |url_rank| is the position of the URL // in the list of TopURLs, zero-based. @@ -46,20 +45,20 @@ class TopSitesDatabase { // thumbnail. virtual void SetPageThumbnail(const MostVisitedURL& url, int url_rank, - const TopSites::Images& thumbnail) = 0; + const Images& thumbnail) = 0; // Update rank of a URL that's already in the database. virtual void UpdatePageRank(const MostVisitedURL& url, int new_rank) = 0; // Convenience wrapper. bool GetPageThumbnail(const MostVisitedURL& url, - TopSites::Images* thumbnail) { + Images* thumbnail) { return GetPageThumbnail(url.url, thumbnail); } // Get a thumbnail for a given page. Returns true iff we have the thumbnail. virtual bool GetPageThumbnail(const GURL& url, - TopSites::Images* thumbnail) = 0; + Images* thumbnail) = 0; // Remove the record for this URL. Returns true iff removed successfully. virtual bool RemoveURL(const MostVisitedURL& url) = 0; @@ -79,7 +78,7 @@ class TopSitesDatabaseImpl : public TopSitesDatabase { // Returns a list of all URLs currently in the table. // WARNING: clears both input arguments. virtual void GetPageThumbnails(MostVisitedURLList* urls, - std::map<GURL, TopSites::Images>* thumbnails); + std::map<GURL, Images>* thumbnails); // Set a thumbnail for a URL. |url_rank| is the position of the URL // in the list of TopURLs, zero-based. @@ -87,7 +86,7 @@ class TopSitesDatabaseImpl : public TopSitesDatabase { // thumbnail and rank. Shift the ranks of other URLs if necessary. virtual void SetPageThumbnail(const MostVisitedURL& url, int new_rank, - const TopSites::Images& thumbnail); + const Images& thumbnail); // Sets the rank for a given URL. The URL must be in the database. // Use SetPageThumbnail if it's not. @@ -95,7 +94,7 @@ class TopSitesDatabaseImpl : public TopSitesDatabase { // Get a thumbnail for a given page. Returns true iff we have the thumbnail. virtual bool GetPageThumbnail(const GURL& url, - TopSites::Images* thumbnail); + Images* thumbnail); // Remove the record for this URL. Returns true iff removed successfully. virtual bool RemoveURL(const MostVisitedURL& url); @@ -108,14 +107,14 @@ class TopSitesDatabaseImpl : public TopSitesDatabase { // Adds a new URL to the database. void AddPageThumbnail(const MostVisitedURL& url, int new_rank, - const TopSites::Images& thumbnail); + const Images& thumbnail); // Sets the page rank. Should be called within an open transaction. void UpdatePageRankNoTransaction(const MostVisitedURL& url, int new_rank); // Updates thumbnail of a URL that's already in the database. void UpdatePageThumbnail(const MostVisitedURL& url, - const TopSites::Images& thumbnail); + const Images& thumbnail); // Returns the URL's current rank or -1 if it is not present. int GetURLRank(const MostVisitedURL& url); diff --git a/chrome/browser/history/top_sites_unittest.cc b/chrome/browser/history/top_sites_unittest.cc index 94db4f2..f8ebf95 100644 --- a/chrome/browser/history/top_sites_unittest.cc +++ b/chrome/browser/history/top_sites_unittest.cc @@ -74,7 +74,7 @@ class TopSitesTest : public testing::Test { virtual void TearDown() { profile_.reset(); - top_sites_ = NULL; + TopSites::DeleteTopSites(top_sites_); EXPECT_TRUE(file_util::Delete(file_name_, false)); } @@ -92,7 +92,6 @@ class TopSitesTest : public testing::Test { } void StoreMostVisited(std::vector<MostVisitedURL>* urls) { - AutoLock lock(top_sites_->lock_); // The function asserts it's in the lock. top_sites_->StoreMostVisited(urls); } @@ -105,6 +104,10 @@ class TopSitesTest : public testing::Test { added_urls, deleted_urls, moved_urls); } + Lock& lock() { + return top_sites_->lock_; + } + private: scoped_refptr<TopSites> top_sites_; MostVisitedURLList urls_; @@ -121,6 +124,7 @@ class TopSitesTest : public testing::Test { DISALLOW_COPY_AND_ASSIGN(TopSitesTest); }; + // A mockup of a HistoryService used for testing TopSites. class MockHistoryServiceImpl : public TopSites::MockHistoryService { public: @@ -188,14 +192,14 @@ class MockHistoryServiceImpl : public TopSites::MockHistoryService { class MockTopSitesDatabaseImpl : public TopSitesDatabase { public: virtual void GetPageThumbnails(MostVisitedURLList* urls, - std::map<GURL, TopSites::Images>* thumbnails) { + std::map<GURL, Images>* thumbnails) { // Return a copy of the vector. *urls = top_sites_list_; *thumbnails = thumbnails_map_; } virtual void SetPageThumbnail(const MostVisitedURL& url, int url_rank, - const TopSites::Images& thumbnail) { + const Images& thumbnail) { SetPageRank(url, url_rank); // Update thubmnail thumbnails_map_[url.url] = thumbnail; @@ -229,8 +233,8 @@ class MockTopSitesDatabaseImpl : public TopSitesDatabase { // Get a thumbnail for a given page. Returns true iff we have the thumbnail. virtual bool GetPageThumbnail(const GURL& url, - TopSites::Images* thumbnail) { - std::map<GURL, TopSites::Images>::const_iterator found = + Images* thumbnail) { + std::map<GURL, Images>::const_iterator found = thumbnails_map_.find(url); if (found == thumbnails_map_.end()) return false; // No thumbnail for this URL. @@ -255,7 +259,7 @@ class MockTopSitesDatabaseImpl : public TopSitesDatabase { private: MostVisitedURLList top_sites_list_; // Keeps the URLs sorted by score (rank). - std::map<GURL, TopSites::Images> thumbnails_map_; + std::map<GURL, Images> thumbnails_map_; }; @@ -448,7 +452,7 @@ TEST_F(TopSitesTest, MockDatabase) { url.url = asdf_url; url.title = asdf_title; url.redirects.push_back(url.url); - TopSites::Images thumbnail; + Images thumbnail; db->SetPageThumbnail(url, 0, thumbnail); top_sites().ReadDatabase(); @@ -495,7 +499,7 @@ TEST_F(TopSitesTest, MockDatabase) { top_sites().StartQueryForMostVisited(); MessageLoop::current()->RunAllPending(); - std::map<GURL, TopSites::Images> thumbnails; + std::map<GURL, Images> thumbnails; MostVisitedURLList result; db->GetPageThumbnails(&result, &thumbnails); ASSERT_EQ(4u, result.size()); @@ -520,18 +524,18 @@ TEST_F(TopSitesTest, TopSitesDB) { url.url = asdf_url; url.title = asdf_title; url.redirects.push_back(url.url); - TopSites::Images thumbnail; + Images thumbnail; thumbnail.thumbnail = random_thumbnail(); // Add asdf at rank 0. db.SetPageThumbnail(url, 0, thumbnail); - TopSites::Images result; + Images result; EXPECT_TRUE(db.GetPageThumbnail(url.url, &result)); EXPECT_EQ(thumbnail.thumbnail->data.size(), result.thumbnail->data.size()); EXPECT_TRUE(ThumbnailsAreEqual(thumbnail.thumbnail, result.thumbnail)); MostVisitedURLList urls; - std::map<GURL, TopSites::Images> thumbnails; + std::map<GURL, Images> thumbnails; db.GetPageThumbnails(&urls, &thumbnails); ASSERT_EQ(1u, urls.size()); EXPECT_EQ(asdf_url, urls[0].url); @@ -607,7 +611,7 @@ TEST_F(TopSitesTest, RealDatabase) { url.url = asdf_url; url.title = asdf_title; url.redirects.push_back(url.url); - TopSites::Images thumbnail; + Images thumbnail; thumbnail.thumbnail = random_thumbnail(); db->SetPageThumbnail(url, 0, thumbnail); @@ -623,7 +627,7 @@ TEST_F(TopSitesTest, RealDatabase) { EXPECT_EQ(welcome_url(), urls()[1].url); EXPECT_EQ(themes_url(), urls()[2].url); - TopSites::Images img_result; + Images img_result; db->GetPageThumbnail(asdf_url, &img_result); EXPECT_TRUE(img_result.thumbnail != NULL); EXPECT_TRUE(ThumbnailsAreEqual(random_thumbnail(), img_result.thumbnail)); @@ -641,7 +645,7 @@ TEST_F(TopSitesTest, RealDatabase) { url2.redirects.push_back(google3_url); // Add new thumbnail at rank 0 and shift the other result to 1. - TopSites::Images g_thumbnail; + Images g_thumbnail; g_thumbnail.thumbnail = google_thumbnail(); db->SetPageThumbnail(url2, 0, g_thumbnail); @@ -676,7 +680,7 @@ TEST_F(TopSitesTest, RealDatabase) { top_sites().StartQueryForMostVisited(); MessageLoop::current()->RunAllPending(); - std::map<GURL, TopSites::Images> thumbnails; + std::map<GURL, Images> thumbnails; MostVisitedURLList results; db->GetPageThumbnails(&results, &thumbnails); ASSERT_EQ(4u, results.size()); @@ -697,7 +701,7 @@ TEST_F(TopSitesTest, RealDatabase) { *weewar_bitmap, medium_score)); RefCountedBytes* out_1; - TopSites::Images out_2; + Images out_2; EXPECT_TRUE(top_sites().GetPageThumbnail(google1_url, &out_1)); MessageLoop::current()->RunAllPending(); @@ -1074,7 +1078,11 @@ TEST_F(TopSitesTest, Blacklisting) { &TopSitesTest::OnTopSitesAvailable)); top_sites().OnTopSitesAvailable(0, pages); MessageLoop::current()->RunAllPending(); - EXPECT_FALSE(top_sites().IsBlacklisted(GURL("http://bbc.com/"))); + { + Lock& l = lock(); + AutoLock lock(l); // IsBlacklisted must be in a lock. + EXPECT_FALSE(top_sites().IsBlacklisted(GURL("http://bbc.com/"))); + } EXPECT_EQ(1u, number_of_callbacks()); @@ -1085,9 +1093,13 @@ TEST_F(TopSitesTest, Blacklisting) { EXPECT_EQ(themes_url(), urls()[3].url); top_sites().AddBlacklistedURL(GURL("http://google.com/")); - EXPECT_TRUE(top_sites().IsBlacklisted(GURL("http://google.com/"))); - EXPECT_FALSE(top_sites().IsBlacklisted(GURL("http://bbc.com/"))); - EXPECT_FALSE(top_sites().IsBlacklisted(welcome_url())); + { + Lock& l = lock(); + AutoLock lock(l); // IsBlacklisted must be in a lock. + EXPECT_TRUE(top_sites().IsBlacklisted(GURL("http://google.com/"))); + EXPECT_FALSE(top_sites().IsBlacklisted(GURL("http://bbc.com/"))); + EXPECT_FALSE(top_sites().IsBlacklisted(welcome_url())); + } top_sites().GetMostVisitedURLs( &c, @@ -1110,7 +1122,11 @@ TEST_F(TopSitesTest, Blacklisting) { EXPECT_EQ(themes_url(), urls()[1].url); top_sites().RemoveBlacklistedURL(GURL("http://google.com/")); - EXPECT_FALSE(top_sites().IsBlacklisted(GURL("http://google.com/"))); + { + Lock& l = lock(); + AutoLock lock(l); // IsBlacklisted must be in a lock. + EXPECT_FALSE(top_sites().IsBlacklisted(GURL("http://google.com/"))); + } top_sites().GetMostVisitedURLs( &c, @@ -1151,7 +1167,6 @@ TEST_F(TopSitesTest, PinnedURLs) { &TopSitesTest::OnTopSitesAvailable)); top_sites().OnTopSitesAvailable(0, pages); MessageLoop::current()->RunAllPending(); - size_t index = 0; EXPECT_FALSE(top_sites().IsURLPinned(GURL("http://bbc.com/"))); ASSERT_EQ(4u, urls().size()); @@ -1161,9 +1176,6 @@ TEST_F(TopSitesTest, PinnedURLs) { EXPECT_EQ(themes_url(), urls()[3].url); top_sites().AddPinnedURL(GURL("http://google.com/"), 3); - EXPECT_TRUE(top_sites().GetIndexOfPinnedURL(GURL("http://google.com/"), - &index)); - EXPECT_EQ(3u, index); EXPECT_FALSE(top_sites().IsURLPinned(GURL("http://bbc.com/"))); EXPECT_FALSE(top_sites().IsURLPinned(welcome_url())); |