Extend the default search provider sanity checks.

BUG=116952
TEST=TemplateURLServiceTest.*, DefaultSearchProviderChangeTest.*


Review URL: http://codereview.chromium.org/10084017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@132980 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 198 insertions, 35 deletions

diff --git a/chrome/browser/protector/default_search_provider_change.cc b/chrome/browser/protector/default_search_provider_change.cc
index 012657e..dbd0027 100644
--- a/chrome/browser/protector/default_search_provider_change.cc
+++ b/chrome/browser/protector/default_search_provider_change.cc
@@ -121,6 +121,10 @@ class DefaultSearchProviderChange : public BaseSettingChange,
   const TemplateURL* SetDefaultSearchProvider(
       scoped_ptr<TemplateURL>* search_provider);
 
+  // Returns true if |new_search_provider_| can be used as the default search
+  // provider.
+  bool NewSearchProviderValid() const;
+
   // Opens the Search engine settings page in a new tab.
   void OpenSearchEngineSettings(Browser* browser);
 
@@ -186,7 +190,8 @@ bool DefaultSearchProviderChange::Init(Profile* profile) {
   if (!BaseSettingChange::Init(profile))
     return false;
 
-  if (!backup_search_provider_.get()) {
+  if (!backup_search_provider_.get() ||
+      !backup_search_provider_->SupportsReplacement()) {
     // Fallback to a prepopulated default search provider, ignoring any
     // overrides in Prefs.
     backup_search_provider_.reset(
@@ -246,7 +251,7 @@ void DefaultSearchProviderChange::Apply(Browser* browser) {
       kProtectorMaxSearchProviderID);
 
   GetTemplateURLService()->RemoveObserver(this);
-  if (new_search_provider_) {
+  if (NewSearchProviderValid()) {
     GetTemplateURLService()->SetDefaultSearchProvider(new_search_provider_);
   } else {
     // Open settings page in case the new setting is invalid.
@@ -302,7 +307,7 @@ string16 DefaultSearchProviderChange::GetBubbleMessage() const {
 }
 
 string16 DefaultSearchProviderChange::GetApplyButtonText() const {
-  if (new_search_provider_) {
+  if (NewSearchProviderValid()) {
     // If backup search engine is lost and fallback was made to the current
     // search provider then there is no need to show this button.
     if (fallback_is_new_)
@@ -333,7 +338,7 @@ string16 DefaultSearchProviderChange::GetDiscardButtonText() const {
 
 BaseSettingChange::DisplayName
 DefaultSearchProviderChange::GetApplyDisplayName() const {
-  if (new_search_provider_ &&
+  if (NewSearchProviderValid() &&
       new_search_provider_->short_name().length() <= kMaxDisplayedNameLength) {
     return DisplayName(kDefaultSearchProviderChangeNamePriority,
                        new_search_provider_->short_name());
@@ -348,7 +353,7 @@ GURL DefaultSearchProviderChange::GetNewSettingURL() const {
     // and Discard behaviour is pretty different.
     return GURL();
   }
-  if (!new_search_provider_)
+  if (!NewSearchProviderValid())
     return GURL();
   return TemplateURLService::GenerateSearchURL(new_search_provider_);
 }
@@ -410,6 +415,10 @@ const TemplateURL* DefaultSearchProviderChange::SetDefaultSearchProvider(
   return new_default_provider;
 }
 
+bool DefaultSearchProviderChange::NewSearchProviderValid() const {
+  return new_search_provider_ && new_search_provider_->SupportsReplacement();
+}
+
 void DefaultSearchProviderChange::OpenSearchEngineSettings(Browser* browser) {
   ProtectorServiceFactory::GetForProfile(profile())->OpenTab(
       GURL(std::string(chrome::kChromeUISettingsURL) +
diff --git a/chrome/browser/protector/default_search_provider_change_browsertest.cc b/chrome/browser/protector/default_search_provider_change_browsertest.cc
index e5a63e7..5d8aca8 100644
--- a/chrome/browser/protector/default_search_provider_change_browsertest.cc
+++ b/chrome/browser/protector/default_search_provider_change_browsertest.cc
@@ -34,10 +34,14 @@ namespace {
 const string16 example_info = ASCIIToUTF16("Example.info");
 const string16 example_info_long = ASCIIToUTF16("ExampleSearchEngine.info");
 const std::string http_example_info = "http://example.info/?q={searchTerms}";
+const std::string http_example_info_no_search_terms =
+    "http://example.net/";
 const std::string example_info_domain = "example.info";
 const string16 example_com = ASCIIToUTF16("Example.com");
 const string16 example_com_long = ASCIIToUTF16("ExampleSearchEngine.com");
 const std::string http_example_com = "http://example.com/?q={searchTerms}";
+const std::string http_example_com_no_search_terms =
+    "http://example.com/";
 const std::string example_com_domain = "example.com";
 const string16 example_net = ASCIIToUTF16("Example.net");
 const std::string http_example_net = "http://example.net/?q={searchTerms}";
@@ -302,6 +306,63 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, BackupInvalid) {
             turl_service_->GetDefaultSearchProvider());
 }
 
+
+IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest,
+                       BackupValidCannotBeDefault) {
+  // Backup is a valid keyword but cannot be the default search provider (has no
+  // search terms), current search provider exists, fallback to the prepopulated
+  // default search, which exists among keywords.
+  TemplateURL* backup_url =
+      MakeTemplateURL(example_info, ASCIIToUTF16("a"),
+                      http_example_info_no_search_terms);
+  int prepopulated_histogram_id =
+      protector::GetSearchProviderHistogramID(prepopulated_url_.get());
+  TemplateURL* current_url =
+      MakeTemplateURL(example_com, ASCIIToUTF16("b"), http_example_com);
+  int current_histogram_id =
+      protector::GetSearchProviderHistogramID(current_url);
+
+  AddAndSetDefault(current_url);
+
+  // Prepopulated default search must exist.
+  ASSERT_TRUE(FindTemplateURL(prepopulated_url_->url()));
+
+  scoped_ptr<BaseSettingChange> change(
+      CreateDefaultSearchProviderChange(current_url, backup_url));
+  ASSERT_TRUE(change.get());
+  ASSERT_TRUE(change->Init(browser()->profile()));
+
+  // Verify that the prepopulated default search is active.
+  EXPECT_EQ(FindTemplateURL(prepopulated_url_->url()),
+            turl_service_->GetDefaultSearchProvider());
+
+  // Verify histograms.
+  ExpectHistogramCount(kProtectorHistogramSearchProviderHijacked,
+                       current_histogram_id, 1);
+  ExpectHistogramCount(kProtectorHistogramSearchProviderRestored,
+                       prepopulated_histogram_id, 1);
+  ExpectHistogramCount(kProtectorHistogramSearchProviderFallback,
+                       prepopulated_histogram_id, 1);
+
+  // Verify text messages.
+  EXPECT_EQ(GetBubbleMessage(prepopulated_url_->short_name()),
+            change->GetBubbleMessage());
+  EXPECT_EQ(GetChangeSearchButtonText(example_com),
+            change->GetApplyButtonText());
+  EXPECT_EQ(GetOpenSettingsButtonText(), change->GetDiscardButtonText());
+
+  // Verify that search engine settings are opened by Discard.
+  ExpectSettingsOpened(chrome::kSearchEnginesSubPage);
+  change->Discard(browser());
+  EXPECT_EQ(FindTemplateURL(prepopulated_url_->url()),
+            turl_service_->GetDefaultSearchProvider());
+
+  // Verify that Apply switches back to |current_url|.
+  change->Apply(browser());
+  EXPECT_EQ(FindTemplateURL(http_example_com),
+            turl_service_->GetDefaultSearchProvider());
+}
+
 IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest,
                        BackupInvalidFallbackRemoved) {
   // Backup is invalid, current search provider exists, fallback to the
@@ -408,6 +469,57 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest,
 }
 
 IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest,
+                       BackupValidCurrentCannotBeDefault) {
+  // Backup is valid, current search provider has no search terms.
+  TemplateURL* backup_url =
+      MakeTemplateURL(example_info, ASCIIToUTF16("a"), http_example_info);
+  int backup_histogram_id = protector::GetSearchProviderHistogramID(backup_url);
+  TemplateURL* current_url =
+      MakeTemplateURL(example_com, ASCIIToUTF16("b"),
+                      http_example_com_no_search_terms);
+  int current_histogram_id =
+      protector::GetSearchProviderHistogramID(current_url);
+
+  turl_service_->Add(new TemplateURL(backup_url->data()));
+  // TODO(ivankr): this may become unsupported soon.
+  AddAndSetDefault(current_url);
+
+  scoped_ptr<BaseSettingChange> change(
+      CreateDefaultSearchProviderChange(current_url, backup_url));
+  ASSERT_TRUE(change.get());
+  ASSERT_TRUE(change->Init(browser()->profile()));
+
+  // Verify that backup is active.
+  EXPECT_EQ(FindTemplateURL(http_example_info),
+            turl_service_->GetDefaultSearchProvider());
+
+  // Verify histograms.
+  ExpectHistogramCount(kProtectorHistogramSearchProviderHijacked,
+                       current_histogram_id, 1);
+  ExpectHistogramCount(kProtectorHistogramSearchProviderRestored,
+                       backup_histogram_id, 1);
+
+  // Verify text messages.
+  EXPECT_EQ(GetBubbleMessage(), change->GetBubbleMessage());
+  EXPECT_EQ(GetOpenSettingsButtonText(), change->GetApplyButtonText());
+  EXPECT_EQ(GetKeepSearchButtonText(example_info),
+            change->GetDiscardButtonText());
+  EXPECT_EQ(GURL(), change->GetNewSettingURL());
+  EXPECT_EQ(kNoDisplayName, change->GetApplyDisplayName());
+
+  // Discard does nothing - backup was already active.
+  change->Discard(browser());
+  EXPECT_EQ(FindTemplateURL(http_example_info),
+            turl_service_->GetDefaultSearchProvider());
+
+   // Verify that search engine settings are opened by Apply (no other changes).
+  ExpectSettingsOpened(chrome::kSearchEnginesSubPage);
+  change->Apply(browser());
+  EXPECT_EQ(FindTemplateURL(http_example_info),
+            turl_service_->GetDefaultSearchProvider());
+}
+
+IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest,
                        BackupInvalidCurrentRemoved) {
   // Backup is invalid, no current search provider, fallback to the prepopulated
   // default search.
diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index decade1..fb6f31e 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -493,10 +493,11 @@ const TemplateURL* TemplateURLService::FindNewDefaultSearchProvider() {
     if ((*i)->prepopulate_id() == prepopulated_default->prepopulate_id())
       return *i;
   }
-  // If not, use the first non-extension keyword of the templates.
+  // If not, use the first non-extension keyword of the templates that supports
+  // search term replacement.
   for (TemplateURLVector::const_iterator i(template_urls_.begin());
        i != template_urls_.end(); ++i) {
-    if (!(*i)->IsExtensionKeyword())
+    if (!(*i)->IsExtensionKeyword() && (*i)->SupportsReplacement())
       return *i;
   }
   return NULL;
@@ -657,6 +658,8 @@ void TemplateURLService::OnWebDataServiceRequestDone(
   if (new_resource_keyword_version)
     service_->SetBuiltinKeywordVersion(new_resource_keyword_version);
 
+  bool check_if_default_search_valid = !is_default_search_managed_;
+
 #if defined(ENABLE_PROTECTOR_SERVICE)
   // Don't do anything if the default search provider has been changed since the
   // check at the beginning (overridden by Sync).
@@ -678,14 +681,19 @@ void TemplateURLService::OnWebDataServiceRequestDone(
       // every time when keywords are loaded until a search provider is added.
       service_->SetDefaultSearchProvider(default_search_provider_);
     }
+    // The default search provider sanity check makes no sense in this case
+    // because ProtectorService is going to change default search eventually.
+    check_if_default_search_valid = false;
   }
 #endif
 
-  if (!is_default_search_managed_) {
+  if (check_if_default_search_valid) {
+    bool has_default_search_provider = default_search_provider_ != NULL &&
+        default_search_provider_->SupportsReplacement();
     UMA_HISTOGRAM_BOOLEAN("Search.HasDefaultSearchProvider",
-                          default_search_provider_ != NULL);
+                          has_default_search_provider);
     // Ensure that default search provider exists. See http://crbug.com/116952.
-    if (!default_search_provider_)
+    if (!has_default_search_provider)
       SetDefaultSearchProviderNoNotify(FindNewDefaultSearchProvider());
   }
 
@@ -1951,8 +1959,8 @@ void TemplateURLService::SetDefaultSearchProviderIfNewlySynced(
     // Make sure this actually exists. We should not be calling this unless we
     // really just added this TemplateURL.
     const TemplateURL* turl_from_sync = GetTemplateURLForGUID(guid);
-    CHECK(turl_from_sync);
-    SetDefaultSearchProvider(turl_from_sync);
+    if (turl_from_sync && turl_from_sync->SupportsReplacement())
+      SetDefaultSearchProvider(turl_from_sync);
     pending_synced_default_search_ = false;
   }
 }
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 9db243c..9fb42a4 100644
--- a/chrome/browser/search_engines/template_url_service_sync_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_sync_unittest.cc
@@ -1115,8 +1115,13 @@ TEST_F(TemplateURLServiceSyncTest, MergeTwiceWithSameSyncData) {
 }
 
 TEST_F(TemplateURLServiceSyncTest, SyncedDefaultGUIDArrivesFirst) {
-  model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES,
-                                    CreateInitialSyncData(), PassProcessor());
+  SyncDataList initial_data = CreateInitialSyncData();
+  // The default search provider should support replacement.
+  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());
   model()->SetDefaultSearchProvider(model()->GetTemplateURLForGUID("key2"));
 
   EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size());
@@ -1145,7 +1150,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultGUIDArrivesFirst) {
   // the default has changed to the new search engine.
   SyncChangeList changes2;
   changes2.push_back(CreateTestSyncChange(SyncChange::ACTION_ADD,
-      CreateTestTemplateURL(ASCIIToUTF16("new"), "http://new.com",
+      CreateTestTemplateURL(ASCIIToUTF16("new"), "http://new.com/{searchTerms}",
                             "newdefault")));
   model()->ProcessSyncChanges(FROM_HERE, changes2);
 
@@ -1157,7 +1162,8 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultGUIDArrivesFirst) {
 TEST_F(TemplateURLServiceSyncTest, SyncedDefaultArrivesAfterStartup) {
   // Start with the default set to something in the model before we start
   // syncing.
-  model()->Add(CreateTestTemplateURL(ASCIIToUTF16("what"), "http://thewhat.com",
+  model()->Add(CreateTestTemplateURL(ASCIIToUTF16("what"),
+                                     "http://thewhat.com/{searchTerms}",
                                      "initdefault"));
   model()->SetDefaultSearchProvider(
       model()->GetTemplateURLForGUID("initdefault"));
@@ -1175,8 +1181,13 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultArrivesAfterStartup) {
 
   // Now sync the initial data, which will include the search engine entry
   // destined to become the new default.
-  model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES,
-                                    CreateInitialSyncData(), PassProcessor());
+  SyncDataList initial_data = CreateInitialSyncData();
+  // The default search provider should support replacement.
+  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());
 
   // Ensure that the new default has been set.
   EXPECT_EQ(4U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size());
@@ -1188,7 +1199,8 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultAlreadySetOnStartup) {
   // Start with the default set to something in the model before we start
   // syncing.
   const char kGUID[] = "initdefault";
-  model()->Add(CreateTestTemplateURL(ASCIIToUTF16("what"), "http://thewhat.com",
+  model()->Add(CreateTestTemplateURL(ASCIIToUTF16("what"),
+                                     "http://thewhat.com/{searchTerms}",
                                      kGUID));
   model()->SetDefaultSearchProvider(model()->GetTemplateURLForGUID(kGUID));
 
@@ -1236,7 +1248,8 @@ TEST_F(TemplateURLServiceSyncTest, SyncWithManagedDefaultSearch) {
   // being managed.
   SyncChangeList changes;
   changes.push_back(CreateTestSyncChange(SyncChange::ACTION_ADD,
-      CreateTestTemplateURL(ASCIIToUTF16("newkeyword"), "http://new.com",
+      CreateTestTemplateURL(ASCIIToUTF16("newkeyword"),
+                            "http://new.com/{searchTerms}",
                             "newdefault")));
   model()->ProcessSyncChanges(FROM_HERE, changes);
 
@@ -1264,13 +1277,18 @@ TEST_F(TemplateURLServiceSyncTest, SyncMergeDeletesDefault) {
   // If the value from Sync is a duplicate of the local default and is newer, it
   // should safely replace the local value and set as the new default.
   TemplateURL* default_turl = CreateTestTemplateURL(ASCIIToUTF16("key1"),
-      "http://key1.com", "whateverguid", 10);
+      "http://key1.com/{searchTerms}", "whateverguid", 10);
   model()->Add(default_turl);
   model()->SetDefaultSearchProvider(default_turl);
 
+  SyncDataList initial_data = CreateInitialSyncData();
   // The key1 entry should be a duplicate of the default.
+  scoped_ptr<TemplateURL> turl(CreateTestTemplateURL(ASCIIToUTF16("key1"),
+      "http://key1.com/{searchTerms}", "key1", 90));
+  initial_data[0] = TemplateURLService::CreateSyncDataFromTemplateURL(*turl);
+
   model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES,
-                                    CreateInitialSyncData(), PassProcessor());
+                                    initial_data, PassProcessor());
 
   EXPECT_EQ(3U, model()->GetAllSyncData(syncable::SEARCH_ENGINES).size());
   EXPECT_FALSE(model()->GetTemplateURLForGUID("whateverguid"));
diff --git a/chrome/browser/search_engines/template_url_service_unittest.cc b/chrome/browser/search_engines/template_url_service_unittest.cc
index 31b63bf..b44b31d 100644
--- a/chrome/browser/search_engines/template_url_service_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_unittest.cc
@@ -40,7 +40,7 @@ TemplateURL* CreatePreloadedTemplateURL(bool safe_for_autoreplace,
   TemplateURLData data;
   data.short_name = ASCIIToUTF16("unittest");
   data.SetKeyword(ASCIIToUTF16("unittest"));
-  data.SetURL("http://www.unittest.com/");
+  data.SetURL("http://www.unittest.com/{searchTerms}");
   data.favicon_url = GURL("http://favicon.url");
   data.show_in_default_list = true;
   data.safe_for_autoreplace = safe_for_autoreplace;
@@ -614,8 +614,9 @@ 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, "http://foo1",
-      "http://sugg1", "http://icon1", true, "UTF-8;UTF-16", Time(), Time());
+  TemplateURL* t_url = AddKeywordWithDate("name1", "key1", false,
+      "http://foo1/{searchTerms}", "http://sugg1", "http://icon1", true,
+      "UTF-8;UTF-16", Time(), Time());
   test_util_.ResetObserverCount();
 
   model()->SetDefaultSearchProvider(t_url);
@@ -718,7 +719,7 @@ TEST_F(TemplateURLServiceTest, DefaultSearchProviderLoadedFromPrefs) {
   TemplateURLData data;
   data.short_name = ASCIIToUTF16("a");
   data.safe_for_autoreplace = true;
-  data.SetURL("http://url");
+  data.SetURL("http://url/{searchTerms}");
   data.suggestions_url = "http://url2";
   data.instant_url = "http://instant";
   data.date_created = Time::FromTimeT(100);
@@ -740,7 +741,7 @@ TEST_F(TemplateURLServiceTest, DefaultSearchProviderLoadedFromPrefs) {
   const TemplateURL* default_turl = model()->GetDefaultSearchProvider();
   ASSERT_TRUE(default_turl);
   EXPECT_EQ(ASCIIToUTF16("a"), default_turl->short_name());
-  EXPECT_EQ("http://url", default_turl->url());
+  EXPECT_EQ("http://url/{searchTerms}", default_turl->url());
   EXPECT_EQ("http://url2", default_turl->suggestions_url());
   EXPECT_EQ("http://instant", default_turl->instant_url());
   EXPECT_EQ(id, default_turl->id());
@@ -1087,7 +1088,21 @@ TEST_F(TemplateURLServiceTest, LoadEnsuresDefaultSearchProviderExists) {
   // Reset the model and load it. There should be a default search provider.
   test_util_.ResetModel(true);
 
-  EXPECT_TRUE(model()->GetDefaultSearchProvider());
+  ASSERT_TRUE(model()->GetDefaultSearchProvider());
+  EXPECT_TRUE(model()->GetDefaultSearchProvider()->SupportsReplacement());
+
+  // Make default search provider unusable (no search terms).
+  model()->ResetTemplateURL(model()->GetDefaultSearchProvider(),
+                            ASCIIToUTF16("test"), ASCIIToUTF16("test"),
+                            "http://example.com/");
+  test_util_.BlockTillServiceProcessesRequests();
+
+  // Reset the model and load it. There should be a usable default search
+  // provider.
+  test_util_.ResetModel(true);
+
+  ASSERT_TRUE(model()->GetDefaultSearchProvider());
+  EXPECT_TRUE(model()->GetDefaultSearchProvider()->SupportsReplacement());
 }
 
 // Make sure that the load does update of auto-keywords correctly.
@@ -1141,8 +1156,8 @@ TEST_F(TemplateURLServiceTest, TestManagedDefaultSearch) {
 
   // Set a regular default search provider.
   TemplateURL* regular_default = AddKeywordWithDate("name1", "key1", false,
-      "http://foo1", "http://sugg1", "http://icon1", true, "UTF-8;UTF-16",
-      Time(), Time());
+      "http://foo1/{searchTerms}", "http://sugg1", "http://icon1", true,
+      "UTF-8;UTF-16", Time(), Time());
   VerifyObserverCount(1);
   model()->SetDefaultSearchProvider(regular_default);
   // Adding the URL and setting the default search provider should have caused
@@ -1240,8 +1255,9 @@ 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",
-      "http://sugg1", "http://icon1", true, "UTF-8;UTF-16", Time(), Time());
+  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());
 
diff --git a/chrome/browser/ui/search_engines/template_url_table_model.cc b/chrome/browser/ui/search_engines/template_url_table_model.cc
index cd238ad..3c7a0e7 100644
--- a/chrome/browser/ui/search_engines/template_url_table_model.cc
+++ b/chrome/browser/ui/search_engines/template_url_table_model.cc
@@ -275,11 +275,11 @@ void TemplateURLTableModel::ModifyTemplateURL(int index,
   DCHECK(index >= 0 && index <= RowCount());
   DCHECK(!url.empty());
   const TemplateURL* template_url = GetTemplateURL(index);
+  // The default search provider should support replacement.
+  DCHECK(template_url_service_->GetDefaultSearchProvider() != template_url ||
+         template_url->SupportsReplacement());
   template_url_service_->RemoveObserver(this);
   template_url_service_->ResetTemplateURL(template_url, title, keyword, url);
-  // The default search provider should support replacement.
-  CHECK(template_url_service_->GetDefaultSearchProvider() != template_url ||
-        template_url->SupportsReplacement());
   template_url_service_->AddObserver(this);
   ReloadIcon(index);  // Also calls NotifyChanged().
 }