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->HasSameKeywordAs(*url2) &&
       (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?

[Nicolas Zea, 2012/05/07 22:27:27]

Sync will coalesce any changes sent within a single ProcessSyncChanges call.
But, if changes span more than one call, there's no guarantee they'll be part of
a single batch.

[Peter Kasting, 2012/05/08 00:37:06]

OK.  I think I may leave this here anyway as I need something like it to prevent
update-before-add even if I don't need to prevent update-before-update.  I've
added more comments, see what you think.

+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) &&

[Nicolas Zea, 2012/05/07 22:27:27]

nit: multi-line condition -> curly brace

[Peter Kasting, 2012/05/08 00:37:06]

Not the style rule.  The rule is multi-line body.  Conditional length doesn't
matter.

[Nicolas Zea, 2012/05/08 00:51:03]

I'll take your word for it. I suppose I based my interpretation on: "Omit {} for
conditional or loop bodies where both the body and the header are just one
physical line each.  Otherwise, use {}." But I guess header != condition.

[Peter Kasting, 2012/05/08 01:31:42]

No, you read correctly, but the style guide is wrong.  This was erroneously
changed in the most recent version of the style guide, less than a month ago. 
I've reverted the change.

+        (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) &&

[Nicolas Zea, 2012/05/07 22:27:27]

I think you might run into issues if you erase an ADD in favor of an UPDATE.
They're not completely interchangeable, as sync will expect an UPDATE to already
exist, and will error otherwise.

[Peter Kasting, 2012/05/08 00:37:06]

This never erases ADDs, only UPDATEs.

[Nicolas Zea, 2012/05/08 00:51:03]

Ah, I see now. Sounds good.

