diff options
author | pkasting@chromium.org <pkasting@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-04-10 20:17:55 +0000 |
---|---|---|
committer | pkasting@chromium.org <pkasting@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-04-10 20:17:55 +0000 |
commit | 3954c3a76994f8e3a8eec65f4d90580ba8ab0e80 (patch) | |
tree | 638d7ff635e7bda3f037785a2096f36ed7bb21c7 /chrome | |
parent | 6fae79736f73e541b0ec3951adb1c4f83ba7428e (diff) | |
download | chromium_src-3954c3a76994f8e3a8eec65f4d90580ba8ab0e80.zip chromium_src-3954c3a76994f8e3a8eec65f4d90580ba8ab0e80.tar.gz chromium_src-3954c3a76994f8e3a8eec65f4d90580ba8ab0e80.tar.bz2 |
More misc. cleanups to minimize future refactoring diffs:
* Fix indentation
* Slightly more const-correct TemplateURLData functions
* Use EXPECT_XXX() where possible, but ASSERT_XXX() to avoid crashes on expectation failure
* Remove unneeded #include
* Simplify AlternateErrorPageTabObserver slightly by having constructor take a Profile
* Shorter/simpler code
* Fix ordering in some places to be more consistent (e.g. matching member declaration order)
* Add some DCHECK()s to make more explicit some conditions that will become important later
Also, template_url_service.cc has some functional changes:
* GetTemplateURLForXXX() functions now return correct values when asked for the default search URL before the model has successfully loaded. I wound up needing this later to fix some case, but I forget what now; the change seems more correct anyway.
* Similarly, UnregisterExtensionKeyword() now works correctly if called before loading has finished
* RegisterExtensionKeyword() now does nothing when there's already a keyword for that extension, as the comments in the header claim. I'd think this wouldn't actually matter in normal use as I think extension upgrades first unregister the old extension? (I don't remember if I checked that, though)
* Don't try to set an extension keyword as default -- this would be a bad UX as these are inherently async. I don't think this should have been possible anyway except maybe if there was some kind of unintentional overlap between extensions and policy??
* Don't try to save extension keywords to disk or sync them (as the extension system should be responsible for registering them if necessary on startup in both these cases). We already avoided syncing these, I just added checks in more places; I no longer recall if we had code to avoid persisting these to disk, I don't seem to see any at the moment. (sky?)
BUG=none
TEST=none
Review URL: https://chromiumcodereview.appspot.com/10033017
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@131619 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome')
29 files changed, 333 insertions, 278 deletions
diff --git a/chrome/browser/autocomplete/keyword_provider.cc b/chrome/browser/autocomplete/keyword_provider.cc index 98ed031..df2d00e 100644 --- a/chrome/browser/autocomplete/keyword_provider.cc +++ b/chrome/browser/autocomplete/keyword_provider.cc @@ -436,8 +436,7 @@ AutocompleteMatch KeywordProvider::CreateAutocompleteMatch( int relevance) { DCHECK(model); // Get keyword data from data store. - const TemplateURL* element( - model->GetTemplateURLForKeyword(keyword)); + const TemplateURL* element = model->GetTemplateURLForKeyword(keyword); DCHECK(element && !element->url().empty()); const bool supports_replacement = element->url_ref().SupportsReplacement(); diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc index 078947c..90a0309 100644 --- a/chrome/browser/autocomplete/search_provider.cc +++ b/chrome/browser/autocomplete/search_provider.cc @@ -62,14 +62,7 @@ bool HasMultipleWords(const string16& text) { }; -// SearchProvider ------------------------------------------------------------- - -// static -const int SearchProvider::kDefaultProviderURLFetcherID = 1; -// static -const int SearchProvider::kKeywordProviderURLFetcherID = 2; -// static -bool SearchProvider::query_suggest_immediately_ = false; +// SearchProvider::Providers -------------------------------------------------- void SearchProvider::Providers::Set(const TemplateURL* default_provider, const TemplateURL* keyword_provider) { @@ -84,6 +77,16 @@ void SearchProvider::Providers::Set(const TemplateURL* default_provider, cached_keyword_provider_ = *keyword_provider; } + +// SearchProvider ------------------------------------------------------------- + +// static +const int SearchProvider::kDefaultProviderURLFetcherID = 1; +// static +const int SearchProvider::kKeywordProviderURLFetcherID = 2; +// static +bool SearchProvider::query_suggest_immediately_ = false; + SearchProvider::SearchProvider(ACProviderListener* listener, Profile* profile) : AutocompleteProvider(listener, profile, "Search"), suggest_results_pending_(0), @@ -242,15 +245,16 @@ void SearchProvider::Run() { // Start a new request with the current input. DCHECK(!done_); suggest_results_pending_ = 0; - if (providers_.valid_suggest_for_keyword_provider()) { - suggest_results_pending_++; - keyword_fetcher_.reset(CreateSuggestFetcher(kKeywordProviderURLFetcherID, - providers_.keyword_provider(), keyword_input_text_)); - } if (providers_.valid_suggest_for_default_provider()) { suggest_results_pending_++; default_fetcher_.reset(CreateSuggestFetcher(kDefaultProviderURLFetcherID, - providers_.default_provider(), input_.text())); + providers_.default_provider().suggestions_url_ref(), input_.text())); + } + if (providers_.valid_suggest_for_keyword_provider()) { + suggest_results_pending_++; + keyword_fetcher_.reset(CreateSuggestFetcher(kKeywordProviderURLFetcherID, + providers_.keyword_provider().suggestions_url_ref(), + keyword_input_text_)); } // We should only get here if we have a suggest url for the keyword or default // providers. @@ -335,14 +339,14 @@ void SearchProvider::DoHistoryQuery(bool minimal_changes) { // require multiple searches and tracking of "single- vs. multi-word" in the // database. int num_matches = kMaxMatches * 5; - if (providers_.valid_keyword_provider()) { - url_db->GetMostRecentKeywordSearchTerms(providers_.keyword_provider().id(), - keyword_input_text_, num_matches, &keyword_history_results_); - } if (providers_.valid_default_provider()) { url_db->GetMostRecentKeywordSearchTerms(providers_.default_provider().id(), input_.text(), num_matches, &default_history_results_); } + if (providers_.valid_keyword_provider()) { + url_db->GetMostRecentKeywordSearchTerms(providers_.keyword_provider().id(), + keyword_input_text_, num_matches, &keyword_history_results_); + } } void SearchProvider::StartOrStopSuggestQuery(bool minimal_changes) { @@ -386,11 +390,11 @@ void SearchProvider::StartOrStopSuggestQuery(bool minimal_changes) { } bool SearchProvider::IsQuerySuitableForSuggest() const { - // Don't run Suggest in incognito mode, the engine doesn't support it, or - // the user has disabled it. + // Don't run Suggest in incognito mode, if the engine doesn't support it, or + // if the user has disabled it. if (profile_->IsOffTheRecord() || - (!providers_.valid_suggest_for_keyword_provider() && - !providers_.valid_suggest_for_default_provider()) || + (!providers_.valid_suggest_for_default_provider() && + !providers_.valid_suggest_for_keyword_provider()) || !profile_->GetPrefs()->GetBoolean(prefs::kSearchSuggestEnabled)) return false; @@ -453,9 +457,8 @@ void SearchProvider::StopSuggest() { content::URLFetcher* SearchProvider::CreateSuggestFetcher( int id, - const TemplateURL& provider, + const TemplateURLRef& suggestions_url, const string16& text) { - const TemplateURLRef& suggestions_url = provider.suggestions_url_ref(); DCHECK(suggestions_url.SupportsReplacement()); content::URLFetcher* fetcher = content::URLFetcher::Create(id, GURL(suggestions_url.ReplaceSearchTermsUsingProfile( @@ -838,8 +841,8 @@ void SearchProvider::AddMatchToMap(const string16& query_string, MatchMap* map) { AutocompleteMatch match(this, relevance, false, type); std::vector<size_t> content_param_offsets; - const TemplateURL& provider = is_keyword ? providers_.keyword_provider() : - providers_.default_provider(); + const TemplateURL& provider = is_keyword ? + providers_.keyword_provider() : providers_.default_provider(); match.template_url = &provider; match.contents.assign(query_string); // We do intra-string highlighting for suggestions - the suggested segment diff --git a/chrome/browser/autocomplete/search_provider.h b/chrome/browser/autocomplete/search_provider.h index 5af9882..188ae3f 100644 --- a/chrome/browser/autocomplete/search_provider.h +++ b/chrome/browser/autocomplete/search_provider.h @@ -119,17 +119,7 @@ class SearchProvider : public AutocompleteProvider, return cached_keyword_provider_; } - // Returns true of the keyword provider is valid. - bool valid_keyword_provider() const { return !!keyword_provider_; } - - // Returns true if the keyword provider is valid and has a valid suggest - // url. - bool valid_suggest_for_keyword_provider() const { - return keyword_provider_ && - !cached_keyword_provider_.suggestions_url().empty(); - } - - // Returns true of the default provider is valid. + // Returns true if the default provider is valid. bool valid_default_provider() const { return !!default_provider_; } // Returns true if the default provider is valid and has a valid suggest @@ -139,6 +129,16 @@ class SearchProvider : public AutocompleteProvider, !cached_default_provider_.suggestions_url().empty(); } + // Returns true if the keyword provider is valid. + bool valid_keyword_provider() const { return !!keyword_provider_; } + + // Returns true if the keyword provider is valid and has a valid suggest + // url. + bool valid_suggest_for_keyword_provider() const { + return keyword_provider_ && + !cached_keyword_provider_.suggestions_url().empty(); + } + // Returns true if |from_keyword_provider| is true, or // the keyword provider is not valid. bool is_primary_provider(bool from_keyword_provider) const { @@ -202,10 +202,11 @@ class SearchProvider : public AutocompleteProvider, void StopSuggest(); // Creates a URLFetcher requesting suggest results from the specified - // |provider|. The caller owns the returned URLFetcher. - content::URLFetcher* CreateSuggestFetcher(int id, - const TemplateURL& provider, - const string16& text); + // |suggestions_url|. The caller owns the returned URLFetcher. + content::URLFetcher* CreateSuggestFetcher( + int id, + const TemplateURLRef& suggestions_url, + const string16& text); // Parses the results from the Suggest server and stores up to kMaxMatches of // them in server_results_. Returns whether parsing succeeded. diff --git a/chrome/browser/autocomplete/search_provider_unittest.cc b/chrome/browser/autocomplete/search_provider_unittest.cc index f2e8bba..e6e3a3d 100644 --- a/chrome/browser/autocomplete/search_provider_unittest.cc +++ b/chrome/browser/autocomplete/search_provider_unittest.cc @@ -200,8 +200,8 @@ void SearchProviderTest::QueryForInputAndSetWYTMatch( return; ASSERT_GE(provider_->matches().size(), 1u); EXPECT_TRUE(FindMatchWithDestination(GURL( - default_t_url_->url_ref().ReplaceSearchTerms(text, - TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, string16())), wyt_match)); + default_t_url_->url_ref().ReplaceSearchTerms(text, + TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, string16())), wyt_match)); } void SearchProviderTest::TearDown() { diff --git a/chrome/browser/importer/profile_import_process_messages.h b/chrome/browser/importer/profile_import_process_messages.h index ceaf9e0..0c49109 100644 --- a/chrome/browser/importer/profile_import_process_messages.h +++ b/chrome/browser/importer/profile_import_process_messages.h @@ -265,7 +265,7 @@ struct ParamTraits<TemplateURL*> { return true; } static void Log(const param_type& p, std::string* l) { - l->append("<TemplateURL>"); + l->append("<TemplateURL*>"); } }; diff --git a/chrome/browser/instant/instant_browsertest.cc b/chrome/browser/instant/instant_browsertest.cc index eec6b18..45d38be 100644 --- a/chrome/browser/instant/instant_browsertest.cc +++ b/chrome/browser/instant/instant_browsertest.cc @@ -279,7 +279,8 @@ IN_PROC_BROWSER_TEST_F(InstantTest, MAYBE(OnChangeEvent)) { GetDefaultSearchProvider(); EXPECT_TRUE(default_turl); EXPECT_EQ(default_turl->url_ref().ReplaceSearchTerms(ASCIIToUTF16("defghi"), - 0, string16()), loader()->url().spec()); + TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, string16()), + loader()->url().spec()); // Check that the value is reflected and onchange is called. EXPECT_EQ("true 0 0 1 true d false def false 3 3", diff --git a/chrome/browser/instant/instant_loader.cc b/chrome/browser/instant/instant_loader.cc index e6feafe..dce9349 100644 --- a/chrome/browser/instant/instant_loader.cc +++ b/chrome/browser/instant/instant_loader.cc @@ -1129,7 +1129,8 @@ void InstantLoader::LoadInstantURL(TabContentsWrapper* tab_contents, // TODO(sky): having to use a replaceable url is a bit of a hack here. GURL instant_url( template_url->instant_url_ref().ReplaceSearchTermsUsingProfile( - tab_contents->profile(), string16(), -1, string16())); + tab_contents->profile(), string16(), + TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, string16())); CommandLine* cl = CommandLine::ForCurrentProcess(); if (cl->HasSwitch(switches::kInstantURL)) instant_url = GURL(cl->GetSwitchValueASCII(switches::kInstantURL)); diff --git a/chrome/browser/protector/default_search_provider_change_browsertest.cc b/chrome/browser/protector/default_search_provider_change_browsertest.cc index 4dae17c..9e4fae8 100644 --- a/chrome/browser/protector/default_search_provider_change_browsertest.cc +++ b/chrome/browser/protector/default_search_provider_change_browsertest.cc @@ -25,12 +25,9 @@ using ::testing::Invoke; using ::testing::NiceMock; using ::testing::Return; -namespace protector { - namespace { // Keyword names and URLs used for testing. - const string16 example_info = ASCIIToUTF16("Example.info"); const string16 example_info_long = ASCIIToUTF16("ExampleSearchEngine.info"); const std::string http_example_info = "http://example.info/%s"; @@ -56,6 +53,8 @@ TemplateURL* MakeTemplateURL(const string16& short_name, }; +namespace protector { + class DefaultSearchProviderChangeTest : public InProcessBrowserTest { public: virtual void SetUpOnMainThread() OVERRIDE { 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 de42cc3..57e1564 100644 --- a/chrome/browser/search_engines/search_host_to_urls_map.cc +++ b/chrome/browser/search_engines/search_host_to_urls_map.cc @@ -65,16 +65,13 @@ void SearchHostToURLsMap::UpdateGoogleBaseURLs( // 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(); - host_map_iterator != host_to_urls_map_.end(); ++host_map_iterator) { - const TemplateURLSet& urls = host_map_iterator->second; - for (TemplateURLSet::const_iterator url_set_iterator = urls.begin(); - url_set_iterator != urls.end(); ++url_set_iterator) { - const TemplateURL* t_url = *url_set_iterator; - if (t_url->url_ref().HasGoogleBaseURLs() || - t_url->suggestions_url_ref().HasGoogleBaseURLs()) { - t_urls_using_base_url.push_back(t_url); - } + for (HostToURLsMap::iterator i(host_to_urls_map_.begin()); + i != host_to_urls_map_.end(); ++i) { + for (TemplateURLSet::const_iterator j(i->second.begin()); + j != i->second.end(); ++j) { + if ((*j)->url_ref().HasGoogleBaseURLs() || + (*j)->suggestions_url_ref().HasGoogleBaseURLs()) + t_urls_using_base_url.push_back(*j); } } diff --git a/chrome/browser/search_engines/search_provider_install_data.cc b/chrome/browser/search_engines/search_provider_install_data.cc index 151183e..c3af1aa 100644 --- a/chrome/browser/search_engines/search_provider_install_data.cc +++ b/chrome/browser/search_engines/search_provider_install_data.cc @@ -129,8 +129,7 @@ void GoogleURLObserver::Observe(int type, const content::NotificationDetails& details) { if (type == chrome::NOTIFICATION_GOOGLE_URL_UPDATED) { BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, - base::Bind(&GoogleURLChangeNotifier::OnChange, - change_notifier_.get(), + base::Bind(&GoogleURLChangeNotifier::OnChange, change_notifier_.get(), UIThreadSearchTermsData().GoogleBaseURLValue())); } else { // This must be the death notification. @@ -145,9 +144,8 @@ static bool IsSameOrigin(const GURL& requested_origin, const TemplateURL* template_url, const SearchTermsData& search_terms_data) { DCHECK(requested_origin == requested_origin.GetOrigin()); - return template_url && requested_origin == - TemplateURLService::GenerateSearchURLUsingTermsData( - template_url, + return requested_origin == + TemplateURLService::GenerateSearchURLUsingTermsData(template_url, search_terms_data).GetOrigin(); } diff --git a/chrome/browser/search_engines/template_url.cc b/chrome/browser/search_engines/template_url.cc index 06f3fdf..e12ab2e 100644 --- a/chrome/browser/search_engines/template_url.cc +++ b/chrome/browser/search_engines/template_url.cc @@ -585,7 +585,7 @@ TemplateURLData::TemplateURLData() TemplateURLData::~TemplateURLData() { } -void TemplateURLData::SetKeyword(const string16& keyword) const { +void TemplateURLData::SetKeyword(const string16& keyword) { // Case sensitive keyword matching is confusing. As such, we force all // keywords to be lower case. keyword_ = base::i18n::ToLower(keyword); @@ -597,7 +597,7 @@ const string16& TemplateURLData::keyword(const TemplateURL* t_url) const { return keyword_; } -void TemplateURLData::SetAutogenerateKeyword(bool autogenerate_keyword) const { +void TemplateURLData::SetAutogenerateKeyword(bool autogenerate_keyword) { autogenerate_keyword_ = autogenerate_keyword; if (autogenerate_keyword_) { keyword_.clear(); @@ -709,12 +709,12 @@ void TemplateURL::SetURL(const std::string& url) { void TemplateURL::SetPrepopulateId(int id) { data_.prepopulate_id = id; const bool prepopulated = id > 0; - suggestions_url_ref_.prepopulated_ = prepopulated; url_ref_.prepopulated_ = prepopulated; + suggestions_url_ref_.prepopulated_ = prepopulated; instant_url_ref_.prepopulated_ = prepopulated; } -void TemplateURL::InvalidateCachedValues() const { +void TemplateURL::InvalidateCachedValues() { url_ref_.InvalidateCachedValues(); suggestions_url_ref_.InvalidateCachedValues(); instant_url_ref_.InvalidateCachedValues(); diff --git a/chrome/browser/search_engines/template_url.h b/chrome/browser/search_engines/template_url.h index 3e56c73..f7d8bc0 100644 --- a/chrome/browser/search_engines/template_url.h +++ b/chrome/browser/search_engines/template_url.h @@ -251,7 +251,7 @@ struct TemplateURLData { string16 short_name; // The shortcut for this TemplateURL. May be empty. - void SetKeyword(const string16& keyword) const; + void SetKeyword(const string16& keyword); const string16& keyword(const TemplateURL* t_url) const; // TODO(pkasting): This should only be necessary until we eliminate keyword // autogeneration. @@ -261,7 +261,7 @@ struct TemplateURLData { // consumers should not need this. // NOTE: Calling SetKeyword() turns this back off. Manual and automatic // keywords are mutually exclusive. - void SetAutogenerateKeyword(bool autogenerate_keyword) const; + void SetAutogenerateKeyword(bool autogenerate_keyword); bool autogenerate_keyword() const { return autogenerate_keyword_; } // Ensures that the keyword is generated. Most consumers should not need this @@ -342,7 +342,7 @@ struct TemplateURLData { std::string url_; // TODO(pkasting): These fields will go away soon. - mutable bool autogenerate_keyword_; + bool autogenerate_keyword_; // True if the keyword was generated. This is used to avoid multiple attempts // if generating a keyword failed. mutable bool keyword_generated_; @@ -395,7 +395,6 @@ class TemplateURL { const GURL& originating_url() const { return data_.originating_url; } bool show_in_default_list() const { return data_.show_in_default_list; } - // Returns true if show_in_default_list() is true and this TemplateURL has a // TemplateURLRef that supports replacement. bool ShowInDefaultList() const; @@ -442,7 +441,7 @@ class TemplateURL { void SetPrepopulateId(int id); // Invalidates cached values on this object and its child TemplateURLRefs. - void InvalidateCachedValues() const; + void InvalidateCachedValues(); TemplateURLData data_; TemplateURLRef url_ref_; diff --git a/chrome/browser/search_engines/template_url_fetcher_unittest.cc b/chrome/browser/search_engines/template_url_fetcher_unittest.cc index 9a9712c..448ccd2 100644 --- a/chrome/browser/search_engines/template_url_fetcher_unittest.cc +++ b/chrome/browser/search_engines/template_url_fetcher_unittest.cc @@ -287,7 +287,6 @@ TEST_F(TemplateURLFetcherTest, ExplicitBeforeLoadTest) { TEST_F(TemplateURLFetcherTest, DuplicateKeywordsTest) { string16 keyword(ASCIIToUTF16("test")); - TemplateURLData data; data.short_name = keyword; data.SetKeyword(keyword); diff --git a/chrome/browser/search_engines/template_url_parser.cc b/chrome/browser/search_engines/template_url_parser.cc index 7ae6a91..88b3ba1 100644 --- a/chrome/browser/search_engines/template_url_parser.cc +++ b/chrome/browser/search_engines/template_url_parser.cc @@ -99,8 +99,8 @@ bool IsHTTPRef(const std::string& url) { if (url.empty()) return true; GURL gurl(url); - return (gurl.is_valid() && (gurl.SchemeIs(chrome::kHttpScheme) || - gurl.SchemeIs(chrome::kHttpsScheme))); + return gurl.is_valid() && (gurl.SchemeIs(chrome::kHttpScheme) || + gurl.SchemeIs(chrome::kHttpsScheme)); } } // namespace @@ -312,7 +312,6 @@ TemplateURL* TemplateURLParsingContext::GetTemplateURL( if (suggestion_method_ == TemplateURLParsingContext::POST) data_.suggestions_url.clear(); - // Give this a keyword to facilitate tab-to-search. string16 keyword(TemplateURLService::GenerateKeyword(url, false)); DCHECK(!keyword.empty()); data_.SetKeyword(keyword); diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc index 6667f84..abdd548 100644 --- a/chrome/browser/search_engines/template_url_service.cc +++ b/chrome/browser/search_engines/template_url_service.cc @@ -254,9 +254,8 @@ bool TemplateURLService::CanReplaceKeyword( // be replaced. We do this to ensure that if the user assigns a different // keyword to a generated TemplateURL, we won't regenerate another keyword for // the same host. - if (url.is_valid() && !url.host().empty()) - return CanReplaceKeywordForHost(url.host(), template_url_to_replace); - return true; + return !url.is_valid() || url.host().empty() || + CanReplaceKeywordForHost(url.host(), template_url_to_replace); } void TemplateURLService::FindMatchingKeywords( @@ -295,23 +294,36 @@ const TemplateURL* TemplateURLService::GetTemplateURLForKeyword( const string16& keyword) const { KeywordToTemplateMap::const_iterator elem( keyword_to_template_map_.find(keyword)); - return (elem == keyword_to_template_map_.end()) ? NULL : elem->second; + if (elem != keyword_to_template_map_.end()) + return elem->second; + return (initial_default_search_provider_.get() && + (initial_default_search_provider_->keyword() == keyword)) ? + initial_default_search_provider_.get() : NULL; } const TemplateURL* TemplateURLService::GetTemplateURLForGUID( const std::string& sync_guid) const { GUIDToTemplateMap::const_iterator elem( guid_to_template_map_.find(sync_guid)); - return (elem == guid_to_template_map_.end()) ? NULL : elem->second; + if (elem != guid_to_template_map_.end()) + return elem->second; + return (initial_default_search_provider_.get() && + (initial_default_search_provider_->sync_guid() == sync_guid)) ? + initial_default_search_provider_.get() : NULL; } const TemplateURL* TemplateURLService::GetTemplateURLForHost( const std::string& host) const { - return provider_map_.GetTemplateURLForHost(host); + const TemplateURL* t_url = provider_map_.GetTemplateURLForHost(host); + if (t_url) + return t_url; + return (initial_default_search_provider_.get() && + (GenerateSearchURL(initial_default_search_provider_.get()).host() == + host)) ? initial_default_search_provider_.get() : NULL; } void TemplateURLService::Add(TemplateURL* template_url) { - AddNoNotify(template_url); + AddNoNotify(template_url, true); NotifyObservers(); } @@ -375,33 +387,30 @@ void TemplateURLService::RegisterExtensionKeyword(const Extension* extension) { return; } - const TemplateURL* existing_url = GetTemplateURLForExtension(extension); - string16 keyword = UTF8ToUTF16(extension->omnibox_keyword()); - - TemplateURLData data; - data.short_name = UTF8ToUTF16(extension->name()); - data.SetKeyword(UTF8ToUTF16(extension->omnibox_keyword())); - // This URL is not actually used for navigation. It holds the extension's - // ID, as well as forcing the TemplateURL to be treated as a search keyword. - data.SetURL(std::string(chrome::kExtensionScheme) + "://" + extension->id() + - "/?q={searchTerms}"); - scoped_ptr<TemplateURL> template_url(new TemplateURL(data)); - - if (existing_url) { - // TODO(mpcomplete): only replace if the user hasn't changed the keyword. - // (We don't have UI for that yet). - UpdateNoNotify(existing_url, *template_url); - } else { - AddNoNotify(template_url.release()); + if (!GetTemplateURLForExtension(extension)) { + TemplateURLData data; + data.short_name = UTF8ToUTF16(extension->name()); + data.SetKeyword(UTF8ToUTF16(extension->omnibox_keyword())); + // This URL is not actually used for navigation. It holds the extension's + // ID, as well as forcing the TemplateURL to be treated as a search keyword. + data.SetURL(std::string(chrome::kExtensionScheme) + "://" + + extension->id() + "/?q={searchTerms}"); + Add(new TemplateURL(data)); } - NotifyObservers(); } void TemplateURLService::UnregisterExtensionKeyword( const Extension* extension) { - const TemplateURL* url = GetTemplateURLForExtension(extension); - if (url) - Remove(url); + if (loaded_) { + const TemplateURL* url = GetTemplateURLForExtension(extension); + if (url) + Remove(url); + } else { + PendingExtensionIDs::iterator i = std::find(pending_extension_ids_.begin(), + pending_extension_ids_.end(), extension->id()); + if (i != pending_extension_ids_.end()) + pending_extension_ids_.erase(i); + } } const TemplateURL* TemplateURLService::GetTemplateURLForExtension( @@ -416,7 +425,8 @@ const TemplateURL* TemplateURLService::GetTemplateURLForExtension( return NULL; } -std::vector<const TemplateURL*> TemplateURLService::GetTemplateURLs() const { +TemplateURLService::TemplateURLVector + TemplateURLService::GetTemplateURLs() const { return template_urls_; } @@ -424,8 +434,11 @@ void TemplateURLService::IncrementUsageCount(const TemplateURL* url) { DCHECK(url && std::find(template_urls_.begin(), template_urls_.end(), url) != template_urls_.end()); ++const_cast<TemplateURL*>(url)->data_.usage_count; - if (service_.get()) - service_->UpdateKeyword(*url); + // Extension keywords are not persisted. + // TODO(mpcomplete): If we allow editing extension keywords, then those should + // be persisted to disk and synced. + if (service_.get() && !url->IsExtensionKeyword()) + service_.get()->UpdateKeyword(*url); } void TemplateURLService::ResetTemplateURL(const TemplateURL* url, @@ -453,10 +466,10 @@ bool TemplateURLService::CanMakeDefault(const TemplateURL* url) { } void TemplateURLService::SetDefaultSearchProvider(const TemplateURL* url) { - if (is_default_search_managed_) { - NOTREACHED(); - return; - } + DCHECK(!is_default_search_managed_); + // Extension keywords cannot be made default, as they are inherently async. + DCHECK(!url || !url->IsExtensionKeyword()); + // Always persist the setting in the database, that way if the backup // signature has changed out from under us it gets reset correctly. SetDefaultSearchProviderNoNotify(url); @@ -472,7 +485,7 @@ const TemplateURL* TemplateURLService::GetDefaultSearchProvider() { } const TemplateURL* TemplateURLService::FindNewDefaultSearchProvider() { - // See if the prepoluated default still exists. + // See if the prepopulated default still exists. scoped_ptr<TemplateURL> prepopulated_default( TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(GetPrefs())); for (TemplateURLVector::iterator i = template_urls_.begin(); @@ -480,8 +493,13 @@ const TemplateURL* TemplateURLService::FindNewDefaultSearchProvider() { if ((*i)->prepopulate_id() == prepopulated_default->prepopulate_id()) return *i; } - // If not, use the first of the templates. - return template_urls_.empty() ? NULL : template_urls_[0]; + // If not, use the first non-extension keyword of the templates. + for (TemplateURLVector::const_iterator i(template_urls_.begin()); + i != template_urls_.end(); ++i) { + if (!(*i)->IsExtensionKeyword()) + return *i; + } + return NULL; } void TemplateURLService::AddObserver(TemplateURLServiceObserver* observer) { @@ -531,12 +549,8 @@ void TemplateURLService::OnWebDataServiceRequestDone( std::vector<TemplateURL*> template_urls; const TemplateURL* default_search_provider = NULL; int new_resource_keyword_version = 0; - GetSearchProvidersUsingKeywordResult(*result, - service_.get(), - GetPrefs(), - &template_urls, - &default_search_provider, - &new_resource_keyword_version); + GetSearchProvidersUsingKeywordResult(*result, service_.get(), GetPrefs(), + &template_urls, &default_search_provider, &new_resource_keyword_version); bool database_specified_a_default = (default_search_provider != NULL); @@ -584,12 +598,14 @@ void TemplateURLService::OnWebDataServiceRequestDone( data.created_by_policy = true; data.id = kInvalidTemplateURLID; TemplateURL* managed_default = new TemplateURL(data); - AddNoNotify(managed_default); + AddNoNotify(managed_default, true); default_search_provider = managed_default; } } // Note that this saves the default search provider to prefs. - SetDefaultSearchProviderNoNotify(default_search_provider); + if (!default_search_provider || + !default_search_provider->IsExtensionKeyword()) + SetDefaultSearchProviderNoNotify(default_search_provider); } else { // If we had a managed default, replace it with the synced default if // applicable, or the first provider of the list. @@ -598,9 +614,14 @@ void TemplateURLService::OnWebDataServiceRequestDone( default_search_provider = synced_default; pending_synced_default_search_ = false; } else if (database_specified_a_default && - default_search_provider == NULL && - !template_urls.empty()) { - default_search_provider = template_urls[0]; + default_search_provider == NULL) { + for (std::vector<TemplateURL*>::const_iterator i = template_urls.begin(); + i != template_urls.end(); ++i) { + if (!(*i)->IsExtensionKeyword()) { + default_search_provider = *i; + break; + } + } } // If the default search provider existed previously, then just @@ -740,6 +761,8 @@ SyncDataList TemplateURLService::GetAllSyncData( for (TemplateURLVector::const_iterator iter = template_urls_.begin(); iter != template_urls_.end(); ++iter) { // We don't sync extension keywords. + // TODO(mpcomplete): If we allow editing extension keywords, then those + // should be persisted to disk and synced. if ((*iter)->IsExtensionKeyword()) continue; // We don't sync keywords managed by policy. @@ -904,11 +927,9 @@ SyncError TemplateURLService::MergeDataAndStartSyncing( const TemplateURL* dupe_turl = FindDuplicateOfSyncTemplateURL(*sync_turl); if (dupe_turl) { // Merge duplicates and remove the processed local TURL from the map. - TemplateURL* modifiable_dupe_turl = - const_cast<TemplateURL*>(dupe_turl); std::string old_guid = dupe_turl->sync_guid(); MergeSyncAndLocalURLDuplicates(sync_turl.release(), - modifiable_dupe_turl, + const_cast<TemplateURL*>(dupe_turl), &new_changes); local_data_map.erase(old_guid); } else { @@ -963,6 +984,8 @@ void TemplateURLService::ProcessTemplateURLChange( return; // These are changes originating from us. Ignore. // Avoid syncing Extension keywords. + // TODO(mpcomplete): If we allow editing extension keywords, then those should + // be persisted to disk and synced. if (turl->IsExtensionKeyword()) return; @@ -1032,17 +1055,19 @@ TemplateURL* TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData( data.created_by_policy = existing_turl->created_by_policy(); data.usage_count = existing_turl->usage_count(); } - return new TemplateURL(data); + + TemplateURL* turl = new TemplateURL(data); + DCHECK(!turl->IsExtensionKeyword()); + return turl; } // static SyncDataMap TemplateURLService::CreateGUIDToSyncDataMap( const SyncDataList& sync_data) { SyncDataMap data_map; - SyncDataList::const_iterator iter; - for (iter = sync_data.begin(); iter != sync_data.end(); ++iter) { - data_map[iter->GetSpecifics().search_engine().sync_guid()] = *iter; - } + for (SyncDataList::const_iterator i(sync_data.begin()); i != sync_data.end(); + ++i) + data_map[i->GetSpecifics().search_engine().sync_guid()] = *i; return data_map; } @@ -1097,7 +1122,7 @@ void TemplateURLService::Init(const Initializer* initializers, data.short_name = UTF8ToUTF16(initializers[i].content); data.SetKeyword(UTF8ToUTF16(initializers[i].keyword)); data.SetURL(osd_url); - AddNoNotify(new TemplateURL(data)); + AddNoNotify(new TemplateURL(data), true); } } @@ -1107,7 +1132,7 @@ void TemplateURLService::Init(const Initializer* initializers, // Request a server check for the correct Google URL if Google is the // default search engine, not in headless mode and not in Chrome Frame. if (initial_default_search_provider_.get() && - initial_default_search_provider_->url_ref().HasGoogleBaseURLs()) { + initial_default_search_provider_->url_ref().HasGoogleBaseURLs()) { scoped_ptr<base::Environment> env(base::Environment::Create()); if (!env->HasVar(env_vars::kHeadless) && !CommandLine::ForCurrentProcess()->HasSwitch(switches::kChromeFrame)) @@ -1118,6 +1143,14 @@ void TemplateURLService::Init(const Initializer* initializers, void TemplateURLService::RemoveFromMaps(const TemplateURL* template_url) { if (!template_url->keyword().empty()) keyword_to_template_map_.erase(template_url->keyword()); + + // If the keyword we're removing is from an extension, we're now done, since + // it won't be synced or stored in the provider map. + // TODO(mpcomplete): If we allow editing extension keywords, then those should + // be synced. + if (template_url->IsExtensionKeyword()) + return; + if (!template_url->sync_guid().empty()) guid_to_template_map_.erase(template_url->sync_guid()); if (loaded_) @@ -1141,6 +1174,15 @@ void TemplateURLService::RemoveFromKeywordMapByPointer( void TemplateURLService::AddToMaps(const TemplateURL* template_url) { if (!template_url->keyword().empty()) keyword_to_template_map_[template_url->keyword()] = template_url; + + // Extension keywords are not synced, so they don't go into the GUID map, + // and do not use host-based search URLs, so they don't go into the provider + // map, so at this point we're done. + // TODO(mpcomplete): If we allow editing extension keywords, then those should + // be persisted to disk and synced. + if (template_url->IsExtensionKeyword()) + return; + if (!template_url->sync_guid().empty()) guid_to_template_map_[template_url->sync_guid()] = template_url; if (loaded_) { @@ -1156,21 +1198,18 @@ void TemplateURLService::SetTemplateURLs( // First, add the items that already have id's, so that the next_id_ // gets properly set. for (std::vector<TemplateURL*>::const_iterator i = urls.begin(); - i != urls.end(); - ++i) { + i != urls.end(); ++i) { if ((*i)->id() != kInvalidTemplateURLID) { next_id_ = std::max(next_id_, (*i)->id()); - AddToMaps(*i); - template_urls_.push_back(*i); + AddNoNotify(*i, false); } } // Next add the new items that don't have id's. for (std::vector<TemplateURL*>::const_iterator i = urls.begin(); - i != urls.end(); - ++i) { + i != urls.end(); ++i) { if ((*i)->id() == kInvalidTemplateURLID) - AddNoNotify(*i); + AddNoNotify(*i, true); } } @@ -1215,6 +1254,7 @@ void TemplateURLService::SaveDefaultSearchProviderToPrefs( std::string id_string; std::string prepopulate_id; if (t_url) { + DCHECK(!t_url->IsExtensionKeyword()); enabled = true; search_url = t_url->url(); suggest_url = t_url->suggestions_url(); @@ -1298,28 +1338,25 @@ bool TemplateURLService::LoadDefaultSearchProviderFromPrefs( data.prepopulate_id = value; } default_provider->reset(new TemplateURL(data)); + DCHECK(!(*default_provider)->IsExtensionKeyword()); return true; } bool TemplateURLService::CanReplaceKeywordForHost( const std::string& host, const TemplateURL** to_replace) { + DCHECK(!to_replace || !*to_replace); const TemplateURLSet* urls = provider_map_.GetURLsForHost(host); - if (urls) { - for (TemplateURLSet::const_iterator i = urls->begin(); - i != urls->end(); ++i) { - const TemplateURL* url = *i; - if (CanReplace(url)) { - if (to_replace) - *to_replace = url; - return true; - } + if (!urls) + return true; + for (TemplateURLSet::const_iterator i(urls->begin()); i != urls->end(); ++i) { + if (CanReplace(*i)) { + if (to_replace) + *to_replace = *i; + return true; } } - - if (to_replace) - *to_replace = NULL; - return !urls; + return false; } bool TemplateURLService::CanReplace(const TemplateURL* t_url) { @@ -1334,8 +1371,14 @@ void TemplateURLService::UpdateNoNotify(const TemplateURL* existing_turl, DCHECK(std::find(template_urls_.begin(), template_urls_.end(), existing_turl) != template_urls_.end()); - if (!existing_turl->keyword().empty()) - keyword_to_template_map_.erase(existing_turl->keyword()); + // TODO(mpcomplete): If we allow editing extension keywords, then those should + // be persisted to disk and synced. In this case this DCHECK should be + // removed. + DCHECK(!existing_turl->IsExtensionKeyword()); + + string16 old_keyword(existing_turl->keyword()); + if (!old_keyword.empty()) + keyword_to_template_map_.erase(old_keyword); if (!existing_turl->sync_guid().empty()) guid_to_template_map_.erase(existing_turl->sync_guid()); @@ -1347,8 +1390,9 @@ void TemplateURLService::UpdateNoNotify(const TemplateURL* existing_turl, UIThreadSearchTermsData search_terms_data; provider_map_.Add(existing_turl, search_terms_data); - if (!existing_turl->keyword().empty()) - keyword_to_template_map_[existing_turl->keyword()] = existing_turl; + const string16& keyword = existing_turl->keyword(); + if (!keyword.empty()) + keyword_to_template_map_[keyword] = existing_turl; if (!existing_turl->sync_guid().empty()) guid_to_template_map_[existing_turl->sync_guid()] = existing_turl; @@ -1414,12 +1458,10 @@ void TemplateURLService::UpdateKeywordSearchTermsForURL( AddTabToSearchVisit(**i); } - QueryTerms::iterator terms_iterator = - query_terms.find(search_ref.GetSearchTermKey()); - if (terms_iterator != query_terms.end() && - !terms_iterator->second.empty()) { + QueryTerms::iterator j(query_terms.find(search_ref.GetSearchTermKey())); + if (j != query_terms.end() && !j->second.empty()) { SetKeywordSearchTermsForURL(*i, row.url(), - search_ref.SearchTermToString16(terms_iterator->second)); + search_ref.SearchTermToString16(j->second)); } } } @@ -1487,15 +1529,18 @@ bool TemplateURLService::BuildQueryTerms(const GURL& url, void TemplateURLService::GoogleBaseURLChanged() { bool something_changed = false; - for (size_t i = 0; i < template_urls_.size(); ++i) { - const TemplateURL* t_url = template_urls_[i]; + for (TemplateURLVector::iterator i(template_urls_.begin()); + i != template_urls_.end(); ++i) { + TemplateURL* t_url = const_cast<TemplateURL*>(*i); if (t_url->url_ref().HasGoogleBaseURLs() || t_url->suggestions_url_ref().HasGoogleBaseURLs()) { - RemoveFromKeywordMapByPointer(t_url); - t_url->InvalidateCachedValues(); - if (!t_url->keyword().empty()) - keyword_to_template_map_[t_url->keyword()] = t_url; something_changed = true; + string16 original_keyword(t_url->keyword()); + t_url->InvalidateCachedValues(); + const string16& new_keyword(t_url->keyword()); + RemoveFromKeywordMapByPointer(t_url); + if (!new_keyword.empty()) + keyword_to_template_map_[new_keyword] = t_url; } } @@ -1555,14 +1600,12 @@ void TemplateURLService::UpdateDefaultSearch() { TemplateURL new_values(data); UpdateNoNotify(default_search_provider_, new_values); } else { - // AddNoNotify will take ownership of new_template, so it's safe to - // release. TemplateURL* new_template = NULL; if (new_default_from_prefs.get()) { TemplateURLData data(new_default_from_prefs->data()); data.created_by_policy = true; new_template = new TemplateURL(data); - AddNoNotify(new_template); + AddNoNotify(new_template, true); } SetDefaultSearchProviderNoNotify(new_template); } @@ -1570,14 +1613,12 @@ void TemplateURLService::UpdateDefaultSearch() { // The default used to be unmanaged and is now managed. Add the new // managed default to the list of URLs and set it as default. is_default_search_managed_ = new_is_default_managed; - // AddNoNotify will take ownership of new_template, so it's safe to - // release. TemplateURL* new_template = NULL; if (new_default_from_prefs.get()) { TemplateURLData data(new_default_from_prefs->data()); data.created_by_policy = true; new_template = new TemplateURL(data); - AddNoNotify(new_template); + AddNoNotify(new_template, true); } SetDefaultSearchProviderNoNotify(new_template); } else { @@ -1606,8 +1647,13 @@ void TemplateURLService::UpdateDefaultSearch() { void TemplateURLService::SetDefaultSearchProviderNoNotify( const TemplateURL* url) { - DCHECK(!url || std::find(template_urls_.begin(), template_urls_.end(), url) != - template_urls_.end()); + if (url) { + DCHECK(std::find(template_urls_.begin(), template_urls_.end(), url) != + template_urls_.end()); + // Extension keywords cannot be made default, as they're inherently async. + DCHECK(!url->IsExtensionKeyword()); + } + default_search_provider_ = url; if (url) { @@ -1650,21 +1696,32 @@ void TemplateURLService::SetDefaultSearchProviderNoNotify( ProcessTemplateURLChange(url, SyncChange::ACTION_UPDATE); } -void TemplateURLService::AddNoNotify(TemplateURL* template_url) { +void TemplateURLService::AddNoNotify(TemplateURL* template_url, + bool newly_adding) { DCHECK(template_url); - DCHECK_EQ(kInvalidTemplateURLID, template_url->id()); - DCHECK(std::find(template_urls_.begin(), template_urls_.end(), - template_url) == template_urls_.end()); - template_url->data_.id = ++next_id_; + + if (newly_adding) { + DCHECK_EQ(kInvalidTemplateURLID, template_url->id()); + DCHECK(std::find(template_urls_.begin(), template_urls_.end(), + template_url) == template_urls_.end()); + template_url->data_.id = ++next_id_; + } + template_urls_.push_back(template_url); AddToMaps(template_url); - if (service_.get()) - service_->AddKeyword(*template_url); - - // Inform sync of the addition. Note that this will assign a GUID to - // template_url and add it to the guid_to_template_map_. - ProcessTemplateURLChange(template_url, SyncChange::ACTION_ADD); + if (newly_adding) { + // Don't persist extension keywords to disk. They'll get re-added on each + // launch as the extensions are loaded. + // TODO(mpcomplete): If we allow editing extension keywords, then those + // should be persisted to disk and synced. + if (service_.get() && !template_url->IsExtensionKeyword()) + service_->AddKeyword(*template_url); + + // Inform sync of the addition. Note that this will assign a GUID to + // template_url and add it to the guid_to_template_map_. + ProcessTemplateURLChange(template_url, SyncChange::ACTION_ADD); + } } void TemplateURLService::RemoveNoNotify(const TemplateURL* template_url) { @@ -1684,7 +1741,10 @@ void TemplateURLService::RemoveNoNotify(const TemplateURL* template_url) { // Remove it from the vector containing all TemplateURLs. template_urls_.erase(i); - if (service_.get()) + // Extension keywords are not persisted. + // TODO(mpcomplete): If we allow editing extension keywords, then those should + // be persisted to disk and synced. + if (service_.get() && !template_url->IsExtensionKeyword()) service_->RemoveKeyword(template_url->id()); // Inform sync of the deletion. @@ -1726,7 +1786,7 @@ void TemplateURLService::RemoveProvidersCreatedByPolicy( DCHECK(template_urls); DCHECK(default_search_provider); for (std::vector<TemplateURL*>::iterator i = template_urls->begin(); - i != template_urls->end(); ) { + i != template_urls->end(); ) { TemplateURL* template_url = *i; if (template_url->created_by_policy()) { if (template_url == *default_search_provider && @@ -1749,7 +1809,10 @@ void TemplateURLService::RemoveProvidersCreatedByPolicy( *default_search_provider = NULL; i = template_urls->erase(i); - if (service_.get()) + // Extension keywords are not persisted. + // TODO(mpcomplete): If we allow editing extension keywords, then those + // should be persisted to disk and synced. + if (service_.get() && !template_url->IsExtensionKeyword()) service_->RemoveKeyword(template_url->id()); delete template_url; } else { @@ -1775,16 +1838,17 @@ string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl) const { // First, try to return the generated keyword for the TemplateURL. GURL gurl(turl.url()); - string16 keyword_candidate = GenerateKeyword(gurl, true); - if (!GetTemplateURLForKeyword(keyword_candidate) && - !keyword_candidate.empty()) { - return keyword_candidate; + if (gurl.is_valid()) { + string16 keyword_candidate = GenerateKeyword(gurl, true); + if (!GetTemplateURLForKeyword(keyword_candidate) && + !keyword_candidate.empty()) + return keyword_candidate; } // We try to uniquify the keyword by appending a special character to the end. // This is a best-effort approach where we try to preserve the original // keyword and let the user do what they will after our attempt. - keyword_candidate = turl.keyword(); + string16 keyword_candidate(turl.keyword()); do { keyword_candidate.append(ASCIIToUTF16("_")); } while (GetTemplateURLForKeyword(keyword_candidate)); @@ -1828,14 +1892,8 @@ const TemplateURL* TemplateURLService::FindDuplicateOfSyncTemplateURL( const TemplateURL& sync_turl) { const TemplateURL* existing_turl = GetTemplateURLForKeyword(sync_turl.keyword()); - if (!existing_turl) - return NULL; - - if (!existing_turl->url().empty() && - existing_turl->url() == sync_turl.url()) { - return existing_turl; - } - return NULL; + return existing_turl && !existing_turl->url().empty() && + (existing_turl->url() == sync_turl.url()) ? existing_turl : NULL; } void TemplateURLService::MergeSyncAndLocalURLDuplicates( @@ -1909,7 +1967,11 @@ void TemplateURLService::PatchMissingSyncGUIDs( i != template_urls->end(); ++i) { TemplateURL* template_url = *i; DCHECK(template_url); - if (template_url->sync_guid().empty()) { + // Extension keywords are never synced. + // TODO(mpcomplete): If we allow editing extension keywords, then those + // should be persisted to disk and synced. + if (template_url->sync_guid().empty() && + !template_url->IsExtensionKeyword()) { template_url->data_.sync_guid = guid::GenerateGUID(); if (service_.get()) service_->UpdateKeyword(*template_url); diff --git a/chrome/browser/search_engines/template_url_service.h b/chrome/browser/search_engines/template_url_service.h index 5fcfe74..2ce807d 100644 --- a/chrome/browser/search_engines/template_url_service.h +++ b/chrome/browser/search_engines/template_url_service.h @@ -406,6 +406,8 @@ class TemplateURLService : public WebDataServiceConsumer, // Updates the information in |existing_turl| using the information from // |new_values|, but the ID for |existing_turl| is retained. // Notifying observers is the responsibility of the caller. + // NOTE: This should not be called with an extension keyword as there are no + // updates needed in that case. void UpdateNoNotify(const TemplateURL* existing_turl, const TemplateURL& new_values); @@ -444,8 +446,11 @@ class TemplateURLService : public WebDataServiceConsumer, // Adds a new TemplateURL to this model. TemplateURLService will own the // reference, and delete it when the TemplateURL is removed. + // If |newly_adding| is false, we assume that this TemplateURL was already + // part of the model in the past, and therefore we don't need to do things + // like assign it an ID or notify sync. // Caller is responsible for notifying observers. - void AddNoNotify(TemplateURL* template_url); + void AddNoNotify(TemplateURL* template_url, bool newly_adding); // Removes the keyword from the model. This deletes the supplied TemplateURL. // This fails if the supplied template_url is the default search provider. diff --git a/chrome/browser/search_engines/template_url_service_sync_unittest.cc b/chrome/browser/search_engines/template_url_service_sync_unittest.cc index 2cbe233..e07beb7 100644 --- a/chrome/browser/search_engines/template_url_service_sync_unittest.cc +++ b/chrome/browser/search_engines/template_url_service_sync_unittest.cc @@ -791,10 +791,10 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) { // retains the original keyword. The local copy should get a uniquified // keyword. Both TemplateURLs should be found locally. const TemplateURL* key2_sync = model()->GetTemplateURLForGUID("key2"); - EXPECT_TRUE(key2_sync); + ASSERT_TRUE(key2_sync); EXPECT_EQ(ASCIIToUTF16("key2"), key2_sync->keyword()); const TemplateURL* key2_local = model()->GetTemplateURLForGUID("bbb"); - EXPECT_TRUE(key2_local); + ASSERT_TRUE(key2_local); EXPECT_EQ(ASCIIToUTF16("expected.com"), key2_local->keyword()); // The last TemplateURL should have had no conflicts and was just added. It diff --git a/chrome/browser/search_engines/template_url_service_test_util.cc b/chrome/browser/search_engines/template_url_service_test_util.cc index 4388e7f..53d09f9 100644 --- a/chrome/browser/search_engines/template_url_service_test_util.cc +++ b/chrome/browser/search_engines/template_url_service_test_util.cc @@ -227,7 +227,7 @@ void TemplateURLServiceTestUtil::SetGoogleBaseURL( } TemplateURLService* TemplateURLServiceTestUtil::model() const { - return TemplateURLServiceFactory::GetForProfile(profile()); + return TemplateURLServiceFactory::GetForProfile(profile_.get()); } TestingProfile* TemplateURLServiceTestUtil::profile() const { diff --git a/chrome/browser/search_engines/template_url_service_unittest.cc b/chrome/browser/search_engines/template_url_service_unittest.cc index 553ebfb..31c7be6 100644 --- a/chrome/browser/search_engines/template_url_service_unittest.cc +++ b/chrome/browser/search_engines/template_url_service_unittest.cc @@ -491,6 +491,9 @@ TEST_F(TemplateURLServiceTest, GenerateKeyword) { // Make sure we don't get a trailing / ASSERT_EQ(ASCIIToUTF16("blah"), TemplateURLService::GenerateKeyword(GURL("http://blah/"), true)); + // Don't generate the empty string. + ASSERT_EQ(ASCIIToUTF16("www"), + TemplateURLService::GenerateKeyword(GURL("http://www."), false)); } TEST_F(TemplateURLServiceTest, GenerateSearchURL) { @@ -795,11 +798,11 @@ TEST_F(TemplateURLServiceTest, DefaultSearchProviderLoadedFromPrefs) { // value are persisted to prefs. const TemplateURL* default_turl = model()->GetDefaultSearchProvider(); ASSERT_TRUE(default_turl); - ASSERT_EQ("http://url", default_turl->url()); - ASSERT_EQ("http://url2", default_turl->suggestions_url()); + EXPECT_EQ(ASCIIToUTF16("a"), default_turl->short_name()); + EXPECT_EQ("http://url", default_turl->url()); + EXPECT_EQ("http://url2", default_turl->suggestions_url()); EXPECT_EQ("http://instant", default_turl->instant_url()); - ASSERT_EQ(ASCIIToUTF16("a"), default_turl->short_name()); - ASSERT_EQ(id, default_turl->id()); + EXPECT_EQ(id, default_turl->id()); // Now do a load and make sure the default search provider really takes. test_util_.VerifyLoad(); diff --git a/chrome/browser/search_engines/template_url_unittest.cc b/chrome/browser/search_engines/template_url_unittest.cc index 68be193..e9a9769 100644 --- a/chrome/browser/search_engines/template_url_unittest.cc +++ b/chrome/browser/search_engines/template_url_unittest.cc @@ -329,7 +329,7 @@ TEST_F(TemplateURLTest, ReplaceSearchTerms) { data.SetURL(test_data[i].url); TemplateURL url(data); EXPECT_TRUE(url.url_ref().IsValid()); - EXPECT_TRUE(url.url_ref().SupportsReplacement()); + ASSERT_TRUE(url.url_ref().SupportsReplacement()); std::string expected_result = test_data[i].expected_result; ReplaceSubstringsAfterOffset(&expected_result, 0, "{language}", g_browser_process->GetApplicationLocale()); @@ -369,6 +369,8 @@ TEST_F(TemplateURLTest, ReplaceArbitrarySearchTerms) { data.input_encodings.clear(); data.input_encodings.push_back(test_data[i].encoding); TemplateURL url(data); + EXPECT_TRUE(url.url_ref().IsValid()); + ASSERT_TRUE(url.url_ref().SupportsReplacement()); GURL result(url.url_ref().ReplaceSearchTerms(test_data[i].search_term, TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, string16())); ASSERT_TRUE(result.is_valid()); @@ -398,7 +400,7 @@ TEST_F(TemplateURLTest, Suggestions) { "{google:originalQueryForSuggestion}q={searchTerms}"); data.input_encodings.push_back("UTF-8"); TemplateURL url(data); - ASSERT_TRUE(url.url_ref().IsValid()); + EXPECT_TRUE(url.url_ref().IsValid()); ASSERT_TRUE(url.url_ref().SupportsReplacement()); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_data); ++i) { GURL result(url.url_ref().ReplaceSearchTerms(ASCIIToUTF16("foobar"), diff --git a/chrome/browser/sync/test/integration/search_engines_helper.cc b/chrome/browser/sync/test/integration/search_engines_helper.cc index dc54be1..97f0667 100644 --- a/chrome/browser/sync/test/integration/search_engines_helper.cc +++ b/chrome/browser/sync/test/integration/search_engines_helper.cc @@ -54,7 +54,6 @@ std::string GetTURLInfoString(const TemplateURL* turl) { bool TURLsMatch(const TemplateURL* turl1, const TemplateURL* turl2) { CHECK(turl1); CHECK(turl2); - bool result = (turl1->url() == turl2->url()) && (turl1->keyword() == turl2->keyword()) && (turl1->short_name() == turl2->short_name()); @@ -188,9 +187,9 @@ TemplateURL* CreateTestTemplateURL(int seed, TemplateURLData data; data.short_name = CreateKeyword(seed); data.SetKeyword(keyword); - data.safe_for_autoreplace = true; data.SetURL(url); data.favicon_url = GURL("http://favicon.url"); + data.safe_for_autoreplace = true; data.date_created = base::Time::FromTimeT(100); data.last_modified = base::Time::FromTimeT(100); data.prepopulate_id = 999999; diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc index 5519ea0..71cc273 100644 --- a/chrome/browser/tab_contents/render_view_context_menu.cc +++ b/chrome/browser/tab_contents/render_view_context_menu.cc @@ -74,7 +74,6 @@ #include "content/public/common/ssl_status.h" #include "grit/generated_resources.h" #include "net/base/escape.h" -#include "net/base/net_util.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebContextMenuData.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebMediaPlayerAction.h" #include "third_party/WebKit/Source/WebKit/chromium/public/WebPluginAction.h" diff --git a/chrome/browser/ui/alternate_error_tab_observer.cc b/chrome/browser/ui/alternate_error_tab_observer.cc index 9d958b2..e45ebfd 100644 --- a/chrome/browser/ui/alternate_error_tab_observer.cc +++ b/chrome/browser/ui/alternate_error_tab_observer.cc @@ -17,9 +17,11 @@ using content::RenderViewHost; using content::WebContents; AlternateErrorPageTabObserver::AlternateErrorPageTabObserver( - WebContents* web_contents) - : content::WebContentsObserver(web_contents) { - PrefService* prefs = GetProfile()->GetPrefs(); + WebContents* web_contents, + Profile* profile) + : content::WebContentsObserver(web_contents), + profile_(profile) { + PrefService* prefs = profile_->GetPrefs(); if (prefs) { pref_change_registrar_.Init(prefs); pref_change_registrar_.Add(prefs::kAlternateErrorPagesEnabled, this); @@ -34,15 +36,10 @@ AlternateErrorPageTabObserver::~AlternateErrorPageTabObserver() { // static void AlternateErrorPageTabObserver::RegisterUserPrefs(PrefService* prefs) { - prefs->RegisterBooleanPref(prefs::kAlternateErrorPagesEnabled, - true, + prefs->RegisterBooleanPref(prefs::kAlternateErrorPagesEnabled, true, PrefService::SYNCABLE_PREF); } -Profile* AlternateErrorPageTabObserver::GetProfile() const { - return Profile::FromBrowserContext(web_contents()->GetBrowserContext()); -} - //////////////////////////////////////////////////////////////////////////////// // WebContentsObserver overrides @@ -57,24 +54,14 @@ void AlternateErrorPageTabObserver::RenderViewCreated( void AlternateErrorPageTabObserver::Observe(int type, const content::NotificationSource& source, const content::NotificationDetails& details) { - switch (type) { - case chrome::NOTIFICATION_GOOGLE_URL_UPDATED: - UpdateAlternateErrorPageURL(web_contents()->GetRenderViewHost()); - break; - case chrome::NOTIFICATION_PREF_CHANGED: { - std::string* pref_name = content::Details<std::string>(details).ptr(); - DCHECK(content::Source<PrefService>(source).ptr() == - GetProfile()->GetPrefs()); - if (*pref_name == prefs::kAlternateErrorPagesEnabled) { - UpdateAlternateErrorPageURL(web_contents()->GetRenderViewHost()); - } else { - NOTREACHED() << "unexpected pref change notification" << *pref_name; - } - break; - } - default: - NOTREACHED(); + if (type == chrome::NOTIFICATION_PREF_CHANGED) { + DCHECK_EQ(profile_->GetPrefs(), content::Source<PrefService>(source).ptr()); + DCHECK_EQ(std::string(prefs::kAlternateErrorPagesEnabled), + *content::Details<std::string>(details).ptr()); + } else { + DCHECK_EQ(chrome::NOTIFICATION_GOOGLE_URL_UPDATED, type); } + UpdateAlternateErrorPageURL(web_contents()->GetRenderViewHost()); } //////////////////////////////////////////////////////////////////////////////// @@ -83,11 +70,10 @@ void AlternateErrorPageTabObserver::Observe(int type, GURL AlternateErrorPageTabObserver::GetAlternateErrorPageURL() const { GURL url; // Disable alternate error pages when in Incognito mode. - Profile* profile = GetProfile(); - if (profile->IsOffTheRecord()) + if (profile_->IsOffTheRecord()) return url; - if (profile->GetPrefs()->GetBoolean(prefs::kAlternateErrorPagesEnabled)) { + if (profile_->GetPrefs()->GetBoolean(prefs::kAlternateErrorPagesEnabled)) { url = google_util::AppendGoogleLocaleParam( GURL(google_util::kLinkDoctorBaseURL)); url = google_util::AppendGoogleTLDParam(url); diff --git a/chrome/browser/ui/alternate_error_tab_observer.h b/chrome/browser/ui/alternate_error_tab_observer.h index da81c74..74bb804 100644 --- a/chrome/browser/ui/alternate_error_tab_observer.h +++ b/chrome/browser/ui/alternate_error_tab_observer.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -17,15 +17,13 @@ class Profile; class AlternateErrorPageTabObserver : public content::WebContentsObserver, public content::NotificationObserver { public: - explicit AlternateErrorPageTabObserver(content::WebContents* web_contents); + AlternateErrorPageTabObserver(content::WebContents* web_contents, + Profile* profile); virtual ~AlternateErrorPageTabObserver(); static void RegisterUserPrefs(PrefService* prefs); private: - // Helper to return the profile for this tab. - Profile* GetProfile() const; - // content::WebContentsObserver overrides: virtual void RenderViewCreated( content::RenderViewHost* render_view_host) OVERRIDE; @@ -44,6 +42,7 @@ class AlternateErrorPageTabObserver : public content::WebContentsObserver, // Send the alternate error page URL to the renderer. void UpdateAlternateErrorPageURL(content::RenderViewHost* rvh); + Profile* profile_; content::NotificationRegistrar registrar_; PrefChangeRegistrar pref_change_registrar_; diff --git a/chrome/browser/ui/search_engines/edit_search_engine_controller.cc b/chrome/browser/ui/search_engines/edit_search_engine_controller.cc index a5ce1c4..1a82510 100644 --- a/chrome/browser/ui/search_engines/edit_search_engine_controller.cc +++ b/chrome/browser/ui/search_engines/edit_search_engine_controller.cc @@ -76,6 +76,7 @@ void EditSearchEngineController::AcceptAddOrEdit( const string16& title_input, const string16& keyword_input, const std::string& url_input) { + DCHECK(!keyword_input.empty()); std::string url_string = GetFixedUpURL(url_input); DCHECK(!url_string.empty()); diff --git a/chrome/browser/ui/search_engines/keyword_editor_controller.cc b/chrome/browser/ui/search_engines/keyword_editor_controller.cc index 5ede231..18899c8 100644 --- a/chrome/browser/ui/search_engines/keyword_editor_controller.cc +++ b/chrome/browser/ui/search_engines/keyword_editor_controller.cc @@ -54,6 +54,7 @@ void KeywordEditorController::ModifyTemplateURL(const TemplateURL* template_url, const string16& title, const string16& keyword, const std::string& url) { + DCHECK(!url.empty()); const int index = table_model_->IndexOfTemplateURL(template_url); if (index == -1) { // Will happen if url was deleted out from under us while the user was diff --git a/chrome/browser/ui/tab_contents/tab_contents_wrapper.cc b/chrome/browser/ui/tab_contents/tab_contents_wrapper.cc index d6508ba..43e49d3 100644 --- a/chrome/browser/ui/tab_contents/tab_contents_wrapper.cc +++ b/chrome/browser/ui/tab_contents/tab_contents_wrapper.cc @@ -117,7 +117,7 @@ TabContentsWrapper::TabContentsWrapper(WebContents* contents) // Create the per-tab observers. alternate_error_page_tab_observer_.reset( - new AlternateErrorPageTabObserver(contents)); + new AlternateErrorPageTabObserver(contents, profile())); download_request_limiter_observer_.reset( new DownloadRequestLimiterObserver(contents)); webnavigation_observer_.reset( diff --git a/chrome/browser/ui/webui/options2/search_engine_manager_handler2.cc b/chrome/browser/ui/webui/options2/search_engine_manager_handler2.cc index 1f74fc3..f62eeb2 100644 --- a/chrome/browser/ui/webui/options2/search_engine_manager_handler2.cc +++ b/chrome/browser/ui/webui/options2/search_engine_manager_handler2.cc @@ -265,6 +265,7 @@ void SearchEngineManagerHandler::OnEditedKeyword( const string16& title, const string16& keyword, const std::string& url) { + DCHECK(!url.empty()); if (template_url) list_controller_->ModifyTemplateURL(template_url, title, keyword, url); else diff --git a/chrome/browser/webdata/keyword_table_unittest.cc b/chrome/browser/webdata/keyword_table_unittest.cc index cad3a40..edba9d2 100644 --- a/chrome/browser/webdata/keyword_table_unittest.cc +++ b/chrome/browser/webdata/keyword_table_unittest.cc @@ -82,6 +82,7 @@ TEST_F(KeywordTableTest, Keywords) { EXPECT_EQ(keyword.autogenerate_keyword(), restored_keyword.autogenerate_keyword()); EXPECT_EQ(keyword.url(), restored_keyword.url()); + EXPECT_EQ(keyword.suggestions_url, restored_keyword.suggestions_url); EXPECT_EQ(keyword.instant_url, restored_keyword.instant_url); EXPECT_EQ(keyword.favicon_url, restored_keyword.favicon_url); EXPECT_EQ(keyword.originating_url, restored_keyword.originating_url); @@ -112,8 +113,8 @@ TEST_F(KeywordTableTest, KeywordMisc) { ASSERT_EQ(sql::INIT_OK, db.Init(file_)); KeywordTable* keyword_table = db.GetKeywordTable(); - ASSERT_EQ(kInvalidTemplateURLID, keyword_table->GetDefaultSearchProviderID()); - ASSERT_EQ(0, keyword_table->GetBuiltinKeywordVersion()); + EXPECT_EQ(kInvalidTemplateURLID, keyword_table->GetDefaultSearchProviderID()); + EXPECT_EQ(0, keyword_table->GetBuiltinKeywordVersion()); TemplateURLData keyword; keyword.short_name = ASCIIToUTF16("short_name"); @@ -133,13 +134,13 @@ TEST_F(KeywordTableTest, KeywordMisc) { keyword.usage_count = 32; keyword.prepopulate_id = 10; TemplateURL url(keyword); - ASSERT_TRUE(keyword_table->AddKeyword(url)); + EXPECT_TRUE(keyword_table->AddKeyword(url)); - ASSERT_TRUE(keyword_table->SetDefaultSearchProviderID(10)); - ASSERT_TRUE(keyword_table->SetBuiltinKeywordVersion(11)); + EXPECT_TRUE(keyword_table->SetDefaultSearchProviderID(10)); + EXPECT_TRUE(keyword_table->SetBuiltinKeywordVersion(11)); - ASSERT_EQ(10, keyword_table->GetDefaultSearchProviderID()); - ASSERT_EQ(11, keyword_table->GetBuiltinKeywordVersion()); + EXPECT_EQ(10, keyword_table->GetDefaultSearchProviderID()); + EXPECT_EQ(11, keyword_table->GetBuiltinKeywordVersion()); } TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { @@ -162,7 +163,7 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { TemplateURL url(keyword); EXPECT_TRUE(keyword_table->AddKeyword(url)); - ASSERT_TRUE(keyword_table->SetDefaultSearchProviderID(1)); + EXPECT_TRUE(keyword_table->SetDefaultSearchProviderID(1)); EXPECT_TRUE(keyword_table->IsBackupSignatureValid()); EXPECT_EQ(1, keyword_table->GetDefaultSearchProviderID()); @@ -180,7 +181,7 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { EXPECT_FALSE(keyword_table->DidDefaultSearchProviderChange()); // Change the actual setting. - ASSERT_TRUE(keyword_table->meta_table_->SetValue( + EXPECT_TRUE(keyword_table->meta_table_->SetValue( KeywordTable::kDefaultSearchProviderKey, 2)); EXPECT_TRUE(keyword_table->IsBackupSignatureValid()); EXPECT_EQ(2, keyword_table->GetDefaultSearchProviderID()); @@ -197,9 +198,9 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { EXPECT_TRUE(keyword_table->DidDefaultSearchProviderChange()); // Change the backup. - ASSERT_TRUE(keyword_table->meta_table_->SetValue( + EXPECT_TRUE(keyword_table->meta_table_->SetValue( KeywordTable::kDefaultSearchProviderKey, 1)); - ASSERT_TRUE(keyword_table->meta_table_->SetValue( + EXPECT_TRUE(keyword_table->meta_table_->SetValue( KeywordTable::kDefaultSearchIDBackupKey, 2)); EXPECT_FALSE(keyword_table->IsBackupSignatureValid()); EXPECT_EQ(1, keyword_table->GetDefaultSearchProviderID()); @@ -207,9 +208,9 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { EXPECT_TRUE(keyword_table->DidDefaultSearchProviderChange()); // Change the signature. - ASSERT_TRUE(keyword_table->meta_table_->SetValue( + EXPECT_TRUE(keyword_table->meta_table_->SetValue( KeywordTable::kDefaultSearchIDBackupKey, 1)); - ASSERT_TRUE(keyword_table->meta_table_->SetValue( + EXPECT_TRUE(keyword_table->meta_table_->SetValue( KeywordTable::kBackupSignatureKey, std::string())); EXPECT_FALSE(keyword_table->IsBackupSignatureValid()); EXPECT_EQ(1, keyword_table->GetDefaultSearchProviderID()); @@ -217,10 +218,10 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { EXPECT_TRUE(keyword_table->DidDefaultSearchProviderChange()); // Change keywords. - ASSERT_TRUE(keyword_table->UpdateBackupSignature()); + EXPECT_TRUE(keyword_table->UpdateBackupSignature()); sql::Statement remove_keyword(keyword_table->db_->GetUniqueStatement( "DELETE FROM keywords WHERE id=1")); - ASSERT_TRUE(remove_keyword.Run()); + EXPECT_TRUE(remove_keyword.Run()); EXPECT_TRUE(keyword_table->IsBackupSignatureValid()); EXPECT_EQ(1, keyword_table->GetDefaultSearchProviderID()); @@ -238,7 +239,7 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { // Change keywords backup. sql::Statement remove_keyword_backup(keyword_table->db_->GetUniqueStatement( "DELETE FROM keywords_backup WHERE id=1")); - ASSERT_TRUE(remove_keyword_backup.Run()); + EXPECT_TRUE(remove_keyword_backup.Run()); EXPECT_FALSE(keyword_table->IsBackupSignatureValid()); EXPECT_EQ(1, keyword_table->GetDefaultSearchProviderID()); EXPECT_FALSE(keyword_table->GetDefaultSearchProviderBackup(&backup)); @@ -281,10 +282,10 @@ TEST_F(KeywordTableTest, GetTableContents) { "00http://instant2/0FEDC-BA09-8765-4321"; std::string contents; - ASSERT_TRUE(keyword_table->GetTableContents("keywords", &contents)); + EXPECT_TRUE(keyword_table->GetTableContents("keywords", &contents)); EXPECT_EQ(kTestContents, contents); - ASSERT_TRUE(keyword_table->GetTableContents("keywords_backup", &contents)); + EXPECT_TRUE(keyword_table->GetTableContents("keywords_backup", &contents)); EXPECT_EQ(kTestContents, contents); } @@ -324,10 +325,10 @@ TEST_F(KeywordTableTest, GetTableContentsOrdering) { "url2000001234-5678-90AB-CDEF"; std::string contents; - ASSERT_TRUE(keyword_table->GetTableContents("keywords", &contents)); + EXPECT_TRUE(keyword_table->GetTableContents("keywords", &contents)); EXPECT_EQ(kTestContents, contents); - ASSERT_TRUE(keyword_table->GetTableContents("keywords_backup", &contents)); + EXPECT_TRUE(keyword_table->GetTableContents("keywords_backup", &contents)); EXPECT_EQ(kTestContents, contents); } |