diff options
22 files changed, 646 insertions, 81 deletions
diff --git a/chrome/browser/android/most_visited_sites.cc b/chrome/browser/android/most_visited_sites.cc index 10078f8..682d800 100644 --- a/chrome/browser/android/most_visited_sites.cc +++ b/chrome/browser/android/most_visited_sites.cc @@ -159,7 +159,7 @@ void GetMostVisitedURLs( new NativeCallback(j_callback_obj, static_cast<int>(num_results)); top_sites->GetMostVisitedURLs( base::Bind(&NativeCallback::OnMostVisitedURLsAvailable, - native_callback)); + native_callback), false); } // May be called from any thread diff --git a/chrome/browser/autocomplete/zero_suggest_provider.cc b/chrome/browser/autocomplete/zero_suggest_provider.cc index ddcfbf2..07d7c2c 100644 --- a/chrome/browser/autocomplete/zero_suggest_provider.cc +++ b/chrome/browser/autocomplete/zero_suggest_provider.cc @@ -368,7 +368,7 @@ void ZeroSuggestProvider::Run(const GURL& suggest_url) { if (ts) { ts->GetMostVisitedURLs( base::Bind(&ZeroSuggestProvider::OnMostVisitedUrlsAvailable, - weak_ptr_factory_.GetWeakPtr())); + weak_ptr_factory_.GetWeakPtr()), false); } } have_pending_request_ = true; diff --git a/chrome/browser/automation/automation_provider_observers.cc b/chrome/browser/automation/automation_provider_observers.cc index b692a77..0da34a3 100644 --- a/chrome/browser/automation/automation_provider_observers.cc +++ b/chrome/browser/automation/automation_provider_observers.cc @@ -1891,7 +1891,7 @@ void NTPInfoObserver::Observe(int type, if (request_ == *request_details.ptr()) { top_sites_->GetMostVisitedURLs( base::Bind(&NTPInfoObserver::OnTopSitesReceived, - base::Unretained(this))); + base::Unretained(this)), false); } } } diff --git a/chrome/browser/extensions/api/top_sites/top_sites_api.cc b/chrome/browser/extensions/api/top_sites/top_sites_api.cc index c6accaf..cead49a 100644 --- a/chrome/browser/extensions/api/top_sites/top_sites_api.cc +++ b/chrome/browser/extensions/api/top_sites/top_sites_api.cc @@ -24,7 +24,7 @@ bool TopSitesGetFunction::RunImpl() { ts->GetMostVisitedURLs( base::Bind(&TopSitesGetFunction::OnMostVisitedURLsAvailable, - weak_ptr_factory_.GetWeakPtr())); + weak_ptr_factory_.GetWeakPtr()), false); return true; } diff --git a/chrome/browser/extensions/api/top_sites/top_sites_apitest.cc b/chrome/browser/extensions/api/top_sites/top_sites_apitest.cc index 7d8086c..6fbf2ed 100644 --- a/chrome/browser/extensions/api/top_sites/top_sites_apitest.cc +++ b/chrome/browser/extensions/api/top_sites/top_sites_apitest.cc @@ -29,7 +29,7 @@ class TopSitesExtensionTest : public InProcessBrowserTest { // before we get to the conditional below. Otherwise, we'll run a nested // message loop until the async callback. top_sites->GetMostVisitedURLs( - base::Bind(&TopSitesExtensionTest::OnTopSitesAvailable, this)); + base::Bind(&TopSitesExtensionTest::OnTopSitesAvailable, this), false); if (!top_sites_inited_) { waiting_ = true; diff --git a/chrome/browser/history/top_sites.h b/chrome/browser/history/top_sites.h index 92e8eee..f373315 100644 --- a/chrome/browser/history/top_sites.h +++ b/chrome/browser/history/top_sites.h @@ -61,15 +61,15 @@ class TopSites typedef base::Callback<void(const MostVisitedURLList&)> GetMostVisitedURLsCallback; - typedef std::vector<GetMostVisitedURLsCallback> PendingCallbacks; - // Returns a list of most visited URLs via a callback. This may be invoked on - // any thread. - // NOTE: the callback is called immediately if we have the data cached. If - // data is not available yet, callback will later be posted to the thread - // called this function. + // Returns a list of most visited URLs via a callback, if + // |include_forced_urls| is false includes only non-forced URLs. This may be + // invoked on any thread. NOTE: the callback is called immediately if we have + // the data cached. If data is not available yet, callback will later be + // posted to the thread called this function. virtual void GetMostVisitedURLs( - const GetMostVisitedURLsCallback& callback) = 0; + const GetMostVisitedURLsCallback& callback, + bool include_forced_urls) = 0; // Gets a thumbnail for a given page. Returns true iff we have the thumbnail. // This may be invoked on any thread. @@ -134,10 +134,15 @@ class TopSites // return it without modification. virtual const std::string& GetCanonicalURLString(const GURL& url) const = 0; - // Returns true if the top sites list is full (i.e. we already have the - // maximum number of top sites). This function also returns false if - // TopSites isn't loaded yet. - virtual bool IsFull() = 0; + // Returns true if the top sites list of non-forced URLs is full (i.e. we + // already have the maximum number of non-forced top sites). This function + // also returns false if TopSites isn't loaded yet. + virtual bool IsNonForcedFull() = 0; + + // Returns true if the top sites list of forced URLs is full (i.e. we already + // have the maximum number of forced top sites). This function also returns + // false if TopSites isn't loaded yet. + virtual bool IsForcedFull() = 0; virtual bool loaded() const = 0; diff --git a/chrome/browser/history/top_sites_cache.cc b/chrome/browser/history/top_sites_cache.cc index 5914d99..4448c89 100644 --- a/chrome/browser/history/top_sites_cache.cc +++ b/chrome/browser/history/top_sites_cache.cc @@ -31,6 +31,7 @@ TopSitesCache::~TopSitesCache() { void TopSitesCache::SetTopSites(const MostVisitedURLList& top_sites) { top_sites_ = top_sites; + CountForcedURLs(); GenerateCanonicalURLs(); } @@ -119,6 +120,31 @@ size_t TopSitesCache::GetURLIndex(const GURL& url) const { return GetCanonicalURLsIterator(url)->second; } +size_t TopSitesCache::GetNumNonForcedURLs() const { + return top_sites_.size() - num_forced_urls_; +} + +size_t TopSitesCache::GetNumForcedURLs() const { + return num_forced_urls_; +} + +void TopSitesCache::CountForcedURLs() { + num_forced_urls_ = 0; + while (num_forced_urls_ < top_sites_.size()) { + // Forced sites are all at the beginning. + if (top_sites_[num_forced_urls_].last_forced_time.is_null()) + break; + num_forced_urls_++; + } + // In debug, ensure the cache user has no forced URLs pass that point. + if (DCHECK_IS_ON()) { + for (size_t i = num_forced_urls_; i < top_sites_.size(); ++i) { + DCHECK(top_sites_[i].last_forced_time.is_null()) + << "All the forced URLs must appear before non-forced URLs."; + } + } +} + void TopSitesCache::GenerateCanonicalURLs() { canonical_urls_.clear(); for (size_t i = 0; i < top_sites_.size(); i++) diff --git a/chrome/browser/history/top_sites_cache.h b/chrome/browser/history/top_sites_cache.h index f66d0f0..1269d3b 100644 --- a/chrome/browser/history/top_sites_cache.h +++ b/chrome/browser/history/top_sites_cache.h @@ -42,7 +42,8 @@ class TopSitesCache { TopSitesCache(); ~TopSitesCache(); - // The top sites. + // Set the top sites. In |top_sites| all forced URLs must appear before + // non-forced URLs. This is only checked in debug. void SetTopSites(const MostVisitedURLList& top_sites); const MostVisitedURLList& top_sites() const { return top_sites_; } @@ -79,6 +80,12 @@ class TopSitesCache { // Returns the index into |top_sites_| for |url|. size_t GetURLIndex(const GURL& url) const; + // Returns the number of non-forced URLs in the cache. + size_t GetNumNonForcedURLs() const; + + // Returns the number of forced URLs in the cache. + size_t GetNumForcedURLs() const; + private: // The entries in CanonicalURLs, see CanonicalURLs for details. The second // argument gives the index of the URL into MostVisitedURLs redirects. @@ -117,6 +124,9 @@ class TopSitesCache { typedef std::map<CanonicalURLEntry, size_t, CanonicalURLComparator> CanonicalURLs; + // Count the number of forced URLs. + void CountForcedURLs(); + // Generates the set of canonical urls from |top_sites_|. void GenerateCanonicalURLs(); @@ -129,7 +139,12 @@ class TopSitesCache { // Returns the GURL corresponding to an iterator in |canonical_urls_|. const GURL& GetURLFromIterator(CanonicalURLs::const_iterator it) const; - // The top sites. + // The number of top sites with forced URLs. + size_t num_forced_urls_; + + // The top sites. This list must always contain the forced URLs first followed + // by the non-forced URLs. This is not strictly enforced but is checked in + // debug. MostVisitedURLList top_sites_; // The images. These map from canonical url to image. diff --git a/chrome/browser/history/top_sites_cache_unittest.cc b/chrome/browser/history/top_sites_cache_unittest.cc index a4a8690..947e7f6 100644 --- a/chrome/browser/history/top_sites_cache_unittest.cc +++ b/chrome/browser/history/top_sites_cache_unittest.cc @@ -23,10 +23,14 @@ class TopSitesCacheTest : public testing::Test { } protected: - // Initializes |top_sites_| and |cache_| based on |spec|, which is a list of - // URL strings with optional indents: indentated URLs redirect to the last - // non-indented URL. Titles are assigned as "Title 1", "Title 2", etc., in the - // order of appearance. See |kTopSitesSpecBasic| for an example. + // Initializes |top_sites_| on |spec|, which is a list of URL strings with + // optional indents: indentated URLs redirect to the last non-indented URL. + // Titles are assigned as "Title 1", "Title 2", etc., in the order of + // appearance. See |kTopSitesSpecBasic| for an example. This function does not + // update |cache_| so you can manipulate |top_sites_| before you update it. + void BuildTopSites(const char** spec, size_t size); + + // Initializes |top_sites_| and |cache_| based on |spec|. void InitTopSiteCache(const char** spec, size_t size); MostVisitedURLList top_sites_; @@ -36,7 +40,7 @@ class TopSitesCacheTest : public testing::Test { DISALLOW_COPY_AND_ASSIGN(TopSitesCacheTest); }; -void TopSitesCacheTest::InitTopSiteCache(const char** spec, size_t size) { +void TopSitesCacheTest::BuildTopSites(const char** spec, size_t size) { std::set<std::string> urls_seen; for (size_t i = 0; i < size; ++i) { const char* spec_item = spec[i]; @@ -54,6 +58,10 @@ void TopSitesCacheTest::InitTopSiteCache(const char** spec, size_t size) { // Set up redirect to canonical URL. Canonical URL redirects to itself, too. top_sites_.back().redirects.push_back(GURL(spec_item)); } +} + +void TopSitesCacheTest::InitTopSiteCache(const char** spec, size_t size) { + BuildTopSites(spec, size); cache_.SetTopSites(top_sites_); } @@ -233,6 +241,18 @@ TEST_F(TopSitesCacheTest, GetPrefixCanonicalURLDiffByQuery) { } } +// This test ensures forced URLs behave in the expected way. +TEST_F(TopSitesCacheTest, CacheForcedURLs) { + // Forced URLs must always appear at the beginning of the list. + BuildTopSites(kTopSitesSpecBasic, arraysize(kTopSitesSpecBasic)); + top_sites_[0].last_forced_time = base::Time::FromJsTime(1000); + top_sites_[1].last_forced_time = base::Time::FromJsTime(2000); + cache_.SetTopSites(top_sites_); + + EXPECT_EQ(2u, cache_.GetNumForcedURLs()); + EXPECT_EQ(2u, cache_.GetNumNonForcedURLs()); +} + } // namespace } // namespace history diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc index db9156d..e86c07f 100644 --- a/chrome/browser/history/top_sites_database.cc +++ b/chrome/browser/history/top_sites_database.cc @@ -312,6 +312,9 @@ void TopSitesDatabase::UpdatePageRank(const MostVisitedURL& url, void TopSitesDatabase::UpdatePageRankNoTransaction( const MostVisitedURL& url, int new_rank) { DCHECK_GT(db_->transaction_nesting(), 0); + DCHECK((url.last_forced_time.is_null()) == (new_rank != kRankOfForcedURL)) + << "Thumbnail without a forced time stamp has a forced rank, or the " + << "opposite."; int prev_rank = GetURLRank(url); if (prev_rank == kRankOfNonExistingURL) { diff --git a/chrome/browser/history/top_sites_database_unittest.cc b/chrome/browser/history/top_sites_database_unittest.cc index 4004065..062c6ae 100644 --- a/chrome/browser/history/top_sites_database_unittest.cc +++ b/chrome/browser/history/top_sites_database_unittest.cc @@ -219,6 +219,7 @@ TEST_F(TopSitesDatabaseTest, AddRemoveEditThumbnails) { EXPECT_EQ(kUrl, urls[3].url); // Change a forced URL to non-forced using SetPageThumbnail. + url3.last_forced_time = base::Time(); db.SetPageThumbnail(url3, 1, Images()); db.GetPageThumbnails(&urls, &thumbnails); @@ -230,7 +231,6 @@ TEST_F(TopSitesDatabaseTest, AddRemoveEditThumbnails) { EXPECT_EQ(plusUrl, urls[3].url); // Plus moves to second non-forced URL. // Change a non-forced URL to earlier non-forced using UpdatePageRank. - url3.last_forced_time = base::Time(); db.UpdatePageRank(url3, 0); db.GetPageThumbnails(&urls, &thumbnails); diff --git a/chrome/browser/history/top_sites_impl.cc b/chrome/browser/history/top_sites_impl.cc index 18f2e21..9eb3a006 100644 --- a/chrome/browser/history/top_sites_impl.cc +++ b/chrome/browser/history/top_sites_impl.cc @@ -55,18 +55,33 @@ namespace { void RunOrPostGetMostVisitedURLsCallback( base::TaskRunner* task_runner, + bool include_forced_urls, const TopSitesImpl::GetMostVisitedURLsCallback& callback, - const MostVisitedURLList& urls) { + const MostVisitedURLList& all_urls, + const MostVisitedURLList& nonforced_urls) { + const MostVisitedURLList* urls = + include_forced_urls ? &all_urls : &nonforced_urls; if (task_runner->RunsTasksOnCurrentThread()) - callback.Run(urls); + callback.Run(*urls); else - task_runner->PostTask(FROM_HERE, base::Bind(callback, urls)); + task_runner->PostTask(FROM_HERE, base::Bind(callback, *urls)); +} + +// Compares two MostVisitedURL having a non-null |last_forced_time|. +bool ForcedURLComparator(const MostVisitedURL& first, + const MostVisitedURL& second) { + DCHECK(!first.last_forced_time.is_null() && + !second.last_forced_time.is_null()); + return first.last_forced_time < second.last_forced_time; } } // namespace -// How many top sites to store in the cache. -static const size_t kTopSitesNumber = 20; +// How many non-forced top sites to store in the cache. +static const size_t kNonForcedTopSitesNumber = 20; + +// How many forced top sites to store in the cache. +static const size_t kForcedTopSitesNumber = 20; // Max number of temporary images we'll cache. See comment above // temp_images_ for details. @@ -132,7 +147,7 @@ bool TopSitesImpl::SetPageThumbnail(const GURL& url, bool add_temp_thumbnail = false; if (!IsKnownURL(url)) { - if (!IsFull()) { + if (!IsNonForcedFull()) { add_temp_thumbnail = true; } else { return false; // This URL is not known to us. @@ -171,7 +186,7 @@ bool TopSitesImpl::SetPageThumbnailToJPEGBytes( bool add_temp_thumbnail = false; if (!IsKnownURL(url)) { - if (!IsFull()) { + if (!IsNonForcedFull()) { add_temp_thumbnail = true; } else { return false; // This URL is not known to us. @@ -194,7 +209,8 @@ bool TopSitesImpl::SetPageThumbnailToJPEGBytes( // WARNING: this function may be invoked on any thread. void TopSitesImpl::GetMostVisitedURLs( - const GetMostVisitedURLsCallback& callback) { + const GetMostVisitedURLsCallback& callback, + bool include_forced_urls) { MostVisitedURLList filtered_urls; { base::AutoLock lock(lock_); @@ -204,10 +220,17 @@ void TopSitesImpl::GetMostVisitedURLs( pending_callbacks_.push_back( base::Bind(&RunOrPostGetMostVisitedURLsCallback, base::MessageLoopProxy::current(), + include_forced_urls, callback)); return; } - filtered_urls = thread_safe_cache_->top_sites(); + if (include_forced_urls) { + filtered_urls = thread_safe_cache_->top_sites(); + } else { + filtered_urls.assign(thread_safe_cache_->top_sites().begin() + + thread_safe_cache_->GetNumForcedURLs(), + thread_safe_cache_->top_sites().end()); + } } callback.Run(filtered_urls); } @@ -366,29 +389,49 @@ void TopSitesImpl::Shutdown() { void TopSitesImpl::DiffMostVisited(const MostVisitedURLList& old_list, const MostVisitedURLList& new_list, TopSitesDelta* delta) { + // Add all the old URLs for quick lookup. This maps URLs to the corresponding // index in the input. std::map<GURL, size_t> all_old_urls; - for (size_t i = 0; i < old_list.size(); i++) + size_t num_old_forced = 0; + for (size_t i = 0; i < old_list.size(); i++) { + if (!old_list[i].last_forced_time.is_null()) + num_old_forced++; + DCHECK(old_list[i].last_forced_time.is_null() || i < num_old_forced) + << "Forced URLs must all appear before non-forced URLs."; all_old_urls[old_list[i].url] = i; + } // Check all the URLs in the new set to see which ones are new or just moved. // When we find a match in the old set, we'll reset its index to our special // marker. This allows us to quickly identify the deleted ones in a later // pass. const size_t kAlreadyFoundMarker = static_cast<size_t>(-1); + int rank = -1; // Forced URLs have a rank of -1. for (size_t i = 0; i < new_list.size(); i++) { + // Increase the rank if we're going through forced URLs. This works because + // non-forced URLs all come after forced URLs. + if (new_list[i].last_forced_time.is_null()) + rank++; + DCHECK(new_list[i].last_forced_time.is_null() == (rank != -1)) + << "Forced URLs must all appear before non-forced URLs."; std::map<GURL, size_t>::iterator found = all_old_urls.find(new_list[i].url); if (found == all_old_urls.end()) { MostVisitedURLWithRank added; added.url = new_list[i]; - added.rank = i; + added.rank = rank; delta->added.push_back(added); } else { - if (found->second != i) { + DCHECK(found->second != kAlreadyFoundMarker) + << "Same URL appears twice in the new list."; + int old_rank = found->second >= num_old_forced ? + found->second - num_old_forced : -1; + if (old_rank != rank || + old_list[found->second].last_forced_time != + new_list[i].last_forced_time) { MostVisitedURLWithRank moved; moved.url = new_list[i]; - moved.rank = i; + moved.rank = rank; delta->moved.push_back(moved); } found->second = kAlreadyFoundMarker; @@ -431,8 +474,12 @@ const std::string& TopSitesImpl::GetCanonicalURLString(const GURL& url) const { return cache_->GetCanonicalURL(url).spec(); } -bool TopSitesImpl::IsFull() { - return loaded_ && cache_->top_sites().size() >= kTopSitesNumber; +bool TopSitesImpl::IsNonForcedFull() { + return loaded_ && cache_->GetNumNonForcedURLs() >= kNonForcedTopSitesNumber; +} + +bool TopSitesImpl::IsForcedFull() { + return loaded_ && cache_->GetNumForcedURLs() >= kForcedTopSitesNumber; } TopSitesImpl::~TopSitesImpl() { @@ -480,9 +527,10 @@ bool TopSitesImpl::SetPageThumbnailEncoded( return false; size_t index = cache_->GetURLIndex(url); + int url_rank = index - cache_->GetNumForcedURLs(); const MostVisitedURL& most_visited = cache_->top_sites()[index]; backend_->SetPageThumbnail(most_visited, - index, + url_rank < 0 ? -1 : url_rank, *(cache_->GetImage(most_visited.url))); return true; } @@ -559,11 +607,12 @@ bool TopSitesImpl::loaded() const { return loaded_; } -bool TopSitesImpl::AddPrepopulatedPages(MostVisitedURLList* urls) { +bool TopSitesImpl::AddPrepopulatedPages(MostVisitedURLList* urls, + size_t num_forced_urls) { bool added = false; MostVisitedURLList prepopulate_urls = GetPrepopulatePages(); for (size_t i = 0; i < prepopulate_urls.size(); ++i) { - if (urls->size() < kTopSitesNumber && + if (urls->size() - num_forced_urls < kNonForcedTopSitesNumber && IndexOf(*urls, prepopulate_urls[i].url) == -1) { urls->push_back(prepopulate_urls[i]); added = true; @@ -572,6 +621,44 @@ bool TopSitesImpl::AddPrepopulatedPages(MostVisitedURLList* urls) { return added; } +size_t TopSitesImpl::MergeCachedForcedURLs(MostVisitedURLList* new_list) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + // Add all the new URLs for quick lookup. Take that opportunity to count the + // number of forced URLs in |new_list|. + std::set<GURL> all_new_urls; + size_t num_forced = 0; + for (size_t i = 0; i < new_list->size(); ++i) { + all_new_urls.insert((*new_list)[i].url); + if (!(*new_list)[i].last_forced_time.is_null()) + ++num_forced; + } + + // Keep the forced URLs from |cache_| that are not found in |new_list|. + MostVisitedURLList filtered_forced_urls; + for (size_t i = 0; i < cache_->GetNumForcedURLs(); ++i) { + if (all_new_urls.find(cache_->top_sites()[i].url) == all_new_urls.end()) + filtered_forced_urls.push_back(cache_->top_sites()[i]); + } + num_forced += filtered_forced_urls.size(); + + // Prepend forced URLs and sort in order of ascending |last_forced_time|. + new_list->insert(new_list->begin(), filtered_forced_urls.begin(), + filtered_forced_urls.end()); + std::inplace_merge( + new_list->begin(), new_list->begin() + filtered_forced_urls.size(), + new_list->begin() + num_forced, ForcedURLComparator); + + // Drop older forced URLs if the list overflows. Since forced URLs are always + // sort in increasing order of |last_forced_time|, drop the first ones. + if (num_forced > kForcedTopSitesNumber) { + new_list->erase(new_list->begin(), + new_list->begin() + (num_forced - kForcedTopSitesNumber)); + num_forced = kForcedTopSitesNumber; + } + + return num_forced; +} + void TopSitesImpl::ApplyBlacklist(const MostVisitedURLList& urls, MostVisitedURLList* out) { // Log the number of times ApplyBlacklist is called so we can compute the @@ -581,9 +668,23 @@ void TopSitesImpl::ApplyBlacklist(const MostVisitedURLList& urls, UMA_HISTOGRAM_BOOLEAN("TopSites.NumberOfApplyBlacklist", true); UMA_HISTOGRAM_COUNTS_100("TopSites.NumberOfBlacklistedItems", (blacklist ? blacklist->size() : 0)); - for (size_t i = 0; i < urls.size() && i < kTopSitesNumber; ++i) { - if (!IsBlacklisted(urls[i].url)) + size_t num_non_forced_urls = 0; + size_t num_forced_urls = 0; + for (size_t i = 0; i < urls.size(); ++i) { + if (!IsBlacklisted(urls[i].url)) { + if (urls[i].last_forced_time.is_null()) { + // Non-forced URL. + if (num_non_forced_urls >= kNonForcedTopSitesNumber) + continue; + num_non_forced_urls++; + } else { + // Forced URL. + if (num_forced_urls >= kForcedTopSitesNumber) + continue; + num_forced_urls++; + } out->push_back(urls[i]); + } } } @@ -638,7 +739,7 @@ void TopSitesImpl::Observe(int type, content::Source<NavigationController>(source).ptr(); Profile* profile = Profile::FromBrowserContext( controller->GetWebContents()->GetBrowserContext()); - if (profile == profile_ && !IsFull()) { + if (profile == profile_ && !IsNonForcedFull()) { content::LoadCommittedDetails* load_details = content::Details<content::LoadCommittedDetails>(details).ptr(); if (!load_details) @@ -657,7 +758,8 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); MostVisitedURLList top_sites(new_top_sites); - AddPrepopulatedPages(&top_sites); + size_t num_forced_urls = MergeCachedForcedURLs(&top_sites); + AddPrepopulatedPages(&top_sites, num_forced_urls); TopSitesDelta delta; DiffMostVisited(cache_->top_sites(), top_sites, &delta); @@ -691,7 +793,7 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites) { } } - if (top_sites.size() >= kTopSitesNumber) + if (top_sites.size() - num_forced_urls >= kNonForcedTopSitesNumber) temp_images_.clear(); ResetThreadSafeCache(); @@ -708,13 +810,14 @@ int TopSitesImpl::num_results_to_request_from_history() const { const DictionaryValue* blacklist = profile_->GetPrefs()->GetDictionary(prefs::kNtpMostVisitedURLsBlacklist); - return kTopSitesNumber + (blacklist ? blacklist->size() : 0); + return kNonForcedTopSitesNumber + (blacklist ? blacklist->size() : 0); } void TopSitesImpl::MoveStateToLoaded() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - MostVisitedURLList filtered_urls; + MostVisitedURLList filtered_urls_all; + MostVisitedURLList filtered_urls_nonforced; PendingCallbacks pending_callbacks; { base::AutoLock lock(lock_); @@ -726,13 +829,18 @@ void TopSitesImpl::MoveStateToLoaded() { // Now that we're loaded we can service the queued up callbacks. Copy them // here and service them outside the lock. if (!pending_callbacks_.empty()) { - filtered_urls = thread_safe_cache_->top_sites(); + // We always filter out forced URLs because callers of GetMostVisitedURLs + // are not interested in them. + filtered_urls_all = thread_safe_cache_->top_sites(); + filtered_urls_nonforced.assign(thread_safe_cache_->top_sites().begin() + + thread_safe_cache_->GetNumForcedURLs(), + thread_safe_cache_->top_sites().end()); pending_callbacks.swap(pending_callbacks_); } } for (size_t i = 0; i < pending_callbacks.size(); i++) - pending_callbacks[i].Run(filtered_urls); + pending_callbacks[i].Run(filtered_urls_all, filtered_urls_nonforced); content::NotificationService::current()->Notify( chrome::NOTIFICATION_TOP_SITES_LOADED, diff --git a/chrome/browser/history/top_sites_impl.h b/chrome/browser/history/top_sites_impl.h index 8355f52..7d94270 100644 --- a/chrome/browser/history/top_sites_impl.h +++ b/chrome/browser/history/top_sites_impl.h @@ -55,14 +55,15 @@ class TopSitesImpl : public TopSites { void Init(const base::FilePath& db_name); virtual bool SetPageThumbnail(const GURL& url, - const gfx::Image& thumbnail, - const ThumbnailScore& score) OVERRIDE; + const gfx::Image& thumbnail, + const ThumbnailScore& score) OVERRIDE; virtual bool SetPageThumbnailToJPEGBytes( const GURL& url, const base::RefCountedMemory* memory, const ThumbnailScore& score) OVERRIDE; virtual void GetMostVisitedURLs( - const GetMostVisitedURLsCallback& callback) OVERRIDE; + const GetMostVisitedURLsCallback& callback, + bool include_forced_urls) OVERRIDE; virtual bool GetPageThumbnail( const GURL& url, bool prefix_match, @@ -82,7 +83,8 @@ class TopSitesImpl : public TopSites { virtual bool IsKnownURL(const GURL& url) OVERRIDE; virtual const std::string& GetCanonicalURLString( const GURL& url) const OVERRIDE; - virtual bool IsFull() OVERRIDE; + virtual bool IsNonForcedFull() OVERRIDE; + virtual bool IsForcedFull() OVERRIDE; virtual MostVisitedURLList GetPrepopulatePages() OVERRIDE; virtual bool loaded() const OVERRIDE; @@ -92,15 +94,22 @@ class TopSitesImpl : public TopSites { private: friend class TopSitesImplTest; FRIEND_TEST_ALL_PREFIXES(TopSitesImplTest, DiffMostVisited); + FRIEND_TEST_ALL_PREFIXES(TopSitesImplTest, DiffMostVisitedWithForced); + + typedef base::Callback<void(const MostVisitedURLList&, + const MostVisitedURLList&)> PendingCallback; typedef std::pair<GURL, Images> TempImage; typedef std::list<TempImage> TempImages; + typedef std::vector<PendingCallback> PendingCallbacks; // Generates the diff of things that happened between "old" and "new." // + // This treats forced URLs separately than non-forced URLs. + // // The URLs that are in "new" but not "old" will be have their index into - // "new" put in |added_urls|. The URLs that are in "old" but not "new" will - // have their index into "old" put into |deleted_urls|. + // "new" put in |added_urls|. The non-forced URLs that are in "old" but not + // "new" will have their index into "old" put into |deleted_urls|. // // URLs appearing in both old and new lists but having different indices will // have their index into "new" be put into |moved_urls|. @@ -146,9 +155,20 @@ class TopSitesImpl : public TopSites { // Add prepopulated pages: 'welcome to Chrome' and themes gallery to |urls|. // Returns true if any pages were added. - bool AddPrepopulatedPages(MostVisitedURLList* urls); + bool AddPrepopulatedPages(MostVisitedURLList* urls, + size_t num_forced_urls); + + // Add all the forced URLs from |cache_| into |new_list|, making sure not to + // add any URL that's already in |new_list|'s non-forced URLs. The forced URLs + // in |cache_| and |new_list| are assumed to appear at the front of the list + // and be sorted in increasing |last_forced_time|. This will still be true + // after the call. If the list of forced URLs overflows the older ones are + // dropped. Returns the number of forced URLs after the merge. + size_t MergeCachedForcedURLs(MostVisitedURLList* new_list); // Takes |urls|, produces it's copy in |out| after removing blacklisted URLs. + // Also ensures we respect the maximum number of forced URLs and non-forced + // URLs. void ApplyBlacklist(const MostVisitedURLList& urls, MostVisitedURLList* out); // Returns an MD5 hash of the URL. Hashing is required for blacklisted URLs. @@ -163,11 +183,15 @@ class TopSitesImpl : public TopSites { const content::NotificationSource& source, const content::NotificationDetails& details) OVERRIDE; - // Resets top_sites_ and updates the db (in the background). All mutations to - // top_sites_ *must* go through this. Should be called from the UI thread. + // Updates URLs in |cache_| and the db (in the background). + // The non-forced URLs in |new_top_sites| replace those in |cache_|. + // The forced URLs of |new_top_sites| are merged with those in |cache_|, + // if the list of forced URLs overflows, the oldest ones are dropped. + // All mutations to cache_ *must* go through this. Should + // be called from the UI thread. void SetTopSites(const MostVisitedURLList& new_top_sites); - // Returns the number of most visted results to request from history. This + // Returns the number of most visited results to request from history. This // changes depending upon how many urls have been blacklisted. Should be // called from the UI thread. int num_results_to_request_from_history() const; @@ -234,7 +258,7 @@ class TopSitesImpl : public TopSites { // Stores thumbnails for unknown pages. When SetPageThumbnail is // called, if we don't know about that URL yet and we don't have // enough Top Sites (new profile), we store it until the next - // SetTopSites call. + // SetNonForcedTopSites call. TempImages temp_images_; // URL List of prepopulated page. diff --git a/chrome/browser/history/top_sites_impl_unittest.cc b/chrome/browser/history/top_sites_impl_unittest.cc index 0062968..a3dff47 100644 --- a/chrome/browser/history/top_sites_impl_unittest.cc +++ b/chrome/browser/history/top_sites_impl_unittest.cc @@ -62,10 +62,19 @@ class TopSitesQuerier { // Queries top sites. If |wait| is true a nested message loop is run until the // callback is notified. void QueryTopSites(TopSitesImpl* top_sites, bool wait) { + QueryAllTopSites(top_sites, wait, false); + } + + // Queries top sites, including potentially forced URLs if + // |include_forced_urls| is true. + void QueryAllTopSites(TopSitesImpl* top_sites, + bool wait, + bool include_forced_urls) { int start_number_of_callbacks = number_of_callbacks_; top_sites->GetMostVisitedURLs( base::Bind(&TopSitesQuerier::OnTopSitesAvailable, - weak_ptr_factory_.GetWeakPtr())); + weak_ptr_factory_.GetWeakPtr()), + include_forced_urls); if (wait && start_number_of_callbacks == number_of_callbacks_) { waiting_ = true; base::MessageLoop::current()->Run(); @@ -304,7 +313,13 @@ class TopSitesImplTest : public HistoryUnitTestBase { bool IsTopSitesLoaded() { return top_sites()->loaded_; } bool AddPrepopulatedPages(MostVisitedURLList* urls) { - return top_sites()->AddPrepopulatedPages(urls); + return top_sites()->AddPrepopulatedPages(urls, 0u); + } + + void EmptyThreadSafeCache() { + base::AutoLock lock(top_sites()->lock_); + MostVisitedURLList empty; + top_sites()->thread_safe_cache_->SetTopSites(empty); } private: @@ -332,6 +347,18 @@ static void AppendMostVisitedURL(std::vector<MostVisitedURL>* list, list->push_back(mv); } +// Helper function for appending a URL to a vector of "most visited" URLs, +// using the default values for everything but the URL. +static void AppendForcedMostVisitedURL(std::vector<MostVisitedURL>* list, + const GURL& url, + double last_forced_time) { + MostVisitedURL mv; + mv.url = url; + mv.last_forced_time = base::Time::FromJsTime(last_forced_time); + mv.redirects.push_back(url); + list->push_back(mv); +} + // Same as AppendMostVisitedURL except that it adds a redirect from the first // URL to the second. static void AppendMostVisitedURLWithRedirect( @@ -380,7 +407,7 @@ TEST_F(TopSitesImplTest, DiffMostVisited) { GURL stays_the_same("http://staysthesame/"); GURL gets_added_1("http://getsadded1/"); GURL gets_added_2("http://getsadded2/"); - GURL gets_deleted_1("http://getsdeleted2/"); + GURL gets_deleted_1("http://getsdeleted1/"); GURL gets_moved_1("http://getsmoved1/"); std::vector<MostVisitedURL> old_list; @@ -398,17 +425,80 @@ TEST_F(TopSitesImplTest, DiffMostVisited) { history::TopSitesImpl::DiffMostVisited(old_list, new_list, &delta); ASSERT_EQ(2u, delta.added.size()); - ASSERT_TRUE(gets_added_1 == delta.added[0].url.url); - ASSERT_EQ(1, delta.added[0].rank); - ASSERT_TRUE(gets_added_2 == delta.added[1].url.url); - ASSERT_EQ(2, delta.added[1].rank); + EXPECT_TRUE(gets_added_1 == delta.added[0].url.url); + EXPECT_EQ(1, delta.added[0].rank); + EXPECT_TRUE(gets_added_2 == delta.added[1].url.url); + EXPECT_EQ(2, delta.added[1].rank); ASSERT_EQ(1u, delta.deleted.size()); - ASSERT_TRUE(gets_deleted_1 == delta.deleted[0].url); + EXPECT_TRUE(gets_deleted_1 == delta.deleted[0].url); ASSERT_EQ(1u, delta.moved.size()); - ASSERT_TRUE(gets_moved_1 == delta.moved[0].url.url); - ASSERT_EQ(3, delta.moved[0].rank); + EXPECT_TRUE(gets_moved_1 == delta.moved[0].url.url); + EXPECT_EQ(3, delta.moved[0].rank); +} + +// Tests DiffMostVisited with forced URLs. +TEST_F(TopSitesImplTest, DiffMostVisitedWithForced) { + // Forced URLs. + GURL stays_the_same_1("http://staysthesame1/"); + GURL new_last_forced_time("http://newlastforcedtime/"); + GURL stays_the_same_2("http://staysthesame2/"); + GURL move_to_nonforced("http://movetononforced/"); + GURL gets_added_1("http://getsadded1/"); + GURL gets_deleted_1("http://getsdeleted1/"); + // Non-forced URLs. + GURL move_to_forced("http://movetoforced/"); + GURL stays_the_same_3("http://staysthesame3/"); + GURL gets_added_2("http://getsadded2/"); + GURL gets_deleted_2("http://getsdeleted2/"); + GURL gets_moved_1("http://getsmoved1/"); + + std::vector<MostVisitedURL> old_list; + AppendForcedMostVisitedURL(&old_list, stays_the_same_1, 1000); + AppendForcedMostVisitedURL(&old_list, new_last_forced_time, 2000); + AppendForcedMostVisitedURL(&old_list, stays_the_same_2, 3000); + AppendForcedMostVisitedURL(&old_list, move_to_nonforced, 4000); + AppendForcedMostVisitedURL(&old_list, gets_deleted_1, 5000); + AppendMostVisitedURL(&old_list, move_to_forced); + AppendMostVisitedURL(&old_list, stays_the_same_3); + AppendMostVisitedURL(&old_list, gets_deleted_2); + AppendMostVisitedURL(&old_list, gets_moved_1); + + std::vector<MostVisitedURL> new_list; + AppendForcedMostVisitedURL(&new_list, stays_the_same_1, 1000); + AppendForcedMostVisitedURL(&new_list, stays_the_same_2, 3000); + AppendForcedMostVisitedURL(&new_list, new_last_forced_time, 4000); + AppendForcedMostVisitedURL(&new_list, gets_added_1, 5000); + AppendForcedMostVisitedURL(&new_list, move_to_forced, 6000); + AppendMostVisitedURL(&new_list, move_to_nonforced); + AppendMostVisitedURL(&new_list, stays_the_same_3); + AppendMostVisitedURL(&new_list, gets_added_2); + AppendMostVisitedURL(&new_list, gets_moved_1); + + history::TopSitesDelta delta; + history::TopSitesImpl::DiffMostVisited(old_list, new_list, &delta); + + ASSERT_EQ(2u, delta.added.size()); + EXPECT_TRUE(gets_added_1 == delta.added[0].url.url); + EXPECT_EQ(-1, delta.added[0].rank); + EXPECT_TRUE(gets_added_2 == delta.added[1].url.url); + EXPECT_EQ(2, delta.added[1].rank); + + ASSERT_EQ(2u, delta.deleted.size()); + EXPECT_TRUE(gets_deleted_1 == delta.deleted[0].url); + EXPECT_TRUE(gets_deleted_2 == delta.deleted[1].url); + + ASSERT_EQ(3u, delta.moved.size()); + EXPECT_TRUE(new_last_forced_time == delta.moved[0].url.url); + EXPECT_EQ(-1, delta.moved[0].rank); + EXPECT_EQ(base::Time::FromJsTime(4000), delta.moved[0].url.last_forced_time); + EXPECT_TRUE(move_to_forced == delta.moved[1].url.url); + EXPECT_EQ(-1, delta.moved[1].rank); + EXPECT_EQ(base::Time::FromJsTime(6000), delta.moved[1].url.last_forced_time); + EXPECT_TRUE(move_to_nonforced == delta.moved[2].url.url); + EXPECT_EQ(0, delta.moved[2].rank); + EXPECT_TRUE(delta.moved[2].url.last_forced_time.is_null()); } // Tests SetPageThumbnail. @@ -615,6 +705,66 @@ TEST_F(TopSitesImplTest, SaveToDB) { } } +// Makes sure forced URLs in top sites get mirrored to the db. +TEST_F(TopSitesImplTest, SaveForcedToDB) { + MostVisitedURL url; + GURL asdf_url("http://asdf.com"); + string16 asdf_title(ASCIIToUTF16("ASDF")); + GURL google_url("http://google.com"); + string16 google_title(ASCIIToUTF16("Google")); + GURL news_url("http://news.google.com"); + string16 news_title(ASCIIToUTF16("Google News")); + + // Add a number of forced URLs. + std::vector<MostVisitedURL> list; + AppendForcedMostVisitedURL(&list, GURL("http://forced1"), 1000); + list[0].title = ASCIIToUTF16("forced1"); + AppendForcedMostVisitedURL(&list, GURL("http://forced2"), 2000); + AppendForcedMostVisitedURL(&list, GURL("http://forced3"), 3000); + AppendForcedMostVisitedURL(&list, GURL("http://forced4"), 4000); + SetTopSites(list); + + // Add a thumbnail. + gfx::Image red_thumbnail(CreateBitmap(SK_ColorRED)); + ASSERT_TRUE(top_sites()->SetPageThumbnail( + GURL("http://forced1"), red_thumbnail, ThumbnailScore())); + + // Get the original thumbnail for later comparison. Some compression can + // happen in |top_sites| and we don't want to depend on that. + SkBitmap orig_thumbnail = GetThumbnail(GURL("http://forced1")); + + // Force-flush the cache to ensure we don't reread from it inadvertently. + EmptyThreadSafeCache(); + + // Make TopSites reread from the db. + StartQueryForMostVisited(); + WaitForHistory(); + + TopSitesQuerier querier; + querier.QueryAllTopSites(top_sites(), true, true); + + ASSERT_EQ(4u + GetPrepopulatePages().size(), querier.urls().size()); + EXPECT_EQ(GURL("http://forced1"), querier.urls()[0].url); + EXPECT_EQ(ASCIIToUTF16("forced1"), querier.urls()[0].title); + SkBitmap thumbnail = GetThumbnail(GURL("http://forced1")); + ASSERT_EQ(orig_thumbnail.getSize(), thumbnail.getSize()); + orig_thumbnail.lockPixels(); + thumbnail.lockPixels(); + EXPECT_EQ(0, memcmp(orig_thumbnail.getPixels(), thumbnail.getPixels(), + orig_thumbnail.getSize())); + thumbnail.unlockPixels(); + orig_thumbnail.unlockPixels(); + EXPECT_EQ(base::Time::FromJsTime(1000), querier.urls()[0].last_forced_time); + EXPECT_EQ(GURL("http://forced2"), querier.urls()[1].url); + EXPECT_EQ(base::Time::FromJsTime(2000), querier.urls()[1].last_forced_time); + EXPECT_EQ(GURL("http://forced3"), querier.urls()[2].url); + EXPECT_EQ(base::Time::FromJsTime(3000), querier.urls()[2].last_forced_time); + EXPECT_EQ(GURL("http://forced4"), querier.urls()[3].url); + EXPECT_EQ(base::Time::FromJsTime(4000), querier.urls()[3].last_forced_time); + + ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 4)); +} + // More permutations of saving to db. TEST_F(TopSitesImplTest, RealDatabase) { MostVisitedURL url; @@ -1095,4 +1245,215 @@ TEST_F(TopSitesImplTest, AddPrepopulatedPages) { ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(q, 0)); } +// Ensure calling SetTopSites with forced sites already in the DB works. +// This test both eviction and +TEST_F(TopSitesImplTest, SetForcedTopSites) { + + const double old_last_forced_time[] = { + 1000, + 4000, + 7000, + 10000, + 11000, + 12000, + 13000, + 18000, + 21000 + }; + size_t num_old_forced = arraysize(old_last_forced_time); + + const double new_last_forced_time[] = { + 2000, + 3000, + 5000, + 6000, + 8000, + 9000, + 14000, + 15000, + 16000, + 17000, + 19000, + 20000, + 22000 + }; + size_t num_new_forced = arraysize(new_last_forced_time); + + const size_t kNumNonForcedURLs = 20; // Maximum number of non-forced URLs. + + MostVisitedURLList old_url_list; + MostVisitedURLList new_url_list; + + old_url_list.resize(num_old_forced + kNumNonForcedURLs); + new_url_list.resize(num_new_forced + kNumNonForcedURLs); + + // Setup a number of forced and non-forced URLs. + for (size_t i = 0; i < num_old_forced; ++i) { + std::ostringstream url; + url << "http://oldforced/" << i; + old_url_list[i].url = GURL(url.str()); + old_url_list[i].last_forced_time = + base::Time::FromJsTime(old_last_forced_time[i]); + } + for (size_t i = num_old_forced; i < old_url_list.size(); ++i) { + std::ostringstream url; + url << "http://oldnonforced/" << (i - num_old_forced); + old_url_list[i].url = GURL(url.str()); + } + for (size_t i = 0; i < num_new_forced; ++i) { + std::ostringstream url; + url << "http://newforced/" << i; + new_url_list[i].url = GURL(url.str()); + new_url_list[i].last_forced_time = + base::Time::FromJsTime(new_last_forced_time[i]); + } + for (size_t i = num_new_forced; i < new_url_list.size(); ++i) { + std::ostringstream url; + url << "http://newnonforced/" << (i - num_new_forced); + new_url_list[i].url = GURL(url.str()); + } + + // Set the initial list of URLs. + SetTopSites(old_url_list); + EXPECT_EQ(num_old_forced + kNumNonForcedURLs, last_num_urls_changed()); + + TopSitesQuerier querier; + // Query only non-forced URLs first. + querier.QueryTopSites(top_sites(), false); + ASSERT_EQ(kNumNonForcedURLs, querier.urls().size()); + + // Check first URL. + EXPECT_EQ("http://oldnonforced/0", querier.urls()[0].url.spec()); + + // Query all URLs. + querier.QueryAllTopSites(top_sites(), false, true); + EXPECT_EQ(num_old_forced + kNumNonForcedURLs, querier.urls().size()); + + // Check first URLs. + EXPECT_EQ("http://oldforced/0", querier.urls()[0].url.spec()); + EXPECT_EQ("http://oldnonforced/0", querier.urls()[num_old_forced].url.spec()); + + // Set the new list of URLs. + SetTopSites(new_url_list); + + // Query all URLs. + querier.QueryAllTopSites(top_sites(), false, true); + + // We should have reached the maximum of 20 forced URLs. + ASSERT_EQ(20 + kNumNonForcedURLs, querier.urls().size()); + + // Check forced URLs. They follow the order of timestamps above, smaller + // timestamps since they were evicted. + EXPECT_EQ("http://newforced/1", querier.urls()[0].url.spec()); // 3000 + EXPECT_EQ("http://oldforced/1", querier.urls()[1].url.spec()); // 4000 + EXPECT_EQ("http://newforced/2", querier.urls()[2].url.spec()); // 5000 + EXPECT_EQ("http://newforced/3", querier.urls()[3].url.spec()); // 6000 + EXPECT_EQ("http://oldforced/2", querier.urls()[4].url.spec()); // 7000 + EXPECT_EQ("http://newforced/4", querier.urls()[5].url.spec()); // 8000 + EXPECT_EQ("http://newforced/5", querier.urls()[6].url.spec()); // 9000 + EXPECT_EQ("http://oldforced/3", querier.urls()[7].url.spec()); // 10000 + EXPECT_EQ("http://oldforced/4", querier.urls()[8].url.spec()); // 11000 + EXPECT_EQ("http://oldforced/5", querier.urls()[9].url.spec()); // 12000 + EXPECT_EQ("http://oldforced/6", querier.urls()[10].url.spec()); // 13000 + EXPECT_EQ("http://newforced/6", querier.urls()[11].url.spec()); // 14000 + EXPECT_EQ("http://newforced/7", querier.urls()[12].url.spec()); // 15000 + EXPECT_EQ("http://newforced/8", querier.urls()[13].url.spec()); // 16000 + EXPECT_EQ("http://newforced/9", querier.urls()[14].url.spec()); // 17000 + EXPECT_EQ("http://oldforced/7", querier.urls()[15].url.spec()); // 18000 + EXPECT_EQ("http://newforced/10", querier.urls()[16].url.spec()); // 19000 + EXPECT_EQ("http://newforced/11", querier.urls()[17].url.spec()); // 20000 + EXPECT_EQ("http://oldforced/8", querier.urls()[18].url.spec()); // 21000 + EXPECT_EQ("http://newforced/12", querier.urls()[19].url.spec()); // 22000 + + // Check first and last non-forced URLs. + EXPECT_EQ("http://newnonforced/0", querier.urls()[20].url.spec()); + EXPECT_EQ("http://newnonforced/19", querier.urls()[39].url.spec()); +} + +TEST_F(TopSitesImplTest, SetForcedTopSitesWithCollisions) { + MostVisitedURLList old_url_list; + MostVisitedURLList new_url_list; + + old_url_list.resize(5); + old_url_list[0].url = GURL("http://url/0"); + old_url_list[0].last_forced_time = base::Time::FromJsTime(1000); + old_url_list[1].url = GURL("http://collision/0"); // Evicted + old_url_list[1].last_forced_time = base::Time::FromJsTime(4000); + old_url_list[2].url = GURL("http://collision/1"); // Evicted + old_url_list[2].last_forced_time = base::Time::FromJsTime(6000); + old_url_list[3].url = GURL("http://collision/2"); // Evicted + old_url_list[3].last_forced_time = base::Time::FromJsTime(7000); + // The following is evicted since all non-forced URLs are, therefore it + // doesn't cause a collision. + old_url_list[4].url = GURL("http://noncollision/0"); + + new_url_list.resize(6); + new_url_list[0].url = GURL("http://collision/1"); + new_url_list[0].last_forced_time = base::Time::FromJsTime(2000); + new_url_list[1].url = GURL("http://url/2"); + new_url_list[1].last_forced_time = base::Time::FromJsTime(3000); + new_url_list[2].url = GURL("http://collision/0"); + new_url_list[2].last_forced_time = base::Time::FromJsTime(5000); + new_url_list[3].url = GURL("http://noncollision/0"); + new_url_list[3].last_forced_time = base::Time::FromJsTime(9000); + new_url_list[4].url = GURL("http://collision/2"); + new_url_list[5].url = GURL("http://url/3"); + + // Set the initial list of URLs. + SetTopSites(old_url_list); + + // Set the new list of URLs. + SetTopSites(new_url_list); + + // Query all URLs. + TopSitesQuerier querier; + querier.QueryAllTopSites(top_sites(), false, true); + + // Check URLs. When collision occurs, the incoming one is always preferred. + ASSERT_EQ(7u + GetPrepopulatePages().size(), querier.urls().size()); + EXPECT_EQ("http://url/0", querier.urls()[0].url.spec()); + EXPECT_EQ(1000u, querier.urls()[0].last_forced_time.ToJsTime()); + EXPECT_EQ("http://collision/1", querier.urls()[1].url.spec()); + EXPECT_EQ(2000u, querier.urls()[1].last_forced_time.ToJsTime()); + EXPECT_EQ("http://url/2", querier.urls()[2].url.spec()); + EXPECT_EQ(3000u, querier.urls()[2].last_forced_time.ToJsTime()); + EXPECT_EQ("http://collision/0", querier.urls()[3].url.spec()); + EXPECT_EQ(5000u, querier.urls()[3].last_forced_time.ToJsTime()); + EXPECT_EQ("http://noncollision/0", querier.urls()[4].url.spec()); + EXPECT_EQ(9000u, querier.urls()[4].last_forced_time.ToJsTime()); + EXPECT_EQ("http://collision/2", querier.urls()[5].url.spec()); + EXPECT_TRUE(querier.urls()[5].last_forced_time.is_null()); + EXPECT_EQ("http://url/3", querier.urls()[6].url.spec()); + EXPECT_TRUE(querier.urls()[6].last_forced_time.is_null()); + ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 7)); +} + + +TEST_F(TopSitesImplTest, SetTopSitesIdentical) { + MostVisitedURLList url_list; + url_list.resize(3); + url_list[0].url = GURL("http://url/0"); + url_list[0].last_forced_time = base::Time::FromJsTime(1000); + url_list[1].url = GURL("http://url/1"); // Evicted + url_list[2].url = GURL("http://url/2"); // Evicted + + // Set the initial list of URLs. + SetTopSites(url_list); + + // Set the new list of URLs. + SetTopSites(MostVisitedURLList(url_list)); + + // Query all URLs. + TopSitesQuerier querier; + querier.QueryAllTopSites(top_sites(), false, true); + + // Check URLs. When collision occurs, the incoming one is always preferred. + ASSERT_EQ(3u + GetPrepopulatePages().size(), querier.urls().size()); + EXPECT_EQ("http://url/0", querier.urls()[0].url.spec()); + EXPECT_EQ(1000u, querier.urls()[0].last_forced_time.ToJsTime()); + EXPECT_EQ("http://url/1", querier.urls()[1].url.spec()); + EXPECT_EQ("http://url/2", querier.urls()[2].url.spec()); + ASSERT_NO_FATAL_FAILURE(ContainsPrepopulatePages(querier, 3)); +} + } // namespace history diff --git a/chrome/browser/jumplist_win.cc b/chrome/browser/jumplist_win.cc index c5436ea..9dad7a6 100644 --- a/chrome/browser/jumplist_win.cc +++ b/chrome/browser/jumplist_win.cc @@ -526,7 +526,7 @@ void JumpList::Observe(int type, if (top_sites) { top_sites->GetMostVisitedURLs( base::Bind(&JumpList::OnMostVisitedURLsAvailable, - weak_ptr_factory_.GetWeakPtr())); + weak_ptr_factory_.GetWeakPtr()), false); } break; } diff --git a/chrome/browser/search/instant_service.cc b/chrome/browser/search/instant_service.cc index 7dab79d..89fad11 100644 --- a/chrome/browser/search/instant_service.cc +++ b/chrome/browser/search/instant_service.cc @@ -251,7 +251,7 @@ void InstantService::Observe(int type, if (top_sites) { top_sites->GetMostVisitedURLs( base::Bind(&InstantService::OnMostVisitedItemsReceived, - weak_ptr_factory_.GetWeakPtr())); + weak_ptr_factory_.GetWeakPtr()), false); } break; } diff --git a/chrome/browser/thumbnails/thumbnail_service_impl.cc b/chrome/browser/thumbnails/thumbnail_service_impl.cc index d584ecd..117d730 100644 --- a/chrome/browser/thumbnails/thumbnail_service_impl.cc +++ b/chrome/browser/thumbnails/thumbnail_service_impl.cc @@ -79,7 +79,7 @@ bool ThumbnailServiceImpl::ShouldAcquirePageThumbnail(const GURL& url) { if (!HistoryService::CanAddURL(url)) return false; // Skip if the top sites list is full, and the URL is not known. - if (local_ptr->IsFull() && !local_ptr->IsKnownURL(url)) + if (local_ptr->IsNonForcedFull() && !local_ptr->IsKnownURL(url)) return false; // Skip if we don't have to udpate the existing thumbnail. ThumbnailScore current_score; diff --git a/chrome/browser/thumbnails/thumbnail_service_unittest.cc b/chrome/browser/thumbnails/thumbnail_service_unittest.cc index 1b128e3..707d880 100644 --- a/chrome/browser/thumbnails/thumbnail_service_unittest.cc +++ b/chrome/browser/thumbnails/thumbnail_service_unittest.cc @@ -21,9 +21,12 @@ class MockTopSites : public history::TopSitesImpl { } // history::TopSitesImpl overrides. - virtual bool IsFull() OVERRIDE { + virtual bool IsNonForcedFull() OVERRIDE { return known_url_map_.size() >= capacity_; } + virtual bool IsForcedFull() OVERRIDE { + return false; + } virtual bool IsKnownURL(const GURL& url) OVERRIDE { return known_url_map_.find(url.spec()) != known_url_map_.end(); } @@ -98,7 +101,7 @@ TEST_F(ThumbnailServiceTest, ShouldUpdateThumbnail) { ThumbnailScore bad_score; bad_score.time_at_snapshot = base::Time::UnixEpoch(); // Ancient time stamp. profile.AddKnownURL(kGoodURL, bad_score); - ASSERT_TRUE(profile.GetTopSites()->IsFull()); + ASSERT_TRUE(profile.GetTopSites()->IsNonForcedFull()); // Should be false, as the top sites data is full, and the new URL is // not known. diff --git a/chrome/browser/ui/gtk/global_history_menu.cc b/chrome/browser/ui/gtk/global_history_menu.cc index 264ff0f..94827e9b 100644 --- a/chrome/browser/ui/gtk/global_history_menu.cc +++ b/chrome/browser/ui/gtk/global_history_menu.cc @@ -144,7 +144,7 @@ void GlobalHistoryMenu::GetTopSitesData() { top_sites_->GetMostVisitedURLs( base::Bind(&GlobalHistoryMenu::OnTopSitesReceived, - weak_ptr_factory_.GetWeakPtr())); + weak_ptr_factory_.GetWeakPtr()), false); } void GlobalHistoryMenu::OnTopSitesReceived( diff --git a/chrome/browser/ui/views/frame/global_menu_bar_x11.cc b/chrome/browser/ui/views/frame/global_menu_bar_x11.cc index 57bd57e..fcecb76 100644 --- a/chrome/browser/ui/views/frame/global_menu_bar_x11.cc +++ b/chrome/browser/ui/views/frame/global_menu_bar_x11.cc @@ -547,7 +547,7 @@ void GlobalMenuBarX11::GetTopSitesData() { top_sites_->GetMostVisitedURLs( base::Bind(&GlobalMenuBarX11::OnTopSitesReceived, - weak_ptr_factory_.GetWeakPtr())); + weak_ptr_factory_.GetWeakPtr()), false); } void GlobalMenuBarX11::OnTopSitesReceived( diff --git a/chrome/browser/ui/webui/ntp/most_visited_handler.cc b/chrome/browser/ui/webui/ntp/most_visited_handler.cc index ba6e7e0..11f7f82 100644 --- a/chrome/browser/ui/webui/ntp/most_visited_handler.cc +++ b/chrome/browser/ui/webui/ntp/most_visited_handler.cc @@ -168,7 +168,7 @@ void MostVisitedHandler::StartQueryForMostVisited() { if (ts) { ts->GetMostVisitedURLs( base::Bind(&MostVisitedHandler::OnMostVisitedUrlsAvailable, - weak_ptr_factory_.GetWeakPtr())); + weak_ptr_factory_.GetWeakPtr()), false); } } diff --git a/chrome/browser/ui/webui/ntp/thumbnail_list_source.cc b/chrome/browser/ui/webui/ntp/thumbnail_list_source.cc index d4bbaf7..ee5a862 100644 --- a/chrome/browser/ui/webui/ntp/thumbnail_list_source.cc +++ b/chrome/browser/ui/webui/ntp/thumbnail_list_source.cc @@ -89,7 +89,7 @@ void ThumbnailListSource::StartDataRequest( profile_->GetTopSites()->GetMostVisitedURLs( base::Bind(&ThumbnailListSource::OnMostVisitedURLsAvailable, weak_ptr_factory_.GetWeakPtr(), - callback)); + callback), false); } std::string ThumbnailListSource::GetMimeType(const std::string& path) const { |