+          (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 52 matching lines @@
       matches->push_back(i->first);
   }
 }
 
 TemplateURL* TemplateURLService::GetTemplateURLForKeyword(
     const string16& keyword) {
   KeywordToTemplateMap::const_iterator elem(
       keyword_to_template_map_.find(keyword));
   if (elem != keyword_to_template_map_.end())
     return elem->second;
-  return (initial_default_search_provider_.get() &&
+  return ((!loaded_ || load_failed_) &&
+      initial_default_search_provider_.get() &&
       (initial_default_search_provider_->keyword() == keyword)) ?
       initial_default_search_provider_.get() : NULL;
 }
 
 TemplateURL* TemplateURLService::GetTemplateURLForGUID(
     const std::string& sync_guid) {
-  GUIDToTemplateMap::const_iterator elem(
-      guid_to_template_map_.find(sync_guid));
+  GUIDToTemplateMap::const_iterator elem(guid_to_template_map_.find(sync_guid));
   if (elem != guid_to_template_map_.end())
     return elem->second;
-  return (initial_default_search_provider_.get() &&
+  return ((!loaded_ || load_failed_) &&
+      initial_default_search_provider_.get() &&
       (initial_default_search_provider_->sync_guid() == sync_guid)) ?
       initial_default_search_provider_.get() : NULL;
 }
 
 TemplateURL* TemplateURLService::GetTemplateURLForHost(
     const std::string& host) {
   TemplateURL* t_url = provider_map_->GetTemplateURLForHost(host);
   if (t_url)
     return t_url;
-  return (initial_default_search_provider_.get() &&
+  return ((!loaded_ || load_failed_) &&
+      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 79 matching lines @@
   }
 
   return NULL;
 }
 
 TemplateURLService::TemplateURLVector TemplateURLService::GetTemplateURLs() {
   return template_urls_;
 }
 
 void TemplateURLService::IncrementUsageCount(TemplateURL* url) {
-  DCHECK(url && std::find(template_urls_.begin(), template_urls_.end(), url) !=
-      template_urls_.end());
+  DCHECK(url);
+  if (std::find(template_urls_.begin(), template_urls_.end(), 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())
-    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) {
+  if (!loaded_)
+    return;
+  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;
   data.last_modified = time_provider_();
   TemplateURL new_url(url->profile(), data);
-  UpdateNoNotify(url, new_url);
-  NotifyObservers();
+  if (UpdateNoNotify(url, new_url))
+    NotifyObservers();
 }
 
 bool TemplateURLService::CanMakeDefault(const TemplateURL* url) {
   return url != GetDefaultSearchProvider() &&
       url->url_ref().SupportsReplacement() && !is_default_search_managed();
 }
 
 void TemplateURLService::SetDefaultSearchProvider(TemplateURL* url) {
   DCHECK(!is_default_search_managed_);
   // Extension keywords cannot be made default, as they are inherently async.
   DCHECK(!url || !url->IsExtensionKeyword());
 
   // Always persist the setting in the database, that way if the backup
   // signature has changed out from under us it gets reset correctly.
-  SetDefaultSearchProviderNoNotify(url);
-  NotifyObservers();
+  if (SetDefaultSearchProviderNoNotify(url))
+    NotifyObservers();
 }
 
 TemplateURL* TemplateURLService::GetDefaultSearchProvider() {
   if (loaded_ && !load_failed_)
     return default_search_provider_;
 
   // We're not loaded, rely on the default search provider stored in prefs.
   return initial_default_search_provider_.get();
 }
 
@@ skipping 100 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);
+         default_search_provider->SupportsReplacement())) {
+      bool success = SetDefaultSearchProviderNoNotify(default_search_provider);
+      DCHECK(success);
+    }
   } else {
     // If we had a managed default, replace it with the synced default if
     // applicable, or the first provider of the list.
     TemplateURL* synced_default = GetPendingSyncedDefaultSearchProvider();
     if (synced_default) {
       default_search_provider = synced_default;
       pending_synced_default_search_ = false;
     } else if (database_specified_a_default &&
                default_search_provider == NULL) {
       default_search_provider = FirstPotentialDefaultEngine(template_urls);
@@ skipping 60 matching lines @@
     check_if_default_search_valid = false;
   }
 #endif
 
   if (check_if_default_search_valid) {
     bool has_default_search_provider = default_search_provider_ != NULL &&
         default_search_provider_->SupportsReplacement();
     UMA_HISTOGRAM_BOOLEAN("Search.HasDefaultSearchProvider",
                           has_default_search_provider);
     // Ensure that default search provider exists. See http://crbug.com/116952.
-    if (!has_default_search_provider)
-      SetDefaultSearchProviderNoNotify(FindNewDefaultSearchProvider());
+    if (!has_default_search_provider) {
+      bool success =
+          SetDefaultSearchProviderNoNotify(FindNewDefaultSearchProvider());
+      DCHECK(success);
+    }
   }
 
   NotifyObservers();
   NotifyLoaded();
 }
 
 string16 TemplateURLService::GetKeywordShortName(const string16& keyword,
                                                  bool* is_extension_keyword) {
   const TemplateURL* template_url = GetTemplateURLForKeyword(keyword);
 
   // TODO(sky): Once LocationBarView adds a listener to the TemplateURLService
   // to track changes to the model, this should become a DCHECK.
   if (template_url) {
     *is_extension_keyword = template_url->IsExtensionKeyword();
     return template_url->AdjustedShortNameForLocaleDirection();
   }
   *is_extension_keyword = false;
   return string16();
 }
 
 void TemplateURLService::Observe(int type,
                                  const content::NotificationSource& source,
                                  const content::NotificationDetails& details) {
   if (type == chrome::NOTIFICATION_HISTORY_URL_VISITED) {
     content::Details<history::URLVisitedDetails> visit_details(details);
-    if (!loaded())
+    if (!loaded_)
       visits_to_add_.push_back(*visit_details.ptr());
     else
       UpdateKeywordSearchTermsForURL(*visit_details.ptr());
   } else if (type == chrome::NOTIFICATION_DEFAULT_SEARCH_POLICY_CHANGED) {
     // Policy has been updated, so the default search prefs may be different.
     // Reload the default search provider from them.
     // TODO(pkasting): Rather than communicating via prefs, we should eventually
     // observe policy changes directly.
     UpdateDefaultSearch();
   } else if (type == chrome::NOTIFICATION_GOOGLE_URL_UPDATED) {
@@ skipping 45 matching lines @@
 }
 
 SyncError TemplateURLService::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const SyncChangeList& change_list) {
   if (!models_associated_) {
     SyncError error(FROM_HERE, "Models not yet associated.",
                     syncable::SEARCH_ENGINES);
     return error;
   }
+  DCHECK(loaded_);
 
   AutoReset<bool> processing_changes(&processing_syncer_changes_, true);
 
   SyncChangeList new_changes;
   SyncError error;
   for (SyncChangeList::const_iterator iter = change_list.begin();
       iter != change_list.end(); ++iter) {
     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);
-      UpdateNoNotify(existing_turl, *turl);
-      NotifyObservers();
+      if (existing_keyword_turl && existing_keyword_turl != existing_turl) {
+        ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
+                                   &new_changes);
+      }
+      if (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;
 }
 
 SyncError TemplateURLService::MergeDataAndStartSyncing(
     syncable::ModelType type,
     const SyncDataList& initial_sync_data,
     scoped_ptr<SyncChangeProcessor> sync_processor,
     scoped_ptr<SyncErrorFactory> sync_error_factory) {
-  DCHECK(loaded());
+  DCHECK(loaded_);
   DCHECK_EQ(type, syncable::SEARCH_ENGINES);
   DCHECK(!sync_processor_.get());
   DCHECK(sync_processor.get());
   DCHECK(sync_error_factory.get());
   sync_processor_ = sync_processor.Pass();
   sync_error_factory_ = sync_error_factory.Pass();
 
   // We just started syncing, so set our wait-for-default flag if we are
   // expecting a default from Sync.
   if (GetPrefs()) {
@@ skipping 29 matching lines @@
     if (local_turl) {
       // This local search engine is already synced. If the timestamp differs
       // from Sync, we need to update locally or to the cloud. Note that if the
       // timestamps are equal, we touch neither.
       if (sync_turl->last_modified() > local_turl->last_modified()) {
         // We've received an update from Sync. We should replace all synced
         // fields in the local TemplateURL. Note that this includes the
         // TemplateURLID and the TemplateURL may have to be reparsed. This
         // also makes the local data's last_modified timestamp equal to Sync's,
         // avoiding an Update on the next MergeData call.
-        UpdateNoNotify(local_turl, *sync_turl);
-        NotifyObservers();
+        if (UpdateNoNotify(local_turl, *sync_turl))
+          NotifyObservers();
       } else if (sync_turl->last_modified() < local_turl->last_modified()) {
         // Otherwise, we know we have newer data, so update Sync with our
         // data fields.
         new_changes.push_back(SyncChange(SyncChange::ACTION_UPDATE,
             local_data_map[local_turl->sync_guid()]));
       }
       local_data_map.erase(iter->first);
     } else {
       // The search engine from the cloud has not been synced locally, but there
       // might be a local search engine that is a duplicate that needs to be
       // merged.
       TemplateURL* dupe_turl = FindDuplicateOfSyncTemplateURL(*sync_turl);
       if (dupe_turl) {
         // 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?

[Nicolas Zea, 2012/05/07 22:27:27]

I think so. In addition, I would modify the protobuf definition
(src/sync/protocol/search_engine_specifics.proto), adding a comment that
autogenerate_keyword is deprecated and commenting it out completely so that it
isn't consumed/set anywhere accidentally (it's safe to comment out a field as
long as you don't modify any field numbers).

That's assuming that you don't actually need to read from it anywhere else of
course.

[Peter Kasting, 2012/05/08 00:37:06]

OK, so it will default to false then?
Can't comment it out, see below; but I've least added the comment to the proto
file.

[Nicolas Zea, 2012/05/08 00:51:03]

Yes.

+  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?

[Nicolas Zea, 2012/05/07 22:27:27]

The rule of thumb I've heard is once the last version that supported an old
field is two stable releases old, we consider phasing it out completely. That
said, it might be useful to keep the support for empty keywords around anyways.

[Peter Kasting, 2012/05/08 00:37:06]

Added some comments.

+  bool reset_keyword =
+      specifics.autogenerate_keyword() || specifics.keyword().empty();

[Nicolas Zea, 2012/05/07 22:27:27]

do you have to check autogenerate_keyword here? Or is an empty keyword enough?

[Peter Kasting, 2012/05/08 00:37:06]

They actually check two different things.  Autogenerated keywords will have a
non-empty keyword field and have this bit set.  So for these I have to read the
bit, and thus can't comment out the field definition in the proto file. 
TemplateURLs with an empty keyword are basically an error case that we didn't do
enough to prevent previously, and I'm now fixing up.

+  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());
 }
 
-void TemplateURLService::UpdateNoNotify(TemplateURL* existing_turl,
+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;
+}
+
+bool 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());
+  if (std::find(template_urls_.begin(), template_urls_.end(), existing_turl) ==
+      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());
-  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);
+  if (default_search_provider_ == existing_turl) {
+      bool success = SetDefaultSearchProviderNoNotify(existing_turl);
+      DCHECK(success);
+  }
+  return true;
 }
 
 PrefService* TemplateURLService::GetPrefs() {
   return profile_ ? profile_->GetPrefs() : NULL;
 }
 
 void TemplateURLService::UpdateKeywordSearchTermsForURL(
     const history::URLVisitedDetails& details) {
   const history::URLRow& row = details.row;
   if (!row.url().is_valid() ||
@@ skipping 114 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 31 matching lines @@
     // if it has changed; we don't want to respond to changes triggered by
     // SaveDefaultSearchProviderToPrefs.
     if (TemplateURLsHaveSamePrefs(default_search_provider_,
                                   new_default_from_prefs.get()))
       return;
     if (new_default_from_prefs.get() == NULL) {
       // default_search_provider_ can't be NULL otherwise
       // TemplateURLsHaveSamePrefs would have returned true.  Remove this now
       // invalid value.
       TemplateURL* old_default = default_search_provider_;
-      SetDefaultSearchProviderNoNotify(NULL);
+      bool success = SetDefaultSearchProviderNoNotify(NULL);
+      DCHECK(success);
       RemoveNoNotify(old_default);
     } else if (default_search_provider_) {
       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);
+      bool success = SetDefaultSearchProviderNoNotify(new_template);
+      DCHECK(success);
     }
   } 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);
