Fix a bug where two loaded keywords clash after a GoogleBaseURL change.

chrome/browser/search_engines/template_url_service.cc

Index: chrome/browser/search_engines/template_url_service.cc
diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index 1b85a0fd457b18a23da8a63a563a9c04ab6572c3..88b99eb5c85e1dbb4fd8192ec42e2700a36418b5 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -1593,6 +1593,7 @@ void TemplateURLService::RemoveFromMaps(TemplateURL* template_url) {
   if (!template_url->sync_guid().empty())
     guid_to_template_map_.erase(template_url->sync_guid());
   if (loaded_) {
+    // |provider_map_| is only initialized after loading has completed.

[Peter Kasting, 2014/05/13 19:04:23]

Move this comment above the conditional instead of inside it (2 places)

     UIThreadSearchTermsData search_terms_data(template_url->profile());
     provider_map_->Remove(template_url, search_terms_data);
   }
@@ -1623,6 +1624,7 @@ void TemplateURLService::AddToMaps(TemplateURL* template_url) {
   if (!template_url->sync_guid().empty())
     guid_to_template_map_[template_url->sync_guid()] = template_url;
   if (loaded_) {
+    // |provider_map_| is only initialized after loading has completed.
     UIThreadSearchTermsData search_terms_data(profile_);
     provider_map_->Add(template_url, search_terms_data);
   }
@@ -1718,7 +1720,6 @@ bool TemplateURLService::UpdateNoNotify(
     TemplateURL* existing_turl,
     const TemplateURL& new_values,
     const SearchTermsData& old_search_terms_data) {
-  DCHECK(loaded_);
   DCHECK(existing_turl);
   if (std::find(template_urls_.begin(), template_urls_.end(), existing_turl) ==
       template_urls_.end())
@@ -1729,12 +1730,19 @@ bool TemplateURLService::UpdateNoNotify(
   if (!existing_turl->sync_guid().empty())
     guid_to_template_map_.erase(existing_turl->sync_guid());
 
-  provider_map_->Remove(existing_turl, old_search_terms_data);
+  // |provider_map_| is only initialized after loading has completed.
+  if (loaded_)
+    provider_map_->Remove(existing_turl, old_search_terms_data);
+
   TemplateURLID previous_id = existing_turl->id();
   existing_turl->CopyFrom(new_values);
   existing_turl->data_.id = previous_id;
-  UIThreadSearchTermsData new_search_terms_data(profile_);
-  provider_map_->Add(existing_turl, new_search_terms_data);
+
+  if (loaded_) {
+    // |provider_map_| is only initialized after loading has completed.

[Peter Kasting, 2014/05/13 19:04:23]

Remove this comment, the one above is clear enough.

+    UIThreadSearchTermsData new_search_terms_data(profile_);
+    provider_map_->Add(existing_turl, new_search_terms_data);
+  }
 
   const base::string16& keyword = existing_turl->keyword();
   KeywordToTemplateMap::const_iterator i =
@@ -2054,7 +2062,19 @@ bool TemplateURLService::AddNoNotify(TemplateURL* template_url,
   // model.
   TemplateURL* existing_keyword_turl =
       GetTemplateURLForKeyword(template_url->keyword());
-  if (existing_keyword_turl != NULL) {
+
+  // We might be processing results from WebData now, in which case we are not
+  // in the loaded state and existing_keyword_turl might be the (not yet added)
+  // initial_default_search_provider_.
+  bool existing_keyword_already_stored =
+      existing_keyword_turl &&
+      (std::find(template_urls_.begin(),
+                 template_urls_.end(),
+                 existing_keyword_turl) != template_urls_.end());
+
+  // We only do the following if there is a conflict already processed into
+  // template_urls_.
+  if (existing_keyword_already_stored) {

[Peter Kasting, 2014/05/13 19:04:23]

Nit: How about this, which seems a bit clearer to me:

  template_url->ResetKeywordIfNecessary(false);

  // Check whether |template_url|'s keyword conflicts with any already in the
  // model.  Note that we can reach here during the loading phase while
  // processing the template URLs from the web data service.  In this case,
  // GetTemplateURLForKeyword() will look not only at what's already in the
  // model, but at the |initial_default_search_provider_|.  Since this engine
  // will presumably also be present in the web data, we need to double-check
  // that any "pre-existing" entries we find are actually coming from
  // |template_urls_|, lest we detect a "conflict" between the
  // |initial_default_search_provider_| and the web data version of itself.
  TemplateURL* existing_keyword_turl =
      GetTemplateURLForKeyword(template_url->keyword());
  if (existing_keyword_turl &&
      (std::find(template_urls_.begin(), template_urls_.end(),
                 existing_keyword_turl) != template_urls_.end())) {
    // We have a conflict.  Only replace one of the TemplateURLs if they are
    // either both extensions, or both not extensions.
    DCHECK_NE(existing_keyword_turl, template_url);
    bool are_same_type = ...

     DCHECK_NE(existing_keyword_turl, template_url);
     // Only replace one of the TemplateURLs if they are either both extensions,
     // or both not extensions.
@@ -2118,7 +2138,7 @@ void TemplateURLService::RemoveNoNotify(TemplateURL* template_url) {
                               DELETE_ENGINE_USER_ACTION, DELETE_ENGINE_MAX);
   }
 
-  if (profile_) {
+  if (loaded_ && profile_) {
     content::Source<Profile> source(profile_);
     TemplateURLID id = template_url->id();
     content::NotificationService::current()->Notify(
@@ -2136,8 +2156,6 @@ bool TemplateURLService::ResetTemplateURLNoNotify(
     const base::string16& title,
     const base::string16& keyword,
     const std::string& search_url) {
-  if (!loaded_)
-    return false;
   DCHECK(!keyword.empty());
   DCHECK(!search_url.empty());
   TemplateURLData data(url->data());