From 59d6c3f7b00186d84fce976057662e970b15886c Mon Sep 17 00:00:00 2001 From: "avayvod@chromium.org" Date: Wed, 21 Dec 2011 12:54:36 +0000 Subject: [PATCH] Return backed up TemplateURL on default search change R=ivankr@chromium.org BUG=None TEST=None Review URL: http://codereview.chromium.org/8965037 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@115318 0039d316-1c4b-4281-b951-d872f2087c98 --- .../protector/default_search_provider_change.cc | 2 ++ .../browser/search_engines/template_url_service.cc | 5 ++- chrome/browser/search_engines/util.cc | 10 +++--- chrome/browser/search_engines/util.h | 4 +-- chrome/browser/webdata/keyword_table.cc | 41 ++++++++++++++++++---- chrome/browser/webdata/keyword_table.h | 5 +-- chrome/browser/webdata/keyword_table_unittest.cc | 41 ++++++++++++++++++---- chrome/browser/webdata/web_data_service.cc | 4 +-- chrome/browser/webdata/web_data_service.h | 4 +-- .../browser/webdata/web_data_service_unittest.cc | 2 +- 10 files changed, 87 insertions(+), 31 deletions(-) diff --git a/chrome/browser/protector/default_search_provider_change.cc b/chrome/browser/protector/default_search_provider_change.cc index cf702be02fb4..75d99572f938 100644 --- a/chrome/browser/protector/default_search_provider_change.cc +++ b/chrome/browser/protector/default_search_provider_change.cc @@ -165,6 +165,8 @@ DefaultSearchProviderChange::DefaultSearchProviderChange( // search provider will be used. old_id_ = 0; } + // TODO(avayvod): Keep the URL and delete it later. + delete old_url; } DefaultSearchProviderChange::~DefaultSearchProviderChange() { diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc index 1b85e63621d6..9fe2248a7879 100644 --- a/chrome/browser/search_engines/template_url_service.cc +++ b/chrome/browser/search_engines/template_url_service.cc @@ -533,11 +533,10 @@ void TemplateURLService::OnWebDataServiceRequestDone( // search may be changed below by Sync which effectively undoes the hijacking. bool is_default_search_hijacked = false; const TemplateURL* hijacked_default_search_provider = NULL; - const TemplateURL* backup_default_search_provider = NULL; + scoped_ptr backup_default_search_provider; // No check is required if the default search is managed. if (!is_default_search_managed_ && DidDefaultSearchProviderChange(*result, - template_urls, &backup_default_search_provider)) { hijacked_default_search_provider = default_search_provider; is_default_search_hijacked = true; @@ -633,7 +632,7 @@ void TemplateURLService::OnWebDataServiceRequestDone( protector::Protector* protector = new protector::Protector(profile()); protector->ShowChange(protector::CreateDefaultSearchProviderChange( hijacked_default_search_provider, - backup_default_search_provider)); + backup_default_search_provider.release())); } } diff --git a/chrome/browser/search_engines/util.cc b/chrome/browser/search_engines/util.cc index bee5baccf321..5bafd436a1cb 100644 --- a/chrome/browser/search_engines/util.cc +++ b/chrome/browser/search_engines/util.cc @@ -211,10 +211,9 @@ void GetSearchProvidersUsingKeywordResult( bool DidDefaultSearchProviderChange( const WDTypedResult& result, - const std::vector& template_urls, - const TemplateURL** backup_default_search_provider) { + scoped_ptr* backup_default_search_provider) { DCHECK(backup_default_search_provider); - DCHECK(*backup_default_search_provider == NULL); + DCHECK(!backup_default_search_provider->get()); DCHECK_EQ(result.GetType(), KEYWORDS_RESULT); WDKeywordsResult keyword_result = reinterpret_cast< @@ -223,9 +222,8 @@ bool DidDefaultSearchProviderChange( if (!keyword_result.did_default_search_provider_change) return false; - *backup_default_search_provider = GetTemplateURLByID( - template_urls, - keyword_result.default_search_provider_id_backup); + backup_default_search_provider->reset( + keyword_result.default_search_provider_backup); return true; } diff --git a/chrome/browser/search_engines/util.h b/chrome/browser/search_engines/util.h index 09f001fb8912..5c6ea743c1da 100644 --- a/chrome/browser/search_engines/util.h +++ b/chrome/browser/search_engines/util.h @@ -9,6 +9,7 @@ // This file contains utility functions for search engine functionality. #include +#include "base/memory/scoped_ptr.h" #include "base/string16.h" class PrefService; @@ -46,7 +47,6 @@ void GetSearchProvidersUsingKeywordResult( // lost. bool DidDefaultSearchProviderChange( const WDTypedResult& result, - const std::vector& template_urls, - const TemplateURL** backup_default_search_provider); + scoped_ptr* backup_default_search_provider); #endif // CHROME_BROWSER_SEARCH_ENGINES_UTIL_H_ diff --git a/chrome/browser/webdata/keyword_table.cc b/chrome/browser/webdata/keyword_table.cc index c7e123e187c9..6ecca88a615b 100644 --- a/chrome/browser/webdata/keyword_table.cc +++ b/chrome/browser/webdata/keyword_table.cc @@ -5,6 +5,7 @@ #include "chrome/browser/webdata/keyword_table.h" #include "base/logging.h" +#include "base/memory/scoped_ptr.h" #include "base/metrics/histogram.h" #include "base/metrics/stats_counters.h" #include "base/string_number_conversions.h" @@ -222,12 +223,40 @@ int64 KeywordTable::GetDefaultSearchProviderID() { return value; } -int64 KeywordTable::GetDefaultSearchProviderIDBackup() { +TemplateURL* KeywordTable::GetDefaultSearchProviderBackup() { if (!IsBackupSignatureValid()) - return 0; - int64 backup_value = 0; - meta_table_->GetValue(kDefaultSearchIDBackupKey, &backup_value); - return backup_value; + return NULL; + + int64 backup_id = 0; + if (!meta_table_->GetValue(kDefaultSearchIDBackupKey, &backup_id)) { + LOG(ERROR) << "No default search id backup found."; + return NULL; + } + sql::Statement s(db_->GetUniqueStatement( + "SELECT 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 " + "FROM keywords_backup WHERE id=?")); + if (!s) { + NOTREACHED() << "Statement prepare failed"; + return NULL; + } + s.BindInt64(0, backup_id); + if (!s.Step()) { + LOG(ERROR) << "No default search provider with backup id."; + return NULL; + } + + scoped_ptr template_url(new TemplateURL()); + GetURLFromStatement(s, template_url.get()); + + if (!s.Succeeded()) { + LOG(ERROR) << "Statement has not succeeded."; + return NULL; + } + return template_url.release(); } bool KeywordTable::DidDefaultSearchProviderChange() { @@ -236,7 +265,7 @@ bool KeywordTable::DidDefaultSearchProviderChange() { protector::kProtectorHistogramDefaultSearchProvider, protector::kProtectorErrorBackupInvalid, protector::kProtectorErrorCount); - LOG(ERROR) << "Backup signature is invalid"; + LOG(ERROR) << "Backup signature is invalid."; return true; } diff --git a/chrome/browser/webdata/keyword_table.h b/chrome/browser/webdata/keyword_table.h index 49143848a7ca..504dcd7d13c8 100644 --- a/chrome/browser/webdata/keyword_table.h +++ b/chrome/browser/webdata/keyword_table.h @@ -107,8 +107,9 @@ class KeywordTable : public WebDatabaseTable { bool SetDefaultSearchProviderID(int64 id); int64 GetDefaultSearchProviderID(); - // Backup of the default search provider. 0 if the setting can't be verified. - int64 GetDefaultSearchProviderIDBackup(); + // Backup of the default search provider. NULL if the backup is invalid. The + // returned TemplateURL is owned by the caller. + TemplateURL* GetDefaultSearchProviderBackup(); // Returns true if the default search provider has been changed out under // us. This can happen if another process modifies our database or the diff --git a/chrome/browser/webdata/keyword_table_unittest.cc b/chrome/browser/webdata/keyword_table_unittest.cc index dcf9bfa6566b..b90dd7fdaa30 100644 --- a/chrome/browser/webdata/keyword_table_unittest.cc +++ b/chrome/browser/webdata/keyword_table_unittest.cc @@ -195,7 +195,6 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { template_url.set_short_name(ASCIIToUTF16("short_name")); template_url.set_keyword(ASCIIToUTF16("keyword")); GURL favicon_url("http://favicon.url/"); - GURL originating_url("http://originating.url/"); template_url.SetFaviconURL(favicon_url); template_url.SetURL("http://url/", 0, 0); template_url.set_safe_for_autoreplace(true); @@ -208,7 +207,17 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { ASSERT_TRUE(db.GetKeywordTable()->SetDefaultSearchProviderID(1)); EXPECT_TRUE(db.GetKeywordTable()->IsBackupSignatureValid()); EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderID()); - EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup()); + + scoped_ptr backup_url( + db.GetKeywordTable()->GetDefaultSearchProviderBackup()); + EXPECT_EQ(1, backup_url->id()); + EXPECT_EQ(ASCIIToUTF16("short_name"), backup_url->short_name()); + EXPECT_EQ(ASCIIToUTF16("keyword"), backup_url->keyword()); + EXPECT_TRUE(favicon_url == backup_url->GetFaviconURL()); + EXPECT_EQ("http://url/", backup_url->url()->url()); + EXPECT_TRUE(backup_url->safe_for_autoreplace()); + EXPECT_TRUE(backup_url->show_in_default_list()); + EXPECT_EQ("url2", backup_url->suggestions_url()->url()); EXPECT_FALSE(db.GetKeywordTable()->DidDefaultSearchProviderChange()); // Change the actual setting. @@ -216,7 +225,16 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { "Default Search Provider ID", 2)); EXPECT_TRUE(db.GetKeywordTable()->IsBackupSignatureValid()); EXPECT_EQ(2, db.GetKeywordTable()->GetDefaultSearchProviderID()); - EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup()); + + backup_url.reset(db.GetKeywordTable()->GetDefaultSearchProviderBackup()); + EXPECT_EQ(1, backup_url->id()); + EXPECT_EQ(ASCIIToUTF16("short_name"), backup_url->short_name()); + EXPECT_EQ(ASCIIToUTF16("keyword"), backup_url->keyword()); + EXPECT_TRUE(favicon_url == backup_url->GetFaviconURL()); + EXPECT_EQ("http://url/", backup_url->url()->url()); + EXPECT_TRUE(backup_url->safe_for_autoreplace()); + EXPECT_TRUE(backup_url->show_in_default_list()); + EXPECT_EQ("url2", backup_url->suggestions_url()->url()); EXPECT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange()); // Change the backup. @@ -226,7 +244,7 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { "Default Search Provider ID Backup", 2)); EXPECT_FALSE(db.GetKeywordTable()->IsBackupSignatureValid()); EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderID()); - EXPECT_EQ(0, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup()); + EXPECT_EQ(NULL, db.GetKeywordTable()->GetDefaultSearchProviderBackup()); EXPECT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange()); // Change the signature. @@ -236,7 +254,7 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { "Default Search Provider ID Backup Signature", "")); EXPECT_FALSE(db.GetKeywordTable()->IsBackupSignatureValid()); EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderID()); - EXPECT_EQ(0, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup()); + EXPECT_EQ(NULL, db.GetKeywordTable()->GetDefaultSearchProviderBackup()); EXPECT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange()); // Change keywords. @@ -246,7 +264,16 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { ASSERT_TRUE(remove_keyword.Run()); EXPECT_TRUE(db.GetKeywordTable()->IsBackupSignatureValid()); EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderID()); - EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup()); + + backup_url.reset(db.GetKeywordTable()->GetDefaultSearchProviderBackup()); + EXPECT_EQ(1, backup_url->id()); + EXPECT_EQ(ASCIIToUTF16("short_name"), backup_url->short_name()); + EXPECT_EQ(ASCIIToUTF16("keyword"), backup_url->keyword()); + EXPECT_TRUE(favicon_url == backup_url->GetFaviconURL()); + EXPECT_EQ("http://url/", backup_url->url()->url()); + EXPECT_TRUE(backup_url->safe_for_autoreplace()); + EXPECT_TRUE(backup_url->show_in_default_list()); + EXPECT_EQ("url2", backup_url->suggestions_url()->url()); EXPECT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange()); // Change keywords backup. @@ -256,7 +283,7 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) { ASSERT_TRUE(remove_keyword_backup.Run()); EXPECT_FALSE(db.GetKeywordTable()->IsBackupSignatureValid()); EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderID()); - EXPECT_EQ(0, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup()); + EXPECT_EQ(NULL, db.GetKeywordTable()->GetDefaultSearchProviderBackup()); EXPECT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange()); } diff --git a/chrome/browser/webdata/web_data_service.cc b/chrome/browser/webdata/web_data_service.cc index f260fe900e50..197e1228bfd7 100644 --- a/chrome/browser/webdata/web_data_service.cc +++ b/chrome/browser/webdata/web_data_service.cc @@ -806,8 +806,8 @@ void WebDataService::GetKeywordsImpl(WebDataRequest* request) { db_->GetKeywordTable()->GetDefaultSearchProviderID(); result.builtin_keyword_version = db_->GetKeywordTable()->GetBuiltinKeywordVersion(); - result.default_search_provider_id_backup = - db_->GetKeywordTable()->GetDefaultSearchProviderIDBackup(); + result.default_search_provider_backup = + db_->GetKeywordTable()->GetDefaultSearchProviderBackup(); result.did_default_search_provider_change = db_->GetKeywordTable()->DidDefaultSearchProviderChange(); request->SetResult( diff --git a/chrome/browser/webdata/web_data_service.h b/chrome/browser/webdata/web_data_service.h index eedc1185d378..61e6fc27671d 100644 --- a/chrome/browser/webdata/web_data_service.h +++ b/chrome/browser/webdata/web_data_service.h @@ -111,8 +111,8 @@ struct WDKeywordsResult { int64 default_search_provider_id; // Version of the built-in keywords. A value of 0 indicates a first run. int builtin_keyword_version; - // Backup of the default search provider ID. - int64 default_search_provider_id_backup; + // Backup of the default search provider. NULL if the backup is invalid. + TemplateURL* default_search_provider_backup; // Indicates if default search provider has been changed by something // other than user's action in the browser. bool did_default_search_provider_change; diff --git a/chrome/browser/webdata/web_data_service_unittest.cc b/chrome/browser/webdata/web_data_service_unittest.cc index a6e4891ab6ae..b816772820ff 100644 --- a/chrome/browser/webdata/web_data_service_unittest.cc +++ b/chrome/browser/webdata/web_data_service_unittest.cc @@ -731,5 +731,5 @@ TEST_F(WebDataServiceTest, DidDefaultSearchProviderChangeOnNewProfile) { KeywordsConsumer::WaitUntilCalled(); ASSERT_TRUE(consumer.load_succeeded); EXPECT_FALSE(consumer.keywords_result.did_default_search_provider_change); - EXPECT_EQ(0, consumer.keywords_result.default_search_provider_id_backup); + EXPECT_EQ(NULL, consumer.keywords_result.default_search_provider_backup); } -- 2.11.4.GIT