diff options
27 files changed, 1337 insertions, 709 deletions
diff --git a/chrome/browser/importer/ie_importer.cc b/chrome/browser/importer/ie_importer.cc index 93fde8c..052ffdd 100644 --- a/chrome/browser/importer/ie_importer.cc +++ b/chrome/browser/importer/ie_importer.cc @@ -586,7 +586,7 @@ void IEImporter::ImportSearchEngines() { if (gurl.is_valid()) { TemplateURLData data; data.short_name = name; - data.SetKeyword(TemplateURLService::GenerateKeyword(gurl, false)); + data.SetKeyword(TemplateURLService::GenerateKeyword(gurl)); data.SetURL(url); data.show_in_default_list = true; t_iter = search_engines_map.insert(std::make_pair(url, diff --git a/chrome/browser/importer/profile_import_process_messages.h b/chrome/browser/importer/profile_import_process_messages.h index 93f62d0..9a6ed03 100644 --- a/chrome/browser/importer/profile_import_process_messages.h +++ b/chrome/browser/importer/profile_import_process_messages.h @@ -195,8 +195,7 @@ struct ParamTraits<TemplateURLData> { typedef TemplateURLData param_type; static void Write(Message* m, const param_type& p) { WriteParam(m, p.short_name); - WriteParam(m, p.raw_keyword()); - WriteParam(m, p.autogenerate_keyword()); + WriteParam(m, p.keyword()); WriteParam(m, p.url()); WriteParam(m, p.suggestions_url); WriteParam(m, p.instant_url); @@ -215,11 +214,9 @@ struct ParamTraits<TemplateURLData> { } static bool Read(const Message* m, PickleIterator* iter, param_type* p) { string16 keyword; - bool autogenerate_keyword; std::string url; if (!ReadParam(m, iter, &p->short_name) || !ReadParam(m, iter, &keyword) || - !ReadParam(m, iter, &autogenerate_keyword) || !ReadParam(m, iter, &url) || !ReadParam(m, iter, &p->suggestions_url) || !ReadParam(m, iter, &p->instant_url) || @@ -237,7 +234,6 @@ struct ParamTraits<TemplateURLData> { !ReadParam(m, iter, &p->sync_guid)) return false; p->SetKeyword(keyword); - p->SetAutogenerateKeyword(autogenerate_keyword); p->SetURL(url); return true; } diff --git a/chrome/browser/protector/default_search_provider_change.cc b/chrome/browser/protector/default_search_provider_change.cc index a632f3f..d27e666 100644 --- a/chrome/browser/protector/default_search_provider_change.cc +++ b/chrome/browser/protector/default_search_provider_change.cc @@ -53,7 +53,7 @@ class TemplateURLIsSame { if (!url || !other_) return false; return url->short_name() == other_->short_name() && - AreKeywordsSame(url, other_) && + url->HasSameKeywordAs(*other_) && url->url() == other_->url() && url->suggestions_url() == other_->suggestions_url() && url->instant_url() == other_->instant_url() && @@ -65,13 +65,6 @@ class TemplateURLIsSame { } private: - // Returns true if both |url1| and |url2| have autogenerated keywords - // or if their keywords are identical. - bool AreKeywordsSame(const TemplateURL* url1, const TemplateURL* url2) { - return (url1->autogenerate_keyword() && url2->autogenerate_keyword()) || - url1->keyword() == url2->keyword(); - } - const TemplateURL* other_; }; diff --git a/chrome/browser/protector/default_search_provider_change_browsertest.cc b/chrome/browser/protector/default_search_provider_change_browsertest.cc index 0709478..9f32e9e 100644 --- a/chrome/browser/protector/default_search_provider_change_browsertest.cc +++ b/chrome/browser/protector/default_search_provider_change_browsertest.cc @@ -75,10 +75,7 @@ class DefaultSearchProviderChangeTest : public InProcessBrowserTest { const std::string& url) { TemplateURLData data; data.short_name = short_name; - if (keyword.empty()) - data.SetAutogenerateKeyword(true); - else - data.SetKeyword(keyword); + data.SetKeyword(keyword); data.SetURL(url); return new TemplateURL(browser()->profile(), data); } 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 878d108..1602011 100644 --- a/chrome/browser/search_engines/search_host_to_urls_map.cc +++ b/chrome/browser/search_engines/search_host_to_urls_map.cc @@ -27,6 +27,7 @@ void SearchHostToURLsMap::Add(TemplateURL* template_url, const SearchTermsData& search_terms_data) { DCHECK(initialized_); DCHECK(template_url); + DCHECK(!template_url->IsExtensionKeyword()); const GURL url(TemplateURLService::GenerateSearchURLUsingTermsData( template_url, search_terms_data)); @@ -39,6 +40,7 @@ void SearchHostToURLsMap::Add(TemplateURL* template_url, void SearchHostToURLsMap::Remove(TemplateURL* template_url) { DCHECK(initialized_); DCHECK(template_url); + DCHECK(!template_url->IsExtensionKeyword()); const GURL url(TemplateURLService::GenerateSearchURL(template_url)); if (!url.is_valid() || !url.has_host()) diff --git a/chrome/browser/search_engines/search_provider_install_data.cc b/chrome/browser/search_engines/search_provider_install_data.cc index defd639..28d691f 100644 --- a/chrome/browser/search_engines/search_provider_install_data.cc +++ b/chrome/browser/search_engines/search_provider_install_data.cc @@ -144,6 +144,7 @@ static bool IsSameOrigin(const GURL& requested_origin, TemplateURL* template_url, const SearchTermsData& search_terms_data) { DCHECK(requested_origin == requested_origin.GetOrigin()); + DCHECK(!template_url->IsExtensionKeyword()); return requested_origin == TemplateURLService::GenerateSearchURLUsingTermsData(template_url, search_terms_data).GetOrigin(); @@ -264,6 +265,8 @@ void SearchProviderInstallData::SetDefault(const TemplateURL* template_url) { return; } + DCHECK(!template_url->IsExtensionKeyword()); + IOThreadSearchTermsData search_terms_data(google_base_url_); const GURL url(TemplateURLService::GenerateSearchURLUsingTermsData( template_url, search_terms_data)); diff --git a/chrome/browser/search_engines/template_url.cc b/chrome/browser/search_engines/template_url.cc index b29c820..7a67ee6 100644 --- a/chrome/browser/search_engines/template_url.cc +++ b/chrome/browser/search_engines/template_url.cc @@ -13,6 +13,7 @@ #include "base/stringprintf.h" #include "base/utf_string_conversions.h" #include "chrome/browser/autocomplete/autocomplete_field_trial.h" +#include "chrome/browser/google/google_util.h" #include "chrome/browser/search_engines/search_terms_data.h" #include "chrome/browser/search_engines/template_url_service.h" #include "chrome/common/guid.h" @@ -535,41 +536,19 @@ TemplateURLData::TemplateURLData() usage_count(0), prepopulate_id(0), sync_guid(guid::GenerateGUID()), - url_("x"), - autogenerate_keyword_(false), - keyword_generated_(false) { + keyword_(ASCIIToUTF16("dummy")), + url_("x") { } TemplateURLData::~TemplateURLData() { } void TemplateURLData::SetKeyword(const string16& keyword) { + DCHECK(!keyword.empty()); + // Case sensitive keyword matching is confusing. As such, we force all // keywords to be lower case. keyword_ = base::i18n::ToLower(keyword); - autogenerate_keyword_ = false; -} - -const string16& TemplateURLData::keyword(TemplateURL* t_url) const { - EnsureKeyword(t_url); - return keyword_; -} - -void TemplateURLData::SetAutogenerateKeyword(bool autogenerate_keyword) { - autogenerate_keyword_ = autogenerate_keyword; - if (autogenerate_keyword_) { - keyword_.clear(); - keyword_generated_ = false; - } -} - -void TemplateURLData::EnsureKeyword(TemplateURL* t_url) const { - if (autogenerate_keyword_ && !keyword_generated_) { - // Generate a keyword and cache it. - keyword_ = base::i18n::ToLower(TemplateURLService::GenerateKeyword( - TemplateURLService::GenerateSearchURL(t_url).GetWithEmptyPath(), true)); - keyword_generated_ = true; - } } void TemplateURLData::SetURL(const std::string& url) { @@ -654,6 +633,18 @@ bool TemplateURL::SupportsReplacementUsingTermsData( return url_ref_.SupportsReplacementUsingTermsData(search_terms_data); } +bool TemplateURL::IsGoogleSearchURLWithReplaceableKeyword() const { + return !IsExtensionKeyword() && url_ref_.HasGoogleBaseURLs() && + google_util::IsGoogleHostname(UTF16ToUTF8(data_.keyword()), + google_util::DISALLOW_SUBDOMAIN); +} + +bool TemplateURL::HasSameKeywordAs(const TemplateURL& other) const { + return (data_.keyword() == other.data_.keyword()) || + (IsGoogleSearchURLWithReplaceableKeyword() && + other.IsGoogleSearchURLWithReplaceableKeyword()); +} + std::string TemplateURL::GetExtensionId() const { DCHECK(IsExtensionKeyword()); return GURL(data_.url()).host(); @@ -676,9 +667,18 @@ void TemplateURL::SetPrepopulateId(int id) { instant_url_ref_.prepopulated_ = prepopulated; } +void TemplateURL::ResetKeywordIfNecessary(bool force) { + if (IsGoogleSearchURLWithReplaceableKeyword() || force) { + DCHECK(!IsExtensionKeyword()); + GURL url(TemplateURLService::GenerateSearchURL(this)); + if (url.is_valid()) + data_.SetKeyword(TemplateURLService::GenerateKeyword(url)); + } +} + void TemplateURL::InvalidateCachedValues() { url_ref_.InvalidateCachedValues(); suggestions_url_ref_.InvalidateCachedValues(); instant_url_ref_.InvalidateCachedValues(); - data_.SetAutogenerateKeyword(data_.autogenerate_keyword()); + ResetKeywordIfNecessary(false); } diff --git a/chrome/browser/search_engines/template_url.h b/chrome/browser/search_engines/template_url.h index 825a60f..f3de598 100644 --- a/chrome/browser/search_engines/template_url.h +++ b/chrome/browser/search_engines/template_url.h @@ -238,24 +238,9 @@ struct TemplateURLData { // shows this when the user selects a substituting match. string16 short_name; - // The shortcut for this TemplateURL. May be empty. + // The shortcut for this TemplateURL. |keyword| must be non-empty. void SetKeyword(const string16& keyword); - const string16& keyword(TemplateURL* t_url) const; - // TODO(pkasting): This should only be necessary until we eliminate keyword - // autogeneration. - const string16& raw_keyword() const { return keyword_; } - - // Whether to autogenerate a keyword in TemplateURL::GetKeyword(). Most - // consumers should not need this. - // NOTE: Calling SetKeyword() turns this back off. Manual and automatic - // keywords are mutually exclusive. - void SetAutogenerateKeyword(bool autogenerate_keyword); - bool autogenerate_keyword() const { return autogenerate_keyword_; } - - // Ensures that the keyword is generated. Most consumers should not need this - // because it is done automatically. Use this method on the UI thread, so - // the keyword may be accessed on another thread. - void EnsureKeyword(TemplateURL* t_url) const; + const string16& keyword() const { return keyword_; } // The raw URL for the TemplateURL, which may not be valid as-is (e.g. because // it requires substitutions first). This must be non-empty. @@ -325,15 +310,8 @@ struct TemplateURLData { private: // Private so we can enforce using the setters and thus enforce that these // fields are never empty. - // TODO(pkasting): |keyword_| is temporarily still allowed to be empty. - mutable string16 keyword_; + string16 keyword_; std::string url_; - - // TODO(pkasting): These fields will go away soon. - 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_; }; @@ -372,18 +350,7 @@ class TemplateURL { // displayed even if it is LTR and the UI is RTL. string16 AdjustedShortNameForLocaleDirection() const; - const string16& keyword() const { - // TODO(pkasting): See comment on EnsureKeyword() below. - return data_.keyword(const_cast<TemplateURL*>(this)); - } - bool autogenerate_keyword() const { - return data_.autogenerate_keyword(); - } - void EnsureKeyword() const { - // TODO(pkasting): EnsureKeyword() will die soon so it's not worth jumping - // through hoops to fix this const_cast<>(). - data_.EnsureKeyword(const_cast<TemplateURL*>(this)); - } + const string16& keyword() const { return data_.keyword(); } const std::string& url() const { return data_.url(); } const std::string& suggestions_url() const { return data_.suggestions_url; } @@ -429,6 +396,15 @@ class TemplateURL { bool SupportsReplacementUsingTermsData( const SearchTermsData& search_terms_data) const; + // Returns true if this TemplateURL uses Google base URLs and has a keyword + // of "google.TLD". We use this to decide whether we can automatically + // update the keyword to reflect the current Google base URL TLD. + bool IsGoogleSearchURLWithReplaceableKeyword() const; + + // Returns true if the keywords match or if + // IsGoogleSearchURLWithReplaceableKeyword() is true for both TemplateURLs. + bool HasSameKeywordAs(const TemplateURL& other) const; + std::string GetExtensionId() const; bool IsExtensionKeyword() const; @@ -438,6 +414,13 @@ class TemplateURL { void SetURL(const std::string& url); void SetPrepopulateId(int id); + // Resets the keyword if IsGoogleSearchURLWithReplaceableKeyword() or |force|. + // The |force| parameter is useful when the existing keyword is known to be + // a placeholder. The resulting keyword is generated using + // TemplateURLService::GenerateSearchURL() and + // TemplateURLService::GenerateKeyword(). + void ResetKeywordIfNecessary(bool force); + // Invalidates cached values on this object and its child TemplateURLRefs. void InvalidateCachedValues(); diff --git a/chrome/browser/search_engines/template_url_parser.cc b/chrome/browser/search_engines/template_url_parser.cc index a9f6939..f48eb39 100644 --- a/chrome/browser/search_engines/template_url_parser.cc +++ b/chrome/browser/search_engines/template_url_parser.cc @@ -309,9 +309,7 @@ TemplateURL* TemplateURLParsingContext::GetTemplateURL( if (suggestion_method_ == TemplateURLParsingContext::POST) data_.suggestions_url.clear(); - string16 keyword(TemplateURLService::GenerateKeyword(url, false)); - DCHECK(!keyword.empty()); - data_.SetKeyword(keyword); + data_.SetKeyword(TemplateURLService::GenerateKeyword(url)); data_.show_in_default_list = show_in_default_list; return new TemplateURL(profile, data_); } diff --git a/chrome/browser/search_engines/template_url_prepopulate_data.cc b/chrome/browser/search_engines/template_url_prepopulate_data.cc index a4a893d..3390b01 100644 --- a/chrome/browser/search_engines/template_url_prepopulate_data.cc +++ b/chrome/browser/search_engines/template_url_prepopulate_data.cc @@ -44,9 +44,6 @@ namespace { struct PrepopulatedEngine { const wchar_t* const name; - // If empty, we'll autogenerate a keyword based on the search_url every time - // someone asks. Only entries which need keywords to auto-track a dynamically - // generated search URL should use this. const wchar_t* const keyword; const char* const favicon_url; // If NULL, there is no favicon. const char* const search_url; @@ -1085,7 +1082,7 @@ const PrepopulatedEngine goo = { const PrepopulatedEngine google = { L"Google", - L"", + L"google.com", // This will be dynamically updated by the TemplateURL system. "http://www.google.com/favicon.ico", "{google:baseURL}search?{google:RLZ}{google:acceptedSuggestion}" "{google:originalQueryForSuggestion}{google:searchFieldtrialParameter}" @@ -3158,10 +3155,7 @@ TemplateURL* MakePrepopulatedTemplateURL(Profile* profile, int id) { TemplateURLData data; data.short_name = name; - if (keyword.empty()) - data.SetAutogenerateKeyword(true); - else - data.SetKeyword(keyword); + data.SetKeyword(keyword); data.SetURL(search_url.as_string()); data.suggestions_url = suggest_url.as_string(); data.instant_url = instant_url.as_string(); @@ -3208,8 +3202,8 @@ void GetPrepopulatedTemplateFromPrefs(Profile* profile, engine->Get("encoding", &val) && val->GetAsString(&encoding) && engine->Get("id", &val) && val->GetAsInteger(&id)) { // These next fields are not allowed to be empty. - if (name.empty() || search_url.empty() || favicon_url.empty() || - encoding.empty()) + if (name.empty() || keyword.empty() || search_url.empty() || + favicon_url.empty() || encoding.empty()) return; } else { // Got a parsing error. No big deal. diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc index 7bdbe87..fcb6e79 100644 --- a/chrome/browser/search_engines/template_url_service.cc +++ b/chrome/browser/search_engines/template_url_service.cc @@ -70,7 +70,7 @@ bool TemplateURLsHaveSamePrefs(const TemplateURL* url1, return true; return (url1 != NULL) && (url2 != NULL) && (url1->short_name() == url2->short_name()) && - (url1->keyword() == url2->keyword()) && + url1->HasSameKeywordAs(*url2) && (url1->url() == url2->url()) && (url1->suggestions_url() == url2->suggestions_url()) && (url1->instant_url() == url2->instant_url()) && @@ -90,6 +90,40 @@ TemplateURL* FirstPotentialDefaultEngine( return NULL; } +// If |change_list| contains ACTION_UPDATEs followed by more ACTION_UPDATEs or +// ACTION_ADDs for the same GUID, remove all but the last one. +// +// Removing UPDATE before ADD is important. This can happen if +// ResolveSyncKeywordConflict() changes a local TemplateURL that hasn't actually +// been seen by the server yet. In this case sending the UPDATE first might +// confuse the sync server. +// +// Removing UPDATE before UPDATE, OTOH, is not really necessary as the server +// will coalesce these before other clients see them; however it's easy to do in +// conjunction with the filtering for UPDATE-before-ADD and saves bandwidth. +void PreventDuplicateGUIDUpdates(SyncChangeList* change_list) { + for (size_t i = change_list->size(); i > 1; ) { + --i; // Prevent underflow that could occur if we did this in the loop body. + const SyncChange& change_i = (*change_list)[i]; + if ((change_i.change_type() != SyncChange::ACTION_ADD) && + (change_i.change_type() != SyncChange::ACTION_UPDATE)) + continue; + std::string guid( + change_i.sync_data().GetSpecifics().search_engine().sync_guid()); + for (size_t j = 0; j < i; ) { + const SyncChange& change_j = (*change_list)[j]; + if ((change_j.change_type() == SyncChange::ACTION_UPDATE) && + (change_j.sync_data().GetSpecifics().search_engine().sync_guid() == + guid)) { + change_list->erase(change_list->begin() + j); + --i; + } else { + ++j; + } + } + } +} + } // namespace @@ -160,22 +194,8 @@ TemplateURLService::~TemplateURLService() { } // static -string16 TemplateURLService::GenerateKeyword(const GURL& url, - bool autodetected) { - // Don't autogenerate keywords for referrers that are the result of a form - // submission (TODO: right now we approximate this by checking for the URL - // having a query, but we should replace this with a call to WebCore to see if - // the originating page was actually a form submission), anything other than - // http, or referrers with a path. - // - // If we relax the path constraint, we need to be sure to sanitize the path - // elements and update AutocompletePopup to look for keywords using the path. - // See http://b/issue?id=863583. - if (!url.is_valid() || - (autodetected && (url.has_query() || !url.SchemeIs(chrome::kHttpScheme) || - ((url.path() != "") && (url.path() != "/"))))) - return string16(); - +string16 TemplateURLService::GenerateKeyword(const GURL& url) { + DCHECK(url.is_valid()); // Strip "www." off the front of the keyword; otherwise the keyword won't work // properly. See http://code.google.com/p/chromium/issues/detail?id=6984 . // Special case: if the host was exactly "www." (not sure this can happen but @@ -231,10 +251,10 @@ GURL TemplateURLService::GenerateSearchURLUsingTermsData( const TemplateURL* t_url, const SearchTermsData& search_terms_data) { DCHECK(t_url); + DCHECK(!t_url->IsExtensionKeyword()); + const TemplateURLRef& search_ref = t_url->url_ref(); - // Extension keywords don't have host-based search URLs. - if (!search_ref.IsValidUsingTermsData(search_terms_data) || - t_url->IsExtensionKeyword()) + if (!search_ref.IsValidUsingTermsData(search_terms_data)) return GURL(); if (!search_ref.SupportsReplacementUsingTermsData(search_terms_data)) @@ -307,18 +327,19 @@ TemplateURL* TemplateURLService::GetTemplateURLForKeyword( keyword_to_template_map_.find(keyword)); if (elem != keyword_to_template_map_.end()) return elem->second; - return (initial_default_search_provider_.get() && + return ((!loaded_ || load_failed_) && + initial_default_search_provider_.get() && (initial_default_search_provider_->keyword() == keyword)) ? initial_default_search_provider_.get() : NULL; } TemplateURL* TemplateURLService::GetTemplateURLForGUID( const std::string& sync_guid) { - GUIDToTemplateMap::const_iterator elem( - guid_to_template_map_.find(sync_guid)); + GUIDToTemplateMap::const_iterator elem(guid_to_template_map_.find(sync_guid)); if (elem != guid_to_template_map_.end()) return elem->second; - return (initial_default_search_provider_.get() && + return ((!loaded_ || load_failed_) && + initial_default_search_provider_.get() && (initial_default_search_provider_->sync_guid() == sync_guid)) ? initial_default_search_provider_.get() : NULL; } @@ -328,14 +349,15 @@ TemplateURL* TemplateURLService::GetTemplateURLForHost( TemplateURL* t_url = provider_map_->GetTemplateURLForHost(host); if (t_url) return t_url; - return (initial_default_search_provider_.get() && + return ((!loaded_ || load_failed_) && + 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, true); - NotifyObservers(); + if (AddNoNotify(template_url, true)) + NotifyObservers(); } void TemplateURLService::AddAndSetProfile(TemplateURL* template_url, @@ -348,6 +370,7 @@ void TemplateURLService::AddWithOverrides(TemplateURL* template_url, const string16& short_name, const string16& keyword, const std::string& url) { + DCHECK(!keyword.empty()); DCHECK(!url.empty()); template_url->data_.short_name = short_name; template_url->data_.SetKeyword(keyword); @@ -447,20 +470,25 @@ TemplateURLService::TemplateURLVector TemplateURLService::GetTemplateURLs() { } void TemplateURLService::IncrementUsageCount(TemplateURL* url) { - DCHECK(url && std::find(template_urls_.begin(), template_urls_.end(), url) != - template_urls_.end()); + DCHECK(url); + if (std::find(template_urls_.begin(), template_urls_.end(), url) == + template_urls_.end()) + return; ++url->data_.usage_count; // 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); + service_.get()->UpdateKeyword(url->data()); } void TemplateURLService::ResetTemplateURL(TemplateURL* url, const string16& title, const string16& keyword, const std::string& search_url) { + if (!loaded_) + return; + DCHECK(!keyword.empty()); DCHECK(!search_url.empty()); TemplateURLData data(url->data()); data.short_name = title; @@ -473,8 +501,8 @@ void TemplateURLService::ResetTemplateURL(TemplateURL* url, data.safe_for_autoreplace = false; data.last_modified = time_provider_(); TemplateURL new_url(url->profile(), data); - UpdateNoNotify(url, new_url); - NotifyObservers(); + if (UpdateNoNotify(url, new_url)) + NotifyObservers(); } bool TemplateURLService::CanMakeDefault(const TemplateURL* url) { @@ -489,8 +517,8 @@ void TemplateURLService::SetDefaultSearchProvider(TemplateURL* url) { // 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); - NotifyObservers(); + if (SetDefaultSearchProviderNoNotify(url)) + NotifyObservers(); } TemplateURL* TemplateURLService::GetDefaultSearchProvider() { @@ -611,14 +639,17 @@ void TemplateURLService::OnWebDataServiceRequestDone( data.created_by_policy = true; data.id = kInvalidTemplateURLID; default_search_provider = new TemplateURL(profile_, data); - AddNoNotify(default_search_provider, true); + if (!AddNoNotify(default_search_provider, true)) + default_search_provider = NULL; } } // Note that this saves the default search provider to prefs. if (!default_search_provider || (!default_search_provider->IsExtensionKeyword() && - default_search_provider->SupportsReplacement())) - SetDefaultSearchProviderNoNotify(default_search_provider); + default_search_provider->SupportsReplacement())) { + bool success = SetDefaultSearchProviderNoNotify(default_search_provider); + DCHECK(success); + } } else { // If we had a managed default, replace it with the synced default if // applicable, or the first provider of the list. @@ -699,8 +730,11 @@ void TemplateURLService::OnWebDataServiceRequestDone( UMA_HISTOGRAM_BOOLEAN("Search.HasDefaultSearchProvider", has_default_search_provider); // Ensure that default search provider exists. See http://crbug.com/116952. - if (!has_default_search_provider) - SetDefaultSearchProviderNoNotify(FindNewDefaultSearchProvider()); + if (!has_default_search_provider) { + bool success = + SetDefaultSearchProviderNoNotify(FindNewDefaultSearchProvider()); + DCHECK(success); + } } NotifyObservers(); @@ -726,7 +760,7 @@ void TemplateURLService::Observe(int type, const content::NotificationDetails& details) { if (type == chrome::NOTIFICATION_HISTORY_URL_VISITED) { content::Details<history::URLVisitedDetails> visit_details(details); - if (!loaded()) + if (!loaded_) visits_to_add_.push_back(*visit_details.ptr()); else UpdateKeywordSearchTermsForURL(*visit_details.ptr()); @@ -792,6 +826,7 @@ SyncError TemplateURLService::ProcessSyncChanges( syncable::SEARCH_ENGINES); return error; } + DCHECK(loaded_); AutoReset<bool> processing_changes(&processing_syncer_changes_, true); @@ -809,9 +844,13 @@ SyncError TemplateURLService::ProcessSyncChanges( if (!turl.get()) continue; + // Explicitly don't check for conflicts against extension keywords; in this + // case the functions which modify the keyword map know how to handle the + // conflicts. + // TODO(mpcomplete): If we allow editing extension keywords, then those will + // need to undergo conflict resolution. TemplateURL* existing_keyword_turl = - GetTemplateURLForKeyword(turl->keyword()); - + FindNonExtensionTemplateURLForKeyword(turl->keyword()); if (iter->change_type() == SyncChange::ACTION_DELETE && existing_turl) { bool delete_default = (existing_turl == GetDefaultSearchProvider()); @@ -829,8 +868,10 @@ SyncError TemplateURLService::ProcessSyncChanges( } else if (iter->change_type() == SyncChange::ACTION_ADD && !existing_turl) { std::string guid = turl->sync_guid(); - if (existing_keyword_turl) - ResolveSyncKeywordConflict(turl.get(), &new_changes); + if (existing_keyword_turl) { + ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl, + &new_changes); + } // Force the local ID to kInvalidTemplateURLID so we can add it. TemplateURLData data(turl->data()); data.id = kInvalidTemplateURLID; @@ -842,10 +883,12 @@ SyncError TemplateURLService::ProcessSyncChanges( existing_turl) { // Possibly resolve a keyword conflict if they have the same keywords but // are not the same entry. - if (existing_keyword_turl && existing_keyword_turl != existing_turl) - ResolveSyncKeywordConflict(turl.get(), &new_changes); - UpdateNoNotify(existing_turl, *turl); - NotifyObservers(); + if (existing_keyword_turl && existing_keyword_turl != existing_turl) { + ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl, + &new_changes); + } + if (UpdateNoNotify(existing_turl, *turl)) + NotifyObservers(); } else { // Something really unexpected happened. Either we received an // ACTION_INVALID, or Sync is in a crazy state: @@ -858,6 +901,7 @@ SyncError TemplateURLService::ProcessSyncChanges( SyncChange::ChangeTypeToString(iter->change_type())); } } + PreventDuplicateGUIDUpdates(&new_changes); // If something went wrong, we want to prematurely exit to avoid pushing // inconsistent data to Sync. We return the last error we received. @@ -874,7 +918,7 @@ SyncError TemplateURLService::MergeDataAndStartSyncing( const SyncDataList& initial_sync_data, scoped_ptr<SyncChangeProcessor> sync_processor, scoped_ptr<SyncErrorFactory> sync_error_factory) { - DCHECK(loaded()); + DCHECK(loaded_); DCHECK_EQ(type, syncable::SEARCH_ENGINES); DCHECK(!sync_processor_.get()); DCHECK(sync_processor.get()); @@ -924,8 +968,8 @@ SyncError TemplateURLService::MergeDataAndStartSyncing( // TemplateURLID and the TemplateURL may have to be reparsed. This // also makes the local data's last_modified timestamp equal to Sync's, // avoiding an Update on the next MergeData call. - UpdateNoNotify(local_turl, *sync_turl); - NotifyObservers(); + if (UpdateNoNotify(local_turl, *sync_turl)) + NotifyObservers(); } else if (sync_turl->last_modified() < local_turl->last_modified()) { // Otherwise, we know we have newer data, so update Sync with our // data fields. @@ -949,8 +993,14 @@ SyncError TemplateURLService::MergeDataAndStartSyncing( // Keyword conflict is possible in this case. Resolve it first before // adding the new TemplateURL. Note that we don't remove the local TURL // from local_data_map in this case as it may still need to be pushed to - // the cloud. - ResolveSyncKeywordConflict(sync_turl.get(), &new_changes); + // the cloud. We also explicitly don't resolve conflicts against + // extension keywords; see comments in ProcessSyncChanges(). + TemplateURL* existing_keyword_turl = + FindNonExtensionTemplateURLForKeyword(sync_turl->keyword()); + if (existing_keyword_turl) { + ResolveSyncKeywordConflict(sync_turl.get(), existing_keyword_turl, + &new_changes); + } // Force the local ID to kInvalidTemplateURLID so we can add it. TemplateURLData data(sync_turl->data()); data.id = kInvalidTemplateURLID; @@ -969,6 +1019,8 @@ SyncError TemplateURLService::MergeDataAndStartSyncing( new_changes.push_back(SyncChange(SyncChange::ACTION_ADD, iter->second)); } + PreventDuplicateGUIDUpdates(&new_changes); + SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes); if (error.IsSet()) return error; @@ -1031,7 +1083,6 @@ SyncData TemplateURLService::CreateSyncDataFromTemplateURL( se_specifics->set_show_in_default_list(turl.show_in_default_list()); se_specifics->set_suggestions_url(turl.suggestions_url()); se_specifics->set_prepopulate_id(turl.prepopulate_id()); - se_specifics->set_autogenerate_keyword(turl.autogenerate_keyword()); se_specifics->set_instant_url(turl.instant_url()); se_specifics->set_last_modified(turl.last_modified().ToInternalValue()); se_specifics->set_sync_guid(turl.sync_guid()); @@ -1061,8 +1112,17 @@ TemplateURL* TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData( TemplateURLData data; data.short_name = UTF8ToUTF16(specifics.short_name()); data.originating_url = GURL(specifics.originating_url()); - data.SetKeyword(UTF8ToUTF16(specifics.keyword())); - data.SetAutogenerateKeyword(specifics.autogenerate_keyword()); + string16 keyword(UTF8ToUTF16(specifics.keyword())); + // NOTE: Once this code has shipped in a couple of stable releases, we can + // probably remove the migration portion, comment out the + // "autogenerate_keyword" field entirely in the .proto file, and fold the + // empty keyword case into the "delete data" block above. + bool reset_keyword = + specifics.autogenerate_keyword() || specifics.keyword().empty(); + if (reset_keyword) + keyword = ASCIIToUTF16("dummy"); // Will be replaced below. + DCHECK(!keyword.empty()); + data.SetKeyword(keyword); data.SetURL(specifics.url()); data.suggestions_url = specifics.suggestions_url(); data.instant_url = specifics.instant_url(); @@ -1082,6 +1142,11 @@ TemplateURL* TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData( TemplateURL* turl = new TemplateURL(profile, data); DCHECK(!turl->IsExtensionKeyword()); + if (reset_keyword) { + turl->ResetKeywordIfNecessary(true); + SyncData sync_data = CreateSyncDataFromTemplateURL(*turl); + change_list->push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data)); + } return turl; } @@ -1167,8 +1232,31 @@ void TemplateURLService::Init(const Initializer* initializers, } void TemplateURLService::RemoveFromMaps(TemplateURL* template_url) { - if (!template_url->keyword().empty()) - keyword_to_template_map_.erase(template_url->keyword()); + const string16& keyword = template_url->keyword(); + DCHECK_NE(0U, keyword_to_template_map_.count(keyword)); + if (keyword_to_template_map_[keyword] == template_url) { + // We need to check whether the keyword can now be provided by another + // TemplateURL. See the comments in AddToMaps() for more information on + // extension keywords and how they can coexist with non-extension keywords. + // In the case of more than one extension, we use the most recently + // installed (which will be the most recently added, which will have the + // highest ID). + TemplateURL* best_fallback = NULL; + for (TemplateURLVector::const_iterator i(template_urls_.begin()); + i != template_urls_.end(); ++i) { + TemplateURL* turl = *i; + // This next statement relies on the fact that there can only be one + // non-extension TemplateURL with a given keyword. + if ((turl != template_url) && (turl->keyword() == keyword) && + (!best_fallback || !best_fallback->IsExtensionKeyword() || + (turl->IsExtensionKeyword() && (turl->id() > best_fallback->id())))) + best_fallback = turl; + } + if (best_fallback) + keyword_to_template_map_[keyword] = best_fallback; + else + keyword_to_template_map_.erase(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. @@ -1198,15 +1286,31 @@ void TemplateURLService::RemoveFromKeywordMapByPointer( } void TemplateURLService::AddToMaps(TemplateURL* template_url) { - if (!template_url->keyword().empty()) - keyword_to_template_map_[template_url->keyword()] = template_url; + bool template_extension = template_url->IsExtensionKeyword(); + const string16& keyword = template_url->keyword(); + KeywordToTemplateMap::const_iterator i = + keyword_to_template_map_.find(keyword); + if (i == keyword_to_template_map_.end()) { + keyword_to_template_map_[keyword] = template_url; + } else { + const TemplateURL* existing_url = i->second; + // We should only have overlapping keywords when at least one comes from + // an extension. In that case, the ranking order is: + // Manually-modified keywords > extension keywords > replaceable keywords + // When there are multiple extensions, the last-added wins. + bool existing_extension = existing_url->IsExtensionKeyword(); + DCHECK(existing_extension || template_extension); + if (existing_extension ? + !CanReplace(template_url) : CanReplace(existing_url)) + keyword_to_template_map_[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()) + if (template_extension) return; if (!template_url->sync_guid().empty()) @@ -1326,6 +1430,13 @@ bool TemplateURLService::LoadDefaultSearchProviderFromPrefs( UTF8ToUTF16(prefs->GetString(prefs::kDefaultSearchProviderName)); string16 keyword = UTF8ToUTF16(prefs->GetString(prefs::kDefaultSearchProviderKeyword)); + // Force keyword to be non-empty. + // TODO(pkasting): This is only necessary as long as we're potentially loading + // older prefs where empty keywords are theoretically possible. Eventually + // this code can be replaced with a DCHECK(!keyword.empty());. + bool update_keyword = keyword.empty(); + if (update_keyword) + keyword = ASCIIToUTF16("dummy"); std::string search_url = prefs->GetString(prefs::kDefaultSearchProviderSearchURL); // Force URL to be non-empty. We've never supported this case, but past bugs @@ -1366,6 +1477,8 @@ bool TemplateURLService::LoadDefaultSearchProviderFromPrefs( } default_provider->reset(new TemplateURL(profile_, data)); DCHECK(!(*default_provider)->IsExtensionKeyword()); + if (update_keyword) + (*default_provider)->ResetKeywordIfNecessary(true); return true; } @@ -1391,12 +1504,28 @@ bool TemplateURLService::CanReplace(const TemplateURL* t_url) { t_url->safe_for_autoreplace()); } -void TemplateURLService::UpdateNoNotify(TemplateURL* existing_turl, +TemplateURL* TemplateURLService::FindNonExtensionTemplateURLForKeyword( + const string16& keyword) { + TemplateURL* keyword_turl = GetTemplateURLForKeyword(keyword); + if (!keyword_turl || !keyword_turl->IsExtensionKeyword()) + return keyword_turl; + // The extension keyword in the model may be hiding a replaceable + // non-extension keyword. Look for it. + for (TemplateURLVector::const_iterator i(template_urls_.begin()); + i != template_urls_.end(); ++i) { + if (!(*i)->IsExtensionKeyword() && ((*i)->keyword() == keyword)) + return *i; + } + return NULL; +} + +bool TemplateURLService::UpdateNoNotify(TemplateURL* existing_turl, const TemplateURL& new_values) { DCHECK(loaded_); DCHECK(existing_turl); - DCHECK(std::find(template_urls_.begin(), template_urls_.end(), - existing_turl) != template_urls_.end()); + if (std::find(template_urls_.begin(), template_urls_.end(), existing_turl) == + template_urls_.end()) + return false; // TODO(mpcomplete): If we allow editing extension keywords, then those should // be persisted to disk and synced. In this case this DCHECK should be @@ -1404,8 +1533,7 @@ void TemplateURLService::UpdateNoNotify(TemplateURL* existing_turl, DCHECK(!existing_turl->IsExtensionKeyword()); string16 old_keyword(existing_turl->keyword()); - if (!old_keyword.empty()) - keyword_to_template_map_.erase(old_keyword); + keyword_to_template_map_.erase(old_keyword); if (!existing_turl->sync_guid().empty()) guid_to_template_map_.erase(existing_turl->sync_guid()); @@ -1417,19 +1545,47 @@ void TemplateURLService::UpdateNoNotify(TemplateURL* existing_turl, provider_map_->Add(existing_turl, search_terms_data); const string16& keyword = existing_turl->keyword(); - if (!keyword.empty()) + KeywordToTemplateMap::const_iterator i = + keyword_to_template_map_.find(keyword); + if (i == keyword_to_template_map_.end()) { keyword_to_template_map_[keyword] = existing_turl; + } else { + // We can theoretically reach here in two cases: + // * There is an existing extension keyword and sync brings in a rename of + // a non-extension keyword to match. In this case we just need to pick + // which keyword has priority to update the keyword map. + // * Autogeneration of the keyword for a Google default search provider + // at load time causes it to conflict with an existing keyword. In this + // case we delete the existing keyword if it's replaceable, or else undo + // the change in keyword for |existing_turl|. + DCHECK(!existing_turl->IsExtensionKeyword()); + TemplateURL* existing_keyword_turl = i->second; + if (existing_keyword_turl->IsExtensionKeyword()) { + if (!CanReplace(existing_turl)) + keyword_to_template_map_[keyword] = existing_turl; + } else { + if (CanReplace(existing_keyword_turl)) { + RemoveNoNotify(existing_keyword_turl); + } else { + existing_turl->data_.SetKeyword(old_keyword); + keyword_to_template_map_[old_keyword] = existing_turl; + } + } + } if (!existing_turl->sync_guid().empty()) guid_to_template_map_[existing_turl->sync_guid()] = existing_turl; if (service_.get()) - service_->UpdateKeyword(*existing_turl); + service_->UpdateKeyword(existing_turl->data()); // Inform sync of the update. ProcessTemplateURLChange(existing_turl, SyncChange::ACTION_UPDATE); - if (default_search_provider_ == existing_turl) - SetDefaultSearchProviderNoNotify(existing_turl); + if (default_search_provider_ == existing_turl) { + bool success = SetDefaultSearchProviderNoNotify(existing_turl); + DCHECK(success); + } + return true; } PrefService* TemplateURLService::GetPrefs() { @@ -1564,9 +1720,21 @@ void TemplateURLService::GoogleBaseURLChanged() { string16 original_keyword(t_url->keyword()); t_url->InvalidateCachedValues(); const string16& new_keyword(t_url->keyword()); + KeywordToTemplateMap::const_iterator i = + keyword_to_template_map_.find(new_keyword); + if ((i != keyword_to_template_map_.end()) && (i->second != t_url)) { + // The new autogenerated keyword conflicts with another TemplateURL. + // Overwrite it if it's replaceable; otherwise just reset |t_url|'s + // keyword. (This will not prevent |t_url| from auto-updating the + // keyword in the future if the conflicting TemplateURL disappears.) + if (!CanReplace(i->second)) { + t_url->data_.SetKeyword(original_keyword); + continue; + } + RemoveNoNotify(i->second); + } RemoveFromKeywordMapByPointer(t_url); - if (!new_keyword.empty()) - keyword_to_template_map_[new_keyword] = t_url; + keyword_to_template_map_[new_keyword] = t_url; } } @@ -1618,7 +1786,8 @@ void TemplateURLService::UpdateDefaultSearch() { // TemplateURLsHaveSamePrefs would have returned true. Remove this now // invalid value. TemplateURL* old_default = default_search_provider_; - SetDefaultSearchProviderNoNotify(NULL); + bool success = SetDefaultSearchProviderNoNotify(NULL); + DCHECK(success); RemoveNoNotify(old_default); } else if (default_search_provider_) { TemplateURLData data(new_default_from_prefs->data()); @@ -1631,9 +1800,11 @@ void TemplateURLService::UpdateDefaultSearch() { TemplateURLData data(new_default_from_prefs->data()); data.created_by_policy = true; new_template = new TemplateURL(profile_, data); - AddNoNotify(new_template, true); + if (!AddNoNotify(new_template, true)) + return; } - SetDefaultSearchProviderNoNotify(new_template); + bool success = SetDefaultSearchProviderNoNotify(new_template); + DCHECK(success); } } else if (!is_default_search_managed_ && new_is_default_managed) { // The default used to be unmanaged and is now managed. Add the new @@ -1644,9 +1815,11 @@ void TemplateURLService::UpdateDefaultSearch() { TemplateURLData data(new_default_from_prefs->data()); data.created_by_policy = true; new_template = new TemplateURL(profile_, data); - AddNoNotify(new_template, true); + if (!AddNoNotify(new_template, true)) + return; } - SetDefaultSearchProviderNoNotify(new_template); + bool success = SetDefaultSearchProviderNoNotify(new_template); + DCHECK(success); } else { // The default was managed and is no longer. DCHECK(is_default_search_managed_ && !new_is_default_managed); @@ -1671,10 +1844,11 @@ void TemplateURLService::UpdateDefaultSearch() { NotifyObservers(); } -void TemplateURLService::SetDefaultSearchProviderNoNotify(TemplateURL* url) { +bool TemplateURLService::SetDefaultSearchProviderNoNotify(TemplateURL* url) { if (url) { - DCHECK(std::find(template_urls_.begin(), template_urls_.end(), url) != - template_urls_.end()); + if (std::find(template_urls_.begin(), template_urls_.end(), url) == + template_urls_.end()) + return false; // Extension keywords cannot be made default, as they're inherently async. DCHECK(!url->IsExtensionKeyword()); } @@ -1686,7 +1860,7 @@ void TemplateURLService::SetDefaultSearchProviderNoNotify(TemplateURL* url) { // template urls we ship with. url->data_.show_in_default_list = true; if (service_.get()) - service_->UpdateKeyword(*url); + service_->UpdateKeyword(url->data()); if (url->url_ref().HasGoogleBaseURLs()) { GoogleURLTracker::RequestServerCheck(); @@ -1718,9 +1892,10 @@ void TemplateURLService::SetDefaultSearchProviderNoNotify(TemplateURL* url) { // Inform sync the change to the show_in_default_list flag. if (url) ProcessTemplateURLChange(url, SyncChange::ACTION_UPDATE); + return true; } -void TemplateURLService::AddNoNotify(TemplateURL* template_url, +bool TemplateURLService::AddNoNotify(TemplateURL* template_url, bool newly_adding) { DCHECK(template_url); @@ -1731,6 +1906,27 @@ void TemplateURLService::AddNoNotify(TemplateURL* template_url, template_url->data_.id = ++next_id_; } + template_url->ResetKeywordIfNecessary(false); + if (!template_url->IsExtensionKeyword()) { + // Check whether |template_url|'s keyword conflicts with any already in the + // model. + TemplateURL* existing_keyword_turl = + FindNonExtensionTemplateURLForKeyword(template_url->keyword()); + if (existing_keyword_turl != NULL) { + DCHECK_NE(existing_keyword_turl, template_url); + if (CanReplace(existing_keyword_turl)) { + RemoveNoNotify(existing_keyword_turl); + } else if (CanReplace(template_url)) { + delete template_url; + return false; + } else { + string16 new_keyword = UniquifyKeyword(*existing_keyword_turl); + ResetTemplateURL(existing_keyword_turl, + existing_keyword_turl->short_name(), new_keyword, + existing_keyword_turl->url()); + } + } + } template_urls_.push_back(template_url); AddToMaps(template_url); @@ -1740,12 +1936,14 @@ void TemplateURLService::AddNoNotify(TemplateURL* template_url, // 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); + service_->AddKeyword(template_url->data()); // 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); } + + return true; } void TemplateURLService::RemoveNoNotify(TemplateURL* template_url) { @@ -1847,6 +2045,7 @@ void TemplateURLService::RemoveProvidersCreatedByPolicy( void TemplateURLService::ResetTemplateURLGUID(TemplateURL* url, const std::string& guid) { + DCHECK(loaded_); DCHECK(!guid.empty()); TemplateURLData data(url->data()); @@ -1863,9 +2062,8 @@ string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl) { // First, try to return the generated keyword for the TemplateURL. GURL gurl(turl.url()); if (gurl.is_valid()) { - string16 keyword_candidate = GenerateKeyword(gurl, true); - if (!GetTemplateURLForKeyword(keyword_candidate) && - !keyword_candidate.empty()) + string16 keyword_candidate = GenerateKeyword(gurl); + if (!GetTemplateURLForKeyword(keyword_candidate)) return keyword_candidate; } @@ -1880,20 +2078,19 @@ string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl) { return keyword_candidate; } -bool TemplateURLService::ResolveSyncKeywordConflict( +void TemplateURLService::ResolveSyncKeywordConflict( TemplateURL* sync_turl, + TemplateURL* local_turl, SyncChangeList* change_list) { + DCHECK(loaded_); DCHECK(sync_turl); + DCHECK(local_turl); + DCHECK(sync_turl->sync_guid() != local_turl->sync_guid()); + DCHECK(!local_turl->IsExtensionKeyword()); DCHECK(change_list); - TemplateURL* existing_turl = - GetTemplateURLForKeyword(sync_turl->keyword()); - // If there is no conflict, or it's just conflicting with itself, return. - if (!existing_turl || existing_turl->sync_guid() == sync_turl->sync_guid()) - return false; - - if (existing_turl->last_modified() > sync_turl->last_modified() || - existing_turl->created_by_policy()) { + if (local_turl->last_modified() > sync_turl->last_modified() || + local_turl->created_by_policy()) { string16 new_keyword = UniquifyKeyword(*sync_turl); DCHECK(!GetTemplateURLForKeyword(new_keyword)); sync_turl->data_.SetKeyword(new_keyword); @@ -1902,14 +2099,24 @@ bool TemplateURLService::ResolveSyncKeywordConflict( SyncData sync_data = CreateSyncDataFromTemplateURL(*sync_turl); change_list->push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data)); } else { - string16 new_keyword = UniquifyKeyword(*existing_turl); - TemplateURLData data(existing_turl->data()); + string16 new_keyword = UniquifyKeyword(*local_turl); + TemplateURLData data(local_turl->data()); data.SetKeyword(new_keyword); - TemplateURL new_turl(existing_turl->profile(), data); - UpdateNoNotify(existing_turl, new_turl); - NotifyObservers(); + TemplateURL new_turl(local_turl->profile(), data); + if (UpdateNoNotify(local_turl, new_turl)) + NotifyObservers(); + if (!models_associated_) { + // We're doing our initial sync, so UpdateNoNotify() won't have generated + // an ACTION_UPDATE. If this local URL is one that was just newly brought + // down from the sync server, we need to go ahead and generate an update + // for it. If it was pre-existing, then this is unnecessary (and in fact + // wrong) because MergeDataAndStartSyncing() will later add an ACTION_ADD + // for this URL; but in this case, PreventDuplicateGUIDUpdates() will + // prune out the ACTION_UPDATE we create here. + SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl); + change_list->push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data)); + } } - return true; } TemplateURL* TemplateURLService::FindDuplicateOfSyncTemplateURL( @@ -1923,6 +2130,7 @@ void TemplateURLService::MergeSyncAndLocalURLDuplicates( TemplateURL* sync_turl, TemplateURL* local_turl, SyncChangeList* change_list) { + DCHECK(loaded_); DCHECK(sync_turl); DCHECK(local_turl); DCHECK(change_list); @@ -1997,7 +2205,7 @@ void TemplateURLService::PatchMissingSyncGUIDs( !template_url->IsExtensionKeyword()) { template_url->data_.sync_guid = guid::GenerateGUID(); if (service_.get()) - service_->UpdateKeyword(*template_url); + service_->UpdateKeyword(template_url->data()); } } } diff --git a/chrome/browser/search_engines/template_url_service.h b/chrome/browser/search_engines/template_url_service.h index 60618e204..cb571f6 100644 --- a/chrome/browser/search_engines/template_url_service.h +++ b/chrome/browser/search_engines/template_url_service.h @@ -83,12 +83,10 @@ class TemplateURLService : public WebDataServiceConsumer, TemplateURLService(const Initializer* initializers, const int count); virtual ~TemplateURLService(); - // Generates a suitable keyword for the specified url. Returns an empty - // string if a keyword couldn't be generated. If |autodetected| is true, we - // don't generate keywords for a variety of situations where we would probably - // not want to auto-add keywords, such as keywords for searches on pages that - // themselves come from form submissions. - static string16 GenerateKeyword(const GURL& url, bool autodetected); + // Generates a suitable keyword for the specified url, which must be valid. + // This is guaranteed not to return an empty string, since TemplateURLs should + // never have an empty keyword. + static string16 GenerateKeyword(const GURL& url); // Removes any unnecessary characters from a user input keyword. // This removes the leading scheme, "www." and any trailing slash. @@ -101,8 +99,9 @@ class TemplateURLService : public WebDataServiceConsumer, static GURL GenerateSearchURL(TemplateURL* t_url); // Just like GenerateSearchURL except that it takes SearchTermsData to supply - // the data for some search terms. Most of the time GenerateSearchURL should - // be called. + // the data for some search terms, e.g. so this can be used on threads other + // than the UI thread. See the various TemplateURLRef::XXXUsingTermsData() + // functions. static GURL GenerateSearchURLUsingTermsData( const TemplateURL* t_url, const SearchTermsData& search_terms_data); @@ -140,7 +139,8 @@ class TemplateURLService : public WebDataServiceConsumer, // or NULL if there are no such TemplateURLs TemplateURL* GetTemplateURLForHost(const std::string& host); - // Takes ownership of |template_url| and adds it to this model. + // Takes ownership of |template_url| and adds it to this model. For obvious + // reasons, it is illegal to Add() the same |template_url| pointer twice. void Add(TemplateURL* template_url); // Like Add(), but overwrites the |template_url|'s values with the provided @@ -413,12 +413,17 @@ class TemplateURLService : public WebDataServiceConsumer, // in the default list and is marked as safe_for_autoreplace. bool CanReplace(const TemplateURL* t_url); + // Like GetTemplateURLForKeyword(), but ignores extension-provided keywords. + TemplateURL* FindNonExtensionTemplateURLForKeyword(const string16& keyword); + // 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. + // |new_values|, but the ID for |existing_turl| is retained. Notifying + // observers is the responsibility of the caller. Returns whether + // |existing_turl| was found in |template_urls_| and thus could be updated. + // // NOTE: This should not be called with an extension keyword as there are no // updates needed in that case. - void UpdateNoNotify(TemplateURL* existing_turl, + bool UpdateNoNotify(TemplateURL* existing_turl, const TemplateURL& new_values); // Returns the preferences we use. @@ -451,16 +456,21 @@ class TemplateURLService : public WebDataServiceConsumer, void UpdateDefaultSearch(); // Set the default search provider even if it is managed. |url| may be null. - // Caller is responsible for notifying observers. - void SetDefaultSearchProviderNoNotify(TemplateURL* url); + // Caller is responsible for notifying observers. Returns whether |url| was + // found in |template_urls_| and thus could be made default. + bool SetDefaultSearchProviderNoNotify(TemplateURL* url); // 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, bool newly_adding); + // This function guarantees that on return the model will not have two + // non-extension TemplateURLs with the same keyword. If that means that it + // cannot add the provided argument, it will delete it and return false. + // Caller is responsible for notifying observers if this function returns + // true. + bool 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. @@ -491,15 +501,14 @@ class TemplateURLService : public WebDataServiceConsumer, // it is unique to the Service. string16 UniquifyKeyword(const TemplateURL& turl); - // Given a TemplateURL from Sync (cloud), resolves any keyword conflicts by - // checking the local keywords and uniquifying either the cloud keyword or a - // conflicting local keyword (whichever is older). If the cloud TURL is - // changed, then an appropriate SyncChange is appended to |change_list|. If - // a local TURL is changed, the service is updated with the new keyword. If - // there was no conflict to begin with, this does nothing. In the case of tied - // last_modified dates, |sync_turl| wins. Returns true iff there was a - // conflict. - bool ResolveSyncKeywordConflict(TemplateURL* sync_turl, + // Given a TemplateURL from Sync (cloud) and a local TemplateURL with the same + // keyword, resolves the conflict by uniquifying either the cloud keyword or + // the local keyword (whichever is older). If the cloud TURL is changed, then + // an appropriate SyncChange is appended to |change_list|. If a local TURL is + // changed, the service is updated with the new keyword. In the case of tied + // last_modified dates, |sync_turl| wins. + void ResolveSyncKeywordConflict(TemplateURL* sync_turl, + TemplateURL* local_turl, SyncChangeList* change_list); // Returns a TemplateURL from the service that has the same keyword and search 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 451538e..5378817 100644 --- a/chrome/browser/search_engines/template_url_service_sync_unittest.cc +++ b/chrome/browser/search_engines/template_url_service_sync_unittest.cc @@ -6,6 +6,7 @@ #include "base/string_util.h" #include "base/time.h" #include "base/utf_string_conversions.h" +#include "chrome/browser/search_engines/search_terms_data.h" #include "chrome/browser/search_engines/template_url.h" #include "chrome/browser/search_engines/template_url_service.h" #include "chrome/browser/search_engines/template_url_service_factory.h" @@ -14,9 +15,11 @@ #include "chrome/browser/sync/api/sync_error_factory_mock.h" #include "chrome/common/chrome_notification_types.h" #include "chrome/common/pref_names.h" +#include "chrome/common/url_constants.h" #include "chrome/test/base/testing_pref_service.h" #include "chrome/test/base/testing_profile.h" #include "content/public/browser/notification_service.h" +#include "net/base/net_util.h" #include "sync/protocol/search_engine_specifics.pb.h" #include "sync/protocol/sync.pb.h" #include "testing/gtest/include/gtest/gtest.h" @@ -41,17 +44,19 @@ std::string GetKeyword(const SyncData& sync_data) { } // Much like TemplateURLService::CreateSyncDataFromTemplateURL(), but allows the -// caller to override the URL or GUID fields with empty strings, in order to -// create illegal data that should be DELETEd when we try to sync it to a +// caller to override the keyword, URL, or GUID fields with empty strings, in +// order to create custom data that should be handled specially when synced to a // client. -SyncData CreateBogusSyncData(const TemplateURL& turl, - const std::string& url, - const std::string& sync_guid) { +SyncData CreateCustomSyncData(const TemplateURL& turl, + bool autogenerate_keyword, + const std::string& url, + const std::string& sync_guid) { sync_pb::EntitySpecifics specifics; sync_pb::SearchEngineSpecifics* se_specifics = specifics.mutable_search_engine(); se_specifics->set_short_name(UTF16ToUTF8(turl.short_name())); - se_specifics->set_keyword(UTF16ToUTF8(turl.keyword())); + se_specifics->set_keyword( + autogenerate_keyword ? std::string() : UTF16ToUTF8(turl.keyword())); se_specifics->set_favicon_url(turl.favicon_url().spec()); se_specifics->set_url(url); se_specifics->set_safe_for_autoreplace(turl.safe_for_autoreplace()); @@ -61,7 +66,7 @@ SyncData CreateBogusSyncData(const TemplateURL& turl, se_specifics->set_show_in_default_list(turl.show_in_default_list()); se_specifics->set_suggestions_url(turl.suggestions_url()); se_specifics->set_prepopulate_id(turl.prepopulate_id()); - se_specifics->set_autogenerate_keyword(turl.autogenerate_keyword()); + se_specifics->set_autogenerate_keyword(autogenerate_keyword); se_specifics->set_instant_url(turl.instant_url()); se_specifics->set_last_modified(turl.last_modified().ToInternalValue()); se_specifics->set_sync_guid(sync_guid); @@ -189,7 +194,8 @@ class TemplateURLServiceSyncTest : public testing::Test { const std::string& url, const std::string& guid = std::string(), time_t last_mod = 100, - bool created_by_policy = false) const; + bool created_by_policy = false, + bool safe_for_autoreplace = true) const; // Verifies the two TemplateURLs are equal. // TODO(stevet): Share this with TemplateURLServiceTest. @@ -264,13 +270,14 @@ TemplateURL* TemplateURLServiceSyncTest::CreateTestTemplateURL( const std::string& url, const std::string& guid, time_t last_mod, - bool created_by_policy) const { + bool created_by_policy, + bool safe_for_autoreplace) const { TemplateURLData data; data.short_name = ASCIIToUTF16("unittest"); data.SetKeyword(keyword); data.SetURL(url); data.favicon_url = GURL("http://favicon.url"); - data.safe_for_autoreplace = true; + data.safe_for_autoreplace = safe_for_autoreplace; data.date_created = Time::FromTimeT(100); data.last_modified = Time::FromTimeT(last_mod); data.created_by_policy = created_by_policy; @@ -381,7 +388,7 @@ TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataNoExtensions) { model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com")); model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key2"), "http://key2.com")); model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key3"), - "chrome-extension://blahblahblah")); + std::string(chrome::kExtensionScheme) + "://blahblahblah")); SyncDataList all_sync_data = model()->GetAllSyncData(syncable::SEARCH_ENGINES); @@ -446,65 +453,58 @@ TEST_F(TemplateURLServiceSyncTest, UniquifyKeyword) { } TEST_F(TemplateURLServiceSyncTest, ResolveSyncKeywordConflict) { + // Create a keyword that conflicts, and make it older. + // Conflict, sync keyword is uniquified, and a SyncChange is added. string16 original_turl_keyword = ASCIIToUTF16("key1"); TemplateURL* original_turl = CreateTestTemplateURL(original_turl_keyword, "http://key1.com", std::string(), 9000); model()->Add(original_turl); - - // Create a key that does not conflict with something in the model. - scoped_ptr<TemplateURL> sync_turl( - CreateTestTemplateURL(ASCIIToUTF16("unique"), "http://new.com")); - string16 sync_keyword = sync_turl->keyword(); + scoped_ptr<TemplateURL> sync_turl(CreateTestTemplateURL(original_turl_keyword, + "http://new.com", "remote", 8999)); SyncChangeList changes; - - // No conflict, no TURLs changed, no changes. - EXPECT_FALSE(model()->ResolveSyncKeywordConflict(sync_turl.get(), &changes)); - EXPECT_EQ(original_turl_keyword, original_turl->keyword()); - EXPECT_EQ(sync_keyword, sync_turl->keyword()); - EXPECT_EQ(0U, changes.size()); - - // Change sync keyword to something that conflicts, and make it older. - // Conflict, sync keyword is uniquified, and a SyncChange is added. - sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com", - std::string(), 8999)); - EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(), &changes)); + model()->ResolveSyncKeywordConflict(sync_turl.get(), original_turl, &changes); EXPECT_NE(original_turl_keyword, sync_turl->keyword()); EXPECT_EQ(original_turl_keyword, original_turl->keyword()); - EXPECT_EQ(1U, changes.size()); + ASSERT_EQ(1U, changes.size()); + EXPECT_EQ("remote", GetGUID(changes[0].sync_data())); changes.clear(); - // Sync is newer. Original TemplateURL keyword is uniquified, no SyncChange - // is added. Also ensure that this does not change the safe_for_autoreplace - // flag or the TemplateURLID in the original. + // Sync is newer. Original TemplateURL keyword is uniquified. A SyncChange is + // added (which in a normal run would be deleted by + // PreventDuplicateGUIDUpdates() after we subsequently add an ACTION_ADD + // change for the local URL). Also ensure that this does not change the + // safe_for_autoreplace flag or the TemplateURLID in the original. model()->Remove(original_turl); original_turl = CreateTestTemplateURL(original_turl_keyword, - "http://key1.com", std::string(), 9000); + "http://key1.com", "local", 9000); model()->Add(original_turl); TemplateURLID original_id = original_turl->id(); sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com", std::string(), 9001)); - EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(), &changes)); + model()->ResolveSyncKeywordConflict(sync_turl.get(), original_turl, &changes); EXPECT_EQ(original_turl_keyword, sync_turl->keyword()); EXPECT_NE(original_turl_keyword, original_turl->keyword()); EXPECT_TRUE(original_turl->safe_for_autoreplace()); EXPECT_EQ(original_id, original_turl->id()); EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(original_turl_keyword)); - EXPECT_EQ(0U, changes.size()); + ASSERT_EQ(1U, changes.size()); + EXPECT_EQ("local", GetGUID(changes[0].sync_data())); changes.clear(); // Equal times. Same result as above. Sync left alone, original uniquified so // sync_turl can fit. model()->Remove(original_turl); original_turl = CreateTestTemplateURL(original_turl_keyword, - "http://key1.com", std::string(), 9000); + "http://key1.com", "local2", 9000); model()->Add(original_turl); sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com", std::string(), 9000)); - EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(), &changes)); + model()->ResolveSyncKeywordConflict(sync_turl.get(), original_turl, &changes); EXPECT_EQ(original_turl_keyword, sync_turl->keyword()); EXPECT_NE(original_turl_keyword, original_turl->keyword()); EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(original_turl_keyword)); - EXPECT_EQ(0U, changes.size()); + ASSERT_EQ(1U, changes.size()); + EXPECT_EQ("local2", GetGUID(changes[0].sync_data())); changes.clear(); // Sync is newer, but original TemplateURL is created by policy, so it wins. @@ -514,12 +514,13 @@ TEST_F(TemplateURLServiceSyncTest, ResolveSyncKeywordConflict) { "http://key1.com", std::string(), 9000, true); model()->Add(original_turl); sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com", - std::string(), 9999)); - EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(), &changes)); + "remote2", 9999)); + model()->ResolveSyncKeywordConflict(sync_turl.get(), original_turl, &changes); EXPECT_NE(original_turl_keyword, sync_turl->keyword()); EXPECT_EQ(original_turl_keyword, original_turl->keyword()); EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(sync_turl->keyword())); - EXPECT_EQ(1U, changes.size()); + ASSERT_EQ(1U, changes.size()); + EXPECT_EQ("remote2", GetGUID(changes[0].sync_data())); changes.clear(); } @@ -581,10 +582,8 @@ TEST_F(TemplateURLServiceSyncTest, MergeSyncAndLocalURLDuplicates) { } TEST_F(TemplateURLServiceSyncTest, StartSyncEmpty) { - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, SyncDataList(), - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, SyncDataList(), + PassProcessor(), CreateAndPassSyncErrorFactory()); EXPECT_EQ(0U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); EXPECT_EQ(0U, processor()->change_list_size()); @@ -593,10 +592,8 @@ TEST_F(TemplateURLServiceSyncTest, StartSyncEmpty) { TEST_F(TemplateURLServiceSyncTest, MergeIntoEmpty) { SyncDataList initial_data = CreateInitialSyncData(); - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, initial_data, - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); // We expect the model to have accepted all of the initial sync data. Search @@ -611,18 +608,16 @@ TEST_F(TemplateURLServiceSyncTest, MergeIntoEmpty) { } TEST_F(TemplateURLServiceSyncTest, MergeInAllNewData) { - model()->Add(CreateTestTemplateURL(ASCIIToUTF16("google.com"), - "http://google.com", "abc")); - model()->Add(CreateTestTemplateURL(ASCIIToUTF16("yahoo.com"), - "http://yahoo.com", "def")); - model()->Add(CreateTestTemplateURL(ASCIIToUTF16("bing.com"), - "http://bing.com", "xyz")); + model()->Add(CreateTestTemplateURL(ASCIIToUTF16("abc.com"), "http://abc.com", + "abc")); + model()->Add(CreateTestTemplateURL(ASCIIToUTF16("def.com"), "http://def.com", + "def")); + model()->Add(CreateTestTemplateURL(ASCIIToUTF16("xyz.com"), "http://xyz.com", + "xyz")); SyncDataList initial_data = CreateInitialSyncData(); - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, initial_data, - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); EXPECT_EQ(6U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); // We expect the model to have accepted all of the initial sync data. Search @@ -633,9 +628,9 @@ TEST_F(TemplateURLServiceSyncTest, MergeInAllNewData) { EXPECT_TRUE(model()->GetTemplateURLForGUID(guid)); } // All the original TemplateURLs should also remain in the model. - EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("google.com"))); - EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("yahoo.com"))); - EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("bing.com"))); + EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("abc.com"))); + EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("def.com"))); + EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("xyz.com"))); // Ensure that Sync received the expected changes. EXPECT_EQ(3U, processor()->change_list_size()); EXPECT_TRUE(processor()->contains_guid("abc")); @@ -654,10 +649,8 @@ TEST_F(TemplateURLServiceSyncTest, MergeSyncIsTheSame) { model()->Add(converted); } - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, initial_data, - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); for (SyncDataList::const_iterator iter = initial_data.begin(); @@ -672,41 +665,39 @@ TEST_F(TemplateURLServiceSyncTest, MergeUpdateFromSync) { // The local data is the same as the sync data merged in, but timestamps have // changed. Ensure the right fields are merged in. SyncDataList initial_data; - TemplateURL* turl1 = CreateTestTemplateURL(ASCIIToUTF16("google.com"), - "http://google.com", "abc", 9000); + TemplateURL* turl1 = CreateTestTemplateURL(ASCIIToUTF16("abc.com"), + "http://abc.com", "abc", 9000); model()->Add(turl1); - TemplateURL* turl2 = CreateTestTemplateURL(ASCIIToUTF16("bing.com"), - "http://bing.com", "xyz", 9000); + TemplateURL* turl2 = CreateTestTemplateURL(ASCIIToUTF16("xyz.com"), + "http://xyz.com", "xyz", 9000); model()->Add(turl2); scoped_ptr<TemplateURL> turl1_newer(CreateTestTemplateURL( - ASCIIToUTF16("google.com"), "http://google.ca", "abc", 9999)); + ASCIIToUTF16("abc.com"), "http://abc.ca", "abc", 9999)); initial_data.push_back( TemplateURLService::CreateSyncDataFromTemplateURL(*turl1_newer)); scoped_ptr<TemplateURL> turl2_older(CreateTestTemplateURL( - ASCIIToUTF16("bing.com"), "http://bing.ca", "xyz", 8888)); + ASCIIToUTF16("xyz.com"), "http://xyz.ca", "xyz", 8888)); initial_data.push_back( TemplateURLService::CreateSyncDataFromTemplateURL(*turl2_older)); - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, initial_data, - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); // Both were local updates, so we expect the same count. EXPECT_EQ(2U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); - // Check that the first replaced the initial Google TemplateURL. + // Check that the first replaced the initial abc TemplateURL. EXPECT_EQ(turl1, model()->GetTemplateURLForGUID("abc")); - EXPECT_EQ("http://google.ca", turl1->url()); + EXPECT_EQ("http://abc.ca", turl1->url()); - // Check that the second produced an upstream update to the Bing TemplateURL. + // Check that the second produced an upstream update to the xyz TemplateURL. EXPECT_EQ(1U, processor()->change_list_size()); ASSERT_TRUE(processor()->contains_guid("xyz")); SyncChange change = processor()->change_for_guid("xyz"); EXPECT_TRUE(change.change_type() == SyncChange::ACTION_UPDATE); - EXPECT_EQ("http://bing.com", GetURL(change.sync_data())); + EXPECT_EQ("http://xyz.com", GetURL(change.sync_data())); } TEST_F(TemplateURLServiceSyncTest, MergeAddFromOlderSyncData) { @@ -722,10 +713,9 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromOlderSyncData) { model()->Add(CreateTestTemplateURL(ASCIIToUTF16("unique"), "http://unique.com", "ccc")); // add - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, CreateInitialSyncData(), - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + CreateInitialSyncData(), PassProcessor(), + CreateAndPassSyncErrorFactory()); // The dupe results in a merge. The other two should be added to the model. EXPECT_EQ(5U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -782,11 +772,9 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) { model()->Add(CreateTestTemplateURL(ASCIIToUTF16("unique"), "http://unique.com", "ccc", 10)); // add - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, - CreateInitialSyncData(), - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + CreateInitialSyncData(), PassProcessor(), + CreateAndPassSyncErrorFactory()); // The dupe results in a merge. The other two should be added to the model. EXPECT_EQ(5U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -824,10 +812,8 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) { TEST_F(TemplateURLServiceSyncTest, ProcessChangesEmptyModel) { // We initially have no data. - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, SyncDataList(), - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, SyncDataList(), + PassProcessor(), CreateAndPassSyncErrorFactory()); // Set up a bunch of ADDs. SyncChangeList changes; @@ -848,10 +834,9 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesEmptyModel) { } TEST_F(TemplateURLServiceSyncTest, ProcessChangesNoConflicts) { - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, - CreateInitialSyncData(), PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + CreateInitialSyncData(), PassProcessor(), + CreateAndPassSyncErrorFactory()); // Process different types of changes, without conflicts. SyncChangeList changes; @@ -879,10 +864,9 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesNoConflicts) { } TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsSyncWins) { - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, - CreateInitialSyncData(), PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + CreateInitialSyncData(), PassProcessor(), + CreateAndPassSyncErrorFactory()); // Process different types of changes, with conflicts. Note that all this data // has a newer timestamp, so Sync will win in these scenarios. @@ -917,10 +901,9 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsSyncWins) { } TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsLocalWins) { - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, - CreateInitialSyncData(), PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + CreateInitialSyncData(), PassProcessor(), + CreateAndPassSyncErrorFactory()); // Process different types of changes, with conflicts. Note that all this data // has an older timestamp, so the local data will win in these scenarios. @@ -966,10 +949,9 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsLocalWins) { TEST_F(TemplateURLServiceSyncTest, ProcessTemplateURLChange) { // Ensure that ProcessTemplateURLChange is called and pushes the correct // changes to Sync whenever local changes are made to TemplateURLs. - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, - CreateInitialSyncData(), PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + CreateInitialSyncData(), PassProcessor(), + CreateAndPassSyncErrorFactory()); // Add a new search engine. TemplateURL* new_turl = @@ -1001,12 +983,179 @@ TEST_F(TemplateURLServiceSyncTest, ProcessTemplateURLChange) { EXPECT_EQ(SyncChange::ACTION_DELETE, change.change_type()); } +TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithLocalExtensions) { + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + CreateInitialSyncData(), PassProcessor(), + CreateAndPassSyncErrorFactory()); + + // Add some extension keywords locally. These shouldn't be synced. + TemplateURL* extension1 = CreateTestTemplateURL(ASCIIToUTF16("keyword1"), + std::string(chrome::kExtensionScheme) + "://extension1"); + model()->Add(extension1); + TemplateURL* extension2 = CreateTestTemplateURL(ASCIIToUTF16("keyword2"), + std::string(chrome::kExtensionScheme) + "://extension2"); + model()->Add(extension2); + EXPECT_EQ(0U, processor()->change_list_size()); + + // Create some sync changes that will conflict with the extension keywords. + SyncChangeList changes; + changes.push_back(CreateTestSyncChange(SyncChange::ACTION_ADD, + CreateTestTemplateURL(ASCIIToUTF16("keyword1"), "http://aaa.com"))); + changes.push_back(CreateTestSyncChange(SyncChange::ACTION_ADD, + CreateTestTemplateURL(ASCIIToUTF16("keyword2"), "http://bbb.com", + std::string(), 100, false, false))); + model()->ProcessSyncChanges(FROM_HERE, changes); + + // The synced keywords should be added unchanged, and the result of + // GetTemplateURLForKeyword() for each keyword should depend on whether the + // synced keyword is replaceable or not. + EXPECT_EQ(extension1, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword1"))); + EXPECT_FALSE(model()->GetTemplateURLForHost("aaa.com") == NULL); + TemplateURL* url_for_keyword2 = + model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword2")); + EXPECT_NE(extension2, url_for_keyword2); + EXPECT_EQ("http://bbb.com", url_for_keyword2->url()); + // Note that extensions don't use host-based keywords, so we can't check that + // |extension2| is still in the model using GetTemplateURLForHost(); and we + // have to create a full-fledged Extension to use + // GetTemplateURLForExtension(), which is annoying, so just grab all the URLs + // from the model and search for |extension2| within them. + TemplateURLService::TemplateURLVector template_urls( + model()->GetTemplateURLs()); + EXPECT_FALSE(std::find(template_urls.begin(), template_urls.end(), + extension2) == template_urls.end()); +} + +TEST_F(TemplateURLServiceSyncTest, AutogeneratedKeywordMigrated) { + // Create a couple of sync entries with autogenerated keywords. + SyncDataList initial_data; + scoped_ptr<TemplateURL> turl( + CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com", "key1")); + initial_data.push_back( + CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid())); + turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key2"), + "{google:baseURL}search?q={searchTerms}", "key2")); + initial_data.push_back( + CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid())); + + // Now try to sync the data locally. + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); + + // Both entries should have been added, with explicit keywords. + TemplateURL* key1 = model()->GetTemplateURLForHost("key1.com"); + ASSERT_FALSE(key1 == NULL); + EXPECT_EQ(ASCIIToUTF16("key1.com"), key1->keyword()); + std::string google_hostname( + GURL(UIThreadSearchTermsData().GoogleBaseURLValue()).host()); + TemplateURL* key2 = model()->GetTemplateURLForHost(google_hostname); + ASSERT_FALSE(key2 == NULL); + string16 google_keyword(net::StripWWW(ASCIIToUTF16(google_hostname))); + EXPECT_EQ(google_keyword, key2->keyword()); + + // We should also have gotten some corresponding UPDATEs pushed upstream. + EXPECT_GE(processor()->change_list_size(), 2U); + ASSERT_TRUE(processor()->contains_guid("key1")); + SyncChange key1_change = processor()->change_for_guid("key1"); + EXPECT_EQ(SyncChange::ACTION_UPDATE, key1_change.change_type()); + EXPECT_EQ("key1.com", GetKeyword(key1_change.sync_data())); + ASSERT_TRUE(processor()->contains_guid("key2")); + SyncChange key2_change = processor()->change_for_guid("key2"); + EXPECT_EQ(SyncChange::ACTION_UPDATE, key2_change.change_type()); + EXPECT_EQ(google_keyword, UTF8ToUTF16(GetKeyword(key2_change.sync_data()))); +} + +TEST_F(TemplateURLServiceSyncTest, AutogeneratedKeywordConflicts) { + // Sync brings in some autogenerated keywords, but the generated keywords we + // try to create conflict with ones in the model. + string16 google_keyword(net::StripWWW(ASCIIToUTF16(GURL( + UIThreadSearchTermsData().GoogleBaseURLValue()).host()))); + TemplateURL* google = CreateTestTemplateURL(google_keyword, + "{google:baseURL}1/search?q={searchTerms}"); + model()->Add(google); + TemplateURL* other = + CreateTestTemplateURL(ASCIIToUTF16("other.com"), "http://other.com/foo"); + model()->Add(other); + SyncDataList initial_data; + scoped_ptr<TemplateURL> turl(CreateTestTemplateURL(ASCIIToUTF16("sync1"), + "{google:baseURL}2/search?q={searchTerms}", "sync1", 50)); + initial_data.push_back( + CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid())); + turl.reset(CreateTestTemplateURL(ASCIIToUTF16("sync2"), + "http://other.com/search?q={searchTerms}", "sync2", 150)); + initial_data.push_back( + CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid())); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); + + // In this case, the conflicts should be handled just like any other keyword + // conflicts -- the later-modified TemplateURL is assumed to be authoritative. + EXPECT_EQ(google_keyword, google->keyword()); + EXPECT_EQ(google_keyword + ASCIIToUTF16("_"), + model()->GetTemplateURLForGUID("sync1")->keyword()); + EXPECT_EQ(ASCIIToUTF16("other.com_"), other->keyword()); + EXPECT_EQ(ASCIIToUTF16("other.com"), + model()->GetTemplateURLForGUID("sync2")->keyword()); + + // Both synced URLs should have associated UPDATEs, since both needed their + // keywords to be generated (and sync1 needed conflict resolution as well). + EXPECT_GE(processor()->change_list_size(), 2U); + ASSERT_TRUE(processor()->contains_guid("sync1")); + SyncChange sync1_change = processor()->change_for_guid("sync1"); + EXPECT_EQ(SyncChange::ACTION_UPDATE, sync1_change.change_type()); + EXPECT_EQ(google_keyword + ASCIIToUTF16("_"), + UTF8ToUTF16(GetKeyword(sync1_change.sync_data()))); + ASSERT_TRUE(processor()->contains_guid("sync2")); + SyncChange sync2_change = processor()->change_for_guid("sync2"); + EXPECT_EQ(SyncChange::ACTION_UPDATE, sync2_change.change_type()); + EXPECT_EQ("other.com", GetKeyword(sync2_change.sync_data())); +} + +TEST_F(TemplateURLServiceSyncTest, TwoAutogeneratedKeywordsUsingGoogleBaseURL) { + // Sync brings in two autogenerated keywords and both use Google base URLs. + // We make the first older so that it will get renamed once before the second + // and then again once after (when we resolve conflicts for the second). + SyncDataList initial_data; + scoped_ptr<TemplateURL> turl(CreateTestTemplateURL(ASCIIToUTF16("key1"), + "{google:baseURL}1/search?q={searchTerms}", "key1", 50)); + initial_data.push_back( + CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid())); + turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key2"), + "{google:baseURL}2/search?q={searchTerms}", "key2")); + initial_data.push_back( + CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid())); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); + + // We should still have coalesced the updates to one each. + string16 google_keyword(net::StripWWW(ASCIIToUTF16(GURL( + UIThreadSearchTermsData().GoogleBaseURLValue()).host()))); + TemplateURL* keyword1 = + model()->GetTemplateURLForKeyword(google_keyword + ASCIIToUTF16("_")); + ASSERT_FALSE(keyword1 == NULL); + EXPECT_EQ("key1", keyword1->sync_guid()); + TemplateURL* keyword2 = model()->GetTemplateURLForKeyword(google_keyword); + ASSERT_FALSE(keyword2 == NULL); + EXPECT_EQ("key2", keyword2->sync_guid()); + EXPECT_GE(processor()->change_list_size(), 2U); + ASSERT_TRUE(processor()->contains_guid("key1")); + SyncChange key1_change = processor()->change_for_guid("key1"); + EXPECT_EQ(SyncChange::ACTION_UPDATE, key1_change.change_type()); + EXPECT_EQ(keyword1->keyword(), + UTF8ToUTF16(GetKeyword(key1_change.sync_data()))); + ASSERT_TRUE(processor()->contains_guid("key2")); + SyncChange key2_change = processor()->change_for_guid("key2"); + EXPECT_EQ(SyncChange::ACTION_UPDATE, key2_change.change_type()); + EXPECT_EQ(keyword2->keyword(), + UTF8ToUTF16(GetKeyword(key2_change.sync_data()))); +} + TEST_F(TemplateURLServiceSyncTest, MergeTwoClientsBasic) { // Start off B with some empty data. - model_b()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, - CreateInitialSyncData(), PassProcessor(), - CreateAndPassSyncErrorFactory()); + model_b()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + CreateInitialSyncData(), PassProcessor(), + CreateAndPassSyncErrorFactory()); // Merge A and B. All of B's data should transfer over to A, which initially // has no data. @@ -1024,10 +1173,9 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwoClientsBasic) { TEST_F(TemplateURLServiceSyncTest, MergeTwoClientsDupesAndConflicts) { // Start off B with some empty data. - model_b()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, - CreateInitialSyncData(), PassProcessor(), - CreateAndPassSyncErrorFactory()); + model_b()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + CreateInitialSyncData(), PassProcessor(), + CreateAndPassSyncErrorFactory()); // Set up A so we have some interesting duplicates and conflicts. model_a()->Add(CreateTestTemplateURL(ASCIIToUTF16("key4"), "http://key4.com", @@ -1042,8 +1190,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwoClientsDupesAndConflicts) { // Merge A and B. scoped_ptr<SyncChangeProcessorDelegate> delegate_b( new SyncChangeProcessorDelegate(model_b())); - model_a()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, + model_a()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, model_b()->GetAllSyncData(syncable::SEARCH_ENGINES), delegate_b.PassAs<SyncChangeProcessor>(), CreateAndPassSyncErrorFactory()); @@ -1054,8 +1201,7 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwoClientsDupesAndConflicts) { } TEST_F(TemplateURLServiceSyncTest, StopSyncing) { - SyncError error = model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, + SyncError error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, CreateInitialSyncData(), PassProcessor(), CreateAndPassSyncErrorFactory()); ASSERT_FALSE(error.IsSet()); @@ -1075,8 +1221,7 @@ TEST_F(TemplateURLServiceSyncTest, StopSyncing) { TEST_F(TemplateURLServiceSyncTest, SyncErrorOnInitialSync) { processor()->set_erroneous(true); - SyncError error = model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, + SyncError error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, CreateInitialSyncData(), PassProcessor(), CreateAndPassSyncErrorFactory()); EXPECT_TRUE(error.IsSet()); @@ -1100,8 +1245,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncErrorOnInitialSync) { TEST_F(TemplateURLServiceSyncTest, SyncErrorOnLaterSync) { // Ensure that if the SyncProcessor succeeds in the initial merge, but fails // in future ProcessSyncChanges, we still return an error. - SyncError error = model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, + SyncError error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, CreateInitialSyncData(), PassProcessor(), CreateAndPassSyncErrorFactory()); ASSERT_FALSE(error.IsSet()); @@ -1124,10 +1268,8 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwiceWithSameSyncData) { model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com", "key1", 10)); // earlier - SyncError error = model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, - initial_data, PassProcessor(), - CreateAndPassSyncErrorFactory()); + SyncError error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + initial_data, PassProcessor(), CreateAndPassSyncErrorFactory()); ASSERT_FALSE(error.IsSet()); // We should have updated the original TemplateURL with Sync's version. @@ -1153,10 +1295,8 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwiceWithSameSyncData) { model()->StopSyncing(syncable::SEARCH_ENGINES); sync_processor_delegate_.reset(new SyncChangeProcessorDelegate( sync_processor_.get())); - error = model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, - initial_data, PassProcessor(), - CreateAndPassSyncErrorFactory()); + error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, + initial_data, PassProcessor(), CreateAndPassSyncErrorFactory()); ASSERT_FALSE(error.IsSet()); // Check that the TemplateURL was not modified. @@ -1171,10 +1311,8 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultGUIDArrivesFirst) { scoped_ptr<TemplateURL> turl(CreateTestTemplateURL(ASCIIToUTF16("key2"), "http://key2.com/{searchTerms}", "key2", 90)); initial_data[1] = TemplateURLService::CreateSyncDataFromTemplateURL(*turl); - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, initial_data, - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); model()->SetDefaultSearchProvider(model()->GetTemplateURLForGUID("key2")); EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -1240,10 +1378,8 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultArrivesAfterStartup) { "http://key2.com/{searchTerms}", "key2", 90)); initial_data[1] = TemplateURLService::CreateSyncDataFromTemplateURL(*turl); - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, initial_data, - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); // Ensure that the new default has been set. EXPECT_EQ(4U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -1271,10 +1407,8 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultAlreadySetOnStartup) { // Now sync the initial data. SyncDataList initial_data = CreateInitialSyncData(); - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, initial_data, - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); // Ensure that the new entries were added and the default has not changed. EXPECT_EQ(4U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -1285,10 +1419,8 @@ TEST_F(TemplateURLServiceSyncTest, SyncWithManagedDefaultSearch) { // First start off with a few entries and make sure we can set an unmanaged // default search provider. SyncDataList initial_data = CreateInitialSyncData(); - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, initial_data, - PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); model()->SetDefaultSearchProvider(model()->GetTemplateURLForGUID("key2")); EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); @@ -1349,10 +1481,8 @@ TEST_F(TemplateURLServiceSyncTest, SyncMergeDeletesDefault) { "http://key1.com/{searchTerms}", "key1", 90)); initial_data[0] = TemplateURLService::CreateSyncDataFromTemplateURL(*turl); - model()->MergeDataAndStartSyncing( - syncable::SEARCH_ENGINES, - initial_data, PassProcessor(), - CreateAndPassSyncErrorFactory()); + model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, + PassProcessor(), CreateAndPassSyncErrorFactory()); EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size()); EXPECT_FALSE(model()->GetTemplateURLForGUID("whateverguid")); @@ -1366,15 +1496,14 @@ TEST_F(TemplateURLServiceSyncTest, DeleteBogusData) { scoped_ptr<TemplateURL> turl( CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com", "key1")); initial_data.push_back( - CreateBogusSyncData(*turl, std::string(), turl->sync_guid())); + CreateCustomSyncData(*turl, false, std::string(), turl->sync_guid())); turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key2"), "http://key2.com")); initial_data.push_back( - CreateBogusSyncData(*turl, turl->url(), std::string())); + CreateCustomSyncData(*turl, false, turl->url(), std::string())); // Now try to sync the data locally. model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES, initial_data, - PassProcessor(), - CreateAndPassSyncErrorFactory()); + PassProcessor(), CreateAndPassSyncErrorFactory()); // Nothing should have been added, and both bogus entries should be marked for // deletion. diff --git a/chrome/browser/search_engines/template_url_service_unittest.cc b/chrome/browser/search_engines/template_url_service_unittest.cc index d214f15..9bf29c6 100644 --- a/chrome/browser/search_engines/template_url_service_unittest.cc +++ b/chrome/browser/search_engines/template_url_service_unittest.cc @@ -21,6 +21,7 @@ #include "chrome/browser/search_engines/template_url_service.h" #include "chrome/browser/search_engines/template_url_service_test_util.h" #include "chrome/browser/webdata/web_database.h" +#include "chrome/common/url_constants.h" #include "chrome/test/base/testing_profile.h" #include "content/test/test_browser_thread.h" #include "testing/gtest/include/gtest/gtest.h" @@ -137,7 +138,6 @@ class TemplateURLServiceTest : public testing::Test { TemplateURL* AddKeywordWithDate(const std::string& short_name, const std::string& keyword, - bool autogenerate_keyword, const std::string& url, const std::string& suggest_url, const std::string& favicon_url, @@ -198,7 +198,6 @@ void TemplateURLServiceTest::TearDown() { TemplateURL* TemplateURLServiceTest::AddKeywordWithDate( const std::string& short_name, const std::string& keyword, - bool autogenerate_keyword, const std::string& url, const std::string& suggest_url, const std::string& favicon_url, @@ -209,7 +208,6 @@ TemplateURL* TemplateURLServiceTest::AddKeywordWithDate( TemplateURLData data; data.short_name = UTF8ToUTF16(short_name); data.SetKeyword(UTF8ToUTF16(keyword)); - data.SetAutogenerateKeyword(autogenerate_keyword); data.SetURL(url); data.suggestions_url = suggest_url; data.favicon_url = GURL(favicon_url); @@ -413,31 +411,167 @@ TEST_F(TemplateURLServiceTest, AddUpdateRemove) { EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("b")) == NULL); } +TEST_F(TemplateURLServiceTest, AddSameKeyword) { + test_util_.VerifyLoad(); + + AddKeywordWithDate("first", "keyword", "http://test1", std::string(), + std::string(), true, "UTF-8", Time(), Time()); + VerifyObserverCount(1); + + // Test what happens when we try to add a TemplateURL with the same keyword as + // one in the model. + TemplateURLData data; + data.short_name = ASCIIToUTF16("second"); + data.SetKeyword(ASCIIToUTF16("keyword")); + data.SetURL("http://test2"); + data.safe_for_autoreplace = false; + TemplateURL* t_url = new TemplateURL(test_util_.profile(), data); + model()->Add(t_url); + + // Because the old TemplateURL was replaceable and the new one wasn't, the new + // one should have replaced the old. + VerifyObserverCount(1); + EXPECT_EQ(t_url, model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword"))); + EXPECT_EQ(ASCIIToUTF16("second"), t_url->short_name()); + EXPECT_EQ(ASCIIToUTF16("keyword"), t_url->keyword()); + EXPECT_FALSE(t_url->safe_for_autoreplace()); + + // Now try adding a replaceable TemplateURL. This should just delete the + // passed-in URL. + data.short_name = ASCIIToUTF16("third"); + data.SetURL("http://test3"); + data.safe_for_autoreplace = true; + model()->Add(new TemplateURL(test_util_.profile(), data)); + VerifyObserverCount(0); + EXPECT_EQ(t_url, model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword"))); + EXPECT_EQ(ASCIIToUTF16("second"), t_url->short_name()); + EXPECT_EQ(ASCIIToUTF16("keyword"), t_url->keyword()); + EXPECT_FALSE(t_url->safe_for_autoreplace()); + + // Now try adding a non-replaceable TemplateURL again. This should uniquify + // the existing entry's keyword. + data.short_name = ASCIIToUTF16("fourth"); + data.SetURL("http://test4"); + data.safe_for_autoreplace = false; + TemplateURL* t_url2 = new TemplateURL(test_util_.profile(), data); + model()->Add(t_url2); + VerifyObserverCount(2); + EXPECT_EQ(t_url2, model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword"))); + EXPECT_EQ(ASCIIToUTF16("fourth"), t_url2->short_name()); + EXPECT_EQ(ASCIIToUTF16("keyword"), t_url2->keyword()); + EXPECT_EQ(ASCIIToUTF16("second"), t_url->short_name()); + EXPECT_EQ(ASCIIToUTF16("test2"), t_url->keyword()); +} + +TEST_F(TemplateURLServiceTest, AddExtensionKeyword) { + TemplateURL* original1 = AddKeywordWithDate("replaceable", "keyword1", + "http://test1", std::string(), std::string(), true, "UTF-8", Time(), + Time()); + TemplateURL* original2 = AddKeywordWithDate("nonreplaceable", "keyword2", + "http://test2", std::string(), std::string(), false, "UTF-8", Time(), + Time()); + TemplateURL* original3 = AddKeywordWithDate("extension", "keyword3", + std::string(chrome::kExtensionScheme) + "://test3", std::string(), + std::string(), false, "UTF-8", Time(), Time()); + + // Add an extension keyword that conflicts with each of the above three + // keywords. + TemplateURLData data; + data.short_name = ASCIIToUTF16("test"); + data.SetKeyword(ASCIIToUTF16("keyword1")); + data.SetURL(std::string(chrome::kExtensionScheme) + "://test4"); + data.safe_for_autoreplace = false; + + // Extension keywords should override replaceable keywords. + TemplateURL* extension1 = new TemplateURL(test_util_.profile(), data); + model()->Add(extension1); + ASSERT_EQ(extension1, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword1"))); + model()->Remove(extension1); + EXPECT_EQ(original1, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword1"))); + + // They should not override non-replaceable keywords. + data.SetKeyword(ASCIIToUTF16("keyword2")); + TemplateURL* extension2 = new TemplateURL(test_util_.profile(), data); + model()->Add(extension2); + ASSERT_EQ(original2, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword2"))); + model()->Remove(original2); + EXPECT_EQ(extension2, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword2"))); + + // They should override extension keywords added earlier. + data.SetKeyword(ASCIIToUTF16("keyword3")); + TemplateURL* extension3 = new TemplateURL(test_util_.profile(), data); + model()->Add(extension3); + ASSERT_EQ(extension3, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword3"))); + model()->Remove(extension3); + EXPECT_EQ(original3, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword3"))); +} + +TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) { + test_util_.VerifyLoad(); + + // Similar to the AddSameKeyword test, but with an extension keyword masking a + // replaceable TemplateURL. We should still do correct conflict resolution + // between the non-template URLs. + AddKeywordWithDate("replaceable", "keyword", "http://test1", std::string(), + std::string(), true, "UTF-8", Time(), Time()); + TemplateURL* extension = AddKeywordWithDate("extension", "keyword", + std::string(chrome::kExtensionScheme) + "://test2", std::string(), + std::string(), false, "UTF-8", Time(), Time()); + + // Adding another replaceable keyword should remove the existing one, but + // leave the extension as the associated URL for this keyword. + TemplateURLData data; + data.short_name = ASCIIToUTF16("name1"); + data.SetKeyword(ASCIIToUTF16("keyword")); + data.SetURL("http://test3"); + data.safe_for_autoreplace = true; + TemplateURL* t_url = new TemplateURL(test_util_.profile(), data); + model()->Add(t_url); + EXPECT_EQ(extension, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword"))); + EXPECT_TRUE(model()->GetTemplateURLForHost("test1") == NULL); + EXPECT_EQ(t_url, model()->GetTemplateURLForHost("test3")); + + // Adding a nonreplaceable keyword should remove the existing replaceable + // keyword and replace the extension as the associated URL for this keyword, + // but not evict the extension from the service entirely. + data.short_name = ASCIIToUTF16("name2"); + data.SetURL("http://test4"); + data.safe_for_autoreplace = false; + TemplateURL* t_url2 = new TemplateURL(test_util_.profile(), data); + model()->Add(t_url2); + EXPECT_EQ(t_url2, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword"))); + EXPECT_TRUE(model()->GetTemplateURLForHost("test3") == NULL); + // Note that extensions don't use host-based keywords, so we can't check that + // the extension is still in the model using GetTemplateURLForHost(); and we + // have to create a full-fledged Extension to use + // GetTemplateURLForExtension(), which is annoying, so just grab all the URLs + // from the model and search for |extension| within them. + TemplateURLService::TemplateURLVector template_urls( + model()->GetTemplateURLs()); + EXPECT_FALSE(std::find(template_urls.begin(), template_urls.end(), + extension) == template_urls.end()); +} + TEST_F(TemplateURLServiceTest, GenerateKeyword) { - ASSERT_EQ(string16(), TemplateURLService::GenerateKeyword(GURL(), true)); - // Shouldn't generate keywords for https. - ASSERT_EQ(string16(), - TemplateURLService::GenerateKeyword(GURL("https://blah"), true)); ASSERT_EQ(ASCIIToUTF16("foo"), - TemplateURLService::GenerateKeyword(GURL("http://foo"), true)); + TemplateURLService::GenerateKeyword(GURL("http://foo"))); // www. should be stripped. ASSERT_EQ(ASCIIToUTF16("foo"), - TemplateURLService::GenerateKeyword(GURL("http://www.foo"), true)); - // Shouldn't generate keywords with paths, if autodetected. - ASSERT_EQ(string16(), - TemplateURLService::GenerateKeyword(GURL("http://blah/foo"), true)); + TemplateURLService::GenerateKeyword(GURL("http://www.foo"))); + // Make sure we don't get a trailing '/'. ASSERT_EQ(ASCIIToUTF16("blah"), - TemplateURLService::GenerateKeyword(GURL("http://blah/foo"), - false)); - // FTP shouldn't generate a keyword. - ASSERT_EQ(string16(), - TemplateURLService::GenerateKeyword(GURL("ftp://blah/"), true)); - // Make sure we don't get a trailing / - ASSERT_EQ(ASCIIToUTF16("blah"), - TemplateURLService::GenerateKeyword(GURL("http://blah/"), true)); + TemplateURLService::GenerateKeyword(GURL("http://blah/"))); // Don't generate the empty string. ASSERT_EQ(ASCIIToUTF16("www"), - TemplateURLService::GenerateKeyword(GURL("http://www."), false)); + TemplateURLService::GenerateKeyword(GURL("http://www."))); } TEST_F(TemplateURLServiceTest, GenerateSearchURL) { @@ -471,19 +605,19 @@ TEST_F(TemplateURLServiceTest, ClearBrowsingData_Keywords) { EXPECT_EQ(0U, model()->GetTemplateURLs().size()); // Create one with a 0 time. - AddKeywordWithDate("name1", "key1", false, "http://foo1", "http://suggest1", + AddKeywordWithDate("name1", "key1", "http://foo1", "http://suggest1", "http://icon1", true, "UTF-8;UTF-16", Time(), Time()); // Create one for now and +/- 1 day. - AddKeywordWithDate("name2", "key2", false, "http://foo2", "http://suggest2", + AddKeywordWithDate("name2", "key2", "http://foo2", "http://suggest2", "http://icon2", true, "UTF-8;UTF-16", now - one_day, Time()); - AddKeywordWithDate("name3", "key3", false, "http://foo3", std::string(), + AddKeywordWithDate("name3", "key3", "http://foo3", std::string(), std::string(), true, std::string(), now, Time()); - AddKeywordWithDate("name4", "key4", false, "http://foo4", std::string(), + AddKeywordWithDate("name4", "key4", "http://foo4", std::string(), std::string(), true, std::string(), now + one_day, Time()); // Try the other three states. - AddKeywordWithDate("name5", "key5", false, "http://foo5", "http://suggest5", + AddKeywordWithDate("name5", "key5", "http://foo5", "http://suggest5", "http://icon5", false, "UTF-8;UTF-16", now, Time()); - AddKeywordWithDate("name6", "key6", false, "http://foo6", "http://suggest6", + AddKeywordWithDate("name6", "key6", "http://foo6", "http://suggest6", "http://icon6", false, "UTF-8;UTF-16", month_ago, Time()); // We just added a few items, validate them. @@ -529,11 +663,11 @@ TEST_F(TemplateURLServiceTest, ClearBrowsingData_KeywordsForOrigin) { EXPECT_EQ(0U, model()->GetTemplateURLs().size()); // Create one for now and +/- 1 day. - AddKeywordWithDate("name1", "key1", false, "http://foo1", "http://suggest1", + AddKeywordWithDate("name1", "key1", "http://foo1", "http://suggest1", "http://icon2", true, "UTF-8;UTF-16", now - one_day, Time()); - AddKeywordWithDate("name2", "key2", false, "http://foo2", std::string(), + AddKeywordWithDate("name2", "key2", "http://foo2", std::string(), std::string(), true, std::string(), now, Time()); - AddKeywordWithDate("name3", "key3", false, "http://foo3", std::string(), + AddKeywordWithDate("name3", "key3", "http://foo3", std::string(), std::string(), true, std::string(), now + one_day, Time()); // We just added a few items, validate them. @@ -618,7 +752,7 @@ TEST_F(TemplateURLServiceTest, DefaultSearchProvider) { // Add a new TemplateURL. test_util_.VerifyLoad(); const size_t initial_count = model()->GetTemplateURLs().size(); - TemplateURL* t_url = AddKeywordWithDate("name1", "key1", false, + TemplateURL* t_url = AddKeywordWithDate("name1", "key1", "http://foo1/{searchTerms}", "http://sugg1", "http://icon1", true, "UTF-8;UTF-16", Time(), Time()); test_util_.ResetObserverCount(); @@ -642,36 +776,10 @@ TEST_F(TemplateURLServiceTest, DefaultSearchProvider) { AssertEquals(*cloned_url, *model()->GetDefaultSearchProvider()); } -TEST_F(TemplateURLServiceTest, TemplateURLWithNoKeyword) { - test_util_.VerifyLoad(); - - const size_t initial_count = model()->GetTemplateURLs().size(); - - AddKeywordWithDate("name1", std::string(), false, "http://foo1", - "http://sugg1", "http://icon1", true, "UTF-8;UTF-16", Time(), Time()); - - // We just added a few items, validate them. - ASSERT_EQ(initial_count + 1, model()->GetTemplateURLs().size()); - - // Reload the model from the database and make sure we get the url back. - test_util_.ResetModel(true); - - ASSERT_EQ(1 + initial_count, model()->GetTemplateURLs().size()); - - bool found_keyword = false; - for (size_t i = 0; i < initial_count + 1; ++i) { - if (model()->GetTemplateURLs()[i]->keyword().empty()) { - found_keyword = true; - break; - } - } - ASSERT_TRUE(found_keyword); -} - TEST_F(TemplateURLServiceTest, CantReplaceWithSameKeyword) { test_util_.ChangeModelToLoadState(); ASSERT_TRUE(model()->CanReplaceKeyword(ASCIIToUTF16("foo"), GURL(), NULL)); - TemplateURL* t_url = AddKeywordWithDate("name1", "foo", false, "http://foo1", + TemplateURL* t_url = AddKeywordWithDate("name1", "foo", "http://foo1", "http://sugg1", "http://icon1", true, "UTF-8;UTF-16", Time(), Time()); // Can still replace, newly added template url is marked safe to replace. @@ -691,9 +799,8 @@ TEST_F(TemplateURLServiceTest, CantReplaceWithSameHosts) { test_util_.ChangeModelToLoadState(); ASSERT_TRUE(model()->CanReplaceKeyword(ASCIIToUTF16("foo"), GURL("http://foo.com"), NULL)); - TemplateURL* t_url = AddKeywordWithDate("name1", "foo", false, - "http://foo.com", "http://sugg1", "http://icon1", true, "UTF-8;UTF-16", - Time(), Time()); + TemplateURL* t_url = AddKeywordWithDate("name1", "foo", "http://foo.com", + "http://sugg1", "http://icon1", true, "UTF-8;UTF-16", Time(), Time()); // Can still replace, newly added template url is marked safe to replace. ASSERT_TRUE(model()->CanReplaceKeyword(ASCIIToUTF16("bar"), @@ -820,7 +927,7 @@ TEST_F(TemplateURLServiceTest, UpdateKeywordSearchTermsForURL) { }; test_util_.ChangeModelToLoadState(); - AddKeywordWithDate("name", "x", false, "http://x/foo?q={searchTerms}", + AddKeywordWithDate("name", "x", "http://x/foo?q={searchTerms}", "http://sugg1", "http://icon1", false, "UTF-8;UTF-16", Time(), Time()); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(data); ++i) { @@ -842,7 +949,7 @@ TEST_F(TemplateURLServiceTest, DontUpdateKeywordSearchForNonReplaceable) { }; test_util_.ChangeModelToLoadState(); - AddKeywordWithDate("name", "x", false, "http://x/foo", "http://sugg1", + AddKeywordWithDate("name", "x", "http://x/foo", "http://sugg1", "http://icon1", false, "UTF-8;UTF-16", Time(), Time()); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(data); ++i) { @@ -860,7 +967,7 @@ TEST_F(TemplateURLServiceTest, ChangeGoogleBaseValue) { // test. test_util_.ChangeModelToLoadState(); test_util_.SetGoogleBaseURL(GURL("http://google.com/")); - const TemplateURL* t_url = AddKeywordWithDate("name", "google.com", true, + const TemplateURL* t_url = AddKeywordWithDate("name", "google.com", "{google:baseURL}?q={searchTerms}", "http://sugg1", "http://icon1", false, "UTF-8;UTF-16", Time(), Time()); ASSERT_EQ(t_url, model()->GetTemplateURLForHost("google.com")); @@ -879,6 +986,35 @@ TEST_F(TemplateURLServiceTest, ChangeGoogleBaseValue) { EXPECT_EQ(ASCIIToUTF16("google.co.uk"), t_url->keyword()); EXPECT_EQ("http://google.co.uk/?q=x", t_url->url_ref().ReplaceSearchTerms( ASCIIToUTF16("x"), TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, string16())); + + // Now add a manual entry and then change the Google base URL such that the + // autogenerated Google search keyword would conflict. + TemplateURL* manual = AddKeywordWithDate("manual", "google.de", + "http://google.de/search?q={searchTerms}", std::string(), std::string(), + false, "UTF-8", Time(), Time()); + test_util_.SetGoogleBaseURL(GURL("http://google.de")); + + // Verify that the manual entry is untouched, and the autogenerated keyword + // has not changed. + ASSERT_EQ(manual, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("google.de"))); + EXPECT_EQ("google.de", manual->url_ref().GetHost()); + ASSERT_EQ(t_url, + model()->GetTemplateURLForKeyword(ASCIIToUTF16("google.co.uk"))); + EXPECT_EQ("google.de", t_url->url_ref().GetHost()); + EXPECT_EQ(ASCIIToUTF16("google.co.uk"), t_url->keyword()); + + // Change the base URL again and verify that the autogenerated keyword follows + // even though it didn't match the base URL, while the manual entry is still + // untouched. + test_util_.SetGoogleBaseURL(GURL("http://google.fr/")); + ASSERT_EQ(manual, model()->GetTemplateURLForHost("google.de")); + EXPECT_EQ("google.de", manual->url_ref().GetHost()); + EXPECT_EQ(ASCIIToUTF16("google.de"), manual->keyword()); + ASSERT_EQ(t_url, model()->GetTemplateURLForHost("google.fr")); + EXPECT_TRUE(model()->GetTemplateURLForHost("google.co.uk") == NULL); + EXPECT_EQ("google.fr", t_url->url_ref().GetHost()); + EXPECT_EQ(ASCIIToUTF16("google.fr"), t_url->keyword()); } struct QueryHistoryCallbackImpl { @@ -907,7 +1043,7 @@ TEST_F(TemplateURLServiceTest, GenerateVisitOnKeyword) { test_util_.profile()->CreateHistoryService(true, false); // Create a keyword. - TemplateURL* t_url = AddKeywordWithDate("keyword", "keyword", false, + TemplateURL* t_url = AddKeywordWithDate("keyword", "keyword", "http://foo.com/foo?query={searchTerms}", "http://sugg1", "http://icon1", true, "UTF-8;UTF-16", base::Time::Now(), base::Time::Now()); @@ -1114,29 +1250,6 @@ TEST_F(TemplateURLServiceTest, LoadEnsuresDefaultSearchProviderExists) { EXPECT_TRUE(model()->GetDefaultSearchProvider()->SupportsReplacement()); } -// Make sure that the load does update of auto-keywords correctly. -// This test basically verifies that no asserts or crashes occur -// during this operation. -TEST_F(TemplateURLServiceTest, LoadDoesAutoKeywordUpdate) { - string16 prepopulated_url; - scoped_ptr<TemplateURL> t_url( - CreateReplaceablePreloadedTemplateURL(false, 0, &prepopulated_url)); - TemplateURLData data(t_url->data()); - data.SetAutogenerateKeyword(true); - data.SetURL("{google:baseURL}?q={searchTerms}"); - - // Then add it to the model and save it all. - test_util_.ChangeModelToLoadState(); - model()->Add(new TemplateURL(test_util_.profile(), data)); - test_util_.BlockTillServiceProcessesRequests(); - - // Now reload the model and verify that the merge updates the url. - test_util_.ResetModel(true); - - // Wait for any saves to finish. - test_util_.BlockTillServiceProcessesRequests(); -} - // Simulates failing to load the webdb and makes sure the default search // provider is valid. TEST_F(TemplateURLServiceTest, FailedInit) { @@ -1164,7 +1277,7 @@ TEST_F(TemplateURLServiceTest, TestManagedDefaultSearch) { test_util_.ResetObserverCount(); // Set a regular default search provider. - TemplateURL* regular_default = AddKeywordWithDate("name1", "key1", false, + TemplateURL* regular_default = AddKeywordWithDate("name1", "key1", "http://foo1/{searchTerms}", "http://sugg1", "http://icon1", true, "UTF-8;UTF-16", Time(), Time()); VerifyObserverCount(1); @@ -1267,11 +1380,11 @@ TEST_F(TemplateURLServiceTest, TestManagedDefaultSearch) { // First, remove the preferences, reset the model, and set a default. test_util_.RemoveManagedDefaultSearchPreferences(); test_util_.ResetModel(true); - TemplateURL* t_url = AddKeywordWithDate("name1", "key1", false, - "http://foo1/{searchTerms}", "http://sugg1", "http://icon1", true, - "UTF-8;UTF-16", Time(), Time()); - model()->SetDefaultSearchProvider(t_url); - EXPECT_EQ(t_url, model()->GetDefaultSearchProvider()); + TemplateURL* new_default = + model()->GetTemplateURLForKeyword(ASCIIToUTF16("key1")); + ASSERT_FALSE(new_default == NULL); + model()->SetDefaultSearchProvider(new_default); + EXPECT_EQ(new_default, model()->GetDefaultSearchProvider()); // Now reset the model again but load it after setting the preferences. test_util_.ResetModel(false); diff --git a/chrome/browser/search_engines/template_url_unittest.cc b/chrome/browser/search_engines/template_url_unittest.cc index dbc5c39..708650b 100644 --- a/chrome/browser/search_engines/template_url_unittest.cc +++ b/chrome/browser/search_engines/template_url_unittest.cc @@ -491,20 +491,6 @@ TEST_F(TemplateURLTest, GoogleBaseSuggestURL) { CheckSuggestBaseURL(data[i].base_url, data[i].base_suggest_url); } -TEST_F(TemplateURLTest, Keyword) { - TemplateURLData data; - data.SetURL("http://www.google.com/search"); - EXPECT_FALSE(data.autogenerate_keyword()); - data.SetKeyword(ASCIIToUTF16("foo")); - EXPECT_EQ(ASCIIToUTF16("foo"), TemplateURL(NULL, data).keyword()); - data.SetAutogenerateKeyword(true); - EXPECT_TRUE(data.autogenerate_keyword()); - EXPECT_EQ(ASCIIToUTF16("google.com"), TemplateURL(NULL, data).keyword()); - data.SetKeyword(ASCIIToUTF16("foo")); - EXPECT_FALSE(data.autogenerate_keyword()); - EXPECT_EQ(ASCIIToUTF16("foo"), TemplateURL(NULL, data).keyword()); -} - TEST_F(TemplateURLTest, ParseParameterKnown) { std::string parsed_url("{searchTerms}"); TemplateURLData data; diff --git a/chrome/browser/search_engines/util.cc b/chrome/browser/search_engines/util.cc index c8bd3c6..216d8f3 100644 --- a/chrome/browser/search_engines/util.cc +++ b/chrome/browser/search_engines/util.cc @@ -130,18 +130,17 @@ void MergeEnginesFromPrepopulateData( if (!existing_url->safe_for_autoreplace()) { data.safe_for_autoreplace = false; data.SetKeyword(existing_url->keyword()); - data.SetAutogenerateKeyword(existing_url->autogenerate_keyword()); data.short_name = existing_url->short_name(); } data.id = existing_url->id(); - url_in_vector = new TemplateURL(profile, data); if (service) - service->UpdateKeyword(*url_in_vector); + service->UpdateKeyword(data); // Replace the entry in |template_urls| with the updated one. std::vector<TemplateURL*>::iterator j = std::find(template_urls->begin(), template_urls->end(), existing_url.get()); - *j = url_in_vector; + *j = new TemplateURL(profile, data); + url_in_vector = *j; if (*default_search_provider == existing_url.get()) *default_search_provider = url_in_vector; } else { diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc index de05fcf..7214e0b 100644 --- a/chrome/browser/tab_contents/render_view_context_menu.cc +++ b/chrome/browser/tab_contents/render_view_context_menu.cc @@ -1831,8 +1831,7 @@ void RenderViewContextMenu::ExecuteCommand(int id, int event_flags) { if (tab_contents_wrapper && tab_contents_wrapper->search_engine_tab_helper() && tab_contents_wrapper->search_engine_tab_helper()->delegate()) { - string16 keyword(TemplateURLService::GenerateKeyword(params_.page_url, - false)); + string16 keyword(TemplateURLService::GenerateKeyword(params_.page_url)); TemplateURLData data; data.short_name = keyword; data.SetKeyword(keyword); @@ -1871,7 +1870,7 @@ void RenderViewContextMenu::ExecuteCommand(int id, int event_flags) { } ProtocolHandlerRegistry::ProtocolHandlerList -RenderViewContextMenu::GetHandlersForLinkUrl() { + RenderViewContextMenu::GetHandlersForLinkUrl() { ProtocolHandlerRegistry::ProtocolHandlerList handlers = protocol_handler_registry_->GetHandlersFor(params_.link_url.scheme()); std::sort(handlers.begin(), handlers.end()); diff --git a/chrome/browser/ui/search_engines/search_engine_tab_helper.cc b/chrome/browser/ui/search_engines/search_engine_tab_helper.cc index 0a93c5b..d256a40 100644 --- a/chrome/browser/ui/search_engines/search_engine_tab_helper.cc +++ b/chrome/browser/ui/search_engines/search_engine_tab_helper.cc @@ -12,6 +12,7 @@ #include "chrome/browser/search_engines/template_url_service_factory.h" #include "chrome/browser/ui/search_engines/template_url_fetcher_ui_callbacks.h" #include "chrome/common/render_messages.h" +#include "chrome/common/url_constants.h" #include "content/public/browser/favicon_status.h" #include "content/public/browser/navigation_controller.h" #include "content/public/browser/navigation_entry.h" @@ -45,7 +46,16 @@ string16 GenerateKeywordFromNavigationEntry(const NavigationEntry* entry) { return string16(); } - return TemplateURLService::GenerateKeyword(url, true); + // Don't autogenerate keywords for referrers that are anything other than HTTP + // or have a path. + // + // If we relax the path constraint, we need to be sure to sanitize the path + // elements and update AutocompletePopup to look for keywords using the path. + // See http://b/issue?id=863583. + if (!url.SchemeIs(chrome::kHttpScheme) || (url.path().length() > 1)) + return string16(); + + return TemplateURLService::GenerateKeyword(url); } } // namespace diff --git a/chrome/browser/webdata/keyword_table.cc b/chrome/browser/webdata/keyword_table.cc index 8df072d..c08a6f7 100644 --- a/chrome/browser/webdata/keyword_table.cc +++ b/chrome/browser/webdata/keyword_table.cc @@ -18,7 +18,10 @@ #include "chrome/browser/history/history_database.h" #include "chrome/browser/protector/histograms.h" #include "chrome/browser/protector/protector_utils.h" +#include "chrome/browser/search_engines/search_terms_data.h" #include "chrome/browser/search_engines/template_url.h" +#include "chrome/browser/search_engines/template_url_service.h" +#include "chrome/browser/webdata/web_database.h" #include "googleurl/src/gurl.h" #include "sql/statement.h" #include "sql/transaction.h" @@ -35,34 +38,49 @@ const char KeywordTable::kBackupSignatureKey[] = const char KeywordTable::kKeywordColumns[] = "id, short_name, keyword, " "favicon_url, url, safe_for_autoreplace, originating_url, date_created, " "usage_count, input_encodings, show_in_default_list, suggest_url, " - "prepopulate_id, autogenerate_keyword, logo_id, created_by_policy, " - "instant_url, last_modified, sync_guid"; + "prepopulate_id, created_by_policy, instant_url, last_modified, sync_guid"; namespace { // Keys used in the meta table. const char kBuiltinKeywordVersion[] = "Builtin Keyword Version"; +// The set of columns up through version 44. (There were different columns +// below version 29 but none of the code below needs to worry about that case.) +const char kKeywordColumnsVersion44Concatenated[] = "id || short_name || " + "keyword || favicon_url || url || safe_for_autoreplace || " + "originating_url || date_created || usage_count || input_encodings || " + "show_in_default_list || suggest_url || prepopulate_id || " + "autogenerate_keyword || logo_id || created_by_policy || instant_url || " + "last_modified || sync_guid"; +const char kKeywordColumnsVersion44[] = "id, short_name, keyword, favicon_url, " + "url, safe_for_autoreplace, originating_url, date_created, usage_count, " + "input_encodings, show_in_default_list, suggest_url, prepopulate_id, " + "autogenerate_keyword, logo_id, created_by_policy, instant_url, " + "last_modified, sync_guid"; +// NOTE: Remember to change what |kKeywordColumnsVersion45| says if the column +// set in |kKeywordColumns| changes, and update any code that needs to switch +// column sets based on a version number! +const char* const kKeywordColumnsVersion45 = KeywordTable::kKeywordColumns; + +// The current columns. const char kKeywordColumnsConcatenated[] = "id || short_name || keyword || " "favicon_url || url || safe_for_autoreplace || originating_url || " "date_created || usage_count || input_encodings || show_in_default_list || " - "suggest_url || prepopulate_id || autogenerate_keyword || logo_id || " - "created_by_policy || instant_url || last_modified || sync_guid"; + "suggest_url || prepopulate_id || created_by_policy || instant_url || " + "last_modified || sync_guid"; // Inserts the data from |data| into |s|. |s| is assumed to have slots for all // the columns in the keyword table. |id_column| is the slot number to bind -// |data|'s id() to; |starting_column| is the slot number of the first of a +// |data|'s |id| to; |starting_column| is the slot number of the first of a // contiguous set of slots to bind all the other fields to. -void BindURLToStatement(const TemplateURL& url, +void BindURLToStatement(const TemplateURLData& data, sql::Statement* s, int id_column, int starting_column) { - const TemplateURLData& data = url.data(); s->BindInt64(id_column, data.id); s->BindString16(starting_column, data.short_name); - // TODO(pkasting): See comment on TempalteURL::EnsureKeyword(). - s->BindString16(starting_column + 1, - data.keyword(const_cast<TemplateURL*>(&url))); + s->BindString16(starting_column + 1, data.keyword()); s->BindString(starting_column + 2, data.favicon_url.is_valid() ? history::HistoryDatabase::GURLToDatabaseURL(data.favicon_url) : std::string()); @@ -77,12 +95,10 @@ void BindURLToStatement(const TemplateURL& url, s->BindBool(starting_column + 9, data.show_in_default_list); s->BindString(starting_column + 10, data.suggestions_url); s->BindInt(starting_column + 11, data.prepopulate_id); - s->BindBool(starting_column + 12, data.autogenerate_keyword()); - s->BindInt(starting_column + 13, 0); - s->BindBool(starting_column + 14, data.created_by_policy); - s->BindString(starting_column + 15, data.instant_url); - s->BindInt64(starting_column + 16, data.last_modified.ToTimeT()); - s->BindString(starting_column + 17, data.sync_guid); + s->BindBool(starting_column + 12, data.created_by_policy); + s->BindString(starting_column + 13, data.instant_url); + s->BindInt64(starting_column + 14, data.last_modified.ToTimeT()); + s->BindString(starting_column + 15, data.sync_guid); } // Signs search provider id and returns its signature. @@ -116,27 +132,25 @@ bool KeywordTable::Init() { "show_in_default_list INTEGER," "suggest_url VARCHAR," "prepopulate_id INTEGER DEFAULT 0," - "autogenerate_keyword INTEGER DEFAULT 0," - "logo_id INTEGER DEFAULT 0," "created_by_policy INTEGER DEFAULT 0," "instant_url VARCHAR," "last_modified INTEGER DEFAULT 0," "sync_guid VARCHAR)") && - UpdateBackupSignature()); + UpdateBackupSignature(WebDatabase::kCurrentVersionNumber)); } bool KeywordTable::IsSyncable() { return true; } -bool KeywordTable::AddKeyword(const TemplateURL& url) { - DCHECK(url.id()); +bool KeywordTable::AddKeyword(const TemplateURLData& data) { + DCHECK(data.id); std::string query("INSERT INTO keywords (" + std::string(kKeywordColumns) + - ") VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)"); + ") VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)"); sql::Statement s(db_->GetUniqueStatement(query.c_str())); - BindURLToStatement(url, &s, 0, 1); + BindURLToStatement(data, &s, 0, 1); - return s.Run() && UpdateBackupSignature(); + return s.Run() && UpdateBackupSignature(WebDatabase::kCurrentVersionNumber); } bool KeywordTable::RemoveKeyword(TemplateURLID id) { @@ -145,7 +159,7 @@ bool KeywordTable::RemoveKeyword(TemplateURLID id) { db_->GetUniqueStatement("DELETE FROM keywords WHERE id = ?")); s.BindInt64(0, id); - return s.Run() && UpdateBackupSignature(); + return s.Run() && UpdateBackupSignature(WebDatabase::kCurrentVersionNumber); } bool KeywordTable::GetKeywords(Keywords* keywords) { @@ -168,17 +182,17 @@ bool KeywordTable::GetKeywords(Keywords* keywords) { return succeeded; } -bool KeywordTable::UpdateKeyword(const TemplateURL& url) { - DCHECK(url.id()); +bool KeywordTable::UpdateKeyword(const TemplateURLData& data) { + DCHECK(data.id); sql::Statement s(db_->GetUniqueStatement("UPDATE keywords SET short_name=?, " "keyword=?, favicon_url=?, url=?, safe_for_autoreplace=?, " "originating_url=?, date_created=?, usage_count=?, input_encodings=?, " "show_in_default_list=?, suggest_url=?, prepopulate_id=?, " - "autogenerate_keyword=?, logo_id=?, created_by_policy=?, instant_url=?, " - "last_modified=?, sync_guid=? WHERE id=?")); - BindURLToStatement(url, &s, 18, 0); // "18" binds id() as the last item. + "created_by_policy=?, instant_url=?, last_modified=?, sync_guid=? WHERE " + "id=?")); + BindURLToStatement(data, &s, 16, 0); // "16" binds id() as the last item. - return s.Run() && UpdateBackupSignature(); + return s.Run() && UpdateBackupSignature(WebDatabase::kCurrentVersionNumber); } bool KeywordTable::SetDefaultSearchProviderID(int64 id) { @@ -186,7 +200,7 @@ bool KeywordTable::SetDefaultSearchProviderID(int64 id) { UMA_HISTOGRAM_COUNTS_100("Search.DefaultSearchProviderID", static_cast<int32>(id)); return meta_table_->SetValue(kDefaultSearchProviderKey, id) && - UpdateBackupSignature(); + UpdateBackupSignature(WebDatabase::kCurrentVersionNumber); } int64 KeywordTable::GetDefaultSearchProviderID() { @@ -196,7 +210,7 @@ int64 KeywordTable::GetDefaultSearchProviderID() { } bool KeywordTable::GetDefaultSearchProviderBackup(TemplateURLData* backup) { - if (!IsBackupSignatureValid()) + if (!IsBackupSignatureValid(WebDatabase::kCurrentVersionNumber)) return false; int64 backup_id = kInvalidTemplateURLID; @@ -212,7 +226,7 @@ bool KeywordTable::GetDefaultSearchProviderBackup(TemplateURLData* backup) { if (!s.Step()) { LOG_IF(ERROR, s.Succeeded()) << "No default search provider with backup id."; - return NULL; + return false; } if (!GetKeywordDataFromStatement(s, backup)) @@ -225,7 +239,7 @@ bool KeywordTable::GetDefaultSearchProviderBackup(TemplateURLData* backup) { } bool KeywordTable::DidDefaultSearchProviderChange() { - if (!IsBackupSignatureValid()) { + if (!IsBackupSignatureValid(WebDatabase::kCurrentVersionNumber)) { UMA_HISTOGRAM_ENUMERATION( protector::kProtectorHistogramDefaultSearchProvider, protector::kProtectorErrorBackupInvalid, @@ -340,7 +354,35 @@ bool KeywordTable::MigrateToVersion39AddSyncGUIDColumn() { } bool KeywordTable::MigrateToVersion44AddDefaultSearchProviderBackup() { - return IsBackupSignatureValid() || UpdateBackupSignature(); + return IsBackupSignatureValid(44) || UpdateBackupSignature(44); +} + +bool KeywordTable::MigrateToVersion45RemoveLogoIDAndAutogenerateColumns() { + sql::Transaction transaction(db_); + if (!transaction.Begin()) + return false; + + // The version 43 migration should have been written to do this, but since it + // wasn't, we'll do it now. Unfortunately a previous change deleted this for + // some users, so we can't be sure this will succeed (so don't bail on error). + meta_table_->DeleteKey("Default Search Provider Backup"); + + if (!MigrateKeywordsTableForVersion45("keywords")) + return false; + + if (IsBackupSignatureValid(44)) { + // Migrate the keywords backup table as well. + if (!MigrateKeywordsTableForVersion45("keywords_backup") || !SignBackup(45)) + return false; + } else { + // Old backup was invalid; drop the table entirely, which will trigger the + // protector code to prompt the user and recreate the table. + if (db_->DoesTableExist("keywords_backup") && + !db_->Execute("DROP TABLE keywords_backup")) + return false; + } + + return transaction.Commit(); } // static @@ -349,7 +391,6 @@ bool KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s, DCHECK(data); data->short_name = s.ColumnString16(1); data->SetKeyword(s.ColumnString16(2)); - data->SetAutogenerateKeyword(s.ColumnBool(13)); // Due to past bugs, we might have persisted entries with empty URLs. Avoid // reading these out. (GetKeywords() will delete these entries on return.) // NOTE: This code should only be needed as long as we might be reading such @@ -358,7 +399,7 @@ bool KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s, return false; data->SetURL(s.ColumnString(4)); data->suggestions_url = s.ColumnString(11); - data->instant_url = s.ColumnString(16); + data->instant_url = s.ColumnString(14); data->favicon_url = GURL(s.ColumnString(3)); data->originating_url = GURL(s.ColumnString(6)); data->show_in_default_list = s.ColumnBool(10); @@ -366,15 +407,15 @@ bool KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s, base::SplitString(s.ColumnString(9), ';', &data->input_encodings); data->id = s.ColumnInt64(0); data->date_created = Time::FromTimeT(s.ColumnInt64(7)); - data->last_modified = Time::FromTimeT(s.ColumnInt64(17)); - data->created_by_policy = s.ColumnBool(15); + data->last_modified = Time::FromTimeT(s.ColumnInt64(15)); + data->created_by_policy = s.ColumnBool(13); data->usage_count = s.ColumnInt(8); data->prepopulate_id = s.ColumnInt(12); - data->sync_guid = s.ColumnString(18); + data->sync_guid = s.ColumnString(16); return true; } -bool KeywordTable::GetSignatureData(std::string* backup) { +bool KeywordTable::GetSignatureData(int table_version, std::string* backup) { DCHECK(backup); int64 backup_value = kInvalidTemplateURLID; @@ -384,7 +425,8 @@ bool KeywordTable::GetSignatureData(std::string* backup) { } std::string keywords_backup_data; - if (!GetTableContents("keywords_backup", &keywords_backup_data)) { + if (!GetTableContents("keywords_backup", table_version, + &keywords_backup_data)) { LOG(ERROR) << "Can't get keywords backup data"; return false; } @@ -393,6 +435,7 @@ bool KeywordTable::GetSignatureData(std::string* backup) { } bool KeywordTable::GetTableContents(const char* table_name, + int table_version, std::string* contents) { DCHECK(contents); @@ -400,16 +443,19 @@ bool KeywordTable::GetTableContents(const char* table_name, return false; contents->clear(); - std::string query("SELECT " + std::string(kKeywordColumnsConcatenated) + + std::string query("SELECT " + + std::string((table_version <= 44) ? + kKeywordColumnsVersion44Concatenated : kKeywordColumnsConcatenated) + " FROM " + std::string(table_name) + " ORDER BY id ASC"); - sql::Statement s(db_->GetCachedStatement(sql::StatementID(table_name), - query.c_str())); + sql::Statement s((table_version == WebDatabase::kCurrentVersionNumber) ? + db_->GetCachedStatement(sql::StatementID(table_name), query.c_str()) : + db_->GetUniqueStatement(query.c_str())); while (s.Step()) *contents += s.ColumnString(0); return s.Succeeded(); } -bool KeywordTable::UpdateBackupSignature() { +bool KeywordTable::UpdateBackupSignature(int table_version) { sql::Transaction transaction(db_); if (!transaction.Begin()) return false; @@ -426,14 +472,20 @@ bool KeywordTable::UpdateBackupSignature() { return false; std::string query("CREATE TABLE keywords_backup AS SELECT " + - std::string(kKeywordColumns) + " FROM keywords ORDER BY id ASC"); + std::string((table_version <= 44) ? + kKeywordColumnsVersion44 : kKeywordColumns) + + " FROM keywords ORDER BY id ASC"); if (!db_->Execute(query.c_str())) { LOG(ERROR) << "Failed to create keywords_backup table."; return false; } + return SignBackup(table_version) && transaction.Commit(); +} + +bool KeywordTable::SignBackup(int table_version) { std::string data_to_sign; - if (!GetSignatureData(&data_to_sign)) { + if (!GetSignatureData(table_version, &data_to_sign)) { LOG(ERROR) << "No data to sign."; return false; } @@ -444,15 +496,14 @@ bool KeywordTable::UpdateBackupSignature() { return false; } - return meta_table_->SetValue(kBackupSignatureKey, signature) && - transaction.Commit(); + return meta_table_->SetValue(kBackupSignatureKey, signature); } -bool KeywordTable::IsBackupSignatureValid() { +bool KeywordTable::IsBackupSignatureValid(int table_version) { std::string signature; std::string signature_data; return meta_table_->GetValue(kBackupSignatureKey, &signature) && - GetSignatureData(&signature_data) && + GetSignatureData(table_version, &signature_data) && protector::IsSettingValid(signature_data, signature); } @@ -489,3 +540,88 @@ bool KeywordTable::UpdateDefaultSearchProviderIDBackup(TemplateURLID* id) { *id = default_search_id; return true; } + +bool KeywordTable::MigrateKeywordsTableForVersion45(const std::string& name) { + // Create a new table without the columns we're dropping. + if (!db_->Execute("CREATE TABLE keywords_temp (" + "id INTEGER PRIMARY KEY," + "short_name VARCHAR NOT NULL," + "keyword VARCHAR NOT NULL," + "favicon_url VARCHAR NOT NULL," + "url VARCHAR NOT NULL," + "safe_for_autoreplace INTEGER," + "originating_url VARCHAR," + "date_created INTEGER DEFAULT 0," + "usage_count INTEGER DEFAULT 0," + "input_encodings VARCHAR," + "show_in_default_list INTEGER," + "suggest_url VARCHAR," + "prepopulate_id INTEGER DEFAULT 0," + "created_by_policy INTEGER DEFAULT 0," + "instant_url VARCHAR," + "last_modified INTEGER DEFAULT 0," + "sync_guid VARCHAR)")) + return false; + std::string sql("INSERT INTO keywords_temp SELECT " + + std::string(kKeywordColumnsVersion45) + " FROM " + name); + if (!db_->Execute(sql.c_str())) + return false; + + // NOTE: The ORDER BY here ensures that the uniquing process for keywords will + // happen identically on both the normal and backup tables. + sql = "SELECT id, keyword, url, autogenerate_keyword FROM " + name + + " ORDER BY id ASC"; + sql::Statement s(db_->GetUniqueStatement(sql.c_str())); + string16 placeholder_keyword(ASCIIToUTF16("dummy")); + std::set<string16> keywords; + while (s.Step()) { + string16 keyword(s.ColumnString16(1)); + bool generate_keyword = keyword.empty() || s.ColumnBool(3); + if (generate_keyword) + keyword = placeholder_keyword; + TemplateURLData data; + data.SetKeyword(keyword); + data.SetURL(s.ColumnString(2)); + TemplateURL turl(NULL, data); + // Don't persist extension keywords to disk. These will get added to the + // TemplateURLService as the extensions are loaded. + bool delete_entry = turl.IsExtensionKeyword(); + if (!delete_entry && generate_keyword) { + // Explicitly generate keywords for all rows with the autogenerate bit set + // or where the keyword is empty. + SearchTermsData terms_data; + GURL url(TemplateURLService::GenerateSearchURLUsingTermsData(&turl, + terms_data)); + if (!url.is_valid()) { + delete_entry = true; + } else { + // Ensure autogenerated keywords are unique. + keyword = TemplateURLService::GenerateKeyword(url); + while (keywords.count(keyword)) + keyword.append(ASCIIToUTF16("_")); + sql::Statement u(db_->GetUniqueStatement( + "UPDATE keywords_temp SET keyword=? WHERE id=?")); + u.BindString16(0, keyword); + u.BindInt64(1, s.ColumnInt64(0)); + if (!u.Run()) + return false; + } + } + if (delete_entry) { + sql::Statement u(db_->GetUniqueStatement( + "DELETE FROM keywords_temp WHERE id=?")); + u.BindInt64(0, s.ColumnInt64(0)); + if (!u.Run()) + return false; + } else { + keywords.insert(keyword); + } + } + + // Replace the old table with the new one. + sql = "DROP TABLE " + name; + if (!db_->Execute(sql.c_str())) + return false; + sql = "ALTER TABLE keywords_temp RENAME TO " + name; + return db_->Execute(sql.c_str()); +} diff --git a/chrome/browser/webdata/keyword_table.h b/chrome/browser/webdata/keyword_table.h index a5285e7..b5d147f 100644 --- a/chrome/browser/webdata/keyword_table.h +++ b/chrome/browser/webdata/keyword_table.h @@ -15,7 +15,6 @@ #include "chrome/browser/webdata/web_database_table.h" #include "chrome/browser/search_engines/template_url_id.h" -class TemplateURL; struct TemplateURLData; namespace sql { @@ -46,7 +45,6 @@ class Statement; // suggest_url // prepopulate_id See TemplateURLData::prepopulate_id. // autogenerate_keyword -// logo_id Deprecated, to be removed; see crbug.com/113248. // created_by_policy See TemplateURLData::created_by_policy. This was // added in version 26. // instant_url See TemplateURLData::instant_url. This was added in @@ -104,7 +102,7 @@ class KeywordTable : public WebDatabaseTable { // Adds a new keyword, updating the id field on success. // Returns true if successful. - bool AddKeyword(const TemplateURL& url); + bool AddKeyword(const TemplateURLData& data); // Removes the specified keyword. // Returns true if successful. @@ -117,7 +115,7 @@ class KeywordTable : public WebDatabaseTable { // Updates the database values for the specified url. // Returns true on success. - bool UpdateKeyword(const TemplateURL& url); + bool UpdateKeyword(const TemplateURLData& data); // ID (TemplateURLData->id) of the default search provider. bool SetDefaultSearchProviderID(int64 id); @@ -145,12 +143,14 @@ class KeywordTable : public WebDatabaseTable { bool MigrateToVersion38AddLastModifiedColumn(); bool MigrateToVersion39AddSyncGUIDColumn(); bool MigrateToVersion44AddDefaultSearchProviderBackup(); + bool MigrateToVersion45RemoveLogoIDAndAutogenerateColumns(); private: FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, DefaultSearchProviderBackup); FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, GetTableContents); FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, GetTableContentsOrdering); FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, SanitizeURLs); + FRIEND_TEST_ALL_PREFIXES(WebDatabaseMigrationTest, MigrateVersion44ToCurrent); // NOTE: Since the table columns have changed in different versions, many // functions below take a |table_version| argument which dictates which @@ -165,19 +165,25 @@ class KeywordTable : public WebDatabaseTable { // Returns contents of |keywords_backup| table and default search provider // id backup as a string through |data|. Return value is true on success, // false otherwise. - bool GetSignatureData(std::string* data); + bool GetSignatureData(int table_version, std::string* data); // Returns contents of selected table as a string in |contents| parameter. // Returns true on success, false otherwise. - bool GetTableContents(const char* table_name, std::string* contents); + bool GetTableContents(const char* table_name, + int table_version, + std::string* contents); // Updates settings backup, signs it and stores the signature in the // database. Returns true on success. - bool UpdateBackupSignature(); + bool UpdateBackupSignature(int table_version); + + // Signs the backup table. This is a subset of what UpdateBackupSignature() + // does. + bool SignBackup(int table_version); // Checks the signature for the current settings backup. Returns true // if signature is valid, false otherwise. - bool IsBackupSignatureValid(); + bool IsBackupSignatureValid(int table_version); // Gets a string representation for keyword with id specified. // Used to store its result in |meta| table or to compare with another @@ -190,6 +196,10 @@ class KeywordTable : public WebDatabaseTable { // true on success. The id is returned back via |id| parameter. bool UpdateDefaultSearchProviderIDBackup(TemplateURLID* id); + // Migrates table |name| (which should be either "keywords" or + // "keywords_backup" from version 44 to version 45. + bool MigrateKeywordsTableForVersion45(const std::string& name); + DISALLOW_COPY_AND_ASSIGN(KeywordTable); }; diff --git a/chrome/browser/webdata/keyword_table_unittest.cc b/chrome/browser/webdata/keyword_table_unittest.cc index 5af4343..9dd5613 100644 --- a/chrome/browser/webdata/keyword_table_unittest.cc +++ b/chrome/browser/webdata/keyword_table_unittest.cc @@ -69,8 +69,7 @@ TEST_F(KeywordTableTest, Keywords) { keyword.created_by_policy = true; keyword.usage_count = 32; keyword.prepopulate_id = 10; - TemplateURL url(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); KeywordTable::Keywords keywords; EXPECT_TRUE(keyword_table->GetKeywords(&keywords)); @@ -78,9 +77,7 @@ TEST_F(KeywordTableTest, Keywords) { const TemplateURLData& restored_keyword = keywords.front(); EXPECT_EQ(keyword.short_name, restored_keyword.short_name); - EXPECT_EQ(url.keyword(), TemplateURL(NULL, restored_keyword).keyword()); - EXPECT_EQ(keyword.autogenerate_keyword(), - restored_keyword.autogenerate_keyword()); + EXPECT_EQ(keyword.keyword(), restored_keyword.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); @@ -133,8 +130,7 @@ TEST_F(KeywordTableTest, KeywordMisc) { keyword.created_by_policy = true; keyword.usage_count = 32; keyword.prepopulate_id = 10; - TemplateURL url(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); EXPECT_TRUE(keyword_table->SetDefaultSearchProviderID(10)); EXPECT_TRUE(keyword_table->SetBuiltinKeywordVersion(11)); @@ -160,11 +156,11 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { keyword.show_in_default_list = true; keyword.safe_for_autoreplace = true; keyword.id = 1; - TemplateURL url(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); EXPECT_TRUE(keyword_table->SetDefaultSearchProviderID(1)); - EXPECT_TRUE(keyword_table->IsBackupSignatureValid()); + EXPECT_TRUE(keyword_table->IsBackupSignatureValid( + WebDatabase::kCurrentVersionNumber)); EXPECT_EQ(1, keyword_table->GetDefaultSearchProviderID()); TemplateURLData backup; @@ -172,7 +168,7 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { // Backup URL should have an invalid ID. EXPECT_EQ(kInvalidTemplateURLID, backup.id); EXPECT_EQ(keyword.short_name, backup.short_name); - EXPECT_EQ(url.keyword(), TemplateURL(NULL, backup).keyword()); + EXPECT_EQ(keyword.keyword(), backup.keyword()); EXPECT_EQ(keyword.url(), backup.url()); EXPECT_EQ(keyword.favicon_url, backup.favicon_url); EXPECT_EQ(keyword.suggestions_url, backup.suggestions_url); @@ -183,13 +179,14 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { // Change the actual setting. EXPECT_TRUE(keyword_table->meta_table_->SetValue( KeywordTable::kDefaultSearchProviderKey, 2)); - EXPECT_TRUE(keyword_table->IsBackupSignatureValid()); + EXPECT_TRUE(keyword_table->IsBackupSignatureValid( + WebDatabase::kCurrentVersionNumber)); EXPECT_EQ(2, keyword_table->GetDefaultSearchProviderID()); EXPECT_TRUE(keyword_table->GetDefaultSearchProviderBackup(&backup)); EXPECT_EQ(kInvalidTemplateURLID, backup.id); EXPECT_EQ(keyword.short_name, backup.short_name); - EXPECT_EQ(url.keyword(), TemplateURL(NULL, backup).keyword()); + EXPECT_EQ(keyword.keyword(), backup.keyword()); EXPECT_EQ(keyword.url(), backup.url()); EXPECT_EQ(keyword.favicon_url, backup.favicon_url); EXPECT_EQ(keyword.suggestions_url, backup.suggestions_url); @@ -202,7 +199,8 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { KeywordTable::kDefaultSearchProviderKey, 1)); EXPECT_TRUE(keyword_table->meta_table_->SetValue( KeywordTable::kDefaultSearchIDBackupKey, 2)); - EXPECT_FALSE(keyword_table->IsBackupSignatureValid()); + EXPECT_FALSE(keyword_table->IsBackupSignatureValid( + WebDatabase::kCurrentVersionNumber)); EXPECT_EQ(1, keyword_table->GetDefaultSearchProviderID()); EXPECT_FALSE(keyword_table->GetDefaultSearchProviderBackup(&backup)); EXPECT_TRUE(keyword_table->DidDefaultSearchProviderChange()); @@ -212,23 +210,26 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { KeywordTable::kDefaultSearchIDBackupKey, 1)); EXPECT_TRUE(keyword_table->meta_table_->SetValue( KeywordTable::kBackupSignatureKey, std::string())); - EXPECT_FALSE(keyword_table->IsBackupSignatureValid()); + EXPECT_FALSE(keyword_table->IsBackupSignatureValid( + WebDatabase::kCurrentVersionNumber)); EXPECT_EQ(1, keyword_table->GetDefaultSearchProviderID()); EXPECT_FALSE(keyword_table->GetDefaultSearchProviderBackup(&backup)); EXPECT_TRUE(keyword_table->DidDefaultSearchProviderChange()); // Change keywords. - EXPECT_TRUE(keyword_table->UpdateBackupSignature()); + EXPECT_TRUE(keyword_table->UpdateBackupSignature( + WebDatabase::kCurrentVersionNumber)); sql::Statement remove_keyword(keyword_table->db_->GetUniqueStatement( "DELETE FROM keywords WHERE id=1")); EXPECT_TRUE(remove_keyword.Run()); - EXPECT_TRUE(keyword_table->IsBackupSignatureValid()); + EXPECT_TRUE(keyword_table->IsBackupSignatureValid( + WebDatabase::kCurrentVersionNumber)); EXPECT_EQ(1, keyword_table->GetDefaultSearchProviderID()); EXPECT_TRUE(keyword_table->GetDefaultSearchProviderBackup(&backup)); EXPECT_EQ(kInvalidTemplateURLID, backup.id); EXPECT_EQ(keyword.short_name, backup.short_name); - EXPECT_EQ(url.keyword(), TemplateURL(NULL, backup).keyword()); + EXPECT_EQ(keyword.keyword(), backup.keyword()); EXPECT_EQ(keyword.url(), backup.url()); EXPECT_EQ(keyword.suggestions_url, backup.suggestions_url); EXPECT_EQ(keyword.favicon_url, backup.favicon_url); @@ -240,7 +241,8 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { sql::Statement remove_keyword_backup(keyword_table->db_->GetUniqueStatement( "DELETE FROM keywords_backup WHERE id=1")); EXPECT_TRUE(remove_keyword_backup.Run()); - EXPECT_FALSE(keyword_table->IsBackupSignatureValid()); + EXPECT_FALSE(keyword_table->IsBackupSignatureValid( + WebDatabase::kCurrentVersionNumber)); EXPECT_EQ(1, keyword_table->GetDefaultSearchProviderID()); EXPECT_FALSE(keyword_table->GetDefaultSearchProviderBackup(&backup)); EXPECT_TRUE(keyword_table->DidDefaultSearchProviderChange()); @@ -263,29 +265,29 @@ TEST_F(KeywordTableTest, GetTableContents) { keyword.date_created = base::Time::UnixEpoch(); keyword.last_modified = base::Time::UnixEpoch(); keyword.sync_guid = "1234-5678-90AB-CDEF"; - TemplateURL url(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); - keyword.SetAutogenerateKeyword(true); + keyword.SetKeyword(ASCIIToUTF16("url")); keyword.instant_url = "http://instant2/"; keyword.originating_url = GURL("http://originating.url/"); keyword.input_encodings.push_back("Shift_JIS"); keyword.id = 2; keyword.prepopulate_id = 5; keyword.sync_guid = "FEDC-BA09-8765-4321"; - TemplateURL url2(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url2)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); const char kTestContents[] = "1short_namekeywordhttp://favicon.url/" - "http://url/1001url2000001234-5678-90AB-CDEF2short_nameurl" - "http://favicon.url/http://url/1http://originating.url/00Shift_JIS1url251" - "00http://instant2/0FEDC-BA09-8765-4321"; + "http://url/1001url20001234-5678-90AB-CDEF2short_nameurl" + "http://favicon.url/http://url/1http://originating.url/00Shift_JIS1url250" + "http://instant2/0FEDC-BA09-8765-4321"; std::string contents; - EXPECT_TRUE(keyword_table->GetTableContents("keywords", &contents)); + EXPECT_TRUE(keyword_table->GetTableContents("keywords", + WebDatabase::kCurrentVersionNumber, &contents)); EXPECT_EQ(kTestContents, contents); - EXPECT_TRUE(keyword_table->GetTableContents("keywords_backup", &contents)); + EXPECT_TRUE(keyword_table->GetTableContents("keywords_backup", + WebDatabase::kCurrentVersionNumber, &contents)); EXPECT_EQ(kTestContents, contents); } @@ -306,29 +308,29 @@ TEST_F(KeywordTableTest, GetTableContentsOrdering) { keyword.date_created = base::Time::UnixEpoch(); keyword.last_modified = base::Time::UnixEpoch(); keyword.sync_guid = "1234-5678-90AB-CDEF"; - TemplateURL url(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); - keyword.SetAutogenerateKeyword(true); + keyword.SetKeyword(ASCIIToUTF16("url")); keyword.instant_url = "http://instant2/"; keyword.originating_url = GURL("http://originating.url/"); keyword.input_encodings.push_back("Shift_JIS"); keyword.id = 1; keyword.prepopulate_id = 5; keyword.sync_guid = "FEDC-BA09-8765-4321"; - TemplateURL url2(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url2)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); const char kTestContents[] = "1short_nameurlhttp://favicon.url/http://url/1" - "http://originating.url/00Shift_JIS1url25100http://instant2/0" + "http://originating.url/00Shift_JIS1url250http://instant2/0" "FEDC-BA09-8765-43212short_namekeywordhttp://favicon.url/http://url/1001" - "url2000001234-5678-90AB-CDEF"; + "url20001234-5678-90AB-CDEF"; std::string contents; - EXPECT_TRUE(keyword_table->GetTableContents("keywords", &contents)); + EXPECT_TRUE(keyword_table->GetTableContents("keywords", + WebDatabase::kCurrentVersionNumber, &contents)); EXPECT_EQ(kTestContents, contents); - EXPECT_TRUE(keyword_table->GetTableContents("keywords_backup", &contents)); + EXPECT_TRUE(keyword_table->GetTableContents("keywords_backup", + WebDatabase::kCurrentVersionNumber, &contents)); EXPECT_EQ(kTestContents, contents); } @@ -346,16 +348,14 @@ TEST_F(KeywordTableTest, UpdateKeyword) { keyword.show_in_default_list = true; keyword.safe_for_autoreplace = true; keyword.id = 1; - TemplateURL url(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); - keyword.originating_url = GURL("http://originating.url/"); - keyword.SetAutogenerateKeyword(true); + keyword.SetKeyword(ASCIIToUTF16("url")); keyword.instant_url = "http://instant2/"; + keyword.originating_url = GURL("http://originating.url/"); keyword.input_encodings.push_back("Shift_JIS"); keyword.prepopulate_id = 5; - TemplateURL url2(NULL, keyword); - EXPECT_TRUE(keyword_table->UpdateKeyword(url2)); + EXPECT_TRUE(keyword_table->UpdateKeyword(keyword)); KeywordTable::Keywords keywords; EXPECT_TRUE(keyword_table->GetKeywords(&keywords)); @@ -363,9 +363,7 @@ TEST_F(KeywordTableTest, UpdateKeyword) { const TemplateURLData& restored_keyword = keywords.front(); EXPECT_EQ(keyword.short_name, restored_keyword.short_name); - EXPECT_EQ(url2.keyword(), TemplateURL(NULL, restored_keyword).keyword()); - EXPECT_EQ(keyword.autogenerate_keyword(), - restored_keyword.autogenerate_keyword()); + EXPECT_EQ(keyword.keyword(), restored_keyword.keyword()); 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); @@ -390,8 +388,7 @@ TEST_F(KeywordTableTest, KeywordWithNoFavicon) { keyword.SetURL("http://url/"); keyword.safe_for_autoreplace = true; keyword.id = -100; - TemplateURL url(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); KeywordTable::Keywords keywords; EXPECT_TRUE(keyword_table->GetKeywords(&keywords)); @@ -399,7 +396,7 @@ TEST_F(KeywordTableTest, KeywordWithNoFavicon) { const TemplateURLData& restored_keyword = keywords.front(); EXPECT_EQ(keyword.short_name, restored_keyword.short_name); - EXPECT_EQ(url.keyword(), TemplateURL(NULL, restored_keyword).keyword()); + EXPECT_EQ(keyword.keyword(), restored_keyword.keyword()); EXPECT_EQ(keyword.favicon_url, restored_keyword.favicon_url); EXPECT_EQ(keyword.safe_for_autoreplace, restored_keyword.safe_for_autoreplace); @@ -416,14 +413,12 @@ TEST_F(KeywordTableTest, SanitizeURLs) { keyword.SetKeyword(ASCIIToUTF16("legit")); keyword.SetURL("http://url/"); keyword.id = 1000; - TemplateURL url(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); keyword.short_name = ASCIIToUTF16("bogus"); keyword.SetKeyword(ASCIIToUTF16("bogus")); keyword.id = 2000; - TemplateURL url2(NULL, keyword); - EXPECT_TRUE(keyword_table->AddKeyword(url2)); + EXPECT_TRUE(keyword_table->AddKeyword(keyword)); KeywordTable::Keywords keywords; EXPECT_TRUE(keyword_table->GetKeywords(&keywords)); @@ -437,7 +432,8 @@ TEST_F(KeywordTableTest, SanitizeURLs) { s.BindString16(0, string16()); s.BindInt64(1, 2000); EXPECT_TRUE(s.Run()); - EXPECT_TRUE(keyword_table->UpdateBackupSignature()); + EXPECT_TRUE(keyword_table->UpdateBackupSignature( + WebDatabase::kCurrentVersionNumber)); // GetKeywords() should erase the entry with the empty URL field. EXPECT_TRUE(keyword_table->GetKeywords(&keywords)); diff --git a/chrome/browser/webdata/web_data_service.cc b/chrome/browser/webdata/web_data_service.cc index b9c5c05..6cf2631 100644 --- a/chrome/browser/webdata/web_data_service.cc +++ b/chrome/browser/webdata/web_data_service.cc @@ -147,12 +147,10 @@ WebDatabase* WebDataService::GetDatabase() { // ////////////////////////////////////////////////////////////////////////////// -void WebDataService::AddKeyword(const TemplateURL& url) { - // Ensure that the keyword is already generated (and cached) before caching - // the TemplateURL for use on another keyword. - url.EnsureKeyword(); - GenericRequest<TemplateURL>* request = - new GenericRequest<TemplateURL>(this, GetNextRequestHandle(), NULL, url); +void WebDataService::AddKeyword(const TemplateURLData& data) { + GenericRequest<TemplateURLData>* request = + new GenericRequest<TemplateURLData>(this, GetNextRequestHandle(), NULL, + data); RegisterRequest(request); ScheduleTask(FROM_HERE, Bind(&WebDataService::AddKeywordImpl, this, request)); } @@ -165,12 +163,10 @@ void WebDataService::RemoveKeyword(TemplateURLID id) { Bind(&WebDataService::RemoveKeywordImpl, this, request)); } -void WebDataService::UpdateKeyword(const TemplateURL& url) { - // Ensure that the keyword is already generated (and cached) before caching - // the TemplateURL for use on another keyword. - url.EnsureKeyword(); - GenericRequest<TemplateURL>* request = - new GenericRequest<TemplateURL>(this, GetNextRequestHandle(), NULL, url); +void WebDataService::UpdateKeyword(const TemplateURLData& data) { + GenericRequest<TemplateURLData>* request = + new GenericRequest<TemplateURLData>(this, GetNextRequestHandle(), NULL, + data); RegisterRequest(request); ScheduleTask(FROM_HERE, Bind(&WebDataService::UpdateKeywordImpl, this, request)); @@ -739,7 +735,7 @@ int WebDataService::GetNextRequestHandle() { // //////////////////////////////////////////////////////////////////////////////// -void WebDataService::AddKeywordImpl(GenericRequest<TemplateURL>* request) { +void WebDataService::AddKeywordImpl(GenericRequest<TemplateURLData>* request) { InitializeDatabaseIfNecessary(); if (db_ && !request->IsCancelled(NULL)) { db_->GetKeywordTable()->AddKeyword(request->arg()); @@ -758,7 +754,8 @@ void WebDataService::RemoveKeywordImpl(GenericRequest<TemplateURLID>* request) { request->RequestComplete(); } -void WebDataService::UpdateKeywordImpl(GenericRequest<TemplateURL>* request) { +void WebDataService::UpdateKeywordImpl( + GenericRequest<TemplateURLData>* request) { InitializeDatabaseIfNecessary(); if (db_ && !request->IsCancelled(NULL)) { if (!db_->GetKeywordTable()->UpdateKeyword(request->arg())) { diff --git a/chrome/browser/webdata/web_data_service.h b/chrome/browser/webdata/web_data_service.h index b4a6630..aea638a 100644 --- a/chrome/browser/webdata/web_data_service.h +++ b/chrome/browser/webdata/web_data_service.h @@ -335,9 +335,9 @@ class WebDataService // the caller (TemplateURLService) does not need to know when the request is // done. - void AddKeyword(const TemplateURL& url); + void AddKeyword(const TemplateURLData& data); void RemoveKeyword(TemplateURLID id); - void UpdateKeyword(const TemplateURL& url); + void UpdateKeyword(const TemplateURLData& data); // Fetches the keywords. // On success, consumer is notified with WDResult<KeywordTable::Keywords>. @@ -597,9 +597,9 @@ class WebDataService // Keywords. // ////////////////////////////////////////////////////////////////////////////// - void AddKeywordImpl(GenericRequest<TemplateURL>* request); + void AddKeywordImpl(GenericRequest<TemplateURLData>* request); void RemoveKeywordImpl(GenericRequest<TemplateURLID>* request); - void UpdateKeywordImpl(GenericRequest<TemplateURL>* request); + void UpdateKeywordImpl(GenericRequest<TemplateURLData>* request); void GetKeywordsImpl(WebDataRequest* request); void SetDefaultSearchProviderImpl(GenericRequest<TemplateURLID>* r); void SetBuiltinKeywordVersionImpl(GenericRequest<int>* r); diff --git a/chrome/browser/webdata/web_database.cc b/chrome/browser/webdata/web_database.cc index 828acd4..48e9262 100644 --- a/chrome/browser/webdata/web_database.cc +++ b/chrome/browser/webdata/web_database.cc @@ -21,11 +21,11 @@ // corresponding changes must happen in the unit tests, and new migration test // added. See |WebDatabaseMigrationTest::kCurrentTestedVersionNumber|. // static -const int WebDatabase::kCurrentVersionNumber = 44; +const int WebDatabase::kCurrentVersionNumber = 45; namespace { -const int kCompatibleVersionNumber = 44; +const int kCompatibleVersionNumber = 45; // Change the version number and possibly the compatibility version of // |meta_table_|. @@ -321,6 +321,14 @@ sql::InitStatus WebDatabase::MigrateOldVersionsAsNeeded() { ChangeVersion(&meta_table_, 44, true); // FALL THROUGH + case 44: + if (!keyword_table_-> + MigrateToVersion45RemoveLogoIDAndAutogenerateColumns()) + return FailedMigrationTo(45); + + ChangeVersion(&meta_table_, 45, true); + // FALL THROUGH + // Add successive versions here. Each should set the version number and // compatible version number as appropriate, then fall through to the next // case. diff --git a/chrome/browser/webdata/web_database_migration_unittest.cc b/chrome/browser/webdata/web_database_migration_unittest.cc index 638c72c..7800579 100644 --- a/chrome/browser/webdata/web_database_migration_unittest.cc +++ b/chrome/browser/webdata/web_database_migration_unittest.cc @@ -194,7 +194,7 @@ class WebDatabaseMigrationTest : public testing::Test { DISALLOW_COPY_AND_ASSIGN(WebDatabaseMigrationTest); }; -const int WebDatabaseMigrationTest::kCurrentTestedVersionNumber = 44; +const int WebDatabaseMigrationTest::kCurrentTestedVersionNumber = 45; void WebDatabaseMigrationTest::LoadDatabase(const FilePath::StringType& file) { std::string contents; @@ -303,7 +303,6 @@ TEST_F(WebDatabaseMigrationTest, MigrateVersion22CorruptedToCurrent) { ASSERT_TRUE( connection.DoesColumnExist("credit_cards", "card_number_encrypted")); ASSERT_TRUE(connection.DoesColumnExist("keywords", "id")); - ASSERT_FALSE(connection.DoesColumnExist("keywords", "logo_id")); } // Load the database via the WebDatabase class and migrate the database to @@ -329,47 +328,6 @@ TEST_F(WebDatabaseMigrationTest, MigrateVersion22CorruptedToCurrent) { EXPECT_TRUE( connection.DoesColumnExist("credit_cards", "card_number_encrypted")); EXPECT_TRUE(connection.DoesColumnExist("keywords", "id")); - EXPECT_TRUE(connection.DoesColumnExist("keywords", "logo_id")); - } -} - -// Tests that the |keywords| |logo_id| column gets added to the schema for a -// version 24 database. -TEST_F(WebDatabaseMigrationTest, MigrateVersion24ToCurrent) { - // This schema is taken from a build prior to the addition of the |keywords| - // |logo_id| column. - ASSERT_NO_FATAL_FAILURE(LoadDatabase(FILE_PATH_LITERAL("version_24.sql"))); - - // Verify pre-conditions. These are expectations for version 24 of the - // database. - { - sql::Connection connection; - ASSERT_TRUE(connection.Open(GetDatabasePath())); - - // Columns existing and not existing before current version. - ASSERT_TRUE(connection.DoesColumnExist("keywords", "id")); - ASSERT_FALSE(connection.DoesColumnExist("keywords", "logo_id")); - } - - // Load the database via the WebDatabase class and migrate the database to - // the current version. - { - WebDatabase db; - ASSERT_EQ(sql::INIT_OK, db.Init(GetDatabasePath())); - } - - // Verify post-conditions. These are expectations for current version of the - // database. - { - sql::Connection connection; - ASSERT_TRUE(connection.Open(GetDatabasePath())); - - // Check version. - EXPECT_EQ(kCurrentTestedVersionNumber, VersionFromConnection(&connection)); - - // |keywords| |logo_id| column should have been added. - EXPECT_TRUE(connection.DoesColumnExist("keywords", "id")); - EXPECT_TRUE(connection.DoesColumnExist("keywords", "logo_id")); } } @@ -618,12 +576,10 @@ TEST_F(WebDatabaseMigrationTest, MigrateVersion27ToCurrent) { EXPECT_EQ(std::string("{google:baseSuggestURL}search?client=chrome&hl=" "{language}&q={searchTerms}"), s2.ColumnString(11)); EXPECT_EQ(1, s2.ColumnInt(12)); - EXPECT_TRUE(s2.ColumnBool(13)); - EXPECT_EQ(6245, s2.ColumnInt(14)); - EXPECT_FALSE(s2.ColumnBool(15)); + EXPECT_EQ(false, s2.ColumnBool(13)); + EXPECT_EQ(std::string(), s2.ColumnString(14)); + EXPECT_EQ(0, s2.ColumnInt(15)); EXPECT_EQ(std::string(), s2.ColumnString(16)); - EXPECT_EQ(0, s2.ColumnInt(17)); - EXPECT_EQ(std::string(), s2.ColumnString(18)); } } @@ -1845,14 +1801,12 @@ TEST_F(WebDatabaseMigrationTest, MigrateVersion42ToCurrent) { EXPECT_EQ("{google:baseSuggestURL}search?client=chrome&hl={language}&" "q={searchTerms}", s.ColumnString(11)); EXPECT_EQ(1, s.ColumnInt(12)); - EXPECT_TRUE(s.ColumnBool(13)); - EXPECT_EQ(6262, s.ColumnInt(14)); - EXPECT_FALSE(s.ColumnBool(15)); + EXPECT_EQ(false, s.ColumnBool(13)); EXPECT_EQ("{google:baseURL}webhp?{google:RLZ}sourceid=chrome-instant&" "ie={inputEncoding}&ion=1{searchTerms}&nord=1", - s.ColumnString(16)); - EXPECT_EQ(0, s.ColumnInt(17)); - EXPECT_EQ("{1234-5678-90AB-CDEF}", s.ColumnString(18)); + s.ColumnString(14)); + EXPECT_EQ(0, s.ColumnInt(15)); + EXPECT_EQ("{1234-5678-90AB-CDEF}", s.ColumnString(16)); EXPECT_FALSE(s.Step()); } @@ -1936,3 +1890,133 @@ TEST_F(WebDatabaseMigrationTest, MigrateVersion43ToCurrent) { EXPECT_FALSE(default_search_provider_id_backup_signature.empty()); } } + +#if !defined(GOOGLE_CHROME_BUILD) +// Tests that the |autogenerate_keyword| and |logo_id| columns get removed from +// the keyword table schema for a version 45 database. +// +// This is enabled on Chromium only because a valid signature is required for +// this test, which makes it key-dependent. +TEST_F(WebDatabaseMigrationTest, MigrateVersion44ToCurrent) { + ASSERT_NO_FATAL_FAILURE(LoadDatabase(FILE_PATH_LITERAL("version_44.sql"))); + + // Verify pre-conditions. These are expectations for version 44 of the + // database. + { + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + ASSERT_TRUE(sql::MetaTable::DoesTableExist(&connection)); + + sql::MetaTable meta_table; + ASSERT_TRUE(meta_table.Init(&connection, 44, 44)); + + ASSERT_TRUE(connection.DoesColumnExist("keywords", "autogenerate_keyword")); + ASSERT_TRUE(connection.DoesColumnExist("keywords", "logo_id")); + } + + // Load the database via the WebDatabase class and migrate the database to + // the current version. + { + WebDatabase db; + ASSERT_EQ(sql::INIT_OK, db.Init(GetDatabasePath())); + ASSERT_FALSE(db.GetKeywordTable()->DidDefaultSearchProviderChange()); + + // The backup table should match the keyword table. + std::string keywords_contents; + EXPECT_TRUE(db.GetKeywordTable()->GetTableContents("keywords", + kCurrentTestedVersionNumber, &keywords_contents)); + std::string keywords_backup_contents; + EXPECT_TRUE(db.GetKeywordTable()->GetTableContents("keywords_backup", + kCurrentTestedVersionNumber, &keywords_backup_contents)); + EXPECT_EQ(keywords_contents, keywords_backup_contents); + } + + // Verify post-conditions. These are expectations for current version of the + // database. + { + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + ASSERT_TRUE(sql::MetaTable::DoesTableExist(&connection)); + + // Check version. + EXPECT_EQ(kCurrentTestedVersionNumber, VersionFromConnection(&connection)); + + sql::MetaTable meta_table; + ASSERT_TRUE(meta_table.Init(&connection, kCurrentTestedVersionNumber, + kCurrentTestedVersionNumber)); + + // We should have removed this obsolete key. + std::string default_search_provider_backup; + EXPECT_FALSE(meta_table.GetValue("Default Search Provider Backup", + &default_search_provider_backup)); + + // Two columns should have been removed. + EXPECT_FALSE(connection.DoesColumnExist("keywords", + "autogenerate_keyword")); + EXPECT_FALSE(connection.DoesColumnExist("keywords", "logo_id")); + } +} +#endif // !defined(GOOGLE_CHROME_BUILD) + +// Like MigrateVersion44ToCurrent above, but with a corrupt backup signature. +// This should result in us dropping the backup table but successfully migrating +// otherwise. +// +// Because this test doesn't rely on a valid signature, we can run it on +// official builds as well. +TEST_F(WebDatabaseMigrationTest, MigrateVersion44CorruptBackupToCurrent) { + ASSERT_NO_FATAL_FAILURE( + LoadDatabase(FILE_PATH_LITERAL("version_44_backup_corrupt.sql"))); + + // Verify pre-conditions. These are expectations for version 44 of the + // database. + { + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + ASSERT_TRUE(sql::MetaTable::DoesTableExist(&connection)); + + sql::MetaTable meta_table; + ASSERT_TRUE(meta_table.Init(&connection, 44, 44)); + + ASSERT_TRUE(connection.DoesColumnExist("keywords", "autogenerate_keyword")); + ASSERT_TRUE(connection.DoesColumnExist("keywords", "logo_id")); + } + + // Load the database via the WebDatabase class and migrate the database to + // the current version. + { + WebDatabase db; + ASSERT_EQ(sql::INIT_OK, db.Init(GetDatabasePath())); + // We should detect a "search provider change" as a side effect of dropping + // the backup table. + ASSERT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange()); + } + + // Verify post-conditions. These are expectations for current version of the + // database. + { + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + ASSERT_TRUE(sql::MetaTable::DoesTableExist(&connection)); + + // Check version. + EXPECT_EQ(kCurrentTestedVersionNumber, VersionFromConnection(&connection)); + + sql::MetaTable meta_table; + ASSERT_TRUE(meta_table.Init(&connection, kCurrentTestedVersionNumber, + kCurrentTestedVersionNumber)); + + // We should have removed this obsolete key. + std::string default_search_provider_backup; + EXPECT_FALSE(meta_table.GetValue("Default Search Provider Backup", + &default_search_provider_backup)); + + // Two columns should have been removed. + EXPECT_FALSE(connection.DoesColumnExist("keywords", + "autogenerate_keyword")); + EXPECT_FALSE(connection.DoesColumnExist("keywords", "logo_id")); + + // The backup table should be gone. + EXPECT_FALSE(connection.DoesTableExist("keywords_backup")); + } +} diff --git a/chrome/test/data/web_database/version_24.sql b/chrome/test/data/web_database/version_24.sql deleted file mode 100644 index 624f7e5..0000000 --- a/chrome/test/data/web_database/version_24.sql +++ /dev/null @@ -1,23 +0,0 @@ -PRAGMA foreign_keys=OFF; -BEGIN TRANSACTION; -CREATE TABLE meta(key LONGVARCHAR NOT NULL UNIQUE PRIMARY KEY,value LONGVARCHAR); -INSERT INTO "meta" VALUES('version','24'); -INSERT INTO "meta" VALUES('last_compatible_version','24'); -INSERT INTO "meta" VALUES('Builtin Keyword Version','29'); -CREATE TABLE keywords (id INTEGER PRIMARY KEY,short_name VARCHAR NOT NULL,keyword VARCHAR NOT NULL,favicon_url VARCHAR NOT NULL,url VARCHAR NOT NULL,show_in_default_list INTEGER,safe_for_autoreplace INTEGER,originating_url VARCHAR,date_created INTEGER DEFAULT 0,usage_count INTEGER DEFAULT 0,input_encodings VARCHAR,suggest_url VARCHAR,prepopulate_id INTEGER DEFAULT 0,autogenerate_keyword INTEGER DEFAULT 0); -CREATE TABLE logins (origin_url VARCHAR NOT NULL, action_url VARCHAR,username_element VARCHAR, username_value VARCHAR,password_element VARCHAR, password_value BLOB, submit_element VARCHAR,signon_realm VARCHAR NOT NULL,ssl_valid INTEGER NOT NULL,preferred INTEGER NOT NULL,date_created INTEGER NOT NULL,blacklisted_by_user INTEGER NOT NULL,scheme INTEGER NOT NULL,UNIQUE (origin_url, username_element,username_value, password_element, submit_element, signon_realm)); -CREATE TABLE web_app_icons (url LONGVARCHAR,width int,height int,image BLOB, UNIQUE (url, width, height)); -CREATE TABLE web_apps (url LONGVARCHAR UNIQUE,has_all_images INTEGER NOT NULL); -CREATE TABLE autofill (name VARCHAR, value VARCHAR, value_lower VARCHAR,pair_id INTEGER PRIMARY KEY, count INTEGER DEFAULT 1); -CREATE TABLE autofill_dates ( pair_id INTEGER DEFAULT 0,date_created INTEGER DEFAULT 0); -CREATE TABLE autofill_profiles ( label VARCHAR,unique_id INTEGER PRIMARY KEY, first_name VARCHAR,middle_name VARCHAR, last_name VARCHAR, email VARCHAR,company_name VARCHAR, address_line_1 VARCHAR, address_line_2 VARCHAR,city VARCHAR, state VARCHAR, zipcode VARCHAR, country VARCHAR,phone VARCHAR, fax VARCHAR); -CREATE TABLE credit_cards ( label VARCHAR, unique_id INTEGER PRIMARY KEY,name_on_card VARCHAR, type VARCHAR, card_number VARCHAR,expiration_month INTEGER, expiration_year INTEGER,verification_code VARCHAR, billing_address VARCHAR,shipping_address VARCHAR, card_number_encrypted BLOB,verification_code_encrypted BLOB); -CREATE TABLE token_service (service VARCHAR PRIMARY KEY NOT NULL,encrypted_token BLOB); -CREATE INDEX logins_signon ON logins (signon_realm); -CREATE INDEX web_apps_url_index ON web_apps (url); -CREATE INDEX autofill_name ON autofill (name); -CREATE INDEX autofill_name_value_lower ON autofill (name, value_lower); -CREATE INDEX autofill_dates_pair_id ON autofill_dates (pair_id); -CREATE INDEX autofill_profiles_label_index ON autofill_profiles (label); -CREATE INDEX credit_cards_label_index ON credit_cards (label); -COMMIT; diff --git a/sync/protocol/search_engine_specifics.proto b/sync/protocol/search_engine_specifics.proto index effaf99..4fbe40f 100644 --- a/sync/protocol/search_engine_specifics.proto +++ b/sync/protocol/search_engine_specifics.proto @@ -41,7 +41,8 @@ message SearchEngineSpecifics { // The ID associated with the prepopulate data this search engine comes from. // Set to zero if it was not prepopulated. optional int32 prepopulate_id = 11; - // Whether to autogenerate a keyword for the search engine or not. + // DEPRECATED: Whether to autogenerate a keyword for the search engine or not. + // Do not use this field in the future. optional bool autogenerate_keyword = 12; // ID 13 reserved - previously used by |logo_id|, now deprecated. // Obsolete field. This used to represent whether or not this search engine |