+    bool success = SetDefaultSearchProviderNoNotify(new_template);
+    DCHECK(success);
   } 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()) {
       TemplateURL* old_default = default_search_provider_;
       default_search_provider_ = NULL;
       RemoveNoNotify(old_default);
     }
 
     // The likely default should be from Sync if we were waiting on Sync.
     // Otherwise, it should be FindNewDefaultSearchProvider.
     TemplateURL* synced_default = GetPendingSyncedDefaultSearchProvider();
     if (synced_default)
       pending_synced_default_search_ = false;
     SetDefaultSearchProviderNoNotify(synced_default ? synced_default :
         FindNewDefaultSearchProvider());
   }
   NotifyObservers();
 }
 
-void TemplateURLService::SetDefaultSearchProviderNoNotify(TemplateURL* url) {
+bool TemplateURLService::SetDefaultSearchProviderNoNotify(TemplateURL* url) {
   if (url) {
-    DCHECK(std::find(template_urls_.begin(), template_urls_.end(), url) !=
-           template_urls_.end());
+    if (std::find(template_urls_.begin(), template_urls_.end(), url) ==
+        template_urls_.end())
+      return false;
     // Extension keywords cannot be made default, as they're inherently async.
     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 11 matching lines @@
                             url->sync_guid());
     }
   }
 
   if (service_.get())
     service_->SetDefaultSearchProvider(url);
 
   // Inform sync the change to the show_in_default_list flag.
   if (url)
     ProcessTemplateURLChange(url, SyncChange::ACTION_UPDATE);
