Remove the "autogenerate keyword" bit on TemplateURL.

chrome/browser/search_engines/template_url_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/search_engines/template_url_service.h"
 
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/environment.h"
 #include "base/i18n/case_conversion.h"
@@ skipping 52 matching lines @@
 // the rare case the term replaces the URL it's unlikely another keyword would
 // have the same url.
 const char kReplacementTerm[] = "blah.blah.blah.blah.blah";
 
 bool TemplateURLsHaveSamePrefs(const TemplateURL* url1,
                                const TemplateURL* url2) {
   if (url1 == url2)
     return true;
   return (url1 != NULL) && (url2 != NULL) &&
       (url1->short_name() == url2->short_name()) &&
-      (url1->keyword() == url2->keyword()) &&
+      ((url1->keyword() == url2->keyword()) ||

[sky, 2012/05/04 23:20:25]

Is it worth moving this to a common place for both here and protector?

[Peter Kasting, 2012/05/07 17:50:24]

Yes, good idea.

+       (url1->IsGoogleSearchURLWithReplaceableKeyword() &&
+        url2->IsGoogleSearchURLWithReplaceableKeyword())) &&
       (url1->url() == url2->url()) &&
       (url1->suggestions_url() == url2->suggestions_url()) &&
       (url1->instant_url() == url2->instant_url()) &&
       (url1->favicon_url() == url2->favicon_url()) &&
       (url1->safe_for_autoreplace() == url2->safe_for_autoreplace()) &&
       (url1->show_in_default_list() == url2->show_in_default_list()) &&
       (url1->input_encodings() == url2->input_encodings());
 }
 
 TemplateURL* FirstPotentialDefaultEngine(
     const TemplateURLService::TemplateURLVector& template_urls) {
   for (TemplateURLService::TemplateURLVector::const_iterator i(
        template_urls.begin()); i != template_urls.end(); ++i) {
     if (!(*i)->IsExtensionKeyword() && (*i)->SupportsReplacement())
       return *i;
   }
   return NULL;
 }
 
