Modify extension omnibox keywords to be user-configurable

Allows users to change extension omnibox keywords through the chrome://settings/searchEngines page, and prevents extensions from registering the same keywords.

Images demonstrating UI change (new on top, old on bottom):
http://imgur.com/a/SDTvD

BUG=74643

Review URL: https://chromiumcodereview.appspot.com/15805002

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

11 files changed, 170 insertions, 208 deletions

diff --git a/chrome/browser/resources/options/search_engine_manager.js b/chrome/browser/resources/options/search_engine_manager.js
index dc5cf25..7d8df03 100644
--- a/chrome/browser/resources/options/search_engine_manager.js
+++ b/chrome/browser/resources/options/search_engine_manager.js
@@ -37,7 +37,8 @@ cr.define('options', function() {
     /**
      * List for extension keywords.
      * @private
-    extensionList_ : null,
+     */
+    extensionList_: null,
 
     /** inheritDoc */
     initializePage: function() {
@@ -112,12 +113,13 @@ cr.define('options', function() {
   };
 
   SearchEngineManager.validityCheckCallback = function(validity, modelIndex) {
-    // Forward to both lists; the one without a matching modelIndex will ignore
-    // it.
+    // Forward to all lists; those without a matching modelIndex will ignore it.
     SearchEngineManager.getInstance().defaultsList_.validationComplete(
         validity, modelIndex);
     SearchEngineManager.getInstance().othersList_.validationComplete(
         validity, modelIndex);
+    SearchEngineManager.getInstance().extensionList_.validationComplete(
+        validity, modelIndex);
   };
 
   // Export
diff --git a/chrome/browser/resources/options/search_engine_manager_engine_list.js b/chrome/browser/resources/options/search_engine_manager_engine_list.js
index 8969587..bbea9b0 100644
--- a/chrome/browser/resources/options/search_engine_manager_engine_list.js
+++ b/chrome/browser/resources/options/search_engine_manager_engine_list.js
@@ -116,26 +116,31 @@ cr.define('options.search_engines', function() {
 
       // And the URL column.
       var urlEl = this.createEditableTextCell(engine.url);
-      var urlWithButtonEl = this.ownerDocument.createElement('div');
-      urlWithButtonEl.appendChild(urlEl);
-      urlWithButtonEl.className = 'url-column';
-      urlWithButtonEl.classList.add('weakrtl');
-      this.contentElement.appendChild(urlWithButtonEl);
-      // Add the Make Default button. Temporary until drag-and-drop re-ordering
-      // is implemented. When this is removed, remove the extra div above.
-      if (engine.canBeDefault) {
-        var makeDefaultButtonEl = this.ownerDocument.createElement('button');
-        makeDefaultButtonEl.className = 'custom-appearance list-inline-button';
-        makeDefaultButtonEl.textContent =
-            loadTimeData.getString('makeDefaultSearchEngineButton');
-        makeDefaultButtonEl.onclick = function(e) {
-          chrome.send('managerSetDefaultSearchEngine', [engine.modelIndex]);
-        };
-        // Don't select the row when clicking the button.
-        makeDefaultButtonEl.onmousedown = function(e) {
-          e.stopPropagation();
-        };
-        urlWithButtonEl.appendChild(makeDefaultButtonEl);
+      // Extensions should not display a URL column.
+      if (!engine.isExtension) {
+        var urlWithButtonEl = this.ownerDocument.createElement('div');
+        urlWithButtonEl.appendChild(urlEl);
+        urlWithButtonEl.className = 'url-column';
+        urlWithButtonEl.classList.add('weakrtl');
+        this.contentElement.appendChild(urlWithButtonEl);
+        // Add the Make Default button. Temporary until drag-and-drop
+        // re-ordering is implemented. When this is removed, remove the extra
+        // div above.
+        if (engine.canBeDefault) {
+          var makeDefaultButtonEl = this.ownerDocument.createElement('button');
+          makeDefaultButtonEl.className =
+              'custom-appearance list-inline-button';
+          makeDefaultButtonEl.textContent =
+              loadTimeData.getString('makeDefaultSearchEngineButton');
+          makeDefaultButtonEl.onclick = function(e) {
+            chrome.send('managerSetDefaultSearchEngine', [engine.modelIndex]);
+          };
+          // Don't select the row when clicking the button.
+          makeDefaultButtonEl.onmousedown = function(e) {
+            e.stopPropagation();
+          };
+          urlWithButtonEl.appendChild(makeDefaultButtonEl);
+        }
       }
 
       // Do final adjustment to the input fields.
@@ -148,6 +153,9 @@ cr.define('options.search_engines', function() {
       if (engine.urlLocked)
         this.urlField_.disabled = true;
 
+      if (engine.isExtension)
+        this.nameField_.disabled = true;
+
       if (this.isPlaceholder) {
         this.nameField_.placeholder =
             loadTimeData.getString('searchEngineTableNamePlaceholder');
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 58fb282..092eac7 100644
--- a/chrome/browser/search_engines/search_host_to_urls_map.cc
+++ b/chrome/browser/search_engines/search_host_to_urls_map.cc
@@ -27,7 +27,6 @@ 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));
@@ -41,7 +40,6 @@ void SearchHostToURLsMap::Remove(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));
diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index f5c9e63..d739034 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -369,7 +369,6 @@ 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();
   if (!search_ref.IsValidUsingTermsData(search_terms_data))
@@ -584,10 +583,7 @@ void TemplateURLService::IncrementUsageCount(TemplateURL* 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())
+  if (service_.get())
     service_.get()->UpdateKeyword(url->data());
 }
 
@@ -707,6 +703,13 @@ void TemplateURLService::ResetNonExtensionURLs() {
                                        &default_search_provider,
                                        &new_resource_keyword_version,
                                        &pre_sync_deletes_);
+  for (TemplateURLVector::iterator i = entries_to_process.begin();
+       i != entries_to_process.end();) {
+    if ((*i)->IsExtensionKeyword())
+      i = entries_to_process.erase(i);
+    else
+      ++i;
+  }
   // Setup search engines and a default one.
   AddTemplateURLsAndSetupDefaultEngine(&entries_to_process,
                                        default_search_provider);
@@ -867,11 +870,6 @@ syncer::SyncDataList TemplateURLService::GetAllSyncData(
   syncer::SyncDataList current_data;
   for (TemplateURLVector::const_iterator iter = template_urls_.begin();
       iter != template_urls_.end(); ++iter) {
-    // We don't sync extension keywords.
-    // TODO(mpcomplete): If we allow editing extension keywords, then those
-    // should be persisted to disk and synced.
-    if ((*iter)->IsExtensionKeyword())
-      continue;
     // We don't sync keywords managed by policy.
     if ((*iter)->created_by_policy())
       continue;
@@ -1182,12 +1180,6 @@ void TemplateURLService::ProcessTemplateURLChange(
   if (processing_syncer_changes_)
     return;  // These are changes originating from us. Ignore.
 
-  // Avoid syncing Extension keywords.
-  // TODO(mpcomplete): If we allow editing extension keywords, then those should
-  // be persisted to disk and synced.
-  if (turl->IsExtensionKeyword())
-    return;
-
   // Avoid syncing keywords managed by policy.
   if (turl->created_by_policy())
     return;
@@ -1296,7 +1288,6 @@ TemplateURL* TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
   // possible that sync is trying to modify fields that should not be touched.
   // Revert these fields to the built-in values.
   UpdateTemplateURLIfPrepopulated(turl, profile);
-  DCHECK(!turl->IsExtensionKeyword());
   if (reset_keyword || deduped) {
     if (reset_keyword)
       turl->ResetKeywordIfNecessary(true);
@@ -1438,13 +1429,6 @@ void TemplateURLService::RemoveFromMaps(TemplateURL* template_url) {
       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.
-  // TODO(mpcomplete): If we allow editing extension keywords, then those should
-  // be synced.
-  if (template_url->IsExtensionKeyword())
-    return;
-
   if (!template_url->sync_guid().empty())
     guid_to_template_map_.erase(template_url->sync_guid());
   if (loaded_) {
@@ -1487,14 +1471,6 @@ void TemplateURLService::AddToMaps(TemplateURL* template_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_extension)
-    return;
-
   if (!template_url->sync_guid().empty())
     guid_to_template_map_[template_url->sync_guid()] = template_url;
   if (loaded_) {
@@ -1731,11 +1707,6 @@ bool TemplateURLService::UpdateNoNotify(
       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
-  // removed.
-  DCHECK(!existing_turl->IsExtensionKeyword());
-
   string16 old_keyword(existing_turl->keyword());
   keyword_to_template_map_.erase(old_keyword);
   if (!existing_turl->sync_guid().empty())
@@ -1762,7 +1733,6 @@ bool TemplateURLService::UpdateNoNotify(
     //     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))
@@ -2098,35 +2068,33 @@ bool TemplateURLService::AddNoNotify(TemplateURL* template_url,
   }
 
   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, false);
-        ResetTemplateURL(existing_keyword_turl,
-                         existing_keyword_turl->short_name(), new_keyword,
-                         existing_keyword_turl->url());
-      }
+  // Check whether |template_url|'s keyword conflicts with any already in the
+  // model.
+  TemplateURL* existing_keyword_turl =
+      GetTemplateURLForKeyword(template_url->keyword());
+  if (existing_keyword_turl != NULL) {
+    DCHECK_NE(existing_keyword_turl, template_url);
+    // Only replace one of the TemplateURLs if they are either both extensions,
+    // or both not extensions.
+    bool are_same_type = existing_keyword_turl->IsExtensionKeyword() ==
+        template_url->IsExtensionKeyword();
+    if (CanReplace(existing_keyword_turl) && are_same_type) {
+      RemoveNoNotify(existing_keyword_turl);
+    } else if (CanReplace(template_url) && are_same_type) {
+      delete template_url;
+      return false;
+    } else {
+      string16 new_keyword = UniquifyKeyword(*existing_keyword_turl, false);
+      ResetTemplateURL(existing_keyword_turl,
+                       existing_keyword_turl->short_name(), new_keyword,
+                       existing_keyword_turl->url());
     }
   }
   template_urls_.push_back(template_url);
   AddToMaps(template_url);
 
   if (newly_adding) {
-    // Don't persist extension keywords to disk.  They'll get re-added on each
-    // launch as the extensions are loaded.
-    // TODO(mpcomplete): If we allow editing extension keywords, then those
-    // should be persisted to disk and synced.
-    if (service_.get() && !template_url->IsExtensionKeyword())
+    if (service_.get())
       service_->AddKeyword(template_url->data());
 
     // Inform sync of the addition. Note that this will assign a GUID to
@@ -2156,10 +2124,7 @@ void TemplateURLService::RemoveNoNotify(TemplateURL* template_url) {
   // Remove it from the vector containing all TemplateURLs.
   template_urls_.erase(i);
 
-  // Extension keywords are not persisted.
-  // TODO(mpcomplete): If we allow editing extension keywords, then those should
-  // be persisted to disk and synced.
-  if (service_.get() && !template_url->IsExtensionKeyword())
+  if (service_.get())
     service_->RemoveKeyword(template_url->id());
 
   // Inform sync of the deletion.
@@ -2228,10 +2193,7 @@ void TemplateURLService::RemoveProvidersCreatedByPolicy(
         *default_search_provider = NULL;
 
       i = template_urls->erase(i);
-      // Extension keywords are not persisted.
-      // TODO(mpcomplete): If we allow editing extension keywords, then those
-      // should be persisted to disk and synced.
-      if (service_.get() && !template_url->IsExtensionKeyword())
+      if (service_.get())
         service_->RemoveKeyword(template_url->id());
       delete template_url;
     } else {
@@ -2259,9 +2221,10 @@ string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl,
     if (!GetTemplateURLForKeyword(turl.keyword()))
       return turl.keyword();
 
-    // First, try to return the generated keyword for the TemplateURL.
+    // First, try to return the generated keyword for the TemplateURL (except
+    // for extensions, as their keywords are not associated with their URLs).
     GURL gurl(turl.url());
-    if (gurl.is_valid()) {
+    if (gurl.is_valid() && !turl.IsExtensionKeyword()) {
       string16 keyword_candidate = GenerateKeyword(gurl);
       if (!GetTemplateURLForKeyword(keyword_candidate))
         return keyword_candidate;
@@ -2297,7 +2260,6 @@ void TemplateURLService::ResolveSyncKeywordConflict(
   DCHECK(applied_sync_turl);
   DCHECK(change_list);
   DCHECK_EQ(applied_sync_turl->keyword(), unapplied_sync_turl->keyword());
-  DCHECK(!applied_sync_turl->IsExtensionKeyword());
 
   // Both |unapplied_sync_turl| and |applied_sync_turl| are known to Sync, so
   // don't delete either of them. Instead, determine which is "better" and
@@ -2449,11 +2411,7 @@ void TemplateURLService::PatchMissingSyncGUIDs(
        i != template_urls->end(); ++i) {
     TemplateURL* template_url = *i;
     DCHECK(template_url);
-    // Extension keywords are never synced.
-    // TODO(mpcomplete): If we allow editing extension keywords, then those
-    // should be persisted to disk and synced.
-    if (template_url->sync_guid().empty() &&
-        !template_url->IsExtensionKeyword()) {
+    if (template_url->sync_guid().empty()) {
       template_url->data_.sync_guid = base::GenerateGUID();
       if (service_.get())
         service_->UpdateKeyword(template_url->data());
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 86ce43c..c2f01f3 100644
--- a/chrome/browser/search_engines/template_url_service_sync_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_sync_unittest.cc
@@ -415,7 +415,7 @@ TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataBasic) {
   }
 }
 
-TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataNoExtensions) {
+TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataWithExtension) {
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com"));
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key2"), "http://key2.com"));
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key3"),
@@ -423,7 +423,7 @@ TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataNoExtensions) {
   syncer::SyncDataList all_sync_data =
       model()->GetAllSyncData(syncer::SEARCH_ENGINES);
 
-  EXPECT_EQ(2U, all_sync_data.size());
+  EXPECT_EQ(3U, all_sync_data.size());
 
   for (syncer::SyncDataList::const_iterator iter = all_sync_data.begin();
       iter != all_sync_data.end(); ++iter) {
@@ -1083,14 +1083,15 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithLocalExtensions) {
                                     CreateInitialSyncData(), PassProcessor(),
                                     CreateAndPassSyncErrorFactory());
 
-  // Add some extension keywords locally.  These shouldn't be synced.
+  // Add some extension keywords locally.
   TemplateURL* extension1 = CreateTestTemplateURL(ASCIIToUTF16("keyword1"),
       std::string(extensions::kExtensionScheme) + "://extension1");
   model()->Add(extension1);
+  EXPECT_EQ(1U, processor()->change_list_size());
   TemplateURL* extension2 = CreateTestTemplateURL(ASCIIToUTF16("keyword2"),
       std::string(extensions::kExtensionScheme) + "://extension2");
   model()->Add(extension2);
-  EXPECT_EQ(0U, processor()->change_list_size());
+  EXPECT_EQ(1U, processor()->change_list_size());
 
   // Create some sync changes that will conflict with the extension keywords.
   syncer::SyncChangeList changes;
@@ -1101,25 +1102,20 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithLocalExtensions) {
     CreateTestTemplateURL(ASCIIToUTF16("keyword2"), "http://bbb.com")));
   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")));
+  // The existing extension keywords should be uniquified.
   EXPECT_FALSE(model()->GetTemplateURLForHost("aaa.com") == NULL);
+  EXPECT_EQ(model()->GetTemplateURLForHost("aaa.com"),
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword1")));
   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());
+
+  // Replaced extension keywords should be uniquified.
+  EXPECT_EQ(extension1,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword1_")));
+  EXPECT_EQ(extension2,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword2_")));
 }
 
 TEST_F(TemplateURLServiceSyncTest, AutogeneratedKeywordMigrated) {
diff --git a/chrome/browser/search_engines/template_url_service_unittest.cc b/chrome/browser/search_engines/template_url_service_unittest.cc
index d681c86..1374893 100644
--- a/chrome/browser/search_engines/template_url_service_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_unittest.cc
@@ -506,6 +506,8 @@ TEST_F(TemplateURLServiceTest, AddSameKeyword) {
 }
 
 TEST_F(TemplateURLServiceTest, AddExtensionKeyword) {
+  test_util_.VerifyLoad();
+
   TemplateURL* original1 = AddKeywordWithDate(
       "replaceable", "keyword1", "http://test1", std::string(), std::string(),
       std::string(), true, "UTF-8", Time(), Time());
@@ -525,24 +527,20 @@ TEST_F(TemplateURLServiceTest, AddExtensionKeyword) {
   data.SetURL(std::string(extensions::kExtensionScheme) + "://test4");
   data.safe_for_autoreplace = false;
 
-  // Extension keywords should override replaceable keywords.
+  // Both replaceable and non-replaceable keywords should be uniquified.
   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.
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("test1")));
   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,
+  ASSERT_EQ(extension2,
             model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword2")));
+  EXPECT_EQ(original2,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("test2")));
 
   // They should override extension keywords added earlier.
   data.SetKeyword(ASCIIToUTF16("keyword3"));
@@ -550,9 +548,8 @@ TEST_F(TemplateURLServiceTest, AddExtensionKeyword) {
   model()->Add(extension3);
   ASSERT_EQ(extension3,
             model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword3")));
-  model()->Remove(extension3);
   EXPECT_EQ(original3,
-            model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword3")));
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword3_")));
 }
 
 TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) {
@@ -561,16 +558,18 @@ TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) {
   // 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(),
-      std::string(), true, "UTF-8", Time(), Time());
   TemplateURL* extension = AddKeywordWithDate(
       "extension", "keyword",
       std::string(extensions::kExtensionScheme) + "://test2", std::string(),
       std::string(), std::string(), false, "UTF-8", Time(), Time());
+  // Adding a keyword that matches the extension should cause the extension
+  // to uniquify.
+  AddKeywordWithDate(
+      "replaceable", "keyword", "http://test1", std::string(),  std::string(),
+      std::string(), true, "UTF-8", Time(), Time());
 
   // Adding another replaceable keyword should remove the existing one, but
-  // leave the extension as the associated URL for this keyword.
+  // leave the extension as is.
   TemplateURLData data;
   data.short_name = ASCIIToUTF16("name1");
   data.SetKeyword(ASCIIToUTF16("keyword"));
@@ -579,13 +578,12 @@ TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) {
   TemplateURL* t_url = new TemplateURL(test_util_.profile(), data);
   model()->Add(t_url);
   EXPECT_EQ(extension,
-            model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")));
+            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.
+  // keyword.
   data.short_name = ASCIIToUTF16("name2");
   data.SetURL("http://test4");
   data.safe_for_autoreplace = false;
@@ -594,15 +592,8 @@ TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) {
   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());
+  EXPECT_EQ(extension,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword_")));
 }
 
 TEST_F(TemplateURLServiceTest, GenerateKeyword) {
@@ -947,14 +938,14 @@ TEST_F(TemplateURLServiceTest, ResetNonExtensionURLs) {
   EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext_keyword")));
   EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")));
 
-  // Reload URLs. Result should be the same except that extension keywords
-  // aren't persisted.
+  // Reload URLs. Result should be the same, as extension keywords are now
+  // persisted.
   test_util_.ResetModel(true);
   default_provider = model()->GetDefaultSearchProvider();
   ASSERT_TRUE(default_provider);
   EXPECT_EQ(SEARCH_ENGINE_GOOGLE,
       TemplateURLPrepopulateData::GetEngineType(default_provider->url()));
-  EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext_keyword")));
+  EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext_keyword")));
   EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")));
 }
 
diff --git a/chrome/browser/ui/search_engines/keyword_editor_controller.cc b/chrome/browser/ui/search_engines/keyword_editor_controller.cc
index 132ce86..a44ab2d 100644
--- a/chrome/browser/ui/search_engines/keyword_editor_controller.cc
+++ b/chrome/browser/ui/search_engines/keyword_editor_controller.cc
@@ -39,12 +39,7 @@ int KeywordEditorController::AddTemplateURL(const string16& title,
 
   content::RecordAction(UserMetricsAction("KeywordEditor_AddKeyword"));
 
-  // There's a bug (1090726) in TableView with groups enabled such that newly
-  // added items in groups ALWAYS appear at the end, regardless of the index
-  // passed in. Worse yet, the selected rows get messed up when this happens
-  // causing other problems. As a work around we always add the item to the end
-  // of the list.
-  const int new_index = table_model_->RowCount();
+  const int new_index = table_model_->last_other_engine_index();
   table_model_->Add(new_index, title, keyword, url);
 
   return new_index;
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 4e610da..a0319bd 100644
--- a/chrome/browser/ui/search_engines/template_url_table_model.cc
+++ b/chrome/browser/ui/search_engines/template_url_table_model.cc
@@ -28,6 +28,7 @@
 // Group IDs used by TemplateURLTableModel.
 static const int kMainGroupID = 0;
 static const int kOtherGroupID = 1;
+static const int kExtensionGroupID = 2;
 
 // ModelEntry ----------------------------------------------------
 
@@ -143,6 +144,7 @@ void TemplateURLTableModel::Reload() {
   TemplateURLService::TemplateURLVector urls =
       template_url_service_->GetTemplateURLs();
 
+  std::vector<ModelEntry*> default_entries, other_entries, extension_entries;
   // Keywords that can be made the default first.
   for (TemplateURLService::TemplateURLVector::iterator i = urls.begin();
        i != urls.end(); ++i) {
@@ -150,22 +152,28 @@ void TemplateURLTableModel::Reload() {
     // NOTE: we don't use ShowInDefaultList here to avoid items bouncing around
     // the lists while editing.
     if (template_url->show_in_default_list())
-      entries_.push_back(new ModelEntry(this, template_url));
+      default_entries.push_back(new ModelEntry(this, template_url));
+    else if (template_url->IsExtensionKeyword())
+      extension_entries.push_back(new ModelEntry(this, template_url));
+    else
+      other_entries.push_back(new ModelEntry(this, template_url));
   }
 
-  last_search_engine_index_ = static_cast<int>(entries_.size());
+  last_search_engine_index_ = static_cast<int>(default_entries.size());
+  last_other_engine_index_ = last_search_engine_index_ +
+      static_cast<int>(other_entries.size());
 
-  // Then the rest.
-  for (TemplateURLService::TemplateURLVector::iterator i = urls.begin();
-       i != urls.end(); ++i) {
-    TemplateURL* template_url = *i;
-    // NOTE: we don't use ShowInDefaultList here to avoid things bouncing
-    // the lists while editing.
-    if (!template_url->show_in_default_list() &&
-        !template_url->IsExtensionKeyword()) {
-      entries_.push_back(new ModelEntry(this, template_url));
-    }
-  }
+  entries_.insert(entries_.end(),
+                  default_entries.begin(),
+                  default_entries.end());
+
+  entries_.insert(entries_.end(),
+                  other_entries.begin(),
+                  other_entries.end());
+
+  entries_.insert(entries_.end(),
+                  extension_entries.begin(),
+                  extension_entries.end());
 
   if (observer_)
     observer_->OnModelChanged();
@@ -222,12 +230,20 @@ TemplateURLTableModel::Groups TemplateURLTableModel::GetGroups() {
   other_group.id = kOtherGroupID;
   groups.push_back(other_group);
 
+  Group extension_group;
+  extension_group.title =
+      l10n_util::GetStringUTF16(IDS_SEARCH_ENGINES_EDITOR_EXTENSIONS_SEPARATOR);
+  extension_group.id = kExtensionGroupID;
+  groups.push_back(extension_group);
+
   return groups;
 }
 
 int TemplateURLTableModel::GetGroupID(int row) {
   DCHECK(row >= 0 && row < RowCount());
-  return row < last_search_engine_index_ ? kMainGroupID : kOtherGroupID;
+  if (row < last_search_engine_index_)
+    return kMainGroupID;
+  return row < last_other_engine_index_ ? kOtherGroupID : kExtensionGroupID;
 }
 
 void TemplateURLTableModel::Remove(int index) {
@@ -239,7 +255,9 @@ void TemplateURLTableModel::Remove(int index) {
   scoped_ptr<ModelEntry> entry(entries_[index]);
   entries_.erase(entries_.begin() + index);
   if (index < last_search_engine_index_)
-    last_search_engine_index_--;
+    --last_search_engine_index_;
+  if (index < last_other_engine_index_)
+    --last_other_engine_index_;
   if (observer_)
     observer_->OnItemsRemoved(index, 1);
 
@@ -265,6 +283,8 @@ void TemplateURLTableModel::Add(int index,
   ModelEntry* entry = new ModelEntry(this, turl);
   template_url_service_->AddObserver(this);
   entries_.insert(entries_.begin() + index, entry);
+  if (index <= last_other_engine_index_)
+    ++last_other_engine_index_;
   if (observer_)
     observer_->OnItemsAdded(index, 1);
 }
diff --git a/chrome/browser/ui/search_engines/template_url_table_model.h b/chrome/browser/ui/search_engines/template_url_table_model.h
index cedd552..6ee70b9 100644
--- a/chrome/browser/ui/search_engines/template_url_table_model.h
+++ b/chrome/browser/ui/search_engines/template_url_table_model.h
@@ -97,6 +97,10 @@ class TemplateURLTableModel : public ui::TableModel,
   // Returns the index of the last entry shown in the search engines group.
   int last_search_engine_index() const { return last_search_engine_index_; }
 
+  // Returns the index of the last entry shown in the other search engines
+  // group.
+  int last_other_engine_index() const { return last_other_engine_index_; }
+
  private:
   friend class ModelEntry;
 
@@ -118,6 +122,10 @@ class TemplateURLTableModel : public ui::TableModel,
   // group boundaries.
   int last_search_engine_index_;
 
+  // Index of the last other engine in entries_. This is used to determine the
+  // group boundaries.
+  int last_other_engine_index_;
+
   DISALLOW_COPY_AND_ASSIGN(TemplateURLTableModel);
 };
 
diff --git a/chrome/browser/ui/webui/options/search_engine_manager_handler.cc b/chrome/browser/ui/webui/options/search_engine_manager_handler.cc
index cae706e..10bb6a4 100644
--- a/chrome/browser/ui/webui/options/search_engine_manager_handler.cc
+++ b/chrome/browser/ui/webui/options/search_engine_manager_handler.cc
@@ -126,29 +126,28 @@ void SearchEngineManagerHandler::OnModelChanged() {
   int last_default_engine_index =
       list_controller_->table_model()->last_search_engine_index();
   for (int i = 0; i < last_default_engine_index; ++i) {
-    defaults_list.Append(CreateDictionaryForEngine(i, i == default_index));
+    // Third argument is false, as the engine is not from an extension.
+    defaults_list.Append(CreateDictionaryForEngine(
+        i, i == default_index, false));
   }
 
   // Build the second list (other search templates).
   ListValue others_list;
+  int last_other_engine_index =
+      list_controller_->table_model()->last_other_engine_index();
   if (last_default_engine_index < 0)
     last_default_engine_index = 0;
-  int engine_count = list_controller_->table_model()->RowCount();
-  for (int i = last_default_engine_index; i < engine_count; ++i) {
-    others_list.Append(CreateDictionaryForEngine(i, i == default_index));
+  for (int i = last_default_engine_index; i < last_other_engine_index; ++i) {
+    others_list.Append(CreateDictionaryForEngine(i, i == default_index, false));
   }
 
   // Build the extension keywords list.
   ListValue keyword_list;
-  ExtensionService* extension_service =
-      Profile::FromWebUI(web_ui())->GetExtensionService();
-  if (extension_service) {
-    const ExtensionSet* extensions = extension_service->extensions();
-    for (ExtensionSet::const_iterator it = extensions->begin();
-         it != extensions->end(); ++it) {
-      if (extensions::OmniboxInfo::GetKeyword(*it).size() > 0)
-        keyword_list.Append(CreateDictionaryForExtension(*(*it)));
-    }
+  if (last_other_engine_index < 0)
+    last_other_engine_index = 0;
+  int engine_count = list_controller_->table_model()->RowCount();
+  for (int i = last_other_engine_index; i < engine_count; ++i) {
+    keyword_list.Append(CreateDictionaryForEngine(i, i == default_index, true));
   }
 
   web_ui()->CallJavascriptFunction("SearchEngineManager.updateSearchEngineList",
@@ -167,22 +166,8 @@ void SearchEngineManagerHandler::OnItemsRemoved(int start, int length) {
   OnModelChanged();
 }
 
-base::DictionaryValue* SearchEngineManagerHandler::CreateDictionaryForExtension(
-    const extensions::Extension& extension) {
-  base::DictionaryValue* dict = new base::DictionaryValue();
-  dict->SetString("name",  extension.name());
-  dict->SetString("displayName", extension.name());
-  dict->SetString("keyword",
-                  extensions::OmniboxInfo::GetKeyword(&extension));
-  GURL icon = extensions::IconsInfo::GetIconURL(
-      &extension, 16, ExtensionIconSet::MATCH_BIGGER);
-  dict->SetString("iconURL", icon.spec());
-  dict->SetString("url", string16());
-  return dict;
-}
-
 base::DictionaryValue* SearchEngineManagerHandler::CreateDictionaryForEngine(
-    int index, bool is_default) {
+    int index, bool is_default, bool is_extension) {
   TemplateURLTableModel* table_model = list_controller_->table_model();
   const TemplateURL* template_url = list_controller_->GetTemplateURL(index);
 
@@ -199,14 +184,13 @@ base::DictionaryValue* SearchEngineManagerHandler::CreateDictionaryForEngine(
     dict->SetString("iconURL", icon_url.spec());
   dict->SetString("modelIndex", base::IntToString(index));
 
-  if (list_controller_->CanRemove(template_url))
-    dict->SetString("canBeRemoved", "1");
-  if (list_controller_->CanMakeDefault(template_url))
-    dict->SetString("canBeDefault", "1");
-  if (is_default)
-    dict->SetString("default", "1");
-  if (list_controller_->CanEdit(template_url))
-    dict->SetString("canBeEdited", "1");
+  dict->SetBoolean("canBeRemoved",
+      list_controller_->CanRemove(template_url) && !is_extension);
+  dict->SetBoolean("canBeDefault",
+      list_controller_->CanMakeDefault(template_url) && !is_extension);
+  dict->SetBoolean("default", is_default);
+  dict->SetBoolean("canBeEdited", list_controller_->CanEdit(template_url));
+  dict->SetBoolean("isExtension", is_extension);
 
   return dict;
 }
@@ -310,6 +294,7 @@ void SearchEngineManagerHandler::EditCompleted(const ListValue* args) {
     NOTREACHED();
     return;
   }
+
   // Recheck validity.  It's possible to get here with invalid input if e.g. the
   // user calls the right JS functions directly from the web inspector.
   if (edit_controller_->IsTitleValid(name) &&
diff --git a/chrome/browser/ui/webui/options/search_engine_manager_handler.h b/chrome/browser/ui/webui/options/search_engine_manager_handler.h
index e28d6a5..337a7da 100644
--- a/chrome/browser/ui/webui/options/search_engine_manager_handler.h
+++ b/chrome/browser/ui/webui/options/search_engine_manager_handler.h
@@ -72,7 +72,8 @@ class SearchEngineManagerHandler : public OptionsPageUIHandler,
   void EditCompleted(const base::ListValue* args);
 
   // Returns a dictionary to pass to WebUI representing the given search engine.
-  base::DictionaryValue* CreateDictionaryForEngine(int index, bool is_default);
+  base::DictionaryValue* CreateDictionaryForEngine(
+      int index, bool is_default, bool is_extension);
 
   // Returns a dictionary to pass to WebUI representing the extension.
   base::DictionaryValue* CreateDictionaryForExtension(