Remove the "autogenerate keyword" bit on TemplateURL.

chrome/browser/search_engines/template_url_service.cc

Index: chrome/browser/search_engines/template_url_service.cc
===================================================================
--- chrome/browser/search_engines/template_url_service.cc	(revision 135424)
+++ chrome/browser/search_engines/template_url_service.cc	(working copy)
@@ -70,7 +70,9 @@
     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()) &&
@@ -90,6 +92,43 @@
   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
 
 
@@ -160,22 +199,8 @@
 }
 
 // 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
@@ -231,10 +256,10 @@
     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))
@@ -334,8 +359,8 @@
 }
 
 void TemplateURLService::Add(TemplateURL* template_url) {
-  AddNoNotify(template_url, true);
-  NotifyObservers();
+  if (AddNoNotify(template_url, true))
+    NotifyObservers();
 }
 
 void TemplateURLService::AddAndSetProfile(TemplateURL* template_url,
@@ -348,6 +373,7 @@
                                           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);
@@ -454,13 +480,14 @@
   // 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;
@@ -611,7 +638,8 @@
         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.
@@ -809,9 +837,13 @@
     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());
 
@@ -829,8 +861,10 @@
     } 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;
@@ -842,8 +876,10 @@
                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 {
@@ -858,6 +894,7 @@
                 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.
@@ -949,8 +986,14 @@
         // 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;
@@ -969,6 +1012,8 @@
     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;
@@ -1031,7 +1076,8 @@
   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());
@@ -1061,8 +1107,15 @@
   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();
@@ -1082,6 +1135,11 @@
 
   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;
 }
 
@@ -1167,8 +1225,31 @@
 }
 
 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.
@@ -1198,15 +1279,31 @@
 }
 
 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())
@@ -1326,6 +1423,13 @@
       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
@@ -1366,6 +1470,8 @@
   }
   default_provider->reset(new TemplateURL(profile_, data));
   DCHECK(!(*default_provider)->IsExtensionKeyword());
+  if (update_keyword)
+    (*default_provider)->ResetKeywordIfNecessary(true);
   return true;
 }
 
@@ -1391,6 +1497,21 @@
           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_);
@@ -1404,8 +1525,7 @@
   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());
 
@@ -1417,13 +1537,38 @@
   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);
@@ -1564,9 +1709,21 @@
       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;
     }
   }
 
@@ -1631,7 +1788,8 @@
         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);
     }
@@ -1644,7 +1802,8 @@
       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 {
@@ -1686,7 +1845,7 @@
     // 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();
@@ -1720,7 +1879,7 @@
     ProcessTemplateURLChange(url, SyncChange::ACTION_UPDATE);
 }
 
-void TemplateURLService::AddNoNotify(TemplateURL* template_url,
+bool TemplateURLService::AddNoNotify(TemplateURL* template_url,
                                      bool newly_adding) {
   DCHECK(template_url);
 
@@ -1731,6 +1890,27 @@
     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);
 
@@ -1740,12 +1920,14 @@
     // 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) {
@@ -1863,9 +2045,8 @@
   // 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;
   }
 
@@ -1880,20 +2061,18 @@
   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);
@@ -1902,14 +2081,24 @@
     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(
@@ -1997,7 +2186,7 @@
         !template_url->IsExtensionKeyword()) {
       template_url->data_.sync_guid = guid::GenerateGUID();
       if (service_.get())
-        service_->UpdateKeyword(*template_url);
+        service_->UpdateKeyword(template_url->data());
     }
   }
 }