+// If |change_list| contains ACTION_UPDATEs followed by more ACTION_UPDATEs or
+// ACTION_ADDs for the same GUID, remove all but the last one.
+//
+// This is necessary because when syncing we may first need to migrate the
+// server-provided TemplateURL in some way, then resolve conflicts against a
+// local URL, generating two different UPDATEs.  If we send up both UPDATEs, and
+// the server does not coalesce them before sending to other clients, then the
+// first update could cause conflicts on the other clients, resulting in them
+// sending back UPDATEs of their own to try to resolve things, thus causing mass
+// confusion.  Since the second UPDATE obviates the need for the first, removing
+// the first locally is safe and avoids any potential for this problem.
+//
+// REVIEWERS: Should we instead do this uniquing at a lower level, or guarantee
+// that the server will coalesce these so the client need not worry?
+void PreventDuplicateGUIDUpdates(SyncChangeList* change_list) {
+  for (size_t i = change_list->size(); i > 1; ) {
+    --i;  // Prevent underflow that could occur if we did this in the loop body.
+    const SyncChange& change_i = (*change_list)[i];
+    if ((change_i.change_type() != SyncChange::ACTION_ADD) &&
+        (change_i.change_type() != SyncChange::ACTION_UPDATE))
+      continue;
+    std::string guid(
+        change_i.sync_data().GetSpecifics().search_engine().sync_guid());
+    for (size_t j = 0; j < i; ) {
+      const SyncChange& change_j = (*change_list)[j];
+      if ((change_j.change_type() == SyncChange::ACTION_UPDATE) &&
+          (change_j.sync_data().GetSpecifics().search_engine().sync_guid() ==
+              guid)) {
+        change_list->erase(change_list->begin() + j);
+        --i;
+      } else {
+        ++j;
+      }
+    }
+  }
+}
+
 }  // namespace
 
 
 class TemplateURLService::LessWithPrefix {
  public:
   // We want to find the set of keywords that begin with a prefix.  The STL
   // algorithms will return the set of elements that are "equal to" the
   // prefix, where "equal(x, y)" means "!(cmp(x, y) || cmp(y, x))".  When
   // cmp() is the typical std::less<>, this results in lexicographic equality;
   // we need to extend this to mark a prefix as "not less than" a keyword it
@@ skipping 50 matching lines @@
 TemplateURLService::~TemplateURLService() {
   if (load_handle_) {
     DCHECK(service_.get());
     service_->CancelRequest(load_handle_);
   }
 
   STLDeleteElements(&template_urls_);
 }
 
 // static
-string16 TemplateURLService::GenerateKeyword(const GURL& url,
-                                             bool autodetected) {
-  // Don't autogenerate keywords for referrers that are the result of a form
-  // submission (TODO: right now we approximate this by checking for the URL
-  // having a query, but we should replace this with a call to WebCore to see if
-  // the originating page was actually a form submission), anything other than
-  // http, or referrers with a path.
-  //
-  // If we relax the path constraint, we need to be sure to sanitize the path
-  // elements and update AutocompletePopup to look for keywords using the path.
-  // See http://b/issue?id=863583.
-  if (!url.is_valid() ||
-      (autodetected && (url.has_query() || !url.SchemeIs(chrome::kHttpScheme) ||
-                        ((url.path() != "") && (url.path() != "/")))))
-    return string16();
-
+string16 TemplateURLService::GenerateKeyword(const GURL& url) {
+  DCHECK(url.is_valid());
   // Strip "www." off the front of the keyword; otherwise the keyword won't work
   // properly.  See http://code.google.com/p/chromium/issues/detail?id=6984 .
   // Special case: if the host was exactly "www." (not sure this can happen but
   // perhaps with some weird intranet and custom DNS server?), ensure we at
   // least don't return the empty string.
   string16 keyword(net::StripWWW(UTF8ToUTF16(url.host())));
   return keyword.empty() ? ASCIIToUTF16("www") : keyword;
 }
 
 // static
@@ skipping 35 matching lines @@
   DCHECK(t_url);
   UIThreadSearchTermsData search_terms_data;
   return GenerateSearchURLUsingTermsData(t_url, search_terms_data);
 }
 
 // static
 GURL TemplateURLService::GenerateSearchURLUsingTermsData(
     const TemplateURL* t_url,
     const SearchTermsData& search_terms_data) {
   DCHECK(t_url);
+  DCHECK(!t_url->IsExtensionKeyword());
+
   const TemplateURLRef& search_ref = t_url->url_ref();
-  // Extension keywords don't have host-based search URLs.
-  if (!search_ref.IsValidUsingTermsData(search_terms_data) ||
-      t_url->IsExtensionKeyword())
+  if (!search_ref.IsValidUsingTermsData(search_terms_data))
     return GURL();
 
   if (!search_ref.SupportsReplacementUsingTermsData(search_terms_data))
     return GURL(t_url->url());
 
   return GURL(search_ref.ReplaceSearchTermsUsingTermsData(
       ASCIIToUTF16(kReplacementTerm), TemplateURLRef::NO_SUGGESTIONS_AVAILABLE,
       string16(), search_terms_data));
 }
 
@@ skipping 79 matching lines @@
     const std::string& host) {
   TemplateURL* t_url = provider_map_->GetTemplateURLForHost(host);
   if (t_url)
     return t_url;
   return (initial_default_search_provider_.get() &&
       (GenerateSearchURL(initial_default_search_provider_.get()).host() ==
           host)) ? initial_default_search_provider_.get() : NULL;
 }
 
 void TemplateURLService::Add(TemplateURL* template_url) {
-  AddNoNotify(template_url, true);
-  NotifyObservers();
+  if (AddNoNotify(template_url, true))
+    NotifyObservers();
 }
 
 void TemplateURLService::AddAndSetProfile(TemplateURL* template_url,
                                           Profile* profile) {
   template_url->profile_ = profile;
   Add(template_url);
 }
 
 void TemplateURLService::AddWithOverrides(TemplateURL* template_url,
                                           const string16& short_name,
                                           const string16& keyword,
                                           const std::string& url) {
+  DCHECK(!keyword.empty());
   DCHECK(!url.empty());
   template_url->data_.short_name = short_name;
   template_url->data_.SetKeyword(keyword);
   template_url->SetURL(url);
   Add(template_url);
 }
 
 void TemplateURLService::Remove(TemplateURL* template_url) {
   RemoveNoNotify(template_url);
   NotifyObservers();
@@ skipping 86 matching lines @@
 }
 
 void TemplateURLService::IncrementUsageCount(TemplateURL* url) {
   DCHECK(url && std::find(template_urls_.begin(), template_urls_.end(), url) !=
       template_urls_.end());
   ++url->data_.usage_count;
   // Extension keywords are not persisted.
   // TODO(mpcomplete): If we allow editing extension keywords, then those should
   // be persisted to disk and synced.
   if (service_.get() && !url->IsExtensionKeyword())
-    service_.get()->UpdateKeyword(*url);
+    service_.get()->UpdateKeyword(url->data());
 }
 
 void TemplateURLService::ResetTemplateURL(TemplateURL* url,
                                           const string16& title,
                                           const string16& keyword,
                                           const std::string& search_url) {
+  DCHECK(!keyword.empty());
   DCHECK(!search_url.empty());
   TemplateURLData data(url->data());
   data.short_name = title;
   data.SetKeyword(keyword);
   if (search_url != data.url()) {
     data.SetURL(search_url);
     // The urls have changed, reset the favicon url.
     data.favicon_url = GURL();
   }
   data.safe_for_autoreplace = false;
@@ skipping 130 matching lines @@
       // The value from the preferences was previously stored in the database.
       // Reuse it.
     } else {
       // The value from the preferences takes over.
       default_search_provider = NULL;
       if (default_from_prefs.get()) {
         TemplateURLData data(default_from_prefs->data());
         data.created_by_policy = true;
         data.id = kInvalidTemplateURLID;
         default_search_provider = new TemplateURL(profile_, data);
-        AddNoNotify(default_search_provider, true);
+        if (!AddNoNotify(default_search_provider, true))
+          default_search_provider = NULL;
       }
     }
     // Note that this saves the default search provider to prefs.
     if (!default_search_provider ||
         (!default_search_provider->IsExtensionKeyword() &&
          default_search_provider->SupportsReplacement()))
       SetDefaultSearchProviderNoNotify(default_search_provider);
   } else {
     // If we had a managed default, replace it with the synced default if
     // applicable, or the first provider of the list.
@@ skipping 177 matching lines @@
     DCHECK_EQ(syncable::SEARCH_ENGINES, iter->sync_data().GetDataType());
 
     std::string guid =
         iter->sync_data().GetSpecifics().search_engine().sync_guid();
     TemplateURL* existing_turl = GetTemplateURLForGUID(guid);
     scoped_ptr<TemplateURL> turl(CreateTemplateURLFromTemplateURLAndSyncData(
         profile_, existing_turl, iter->sync_data(), &new_changes));
     if (!turl.get())
       continue;
 
+    // Explicitly don't check for conflicts against extension keywords; in this
+    // case the functions which modify the keyword map know how to handle the
+    // conflicts.
+    // TODO(mpcomplete): If we allow editing extension keywords, then those will
+    // need to undergo conflict resolution.
     TemplateURL* existing_keyword_turl =
-        GetTemplateURLForKeyword(turl->keyword());
-
+        FindNonExtensionTemplateURLForKeyword(turl->keyword());
     if (iter->change_type() == SyncChange::ACTION_DELETE && existing_turl) {
       bool delete_default = (existing_turl == GetDefaultSearchProvider());
 
       if (delete_default && is_default_search_managed_) {
         NOTREACHED() << "Tried to delete managed default search provider";
       } else {
         if (delete_default)
           default_search_provider_ = NULL;
 
         Remove(existing_turl);
 
         if (delete_default)
           SetDefaultSearchProvider(FindNewDefaultSearchProvider());
       }
     } else if (iter->change_type() == SyncChange::ACTION_ADD &&
                !existing_turl) {
       std::string guid = turl->sync_guid();
-      if (existing_keyword_turl)
-        ResolveSyncKeywordConflict(turl.get(), &new_changes);
+      if (existing_keyword_turl) {
+        ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
+                                   &new_changes);
+      }
       // Force the local ID to kInvalidTemplateURLID so we can add it.
       TemplateURLData data(turl->data());
       data.id = kInvalidTemplateURLID;
       Add(new TemplateURL(profile_, data));
 
       // Possibly set the newly added |turl| as the default search provider.
       SetDefaultSearchProviderIfNewlySynced(guid);
     } else if (iter->change_type() == SyncChange::ACTION_UPDATE &&
                existing_turl) {
       // Possibly resolve a keyword conflict if they have the same keywords but
       // are not the same entry.
-      if (existing_keyword_turl && existing_keyword_turl != existing_turl)
-        ResolveSyncKeywordConflict(turl.get(), &new_changes);
+      if (existing_keyword_turl && existing_keyword_turl != existing_turl) {
+        ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
+                                   &new_changes);
+      }
       UpdateNoNotify(existing_turl, *turl);
       NotifyObservers();
     } else {
       // Something really unexpected happened. Either we received an
       // ACTION_INVALID, or Sync is in a crazy state:
       //  . Trying to DELETE or UPDATE a non-existent search engine.
       //  . Trying to ADD a search engine that already exists.
       NOTREACHED() << "Unexpected sync change state.";
       error = sync_error_factory_->CreateAndUploadError(
             FROM_HERE,
             "ProcessSyncChanges failed on ChangeType " +
                 SyncChange::ChangeTypeToString(iter->change_type()));
     }
   }
