diff options
author | levin@chromium.org <levin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-08-31 23:23:26 +0000 |
---|---|---|
committer | levin@chromium.org <levin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-08-31 23:23:26 +0000 |
commit | 10e0adff3c1f0bd8d53b504f2e64ff60cb5570da (patch) | |
tree | aa62cd3b3f62f41c94e1252c3c0e784fda210e1b /chrome/browser | |
parent | 2f6d086da79c737bc2a0ee3023ca80c608c75d19 (diff) | |
download | chromium_src-10e0adff3c1f0bd8d53b504f2e64ff60cb5570da.zip chromium_src-10e0adff3c1f0bd8d53b504f2e64ff60cb5570da.tar.gz chromium_src-10e0adff3c1f0bd8d53b504f2e64ff60cb5570da.tar.bz2 |
Prepare to load search provider information directly on the I/O thread.
BUG=38475
TEST=unit_tests --gtest_filter=Temp*:Search*:Key*
Review URL: http://codereview.chromium.org/3250011
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@58098 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser')
5 files changed, 56 insertions, 254 deletions
diff --git a/chrome/browser/search_engines/search_host_to_urls_map.cc b/chrome/browser/search_engines/search_host_to_urls_map.cc index 584ecc5..8ff2ac0 100644 --- a/chrome/browser/search_engines/search_host_to_urls_map.cc +++ b/chrome/browser/search_engines/search_host_to_urls_map.cc @@ -6,72 +6,64 @@ #include "base/scoped_ptr.h" #include "base/task.h" -#include "chrome/browser/chrome_thread.h" #include "chrome/browser/search_engines/template_url.h" #include "chrome/browser/search_engines/template_url_model.h" -// Indicates if the two inputs have the same security origin. -// |requested_origin| should only be a security origin (no path, etc.). -// It is ok if |template_url| is NULL. -static bool IsSameOrigin(const GURL& requested_origin, - const TemplateURL* template_url) { - // TODO(levin): This isn't safe for the I/O thread yet. - DCHECK(!ChromeThread::IsWellKnownThread(ChromeThread::UI) || - ChromeThread::CurrentlyOn(ChromeThread::UI)); - - DCHECK(requested_origin == requested_origin.GetOrigin()); - return template_url && requested_origin == - TemplateURLModel::GenerateSearchURL(template_url).GetOrigin(); -} - SearchHostToURLsMap::SearchHostToURLsMap() : initialized_(false) { } +SearchHostToURLsMap::~SearchHostToURLsMap() { +} + void SearchHostToURLsMap::Init( - const std::vector<const TemplateURL*>& template_urls, - const TemplateURL* default_search_provider) { - DCHECK(CalledOnValidThread()); + const std::vector<const TemplateURL*>& template_urls) { DCHECK(!initialized_); - { - AutoLock locker(lock_); - for (size_t i = 0; i < template_urls.size(); ++i) { - AddNoLock(template_urls[i]); - } - SetDefaultNoLock(default_search_provider); - } - // Wait to set initialized until the lock is let go, which should force a - // memory write flush. + + // Set as initialized here so Add doesn't assert. initialized_ = true; + + for (size_t i = 0; i < template_urls.size(); ++i) { + Add(template_urls[i]); + } } void SearchHostToURLsMap::Add(const TemplateURL* template_url) { - DCHECK(CalledOnValidThread()); DCHECK(initialized_); DCHECK(template_url); - AutoLock locker(lock_); - AddNoLock(template_url); + const GURL url(TemplateURLModel::GenerateSearchURL(template_url)); + if (!url.is_valid() || !url.has_host()) + return; + + host_to_urls_map_[url.host()].insert(template_url); } void SearchHostToURLsMap::Remove(const TemplateURL* template_url) { - DCHECK(CalledOnValidThread()); DCHECK(initialized_); DCHECK(template_url); - AutoLock locker(lock_); - RemoveNoLock(template_url); + const GURL url(TemplateURLModel::GenerateSearchURL(template_url)); + if (!url.is_valid() || !url.has_host()) + return; + + const std::string host(url.host()); + DCHECK(host_to_urls_map_.find(host) != host_to_urls_map_.end()); + + TemplateURLSet& urls = host_to_urls_map_[host]; + DCHECK(urls.find(template_url) != urls.end()); + + urls.erase(urls.find(template_url)); + if (urls.empty()) + host_to_urls_map_.erase(host_to_urls_map_.find(host)); } void SearchHostToURLsMap::Update(const TemplateURL* existing_turl, const TemplateURL& new_values) { - DCHECK(CalledOnValidThread()); DCHECK(initialized_); DCHECK(existing_turl); - AutoLock locker(lock_); - - RemoveNoLock(existing_turl); + Remove(existing_turl); // Use the information from new_values but preserve existing_turl's id. TemplateURLID previous_id = existing_turl->id(); @@ -79,22 +71,12 @@ void SearchHostToURLsMap::Update(const TemplateURL* existing_turl, *modifiable_turl = new_values; modifiable_turl->set_id(previous_id); - AddNoLock(existing_turl); -} - -void SearchHostToURLsMap::RemoveAll() { - DCHECK(CalledOnValidThread()); - - AutoLock locker(lock_); - host_to_urls_map_.clear(); + Add(existing_turl); } void SearchHostToURLsMap::UpdateGoogleBaseURLs() { - DCHECK(CalledOnValidThread()); DCHECK(initialized_); - AutoLock locker(lock_); - // Create a list of the the TemplateURLs to update. std::vector<const TemplateURL*> t_urls_using_base_url; for (HostToURLsMap::iterator host_map_iterator = host_to_urls_map_.begin(); @@ -112,23 +94,14 @@ void SearchHostToURLsMap::UpdateGoogleBaseURLs() { } for (size_t i = 0; i < t_urls_using_base_url.size(); ++i) - RemoveByPointerNoLock(t_urls_using_base_url[i]); + RemoveByPointer(t_urls_using_base_url[i]); for (size_t i = 0; i < t_urls_using_base_url.size(); ++i) - AddNoLock(t_urls_using_base_url[i]); -} - -void SearchHostToURLsMap::SetDefault(const TemplateURL* template_url) { - DCHECK(CalledOnValidThread()); - DCHECK(initialized_); - - AutoLock locker(lock_); - SetDefaultNoLock(template_url); + Add(t_urls_using_base_url[i]); } const TemplateURL* SearchHostToURLsMap::GetTemplateURLForHost( const std::string& host) const { - DCHECK(CalledOnValidThread()); DCHECK(initialized_); HostToURLsMap::const_iterator iter = host_to_urls_map_.find(host); @@ -139,7 +112,6 @@ const TemplateURL* SearchHostToURLsMap::GetTemplateURLForHost( const SearchHostToURLsMap::TemplateURLSet* SearchHostToURLsMap::GetURLsForHost( const std::string& host) const { - DCHECK(CalledOnValidThread()); DCHECK(initialized_); HostToURLsMap::const_iterator urls_for_host = host_to_urls_map_.find(host); @@ -148,72 +120,8 @@ const SearchHostToURLsMap::TemplateURLSet* SearchHostToURLsMap::GetURLsForHost( return &urls_for_host->second; } -SearchProviderInstallData::State SearchHostToURLsMap::GetInstallState( - const GURL& requested_origin) { - AutoLock locker(lock_); - if (!initialized_) - return NOT_READY; - - // First check to see if the origin is the default search provider. - if (requested_origin.spec() == default_search_origin_) - return INSTALLED_AS_DEFAULT; - - // Is the url any search provider? - HostToURLsMap::const_iterator urls_for_host = - host_to_urls_map_.find(requested_origin.host()); - if (urls_for_host == host_to_urls_map_.end() || - urls_for_host->second.empty()) { - return NOT_INSTALLED; - } - - const TemplateURLSet& urls = urls_for_host->second; - for (TemplateURLSet::const_iterator i = urls.begin(); - i != urls.end(); ++i) { - const TemplateURL* template_url = *i; - if (IsSameOrigin(requested_origin, template_url)) - return INSTALLED_BUT_NOT_DEFAULT; - } - return NOT_INSTALLED; -} - -SearchHostToURLsMap::~SearchHostToURLsMap() { -} - -void SearchHostToURLsMap::AddNoLock(const TemplateURL* template_url) { - DCHECK(CalledOnValidThread()); - - lock_.AssertAcquired(); - const GURL url(TemplateURLModel::GenerateSearchURL(template_url)); - if (!url.is_valid() || !url.has_host()) - return; - - host_to_urls_map_[url.host()].insert(template_url); -} - -void SearchHostToURLsMap::RemoveNoLock(const TemplateURL* template_url) { - DCHECK(CalledOnValidThread()); - lock_.AssertAcquired(); - - const GURL url(TemplateURLModel::GenerateSearchURL(template_url)); - if (!url.is_valid() || !url.has_host()) - return; - - const std::string host(url.host()); - DCHECK(host_to_urls_map_.find(host) != host_to_urls_map_.end()); - - TemplateURLSet& urls = host_to_urls_map_[host]; - DCHECK(urls.find(template_url) != urls.end()); - - urls.erase(urls.find(template_url)); - if (urls.empty()) - host_to_urls_map_.erase(host_to_urls_map_.find(host)); -} - -void SearchHostToURLsMap::RemoveByPointerNoLock( +void SearchHostToURLsMap::RemoveByPointer( const TemplateURL* template_url) { - DCHECK(CalledOnValidThread()); - lock_.AssertAcquired(); - for (HostToURLsMap::iterator i = host_to_urls_map_.begin(); i != host_to_urls_map_.end(); ++i) { TemplateURLSet::iterator url_set_iterator = i->second.find(template_url); @@ -227,20 +135,3 @@ void SearchHostToURLsMap::RemoveByPointerNoLock( } } } - -void SearchHostToURLsMap::SetDefaultNoLock(const TemplateURL* template_url) { - DCHECK(CalledOnValidThread()); - lock_.AssertAcquired(); - - if (!template_url) { - default_search_origin_.clear(); - return; - } - - const GURL url(TemplateURLModel::GenerateSearchURL(template_url)); - if (!url.is_valid() || !url.has_host()) { - default_search_origin_.clear(); - return; - } - default_search_origin_ = url.GetOrigin().spec(); -} diff --git a/chrome/browser/search_engines/search_host_to_urls_map.h b/chrome/browser/search_engines/search_host_to_urls_map.h index 90ae63d..bf08b0d 100644 --- a/chrome/browser/search_engines/search_host_to_urls_map.h +++ b/chrome/browser/search_engines/search_host_to_urls_map.h @@ -12,33 +12,21 @@ #include <vector> #include "base/basictypes.h" -#include "base/lock.h" -#include "base/thread_checker.h" -#include "base/ref_counted.h" -#include "chrome/browser/search_engines/search_provider_install_data.h" - -class FilePath; -class GURL; -class Task; + class TemplateURL; -class WebDataService; - -// Holds the host to template url mappings for the search providers. All public -// methods should happen from the same thread except for GetInstallState which -// may be invoked on any thread. WARNING: This class does not own any -// TemplateURLs passed to it and it is up to the caller to ensure the right -// lifetime of them. -class SearchHostToURLsMap - : public SearchProviderInstallData, - public ThreadChecker { + +// Holds the host to template url mappings for the search providers. WARNING: +// This class does not own any TemplateURLs passed to it and it is up to the +// caller to ensure the right lifetime of them. +class SearchHostToURLsMap { public: typedef std::set<const TemplateURL*> TemplateURLSet; SearchHostToURLsMap(); + ~SearchHostToURLsMap(); // Initializes the map. - void Init(const std::vector<const TemplateURL*>& template_urls, - const TemplateURL* default_search_provider); + void Init(const std::vector<const TemplateURL*>& template_urls); // Adds a new TemplateURL to the map. Since |template_url| is owned // externally, Remove or RemoveAll should be called if it becomes invalid. @@ -47,9 +35,6 @@ class SearchHostToURLsMap // Removes the TemplateURL from the lookup. void Remove(const TemplateURL* template_url); - // Removes all TemplateURLs. - void RemoveAll(); - // Updates information in an existing TemplateURL. Note: Using Remove and // then Add separately would lead to race conditions in reading because the // lock would be released inbetween the calls. @@ -58,10 +43,6 @@ class SearchHostToURLsMap // Updates all search providers which have a google base url. void UpdateGoogleBaseURLs(); - // Sets the default search provider. This method doesn't store a reference to - // the TemplateURL passed in. - void SetDefault(const TemplateURL* template_url); - // Returns the first TemplateURL found with a URL using the specified |host|, // or NULL if there are no such TemplateURLs const TemplateURL* GetTemplateURLForHost(const std::string& host) const; @@ -70,31 +51,15 @@ class SearchHostToURLsMap // none. const TemplateURLSet* GetURLsForHost(const std::string& host) const; - // Returns the search provider install state for the given origin. - // This method may be called on any thread. - // TODO(levin): Make the above statement true about "any thread". - virtual State GetInstallState(const GURL& requested_origin); - private: friend class SearchHostToURLsMapTest; typedef std::map<std::string, TemplateURLSet> HostToURLsMap; - virtual ~SearchHostToURLsMap(); - - // Same as Add but the lock should already be taken. - void AddNoLock(const TemplateURL* template_url); - - // Same as Remove but the lock should already be taken. - void RemoveNoLock(const TemplateURL* template_url); - // Removes the given template_url using only the pointer instead of the value. // This is useful when the value may have changed before being updated in the // map. (Specifically when the GoogleBaseURLValue changes.) - void RemoveByPointerNoLock(const TemplateURL* template_url); - - // Same as SetDefaultNoLock but the lock should already be taken. - void SetDefaultNoLock(const TemplateURL* template_url); + void RemoveByPointer(const TemplateURL* template_url); // Maps from host to set of TemplateURLs whose search url host is host. HostToURLsMap host_to_urls_map_; @@ -105,9 +70,6 @@ class SearchHostToURLsMap // Has Init been called? bool initialized_; - // Used to guard writes from the UI thread and reads from other threads. - Lock lock_; - DISALLOW_COPY_AND_ASSIGN(SearchHostToURLsMap); }; diff --git a/chrome/browser/search_engines/search_host_to_urls_map_unittest.cc b/chrome/browser/search_engines/search_host_to_urls_map_unittest.cc index 7eb6daf..4ba1ac6 100644 --- a/chrome/browser/search_engines/search_host_to_urls_map_unittest.cc +++ b/chrome/browser/search_engines/search_host_to_urls_map_unittest.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "base/basictypes.h" +#include "base/scoped_ptr.h" #include "chrome/browser/search_engines/search_host_to_urls_map.h" #include "chrome/browser/search_engines/template_url.h" #include "testing/gtest/include/gtest/gtest.h" @@ -28,7 +29,7 @@ class SearchHostToURLsMapTest : public testing::Test { ASSERT_STREQ(origin.c_str(), provider_map_->default_search_origin_.c_str()); } - scoped_refptr<SearchHostToURLsMap> provider_map_; + scoped_ptr<SearchHostToURLsMap> provider_map_; TemplateURL t_urls_[2]; std::string host_; @@ -45,8 +46,8 @@ void SearchHostToURLsMapTest::SetUp() { template_urls.push_back(&t_urls_[0]); template_urls.push_back(&t_urls_[1]); - provider_map_ = new SearchHostToURLsMap; - provider_map_->Init(template_urls, NULL); + provider_map_.reset(new SearchHostToURLsMap); + provider_map_->Init(template_urls); } TEST_F(SearchHostToURLsMapTest, Add) { @@ -76,16 +77,6 @@ TEST_F(SearchHostToURLsMapTest, Remove) { ASSERT_EQ(1, url_count); } -TEST_F(SearchHostToURLsMapTest, RemoveAll) { - provider_map_->RemoveAll(); - - const TemplateURL* found_url = provider_map_->GetTemplateURLForHost(host_); - ASSERT_TRUE(found_url == NULL); - - const TemplateURLSet* urls = provider_map_->GetURLsForHost(host_); - ASSERT_TRUE(urls == NULL); -} - TEST_F(SearchHostToURLsMapTest, Update) { std::string new_host = "example.com"; TemplateURL new_values; @@ -115,11 +106,6 @@ TEST_F(SearchHostToURLsMapTest, UpdateGoogleBaseURLs) { new_google_base_url)); } -TEST_F(SearchHostToURLsMapTest, SetDefault) { - provider_map_->SetDefault(&t_urls_[0]); - VerifyDefault("http://" + host_ + "/"); -} - TEST_F(SearchHostToURLsMapTest, GetTemplateURLForKnownHost) { const TemplateURL* found_url = provider_map_->GetTemplateURLForHost(host_); ASSERT_TRUE(found_url == &t_urls_[0] || found_url == &t_urls_[1]); @@ -156,38 +142,3 @@ TEST_F(SearchHostToURLsMapTest, GetURLsForUnknownHost) { const TemplateURLSet* urls = provider_map_->GetURLsForHost("a" + host_); ASSERT_TRUE(urls == NULL); } - -TEST_F(SearchHostToURLsMapTest, GetInstallStateNotReady) { - scoped_refptr<SearchHostToURLsMap> not_init_map(new SearchHostToURLsMap); - ASSERT_EQ(SearchProviderInstallData::NOT_READY, - not_init_map->GetInstallState(GURL("http://" + host_ + "/"))); -} - -TEST_F(SearchHostToURLsMapTest, GetInstallStateNotDefault) { - ASSERT_EQ(SearchProviderInstallData::INSTALLED_BUT_NOT_DEFAULT, - provider_map_->GetInstallState(GURL("http://" + host_ + "/"))); - ASSERT_EQ(SearchProviderInstallData::INSTALLED_BUT_NOT_DEFAULT, - provider_map_->GetInstallState(GURL("http://" + host_ + ":80/"))); -} - -TEST_F(SearchHostToURLsMapTest, GetInstallStateNotInstalledDifferentPort) { - ASSERT_EQ(SearchProviderInstallData::NOT_INSTALLED, - provider_map_->GetInstallState(GURL("http://" + host_ + ":96/"))); -} - -TEST_F(SearchHostToURLsMapTest, GetInstallStateNotInstalledDifferentScheme) { - ASSERT_EQ(SearchProviderInstallData::NOT_INSTALLED, - provider_map_->GetInstallState(GURL("https://" + host_ + "/"))); -} - -TEST_F(SearchHostToURLsMapTest, GetInstallStateNotInstalled) { - ASSERT_EQ(SearchProviderInstallData::NOT_INSTALLED, - provider_map_->GetInstallState(GURL("http://a" + host_ + "/"))); -} - -TEST_F(SearchHostToURLsMapTest, GetInstallStateDefault) { - provider_map_->SetDefault(&t_urls_[0]); - ASSERT_EQ(SearchProviderInstallData::INSTALLED_AS_DEFAULT, - provider_map_->GetInstallState(GURL("http://" + host_ + "/"))); -} - diff --git a/chrome/browser/search_engines/template_url_model.cc b/chrome/browser/search_engines/template_url_model.cc index b5cf1df..5a119e2 100644 --- a/chrome/browser/search_engines/template_url_model.cc +++ b/chrome/browser/search_engines/template_url_model.cc @@ -97,7 +97,6 @@ TemplateURLModel::~TemplateURLModel() { service_->CancelRequest(load_handle_); } - provider_map_->RemoveAll(); STLDeleteElements(&template_urls_); } @@ -236,7 +235,7 @@ const TemplateURL* TemplateURLModel::GetTemplateURLForKeyword( const TemplateURL* TemplateURLModel::GetTemplateURLForHost( const std::string& host) const { - return provider_map_->GetTemplateURLForHost(host); + return provider_map_.GetTemplateURLForHost(host); } void TemplateURLModel::Add(TemplateURL* template_url) { @@ -425,7 +424,6 @@ void TemplateURLModel::SetDefaultSearchProvider(const TemplateURL* url) { service_->SetDefaultSearchProvider(url); if (loaded_) { - provider_map_->SetDefault(url); FOR_EACH_OBSERVER(TemplateURLModelObserver, model_observers_, OnTemplateURLModelChanged()); } @@ -602,7 +600,6 @@ void TemplateURLModel::Init(const Initializer* initializers, } registrar_.Add(this, NotificationType::GOOGLE_URL_UPDATED, NotificationService::AllSources()); - provider_map_ = new SearchHostToURLsMap(); if (num_initializers > 0) { // This path is only hit by test code and is used to simulate a loaded @@ -647,7 +644,7 @@ void TemplateURLModel::RemoveFromMaps(const TemplateURL* template_url) { if (!template_url->keyword().empty()) keyword_to_template_map_.erase(template_url->keyword()); if (loaded_) - provider_map_->Remove(template_url); + provider_map_.Remove(template_url); } void TemplateURLModel::RemoveFromKeywordMapByPointer( @@ -668,7 +665,7 @@ void TemplateURLModel::AddToMaps(const TemplateURL* template_url) { if (!template_url->keyword().empty()) keyword_to_template_map_[template_url->keyword()] = template_url; if (loaded_) - provider_map_->Add(template_url); + provider_map_.Add(template_url); } void TemplateURLModel::SetTemplateURLs(const std::vector<TemplateURL*>& urls) { @@ -699,7 +696,7 @@ void TemplateURLModel::SetTemplateURLs(const std::vector<TemplateURL*>& urls) { void TemplateURLModel::ChangeToLoadedState() { DCHECK(!loaded_); - provider_map_->Init(template_urls_, default_search_provider_); + provider_map_.Init(template_urls_); loaded_ = true; } @@ -815,7 +812,7 @@ void TemplateURLModel::RegisterPrefs(PrefService* prefs) { bool TemplateURLModel::CanReplaceKeywordForHost( const std::string& host, const TemplateURL** to_replace) { - const TemplateURLSet* urls = provider_map_->GetURLsForHost(host); + const TemplateURLSet* urls = provider_map_.GetURLsForHost(host); if (urls) { for (TemplateURLSet::const_iterator i = urls->begin(); i != urls->end(); ++i) { @@ -849,7 +846,7 @@ void TemplateURLModel::Update(const TemplateURL* existing_turl, keyword_to_template_map_.erase(existing_turl->keyword()); // This call handles copying over the values (while retaining the id). - provider_map_->Update(existing_turl, new_values); + provider_map_.Update(existing_turl, new_values); if (!existing_turl->keyword().empty()) keyword_to_template_map_[existing_turl->keyword()] = existing_turl; @@ -881,7 +878,7 @@ void TemplateURLModel::UpdateKeywordSearchTermsForURL( } const TemplateURLSet* urls_for_host = - provider_map_->GetURLsForHost(row.url().host()); + provider_map_.GetURLsForHost(row.url().host()); if (!urls_for_host) return; @@ -1007,7 +1004,7 @@ void TemplateURLModel::GoogleBaseURLChanged() { } if (something_changed && loaded_) { - provider_map_->UpdateGoogleBaseURLs(); + provider_map_.UpdateGoogleBaseURLs(); FOR_EACH_OBSERVER(TemplateURLModelObserver, model_observers_, OnTemplateURLModelChanged()); } diff --git a/chrome/browser/search_engines/template_url_model.h b/chrome/browser/search_engines/template_url_model.h index ab6e171..63f6dc3 100644 --- a/chrome/browser/search_engines/template_url_model.h +++ b/chrome/browser/search_engines/template_url_model.h @@ -13,6 +13,7 @@ #include "base/scoped_ptr.h" #include "chrome/browser/search_engines/template_url_id.h" #include "chrome/browser/webdata/web_data_service.h" +#include "chrome/browser/search_engines/search_host_to_urls_map.h" #include "chrome/common/notification_observer.h" #include "chrome/common/notification_registrar.h" @@ -333,7 +334,7 @@ class TemplateURLModel : public WebDataServiceConsumer, ObserverList<TemplateURLModelObserver> model_observers_; // Maps from host to set of TemplateURLs whose search url host is host. - scoped_refptr<SearchHostToURLsMap> provider_map_; + SearchHostToURLsMap provider_map_; // Used to obtain the WebDataService. // When Load is invoked, if we haven't yet loaded, the WebDataService is |