+  return true;
 }
 
-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 81 matching lines @@
         service_->RemoveKeyword(template_url->id());
       delete template_url;
     } else {
       ++i;
     }
   }
 }
 
 void TemplateURLService::ResetTemplateURLGUID(TemplateURL* url,
                                               const std::string& guid) {
+  DCHECK(loaded_);
   DCHECK(!guid.empty());
 
   TemplateURLData data(url->data());
   data.sync_guid = guid;
   TemplateURL new_url(url->profile(), data);
   UpdateNoNotify(url, new_url);
 }
 
 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(loaded_);
   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);
-    NotifyObservers();
+    TemplateURL new_turl(local_turl->profile(), data);
+    if (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(
     TemplateURL* sync_turl,
     TemplateURL* local_turl,
     SyncChangeList* change_list) {
+  DCHECK(loaded_);
   DCHECK(sync_turl);
   DCHECK(local_turl);
   DCHECK(change_list);
   scoped_ptr<TemplateURL> scoped_sync_turl(sync_turl);
   if (sync_turl->last_modified() > local_turl->last_modified()) {
     // Fully replace local_url with Sync's copy. Note that because use Add
     // rather than ResetTemplateURL, |sync_url| is added with a fresh
     // TemplateURLID. We don't need to sync the new ID back to the server since
     // it's only relevant locally.
     bool delete_default = (local_turl == GetDefaultSearchProvider());
@@ skipping 54 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());
     }
   }
 }