+  PreventDuplicateGUIDUpdates(&new_changes);
 
   // If something went wrong, we want to prematurely exit to avoid pushing
   // inconsistent data to Sync. We return the last error we received.
   if (error.IsSet())
     return error;
 
   error = sync_processor_->ProcessSyncChanges(from_here, new_changes);
 
   return error;
 }
@@ skipping 71 matching lines @@
         // Merge duplicates and remove the processed local TURL from the map.
         std::string old_guid = dupe_turl->sync_guid();
         MergeSyncAndLocalURLDuplicates(sync_turl.release(), dupe_turl,
                                        &new_changes);
         local_data_map.erase(old_guid);
       } else {
         std::string guid = sync_turl->sync_guid();
         // Keyword conflict is possible in this case. Resolve it first before
         // adding the new TemplateURL. Note that we don't remove the local TURL
         // from local_data_map in this case as it may still need to be pushed to
-        // the cloud.
-        ResolveSyncKeywordConflict(sync_turl.get(), &new_changes);
+        // the cloud.  We also explicitly don't resolve conflicts against
+        // extension keywords; see comments in ProcessSyncChanges().
+        TemplateURL* existing_keyword_turl =
+            FindNonExtensionTemplateURLForKeyword(sync_turl->keyword());
+        if (existing_keyword_turl) {
+          ResolveSyncKeywordConflict(sync_turl.get(), existing_keyword_turl,
+                                     &new_changes);
+        }
         // Force the local ID to kInvalidTemplateURLID so we can add it.
         TemplateURLData data(sync_turl->data());
         data.id = kInvalidTemplateURLID;
         Add(new TemplateURL(profile_, data));
 
         // Possibly set the newly added |turl| as the default search provider.
         SetDefaultSearchProviderIfNewlySynced(guid);
       }
     }
   }
 
   // The remaining SyncData in local_data_map should be everything that needs to
   // be pushed as ADDs to sync.
   for (SyncDataMap::const_iterator iter = local_data_map.begin();
       iter != local_data_map.end(); ++iter) {
     new_changes.push_back(SyncChange(SyncChange::ACTION_ADD, iter->second));
   }
 
