diff options
| author | levin@chromium.org <levin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-11 22:58:20 +0000 |
|---|---|---|
| committer | levin@chromium.org <levin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-11 22:58:20 +0000 |
| commit | 2de7e004b9e0cede80a1955d99d01a1e484182d9 (patch) | |
| tree | b557dd9dec9b168190d96521c8e5d028f576b0b6 /chrome/browser/search_engines | |
| parent | 13287f25a7562744b3570c2bc6ab9f329f35e239 (diff) | |
| download | chromium_src-2de7e004b9e0cede80a1955d99d01a1e484182d9.zip chromium_src-2de7e004b9e0cede80a1955d99d01a1e484182d9.tar.gz chromium_src-2de7e004b9e0cede80a1955d99d01a1e484182d9.tar.bz2 | |
Wire up InstallSearchProvider to allow setting the default search provider.
Depends on http://codereview.chromium.org/3673002/show.
BUG=38475
TEST=Next patch changes the callback mechanism TemplateURLFetcher to make it much more testable and adds tests. (I kept it out of this one to make this more focused.)
Review URL: http://codereview.chromium.org/3652003
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@62204 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser/search_engines')
6 files changed, 141 insertions, 47 deletions
diff --git a/chrome/browser/search_engines/keyword_editor_controller.cc b/chrome/browser/search_engines/keyword_editor_controller.cc index ac7d636..4755bc9 100644 --- a/chrome/browser/search_engines/keyword_editor_controller.cc +++ b/chrome/browser/search_engines/keyword_editor_controller.cc @@ -84,10 +84,7 @@ bool KeywordEditorController::CanEdit(const TemplateURL* url) const { } bool KeywordEditorController::CanMakeDefault(const TemplateURL* url) const { - return (url != url_model()->GetDefaultSearchProvider() && - url->url() && - url->url()->SupportsReplacement() && - !url_model()->is_default_search_managed()); + return url_model()->CanMakeDefault(url); } bool KeywordEditorController::CanRemove(const TemplateURL* url) const { diff --git a/chrome/browser/search_engines/template_url_fetcher.cc b/chrome/browser/search_engines/template_url_fetcher.cc index e6ab568..0a7f352 100644 --- a/chrome/browser/search_engines/template_url_fetcher.cc +++ b/chrome/browser/search_engines/template_url_fetcher.cc @@ -27,14 +27,14 @@ class TemplateURLFetcher::RequestDelegate : public URLFetcher::Delegate, const GURL& osdd_url, const GURL& favicon_url, TabContents* source, - bool autodetected) + ProviderType provider_type) : ALLOW_THIS_IN_INITIALIZER_LIST(url_fetcher_(osdd_url, URLFetcher::GET, this)), fetcher_(fetcher), keyword_(keyword), osdd_url_(osdd_url), favicon_url_(favicon_url), - autodetected_(autodetected), + provider_type_(provider_type), source_(source) { url_fetcher_.set_request_context(fetcher->profile()->GetRequestContext()); url_fetcher_.Start(); @@ -68,6 +68,9 @@ class TemplateURLFetcher::RequestDelegate : public URLFetcher::Delegate, // Keyword to use. const std::wstring keyword() const { return keyword_; } + // The type of search provider being fetched. + ProviderType provider_type() const { return provider_type_; } + private: void AddSearchProvider(); @@ -77,7 +80,7 @@ class TemplateURLFetcher::RequestDelegate : public URLFetcher::Delegate, std::wstring keyword_; const GURL osdd_url_; const GURL favicon_url_; - bool autodetected_; + const ProviderType provider_type_; // The TabContents where this request originated. Can be NULL if the // originating tab is closed. If NULL, the engine is not added. @@ -123,7 +126,7 @@ void TemplateURLFetcher::RequestDelegate::OnURLFetchComplete( void TemplateURLFetcher::RequestDelegate::AddSearchProvider() { DCHECK(template_url_.get()); - if (!autodetected_ || keyword_.empty()) { + if (provider_type_ != AUTODETECTED_PROVIDER || keyword_.empty()) { // Generate new keyword from URL in OSDD for none autodetected case. // Previous keyword was generated from URL where OSDD was placed and // it gives wrong result when OSDD is located on third party site that @@ -140,7 +143,7 @@ void TemplateURLFetcher::RequestDelegate::AddSearchProvider() { !model || !model->loaded() || !model->CanReplaceKeyword(keyword_, GURL(template_url_->url()->url()), &existing_url)) { - if (autodetected_ || !model || !model->loaded()) { + if (provider_type_ == AUTODETECTED_PROVIDER || !model || !model->loaded()) { fetcher_->RequestCompleted(this); // WARNING: RequestCompleted deletes us. return; @@ -166,20 +169,34 @@ void TemplateURLFetcher::RequestDelegate::AddSearchProvider() { if (!template_url_->GetFavIconURL().is_valid()) template_url_->SetFavIconURL(favicon_url_); - if (autodetected_) { - // Mark the keyword as replaceable so it can be removed if necessary. - template_url_->set_safe_for_autoreplace(true); - model->Add(template_url_.release()); - } else if (source_ && source_->delegate()) { - // Confirm addition and allow user to edit default choices. It's ironic - // that only *non*-autodetected additions get confirmed, but the user - // expects feedback that his action did something. - // The source TabContents' delegate takes care of adding the URL to the - // model, which takes ownership, or of deleting it if the add is - // cancelled. - source_->delegate()->ConfirmAddSearchProvider(template_url_.release(), - fetcher_->profile()); + switch (provider_type_) { + case AUTODETECTED_PROVIDER: + // Mark the keyword as replaceable so it can be removed if necessary. + template_url_->set_safe_for_autoreplace(true); + model->Add(template_url_.release()); + break; + + case EXPLICIT_PROVIDER: + if (source_ && source_->delegate()) { + // Confirm addition and allow user to edit default choices. It's ironic + // that only *non*-autodetected additions get confirmed, but the user + // expects feedback that his action did something. + // The source TabContents' delegate takes care of adding the URL to the + // model, which takes ownership, or of deleting it if the add is + // cancelled. + source_->delegate()->ConfirmAddSearchProvider(template_url_.release(), + fetcher_->profile()); + } + break; + + case EXPLICIT_DEFAULT_PROVIDER: + source_->delegate()->ConfirmSetDefaultSearchProvider( + source_, + template_url_.release(), + model); + break; } + fetcher_->RequestCompleted(this); // WARNING: RequestCompleted deletes us. } @@ -197,18 +214,25 @@ void TemplateURLFetcher::ScheduleDownload(const std::wstring& keyword, const GURL& osdd_url, const GURL& favicon_url, TabContents* source, - bool autodetected) { + ProviderType provider_type) { DCHECK(!keyword.empty() && osdd_url.is_valid()); + // Make sure we aren't already downloading this request. for (std::vector<RequestDelegate*>::iterator i = requests_->begin(); i != requests_->end(); ++i) { - if ((*i)->url() == osdd_url || (*i)->keyword() == keyword) + bool keyword_or_osdd_match = (*i)->url() == osdd_url || + (*i)->keyword() == keyword; + bool same_type_or_neither_is_default = + (*i)->provider_type() == provider_type || + ((*i)->provider_type() != EXPLICIT_DEFAULT_PROVIDER && + provider_type != EXPLICIT_DEFAULT_PROVIDER); + if (keyword_or_osdd_match && same_type_or_neither_is_default) return; } requests_->push_back( new RequestDelegate(this, keyword, osdd_url, favicon_url, source, - autodetected)); + provider_type)); } void TemplateURLFetcher::RequestCompleted(RequestDelegate* request) { diff --git a/chrome/browser/search_engines/template_url_fetcher.h b/chrome/browser/search_engines/template_url_fetcher.h index e773890..c254946 100644 --- a/chrome/browser/search_engines/template_url_fetcher.h +++ b/chrome/browser/search_engines/template_url_fetcher.h @@ -20,6 +20,12 @@ class TabContents; // class TemplateURLFetcher { public: + enum ProviderType { + AUTODETECTED_PROVIDER, + EXPLICIT_PROVIDER, // Supplied by Javascript. + EXPLICIT_DEFAULT_PROVIDER // Supplied by Javascript as default provider. + }; + // Creates a TemplateURLFetcher with the specified Profile. explicit TemplateURLFetcher(Profile* profile); ~TemplateURLFetcher(); @@ -31,7 +37,10 @@ class TemplateURLFetcher { const GURL& osdd_url, const GURL& favicon_url, TabContents* source, - bool autodetected); + ProviderType provider_type); + + // The current number of outstanding requests. + int requests_count() const { return requests_->size(); } private: friend class RequestDelegate; diff --git a/chrome/browser/search_engines/template_url_fetcher_unittest.cc b/chrome/browser/search_engines/template_url_fetcher_unittest.cc index 2b4d38eb..57bb88c 100644 --- a/chrome/browser/search_engines/template_url_fetcher_unittest.cc +++ b/chrome/browser/search_engines/template_url_fetcher_unittest.cc @@ -65,6 +65,15 @@ class TemplateURLFetcherTest : public testing::Test { } protected: + // Schedules the download of the url. + void StartDownload(const std::wstring& keyword, + const std::string& osdd_file_name, + TemplateURLFetcher::ProviderType provider_type, + bool check_that_file_exists); + + // Waits for any downloads to finish. + void WaitForDownloadToFinish(); + TemplateURLModelTestUtil test_util_; net::TestServer test_server_; @@ -77,6 +86,33 @@ TemplateURLFetcherTest::TemplateURLFetcherTest() FilePath(FILE_PATH_LITERAL("chrome/test/data"))) { } +void TemplateURLFetcherTest::StartDownload( + const std::wstring& keyword, + const std::string& osdd_file_name, + TemplateURLFetcher::ProviderType provider_type, + bool check_that_file_exists) { + + if (check_that_file_exists) { + FilePath osdd_full_path; + ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &osdd_full_path)); + osdd_full_path = osdd_full_path.AppendASCII(osdd_file_name); + ASSERT_TRUE(file_util::PathExists(osdd_full_path)); + ASSERT_FALSE(file_util::DirectoryExists(osdd_full_path)); + } + + // Start the fetch. + GURL osdd_url = test_server_.GetURL("files/" + osdd_file_name); + GURL favicon_url; + test_util_.profile()->GetTemplateURLFetcher()->ScheduleDownload( + keyword, osdd_url, favicon_url, NULL, provider_type); + ASSERT_EQ(1, test_util_.profile()->GetTemplateURLFetcher()->requests_count()); +} + +void TemplateURLFetcherTest::WaitForDownloadToFinish() { + QuitOnChangedObserver quit_on_changed_observer(test_util_.model()); + MessageLoop::current()->Run(); +} + TEST_F(TemplateURLFetcherTest, BasicTest) { std::wstring keyword(L"test"); @@ -85,24 +121,9 @@ TEST_F(TemplateURLFetcherTest, BasicTest) { ASSERT_FALSE(test_util_.model()->GetTemplateURLForKeyword(keyword)); std::string osdd_file_name("simple_open_search.xml"); - - // Verify that the file exists. - FilePath osdd_full_path; - ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &osdd_full_path)); - osdd_full_path = osdd_full_path.AppendASCII(osdd_file_name); - ASSERT_TRUE(file_util::PathExists(osdd_full_path)); - ASSERT_FALSE(file_util::DirectoryExists(osdd_full_path)); - - // Start the fetch. - GURL osdd_url = test_server_.GetURL("files/" + osdd_file_name); - GURL favicon_url; - test_util_.profile()->GetTemplateURLFetcher()->ScheduleDownload( - keyword, osdd_url, favicon_url, NULL, true); - - { // Wait for the url to be downloaded. - QuitOnChangedObserver quit_on_changed_observer(test_util_.model()); - MessageLoop::current()->Run(); - } + StartDownload(keyword, osdd_file_name, + TemplateURLFetcher::AUTODETECTED_PROVIDER, true); + WaitForDownloadToFinish(); const TemplateURL* t_url = test_util_.model()->GetTemplateURLForKeyword( keyword); @@ -112,3 +133,41 @@ TEST_F(TemplateURLFetcherTest, BasicTest) { EXPECT_EQ(true, t_url->safe_for_autoreplace()); } +TEST_F(TemplateURLFetcherTest, DuplicateThrownAway) { + std::wstring keyword(L"test"); + + test_util_.ChangeModelToLoadState(); + test_util_.ResetObserverCount(); + ASSERT_FALSE(test_util_.model()->GetTemplateURLForKeyword(keyword)); + + std::string osdd_file_name("simple_open_search.xml"); + StartDownload(keyword, osdd_file_name, + TemplateURLFetcher::AUTODETECTED_PROVIDER, true); + + struct { + std::string description; + std::string osdd_file_name; + std::wstring keyword; + TemplateURLFetcher::ProviderType provider_type; + } test_cases[] = { + { "Duplicate keyword and osdd url with autodetected provider.", + osdd_file_name, keyword, TemplateURLFetcher::AUTODETECTED_PROVIDER }, + { "Duplicate keyword and osdd url with explicit provider.", + osdd_file_name, keyword, TemplateURLFetcher::EXPLICIT_PROVIDER }, + { "Duplicate osdd url with explicit provider.", + osdd_file_name, keyword + L"1", TemplateURLFetcher::EXPLICIT_PROVIDER }, + { "Duplicate keyword with explicit provider.", + osdd_file_name + "1", keyword, TemplateURLFetcher::EXPLICIT_PROVIDER } + }; + + for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_cases); ++i) { + StartDownload(test_cases[i].keyword, test_cases[i].osdd_file_name, + test_cases[i].provider_type, false); + ASSERT_EQ( + 1, + test_util_.profile()->GetTemplateURLFetcher()->requests_count()) << + test_cases[i].description; + } + + WaitForDownloadToFinish(); +} diff --git a/chrome/browser/search_engines/template_url_model.cc b/chrome/browser/search_engines/template_url_model.cc index 043633c..aa543fb 100644 --- a/chrome/browser/search_engines/template_url_model.cc +++ b/chrome/browser/search_engines/template_url_model.cc @@ -390,6 +390,13 @@ void TemplateURLModel::ResetTemplateURL(const TemplateURL* url, NotifyObservers(); } +bool TemplateURLModel::CanMakeDefault(const TemplateURL* url) { + return url != GetDefaultSearchProvider() && + url->url() && + url->url()->SupportsReplacement() && + !is_default_search_managed(); +} + void TemplateURLModel::SetDefaultSearchProvider(const TemplateURL* url) { if (is_default_search_managed_) { NOTREACHED(); diff --git a/chrome/browser/search_engines/template_url_model.h b/chrome/browser/search_engines/template_url_model.h index 81cc3bb..ce9a874 100644 --- a/chrome/browser/search_engines/template_url_model.h +++ b/chrome/browser/search_engines/template_url_model.h @@ -171,9 +171,7 @@ class TemplateURLModel : public WebDataServiceConsumer, const std::string& search_url); // Return true if the given |url| can be made the default. - // TODO(levin): Fill in. This is just a placeholder to make the dialog change - // more independent from other changes. - bool CanMakeDefault(const TemplateURL* url) const { return true; } + bool CanMakeDefault(const TemplateURL* url); // Set the default search provider. |url| may be null. // This will assert if the default search is managed; the UI should not be |