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

4 files changed, 70 insertions, 127 deletions

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")));
 }