+  PreventDuplicateGUIDUpdates(&new_changes);
+
   SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes);
   if (error.IsSet())
     return error;
 
   models_associated_ = true;
   return SyncError();
 }
 
 void TemplateURLService::StopSyncing(syncable::ModelType type) {
   DCHECK_EQ(type, syncable::SEARCH_ENGINES);
@@ skipping 42 matching lines @@
   se_specifics->set_keyword(UTF16ToUTF8(turl.keyword()));
   se_specifics->set_favicon_url(turl.favicon_url().spec());
   se_specifics->set_url(turl.url());
   se_specifics->set_safe_for_autoreplace(turl.safe_for_autoreplace());
   se_specifics->set_originating_url(turl.originating_url().spec());
   se_specifics->set_date_created(turl.date_created().ToInternalValue());
   se_specifics->set_input_encodings(JoinString(turl.input_encodings(), ';'));
   se_specifics->set_show_in_default_list(turl.show_in_default_list());
   se_specifics->set_suggestions_url(turl.suggestions_url());
   se_specifics->set_prepopulate_id(turl.prepopulate_id());
-  se_specifics->set_autogenerate_keyword(turl.autogenerate_keyword());
+  // REVIEWERS: Should I instead remove this line entirely?
+  se_specifics->set_autogenerate_keyword(false);
   se_specifics->set_instant_url(turl.instant_url());
   se_specifics->set_last_modified(turl.last_modified().ToInternalValue());
   se_specifics->set_sync_guid(turl.sync_guid());
   return SyncData::CreateLocalData(se_specifics->sync_guid(),
                                    se_specifics->keyword(),
                                    specifics);
 }
 
 // static
 TemplateURL* TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
     Profile* profile,
     TemplateURL* existing_turl,
     const SyncData& sync_data,
     SyncChangeList* change_list) {
   DCHECK(change_list);
 
   sync_pb::SearchEngineSpecifics specifics =
       sync_data.GetSpecifics().search_engine();
 
   // Past bugs might have caused either of these fields to be empty.  Just
   // delete this data off the server.
   if (specifics.url().empty() || specifics.sync_guid().empty()) {
     change_list->push_back(SyncChange(SyncChange::ACTION_DELETE, sync_data));
     return NULL;
   }
 
   TemplateURLData data;
   data.short_name = UTF8ToUTF16(specifics.short_name());
   data.originating_url = GURL(specifics.originating_url());
-  data.SetKeyword(UTF8ToUTF16(specifics.keyword()));
-  data.SetAutogenerateKeyword(specifics.autogenerate_keyword());
+  string16 keyword(UTF8ToUTF16(specifics.keyword()));
+  // REVIEWERS: Can we ever get to a state where this migration code can be
+  // eliminated?
+  bool reset_keyword =
+      specifics.autogenerate_keyword() || specifics.keyword().empty();
+  if (reset_keyword)
+    keyword = ASCIIToUTF16("dummy");  // Will be replaced below.
+  DCHECK(!keyword.empty());
+  data.SetKeyword(keyword);
   data.SetURL(specifics.url());
   data.suggestions_url = specifics.suggestions_url();
   data.instant_url = specifics.instant_url();
   data.favicon_url = GURL(specifics.favicon_url());
   data.show_in_default_list = specifics.show_in_default_list();
   data.safe_for_autoreplace = specifics.safe_for_autoreplace();
   base::SplitString(specifics.input_encodings(), ';', &data.input_encodings);
   data.date_created = base::Time::FromInternalValue(specifics.date_created());
   data.last_modified = base::Time::FromInternalValue(specifics.last_modified());
   data.prepopulate_id = specifics.prepopulate_id();
   data.sync_guid = specifics.sync_guid();
   if (existing_turl) {
     data.id = existing_turl->id();
     data.created_by_policy = existing_turl->created_by_policy();
     data.usage_count = existing_turl->usage_count();
   }
 
   TemplateURL* turl = new TemplateURL(profile, data);
   DCHECK(!turl->IsExtensionKeyword());
+  if (reset_keyword) {
+    turl->ResetKeywordIfNecessary(true);
+    SyncData sync_data = CreateSyncDataFromTemplateURL(*turl);
+    change_list->push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data));
+  }
   return turl;
 }
 
 // static
 SyncDataMap TemplateURLService::CreateGUIDToSyncDataMap(
     const SyncDataList& sync_data) {
   SyncDataMap data_map;
   for (SyncDataList::const_iterator i(sync_data.begin()); i != sync_data.end();
        ++i)
     data_map[i->GetSpecifics().search_engine().sync_guid()] = *i;
@@ skipping 65 matching lines @@
   if (initial_default_search_provider_.get() &&
       initial_default_search_provider_->url_ref().HasGoogleBaseURLs()) {
     scoped_ptr<base::Environment> env(base::Environment::Create());
     if (!env->HasVar(env_vars::kHeadless) &&
         !CommandLine::ForCurrentProcess()->HasSwitch(switches::kChromeFrame))
       GoogleURLTracker::RequestServerCheck();
   }
 }
 
 void TemplateURLService::RemoveFromMaps(TemplateURL* template_url) {
-  if (!template_url->keyword().empty())
-    keyword_to_template_map_.erase(template_url->keyword());
+  const string16& keyword = template_url->keyword();
+  DCHECK_NE(0U, keyword_to_template_map_.count(keyword));
+  if (keyword_to_template_map_[keyword] == template_url) {
+    // We need to check whether the keyword can now be provided by another
+    // TemplateURL.  See the comments in AddToMaps() for more information on
+    // extension keywords and how they can coexist with non-extension keywords.
+    // In the case of more than one extension, we use the most recently
+    // installed (which will be the most recently added, which will have the
+    // highest ID).
+    TemplateURL* best_fallback = NULL;
+    for (TemplateURLVector::const_iterator i(template_urls_.begin());
+         i != template_urls_.end(); ++i) {
+      TemplateURL* turl = *i;
+      // This next statement relies on the fact that there can only be one
+      // non-extension TemplateURL with a given keyword.
+      if ((turl != template_url) && (turl->keyword() == keyword) &&
+          (!best_fallback || !best_fallback->IsExtensionKeyword() ||
+           (turl->IsExtensionKeyword() && (turl->id() > best_fallback->id()))))
+        best_fallback = turl;
+    }
+    if (best_fallback)
+      keyword_to_template_map_[keyword] = best_fallback;
+    else
+      keyword_to_template_map_.erase(keyword);
+  }
 
   // If the keyword we're removing is from an extension, we're now done, since
   // it won't be synced or stored in the provider map.
   // 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_)
     provider_map_->Remove(template_url);
 }
 
 void TemplateURLService::RemoveFromKeywordMapByPointer(
     TemplateURL* template_url) {
   DCHECK(template_url);
   for (KeywordToTemplateMap::iterator i = keyword_to_template_map_.begin();
        i != keyword_to_template_map_.end(); ++i) {
     if (i->second == template_url) {
       keyword_to_template_map_.erase(i);
       // A given TemplateURL only occurs once in the map. As soon as we find the
       // entry, stop.
       break;
     }
   }
 }
 
 void TemplateURLService::AddToMaps(TemplateURL* template_url) {
-  if (!template_url->keyword().empty())
-    keyword_to_template_map_[template_url->keyword()] = template_url;
+  bool template_extension = template_url->IsExtensionKeyword();
+  const string16& keyword = template_url->keyword();
+  KeywordToTemplateMap::const_iterator i =
+      keyword_to_template_map_.find(keyword);
+  if (i == keyword_to_template_map_.end()) {
+    keyword_to_template_map_[keyword] = template_url;
+  } else {
+    const TemplateURL* existing_url = i->second;
+    // We should only have overlapping keywords when at least one comes from
+    // an extension.  In that case, the ranking order is:
+    //   Manually-modified keywords > extension keywords > replaceable keywords
+    // When there are multiple extensions, the last-added wins.
+    bool existing_extension = existing_url->IsExtensionKeyword();
+    DCHECK(existing_extension || template_extension);
+    if (existing_extension ?
+        !CanReplace(template_url) : CanReplace(existing_url))
+      keyword_to_template_map_[keyword] = template_url;
+  }
 
   // Extension keywords are not synced, so they don't go into the GUID map,
   // and do not use host-based search URLs, so they don't go into the provider
   // map, so at this point we're done.
   // TODO(mpcomplete): If we allow editing extension keywords, then those should
   // be persisted to disk and synced.
-  if (template_url->IsExtensionKeyword())
+  if (template_extension)
     return;
 
   if (!template_url->sync_guid().empty())
     guid_to_template_map_[template_url->sync_guid()] = template_url;
   if (loaded_) {
     UIThreadSearchTermsData search_terms_data;
     provider_map_->Add(template_url, search_terms_data);
   }
 }
 
@@ skipping 99 matching lines @@
   if (!prefs->GetBoolean(prefs::kDefaultSearchProviderEnabled)) {
     // The user doesn't want a default search provider.
     default_provider->reset(NULL);
     return true;
   }
 
   string16 name =
       UTF8ToUTF16(prefs->GetString(prefs::kDefaultSearchProviderName));
   string16 keyword =
       UTF8ToUTF16(prefs->GetString(prefs::kDefaultSearchProviderKeyword));
+  // Force keyword to be non-empty.
+  // TODO(pkasting): This is only necessary as long as we're potentially loading
+  // older prefs where empty keywords are theoretically possible.  Eventually
+  // this code can be replaced with a DCHECK(!keyword.empty());.
+  bool update_keyword = keyword.empty();
+  if (update_keyword)
+    keyword = ASCIIToUTF16("dummy");
   std::string search_url =
       prefs->GetString(prefs::kDefaultSearchProviderSearchURL);
   // Force URL to be non-empty.  We've never supported this case, but past bugs
   // might have resulted in it slipping through; eventually this code can be
   // replaced with a DCHECK(!search_url.empty());.
   if (search_url.empty())
     return false;
   std::string suggest_url =
       prefs->GetString(prefs::kDefaultSearchProviderSuggestURL);
   std::string instant_url =
@@ skipping 20 matching lines @@
     base::StringToInt64(id_string, &value);
     data.id = value;
   }
   if (!prepopulate_id.empty() && !*is_managed) {
     int value;
     base::StringToInt(prepopulate_id, &value);
     data.prepopulate_id = value;
   }
   default_provider->reset(new TemplateURL(profile_, data));
   DCHECK(!(*default_provider)->IsExtensionKeyword());
+  if (update_keyword)
+    (*default_provider)->ResetKeywordIfNecessary(true);
   return true;
 }
 
 bool TemplateURLService::CanReplaceKeywordForHost(
     const std::string& host,
     TemplateURL** to_replace) {
   DCHECK(!to_replace || !*to_replace);
   const TemplateURLSet* urls = provider_map_->GetURLsForHost(host);
   if (!urls)
     return true;
   for (TemplateURLSet::const_iterator i(urls->begin()); i != urls->end(); ++i) {
     if (CanReplace(*i)) {
       if (to_replace)
         *to_replace = *i;
       return true;
     }
   }
   return false;
 }
 
 bool TemplateURLService::CanReplace(const TemplateURL* t_url) {
   return (t_url != default_search_provider_ && !t_url->show_in_default_list() &&
           t_url->safe_for_autoreplace());
 }
 
+TemplateURL* TemplateURLService::FindNonExtensionTemplateURLForKeyword(
+    const string16& keyword) {
+  TemplateURL* keyword_turl = GetTemplateURLForKeyword(keyword);
+  if (!keyword_turl || !keyword_turl->IsExtensionKeyword())
+    return keyword_turl;
+  // The extension keyword in the model may be hiding a replaceable
+  // non-extension keyword.  Look for it.
+  for (TemplateURLVector::const_iterator i(template_urls_.begin());
+       i != template_urls_.end(); ++i) {
+    if (!(*i)->IsExtensionKeyword() && ((*i)->keyword() == keyword))
+      return *i;
+  }
+  return NULL;
+}
+
 void TemplateURLService::UpdateNoNotify(TemplateURL* existing_turl,
                                         const TemplateURL& new_values) {
   DCHECK(loaded_);
   DCHECK(existing_turl);
   DCHECK(std::find(template_urls_.begin(), template_urls_.end(),
                    existing_turl) != template_urls_.end());
 
   // 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());
-  if (!old_keyword.empty())
-    keyword_to_template_map_.erase(old_keyword);
+  keyword_to_template_map_.erase(old_keyword);
   if (!existing_turl->sync_guid().empty())
     guid_to_template_map_.erase(existing_turl->sync_guid());
 
   provider_map_->Remove(existing_turl);
   TemplateURLID previous_id = existing_turl->id();
   *existing_turl = new_values;
   existing_turl->data_.id = previous_id;
   UIThreadSearchTermsData search_terms_data;
   provider_map_->Add(existing_turl, search_terms_data);
 
   const string16& keyword = existing_turl->keyword();
-  if (!keyword.empty())
+  KeywordToTemplateMap::const_iterator i =
+      keyword_to_template_map_.find(keyword);
+  if (i == keyword_to_template_map_.end()) {
     keyword_to_template_map_[keyword] = existing_turl;
+  } else {
+    // We can theoretically reach here in two cases:
+    //   * There is an existing extension keyword and sync brings in a rename of
+    //     a non-extension keyword to match.  In this case we just need to pick
+    //     which keyword has priority to update the keyword map.
+    //   * Autogeneration of the keyword for a Google default search provider
+    //     at load time causes it to conflict with an existing keyword.  In this
+    //     case we delete the existing keyword if it's replaceable, or else undo
+    //     the change in keyword for |existing_turl|.
+    DCHECK(!existing_turl->IsExtensionKeyword());
+    TemplateURL* existing_keyword_turl = i->second;
+    if (existing_keyword_turl->IsExtensionKeyword()) {
+      if (!CanReplace(existing_turl))
+        keyword_to_template_map_[keyword] = existing_turl;
+    } else {
+      if (CanReplace(existing_keyword_turl)) {
+        RemoveNoNotify(existing_keyword_turl);
+      } else {
+        existing_turl->data_.SetKeyword(old_keyword);
+        keyword_to_template_map_[old_keyword] = existing_turl;
+      }
+    }
+  }
   if (!existing_turl->sync_guid().empty())
     guid_to_template_map_[existing_turl->sync_guid()] = existing_turl;
 
   if (service_.get())
-    service_->UpdateKeyword(*existing_turl);
+    service_->UpdateKeyword(existing_turl->data());
 
   // Inform sync of the update.
   ProcessTemplateURLChange(existing_turl, SyncChange::ACTION_UPDATE);
 
   if (default_search_provider_ == existing_turl)
     SetDefaultSearchProviderNoNotify(existing_turl);
 }
 
 PrefService* TemplateURLService::GetPrefs() {
   return profile_ ? profile_->GetPrefs() : NULL;
@@ skipping 120 matching lines @@
   bool something_changed = false;
   for (TemplateURLVector::iterator i(template_urls_.begin());
        i != template_urls_.end(); ++i) {
     TemplateURL* t_url = *i;
     if (t_url->url_ref().HasGoogleBaseURLs() ||
         t_url->suggestions_url_ref().HasGoogleBaseURLs()) {
       something_changed = true;
       string16 original_keyword(t_url->keyword());
       t_url->InvalidateCachedValues();
       const string16& new_keyword(t_url->keyword());
+      KeywordToTemplateMap::const_iterator i =
+          keyword_to_template_map_.find(new_keyword);
+      if ((i != keyword_to_template_map_.end()) && (i->second != t_url)) {
+        // The new autogenerated keyword conflicts with another TemplateURL.
+        // Overwrite it if it's replaceable; otherwise just reset |t_url|'s
+        // keyword.  (This will not prevent |t_url| from auto-updating the
+        // keyword in the future if the conflicting TemplateURL disappears.)
+        if (!CanReplace(i->second)) {
+          t_url->data_.SetKeyword(original_keyword);
+          continue;
+        }
+        RemoveNoNotify(i->second);
+      }
       RemoveFromKeywordMapByPointer(t_url);
-      if (!new_keyword.empty())
-        keyword_to_template_map_[new_keyword] = t_url;
+      keyword_to_template_map_[new_keyword] = t_url;
     }
   }
 
   if (something_changed && loaded_) {
     UIThreadSearchTermsData search_terms_data;
     provider_map_->UpdateGoogleBaseURLs(search_terms_data);
     NotifyObservers();
   }
 }
 
@@ skipping 44 matching lines @@
       TemplateURLData data(new_default_from_prefs->data());
       data.created_by_policy = true;
       TemplateURL new_values(profile_, data);
       UpdateNoNotify(default_search_provider_, new_values);
     } else {
       TemplateURL* new_template = NULL;
       if (new_default_from_prefs.get()) {
         TemplateURLData data(new_default_from_prefs->data());
         data.created_by_policy = true;
         new_template = new TemplateURL(profile_, data);
-        AddNoNotify(new_template, true);
+        if (!AddNoNotify(new_template, true))
+          return;
       }
       SetDefaultSearchProviderNoNotify(new_template);
     }
   } else if (!is_default_search_managed_ && new_is_default_managed) {
     // The default used to be unmanaged and is now managed.  Add the new
     // managed default to the list of URLs and set it as default.
     is_default_search_managed_ = new_is_default_managed;
     TemplateURL* new_template = NULL;
     if (new_default_from_prefs.get()) {
       TemplateURLData data(new_default_from_prefs->data());
       data.created_by_policy = true;
       new_template = new TemplateURL(profile_, data);
-      AddNoNotify(new_template, true);
+      if (!AddNoNotify(new_template, true))
+        return;
     }
     SetDefaultSearchProviderNoNotify(new_template);
   } else {
     // The default was managed and is no longer.
     DCHECK(is_default_search_managed_ && !new_is_default_managed);
     is_default_search_managed_ = new_is_default_managed;
     // If we had a default, delete the previous default if created by policy
     // and set a likely default.
     if ((default_search_provider_ != NULL) &&
         default_search_provider_->created_by_policy()) {
@@ skipping 21 matching lines @@
     DCHECK(!url->IsExtensionKeyword());
   }
 
   default_search_provider_ = url;
 
   if (url) {
     // Don't mark the url as edited, otherwise we won't be able to rev the
     // template urls we ship with.
     url->data_.show_in_default_list = true;
     if (service_.get())
-      service_->UpdateKeyword(*url);
+      service_->UpdateKeyword(url->data());
 
     if (url->url_ref().HasGoogleBaseURLs()) {
       GoogleURLTracker::RequestServerCheck();
 #if defined(ENABLE_RLZ)
       // Needs to be evaluated. See http://crbug.com/62328.
       base::ThreadRestrictions::ScopedAllowIO allow_io;
       RLZTracker::RecordProductEvent(rlz_lib::CHROME,
                                      rlz_lib::CHROME_OMNIBOX,
                                      rlz_lib::SET_TO_GOOGLE);
 #endif
@@ skipping 13 matching lines @@
   }
 
   if (service_.get())
     service_->SetDefaultSearchProvider(url);
 
   // Inform sync the change to the show_in_default_list flag.
   if (url)
     ProcessTemplateURLChange(url, SyncChange::ACTION_UPDATE);
 }
 
-void TemplateURLService::AddNoNotify(TemplateURL* template_url,
+bool TemplateURLService::AddNoNotify(TemplateURL* template_url,
                                      bool newly_adding) {
   DCHECK(template_url);
 
   if (newly_adding) {
     DCHECK_EQ(kInvalidTemplateURLID, template_url->id());
     DCHECK(std::find(template_urls_.begin(), template_urls_.end(),
                      template_url) == template_urls_.end());
     template_url->data_.id = ++next_id_;
   }
 
+  template_url->ResetKeywordIfNecessary(false);
+  if (!template_url->IsExtensionKeyword()) {
+    // Check whether |template_url|'s keyword conflicts with any already in the
+    // model.
+    TemplateURL* existing_keyword_turl =
+        FindNonExtensionTemplateURLForKeyword(template_url->keyword());
+    if (existing_keyword_turl != NULL) {
+      DCHECK_NE(existing_keyword_turl, template_url);
+      if (CanReplace(existing_keyword_turl)) {
+        RemoveNoNotify(existing_keyword_turl);
+      } else if (CanReplace(template_url)) {
+        delete template_url;
+        return false;
+      } else {
+        string16 new_keyword = UniquifyKeyword(*existing_keyword_turl);
+        ResetTemplateURL(existing_keyword_turl,
+                         existing_keyword_turl->short_name(), new_keyword,
+                         existing_keyword_turl->url());
+      }
+    }
+  }
   template_urls_.push_back(template_url);
   AddToMaps(template_url);
 
   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())
-      service_->AddKeyword(*template_url);
+      service_->AddKeyword(template_url->data());
 
     // Inform sync of the addition. Note that this will assign a GUID to
     // template_url and add it to the guid_to_template_map_.
     ProcessTemplateURLChange(template_url, SyncChange::ACTION_ADD);
   }
+
+  return true;
 }
 
 void TemplateURLService::RemoveNoNotify(TemplateURL* template_url) {
   TemplateURLVector::iterator i =
       std::find(template_urls_.begin(), template_urls_.end(), template_url);
   if (i == template_urls_.end())
     return;
 
   if (template_url == default_search_provider_) {
     // Should never delete the default search provider.
@@ skipping 97 matching lines @@
 }
 
 string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl) {
   // Already unique.
   if (!GetTemplateURLForKeyword(turl.keyword()))
     return turl.keyword();
 
   // First, try to return the generated keyword for the TemplateURL.
   GURL gurl(turl.url());
   if (gurl.is_valid()) {
-    string16 keyword_candidate = GenerateKeyword(gurl, true);
-    if (!GetTemplateURLForKeyword(keyword_candidate) &&
-        !keyword_candidate.empty())
+    string16 keyword_candidate = GenerateKeyword(gurl);
+    if (!GetTemplateURLForKeyword(keyword_candidate))
       return keyword_candidate;
   }
 
   // We try to uniquify the keyword by appending a special character to the end.
   // This is a best-effort approach where we try to preserve the original
   // keyword and let the user do what they will after our attempt.
   string16 keyword_candidate(turl.keyword());
   do {
     keyword_candidate.append(ASCIIToUTF16("_"));
   } while (GetTemplateURLForKeyword(keyword_candidate));
 
   return keyword_candidate;
 }
 
-bool TemplateURLService::ResolveSyncKeywordConflict(
+void TemplateURLService::ResolveSyncKeywordConflict(
     TemplateURL* sync_turl,
+    TemplateURL* local_turl,
     SyncChangeList* change_list) {
   DCHECK(sync_turl);
+  DCHECK(local_turl);
+  DCHECK(sync_turl->sync_guid() != local_turl->sync_guid());
+  DCHECK(!local_turl->IsExtensionKeyword());
   DCHECK(change_list);
 
-  TemplateURL* existing_turl =
-      GetTemplateURLForKeyword(sync_turl->keyword());
-  // If there is no conflict, or it's just conflicting with itself, return.
-  if (!existing_turl || existing_turl->sync_guid() == sync_turl->sync_guid())
-    return false;
-
-  if (existing_turl->last_modified() > sync_turl->last_modified() ||
-      existing_turl->created_by_policy()) {
+  if (local_turl->last_modified() > sync_turl->last_modified() ||
+      local_turl->created_by_policy()) {
     string16 new_keyword = UniquifyKeyword(*sync_turl);
     DCHECK(!GetTemplateURLForKeyword(new_keyword));
     sync_turl->data_.SetKeyword(new_keyword);
     // If we update the cloud TURL, we need to push an update back to sync
     // informing it that something has changed.
     SyncData sync_data = CreateSyncDataFromTemplateURL(*sync_turl);
     change_list->push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data));
   } else {
-    string16 new_keyword = UniquifyKeyword(*existing_turl);
-    TemplateURLData data(existing_turl->data());
+    string16 new_keyword = UniquifyKeyword(*local_turl);
+    TemplateURLData data(local_turl->data());
     data.SetKeyword(new_keyword);
-    TemplateURL new_turl(existing_turl->profile(), data);
-    UpdateNoNotify(existing_turl, new_turl);
+    TemplateURL new_turl(local_turl->profile(), data);
+    UpdateNoNotify(local_turl, new_turl);
     NotifyObservers();
+    if (!models_associated_) {
+      // We're doing our initial sync, so UpdateNoNotify() won't have generated
+      // an ACTION_UPDATE.  If this local URL is one that was just newly brought
+      // down from the sync server, we need to go ahead and generate an update
+      // for it.  If it was pre-existing, then this is unnecessary (and in fact
+      // wrong) because MergeDataAndStartSyncing() will later add an ACTION_ADD
+      // for this URL; but in this case, PreventDuplicateGUIDUpdates() will
+      // prune out the ACTION_UPDATE we create here.
+      SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
+      change_list->push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data));
+    }
   }
-  return true;
 }
 
 TemplateURL* TemplateURLService::FindDuplicateOfSyncTemplateURL(
     const TemplateURL& sync_turl) {
   TemplateURL* existing_turl = GetTemplateURLForKeyword(sync_turl.keyword());
   return existing_turl && (existing_turl->url() == sync_turl.url()) ?
       existing_turl : NULL;
 }
 
 void TemplateURLService::MergeSyncAndLocalURLDuplicates(
@@ skipping 67 matching lines @@
        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()) {
       template_url->data_.sync_guid = guid::GenerateGUID();
       if (service_.get())
-        service_->UpdateKeyword(*template_url);
+        service_->UpdateKeyword(template_url->data());
     }
   }
 }