Use the DefaultSearchManager as the exclusive authority on DSE, ignoring Web Data.

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 64f70b86d5a860c177fe56107bcd24c543ed0306..0c2f3733949b0a766fca56bf57d9f591e69900c3 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -17,7 +17,6 @@
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
-#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -51,12 +50,20 @@
 #include "sync/protocol/search_engine_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "url/gurl.h"
 
 typedef SearchHostToURLsMap::TemplateURLSet TemplateURLSet;
 typedef TemplateURLService::SyncDataMap SyncDataMap;
 
 namespace {
 
+bool IdenticalSyncGUIDs(const TemplateURLData* data, const TemplateURL* turl) {
+  if (!data || !turl)
+    return !data && !turl;
+
+  return data->sync_guid == turl->sync_guid();
+}
+
 bool TemplateURLMatchesData(const TemplateURL* url1,
                             const TemplateURLData* url2) {
   if (!url1 || !url2)
@@ -83,20 +90,6 @@ bool TemplateURLMatchesData(const TemplateURL* url1,
           url2->search_terms_replacement_key);
 }
 
-const char kFirstPotentialEngineHistogramName[] =
-    "Search.FirstPotentialEngineCalled";
-
-// Values for an enumerated histogram used to track whenever
-// FirstPotentialDefaultEngine is called, and from where.
-enum FirstPotentialEngineCaller {
-  FIRST_POTENTIAL_CALLSITE_FIND_NEW_DSP,
-  FIRST_POTENTIAL_CALLSITE_FIND_NEW_DSP_PROCESSING_SYNC_CHANGES,
-  FIRST_POTENTIAL_CALLSITE_ON_LOAD,
-  FIRST_POTENTIAL_CALLSITE_FIND_NEW_DSP_SYNCING,
-  FIRST_POTENTIAL_CALLSITE_FIND_NEW_DSP_NOT_SYNCING,
-  FIRST_POTENTIAL_CALLSITE_MAX,
-};
-
 const char kDeleteSyncedEngineHistogramName[] =
     "Search.DeleteSyncedSearchEngine";
 
@@ -109,17 +102,6 @@ enum DeleteSyncedSearchEngineEvent {
   DELETE_ENGINE_MAX,
 };
 
-TemplateURL* FirstPotentialDefaultEngine(
-    const TemplateURLService::TemplateURLVector& template_urls) {
-  for (TemplateURLService::TemplateURLVector::const_iterator i(
-       template_urls.begin()); i != template_urls.end(); ++i) {
-    if ((*i)->ShowInDefaultList() &&
-        ((*i)->GetType() == TemplateURL::NORMAL))
-      return *i;
-  }
-  return NULL;
-}
-
 // Returns true iff the change in |change_list| at index |i| should not be sent
 // up to the server based on its GUIDs presence in |sync_data| or when compared
 // to changes after it in |change_list|.
@@ -273,6 +255,8 @@ class TemplateURLService::LessWithPrefix {
   }
 };
 
+// static
+bool TemplateURLService::fallback_search_engines_disabled_ = false;

[Peter Kasting, 2014/05/07 23:38:29]

Nit: This belongs below the subsequent divider, not above it

[erikwright (departed), 2014/05/08 12:46:24]

Done.

 
 // TemplateURLService ---------------------------------------------------------
 
@@ -283,16 +267,14 @@ TemplateURLService::TemplateURLService(Profile* profile)
       load_failed_(false),
       load_handle_(0),
       default_search_provider_(NULL),
-      is_default_search_managed_(false),
       next_id_(kInvalidTemplateURLID + 1),
       time_provider_(&base::Time::Now),
       models_associated_(false),
       processing_syncer_changes_(false),
-      pending_synced_default_search_(false),
-      dsp_change_origin_(DSP_CHANGE_OTHER),
       default_search_manager_(
-          new DefaultSearchManager(GetPrefs(),
-                                   DefaultSearchManager::ObserverCallback())) {
+          GetPrefs(),
+          base::Bind(&TemplateURLService::OnDefaultSearchChange,
+                     base::Unretained(this))) {
   DCHECK(profile_);
   Init(NULL, 0);
 }
@@ -306,13 +288,14 @@ TemplateURLService::TemplateURLService(const Initializer* initializers,
       load_handle_(0),
       service_(NULL),
       default_search_provider_(NULL),
-      is_default_search_managed_(false),
       next_id_(kInvalidTemplateURLID + 1),
       time_provider_(&base::Time::Now),
       models_associated_(false),
       processing_syncer_changes_(false),
-      pending_synced_default_search_(false),
-      dsp_change_origin_(DSP_CHANGE_OTHER) {
+      default_search_manager_(
+          GetPrefs(),
+          base::Bind(&TemplateURLService::OnDefaultSearchChange,
+                     base::Unretained(this))) {
   Init(initializers, count);
 }
 
@@ -323,103 +306,6 @@ TemplateURLService::~TemplateURLService() {
 }
 
 // static
-bool TemplateURLService::LoadDefaultSearchProviderFromPrefs(
-    PrefService* prefs,
-    scoped_ptr<TemplateURLData>* default_provider_data,
-    bool* is_managed) {
-  if (!prefs || !prefs->HasPrefPath(prefs::kDefaultSearchProviderSearchURL) ||
-      !prefs->HasPrefPath(prefs::kDefaultSearchProviderKeyword))
-    return false;
-
-  const PrefService::Preference* pref =
-      prefs->FindPreference(prefs::kDefaultSearchProviderSearchURL);
-  *is_managed = pref && pref->IsManaged();
-
-  if (!prefs->GetBoolean(prefs::kDefaultSearchProviderEnabled)) {
-    // The user doesn't want a default search provider.
-    default_provider_data->reset(NULL);
-    return true;
-  }
-
-  base::string16 name =
-      base::UTF8ToUTF16(prefs->GetString(prefs::kDefaultSearchProviderName));
-  base::string16 keyword =
-      base::UTF8ToUTF16(prefs->GetString(prefs::kDefaultSearchProviderKeyword));
-  if (keyword.empty())
-    return false;
-  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 =
-      prefs->GetString(prefs::kDefaultSearchProviderInstantURL);
-  std::string image_url =
-      prefs->GetString(prefs::kDefaultSearchProviderImageURL);
-  std::string new_tab_url =
-      prefs->GetString(prefs::kDefaultSearchProviderNewTabURL);
-  std::string search_url_post_params =
-      prefs->GetString(prefs::kDefaultSearchProviderSearchURLPostParams);
-  std::string suggest_url_post_params =
-      prefs->GetString(prefs::kDefaultSearchProviderSuggestURLPostParams);
-  std::string instant_url_post_params =
-      prefs->GetString(prefs::kDefaultSearchProviderInstantURLPostParams);
-  std::string image_url_post_params =
-      prefs->GetString(prefs::kDefaultSearchProviderImageURLPostParams);
-  std::string icon_url =
-      prefs->GetString(prefs::kDefaultSearchProviderIconURL);
-  std::string encodings =
-      prefs->GetString(prefs::kDefaultSearchProviderEncodings);
-  std::string id_string = prefs->GetString(prefs::kDefaultSearchProviderID);
-  std::string prepopulate_id =
-      prefs->GetString(prefs::kDefaultSearchProviderPrepopulateID);
-  const base::ListValue* alternate_urls =
-      prefs->GetList(prefs::kDefaultSearchProviderAlternateURLs);
-  std::string search_terms_replacement_key = prefs->GetString(
-      prefs::kDefaultSearchProviderSearchTermsReplacementKey);
-
-  default_provider_data->reset(new TemplateURLData);
-  (*default_provider_data)->short_name = name;
-  (*default_provider_data)->SetKeyword(keyword);
-  (*default_provider_data)->SetURL(search_url);
-  (*default_provider_data)->suggestions_url = suggest_url;
-  (*default_provider_data)->instant_url = instant_url;
-  (*default_provider_data)->image_url = image_url;
-  (*default_provider_data)->new_tab_url = new_tab_url;
-  (*default_provider_data)->search_url_post_params = search_url_post_params;
-  (*default_provider_data)->suggestions_url_post_params =
-      suggest_url_post_params;
-  (*default_provider_data)->instant_url_post_params = instant_url_post_params;
-  (*default_provider_data)->image_url_post_params = image_url_post_params;
-  (*default_provider_data)->favicon_url = GURL(icon_url);
-  (*default_provider_data)->show_in_default_list = true;
-  (*default_provider_data)->alternate_urls.clear();
-  for (size_t i = 0; i < alternate_urls->GetSize(); ++i) {
-    std::string alternate_url;
-    if (alternate_urls->GetString(i, &alternate_url))
-      (*default_provider_data)->alternate_urls.push_back(alternate_url);
-  }
-  (*default_provider_data)->search_terms_replacement_key =
-      search_terms_replacement_key;
-  base::SplitString(encodings, ';', &(*default_provider_data)->input_encodings);
-  if (!id_string.empty() && !*is_managed) {
-    int64 value;
-    base::StringToInt64(id_string, &value);
-    (*default_provider_data)->id = value;
-  }
-  if (!prepopulate_id.empty() && !*is_managed) {
-    int value;
-    base::StringToInt(prepopulate_id, &value);
-    (*default_provider_data)->prepopulate_id = value;
-  }
-  return true;
-}
-
-// static
 base::string16 TemplateURLService::GenerateKeyword(const GURL& url) {
   DCHECK(url.is_valid());
   // Strip "www." off the front of the keyword; otherwise the keyword won't work
@@ -497,77 +383,9 @@ GURL TemplateURLService::GenerateSearchURLUsingTermsData(
       search_terms_data, NULL));
 }
 
-void TemplateURLService::SaveDefaultSearchProviderToPrefs(
-    const TemplateURL* t_url,
-    PrefService* prefs) const {
-  if (!prefs || load_failed_)
-    return;
-
-  bool enabled = false;
-  std::string search_url;
-  std::string suggest_url;
-  std::string instant_url;
-  std::string image_url;
-  std::string new_tab_url;
-  std::string search_url_post_params;
-  std::string suggest_url_post_params;
-  std::string instant_url_post_params;
-  std::string image_url_post_params;
-  std::string icon_url;
-  std::string encodings;
-  std::string short_name;
-  std::string keyword;
-  std::string id_string;
-  std::string prepopulate_id;
-  base::ListValue alternate_urls;
-  std::string search_terms_replacement_key;
-  if (t_url) {
-    DCHECK_EQ(TemplateURL::NORMAL, t_url->GetType());
-    enabled = true;
-    search_url = t_url->url();
-    suggest_url = t_url->suggestions_url();
-    instant_url = t_url->instant_url();
-    image_url = t_url->image_url();
-    new_tab_url = t_url->new_tab_url();
-    search_url_post_params = t_url->search_url_post_params();
-    suggest_url_post_params = t_url->suggestions_url_post_params();
-    instant_url_post_params = t_url->instant_url_post_params();
-    image_url_post_params = t_url->image_url_post_params();
-    GURL icon_gurl = t_url->favicon_url();
-    if (!icon_gurl.is_empty())
-      icon_url = icon_gurl.spec();
-    encodings = JoinString(t_url->input_encodings(), ';');
-    short_name = base::UTF16ToUTF8(t_url->short_name());
-    keyword = base::UTF16ToUTF8(t_url->keyword());
-    id_string = base::Int64ToString(t_url->id());
-    prepopulate_id = base::Int64ToString(t_url->prepopulate_id());
-    for (size_t i = 0; i < t_url->alternate_urls().size(); ++i)
-      alternate_urls.AppendString(t_url->alternate_urls()[i]);
-    search_terms_replacement_key = t_url->search_terms_replacement_key();
-  }
-  prefs->SetBoolean(prefs::kDefaultSearchProviderEnabled, enabled);
-  prefs->SetString(prefs::kDefaultSearchProviderSearchURL, search_url);
-  prefs->SetString(prefs::kDefaultSearchProviderSuggestURL, suggest_url);
-  prefs->SetString(prefs::kDefaultSearchProviderInstantURL, instant_url);
-  prefs->SetString(prefs::kDefaultSearchProviderImageURL, image_url);
-  prefs->SetString(prefs::kDefaultSearchProviderNewTabURL, new_tab_url);
-  prefs->SetString(prefs::kDefaultSearchProviderSearchURLPostParams,
-                   search_url_post_params);
-  prefs->SetString(prefs::kDefaultSearchProviderSuggestURLPostParams,
-                   suggest_url_post_params);
-  prefs->SetString(prefs::kDefaultSearchProviderInstantURLPostParams,
-                   instant_url_post_params);
-  prefs->SetString(prefs::kDefaultSearchProviderImageURLPostParams,
-                   image_url_post_params);
-  prefs->SetString(prefs::kDefaultSearchProviderIconURL, icon_url);
-  prefs->SetString(prefs::kDefaultSearchProviderEncodings, encodings);
-  prefs->SetString(prefs::kDefaultSearchProviderName, short_name);
-  prefs->SetString(prefs::kDefaultSearchProviderKeyword, keyword);
-  prefs->SetString(prefs::kDefaultSearchProviderID, id_string);
-  prefs->SetString(prefs::kDefaultSearchProviderPrepopulateID, prepopulate_id);
-  prefs->Set(prefs::kDefaultSearchProviderAlternateURLs, alternate_urls);
-  prefs->SetString(prefs::kDefaultSearchProviderSearchTermsReplacementKey,
-      search_terms_replacement_key);
+// static
+void TemplateURLService::DisableFallbackSearchEngines() {
+  fallback_search_engines_disabled_ = true;
 }
 
 bool TemplateURLService::CanReplaceKeyword(
@@ -598,7 +416,7 @@ bool TemplateURLService::CanReplaceKeyword(
 void TemplateURLService::FindMatchingKeywords(
     const base::string16& prefix,
     bool support_replacement_only,
-    TemplateURLVector* matches) const {
+    TemplateURLVector* matches) {
   // Sanity check args.
   if (prefix.empty())
     return;
@@ -632,8 +450,7 @@ TemplateURL* TemplateURLService::GetTemplateURLForKeyword(
       keyword_to_template_map_.find(keyword));
   if (elem != keyword_to_template_map_.end())
     return elem->second;
-  return ((!loaded_ || load_failed_) &&
-      initial_default_search_provider_.get() &&
+  return (!loaded_ && initial_default_search_provider_.get() &&
       (initial_default_search_provider_->keyword() == keyword)) ?
       initial_default_search_provider_.get() : NULL;
 }
@@ -643,8 +460,7 @@ TemplateURL* TemplateURLService::GetTemplateURLForGUID(
   GUIDToTemplateMap::const_iterator elem(guid_to_template_map_.find(sync_guid));
   if (elem != guid_to_template_map_.end())
     return elem->second;
-  return ((!loaded_ || load_failed_) &&
-      initial_default_search_provider_.get() &&
+  return (!loaded_ && initial_default_search_provider_.get() &&
       (initial_default_search_provider_->sync_guid() == sync_guid)) ?
       initial_default_search_provider_.get() : NULL;
 }
@@ -656,16 +472,17 @@ TemplateURL* TemplateURLService::GetTemplateURLForHost(
     if (t_url)
       return t_url;
   }
-  return ((!loaded_ || load_failed_) &&
-      initial_default_search_provider_.get() &&
+  return (!loaded_ && 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) {
+bool TemplateURLService::Add(TemplateURL* template_url) {
   WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
-  if (AddNoNotify(template_url, true))
+  bool result = AddNoNotify(template_url, true);
+  if (result)
     NotifyObservers();
+  return result;

[Peter Kasting, 2014/05/07 23:38:29]

Nit: Eliminates unnecessary temp:

  if (!AddNoNotify(template_url, true))
    return false;
  NotifyObservers();
  return true;

[erikwright (departed), 2014/05/08 12:46:24]

Done.

 }
 
 void TemplateURLService::AddAndSetProfile(TemplateURL* template_url,
@@ -702,18 +519,8 @@ void TemplateURLService::AddExtensionControlledTURL(
 
   WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
   if (AddNoNotify(template_url, true)) {
-    // Note that we can't call CanMakeDefault() here, since it would return
-    // false when another extension is already controlling the default search
-    // engine, and we want to allow new extensions to take over.
-    if (template_url->extension_info_->wants_to_be_default_engine &&
-        !is_default_search_managed()) {
-      TemplateURL* default_candidate = FindExtensionDefaultSearchEngine();
-      if (default_candidate == template_url) {
-        base::AutoReset<DefaultSearchChangeOrigin> change_origin(
-            &dsp_change_origin_, DSP_CHANGE_OVERRIDE_SETTINGS_EXTENSION);
-        SetDefaultSearchProviderNoNotify(template_url);
-      }
-    }
+    if (template_url->extension_info_->wants_to_be_default_engine)
+      UpdateExtensionDefaultSearchEngine();
     NotifyObservers();
   }
 }
@@ -730,15 +537,13 @@ void TemplateURLService::RemoveExtensionControlledTURL(
       extension_id, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
   if (!url)
     return;
-  bool restore_dse = (url == GetDefaultSearchProvider());
-  if (restore_dse) {
-    DCHECK(!is_default_search_managed());
+  // NULL this out so that we can call RemoveNoNotify. The subsequent
+  // UpdateExtensionDefaultSearchEngine will cause it to be reset.
+  if (default_search_provider_ == url)
     default_search_provider_ = NULL;
-  }
   WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
   RemoveNoNotify(url);
-  if (restore_dse)
-    SetDefaultSearchProviderAfterRemovingDefaultExtension();
+  UpdateExtensionDefaultSearchEngine();
   NotifyObservers();
 }
 
@@ -834,33 +639,38 @@ void TemplateURLService::ResetTemplateURL(TemplateURL* url,
   }
   data.safe_for_autoreplace = false;
   data.last_modified = time_provider_();
-  TemplateURL new_url(url->profile(), data);
+
   UIThreadSearchTermsData search_terms_data(url->profile());
-  if (UpdateNoNotify(url, new_url, search_terms_data))
+  if (UpdateNoNotify(url, data, search_terms_data))
     NotifyObservers();
 }
 
 bool TemplateURLService::CanMakeDefault(const TemplateURL* url) {
-  return  !is_default_search_managed() &&
-      !IsExtensionControlledDefaultSearch() &&
-      (url != GetDefaultSearchProvider()) &&
-      url->url_ref().SupportsReplacement() &&
-      (url->GetType() == TemplateURL::NORMAL);
+  return ((default_search_provider_source_ ==
+           DefaultSearchManager::FROM_USER) ||
+          (default_search_provider_source_ ==
+           DefaultSearchManager::FROM_FALLBACK)) &&
+         (url != GetDefaultSearchProvider()) &&
+         url->url_ref().SupportsReplacement() &&
+         (url->GetType() == TemplateURL::NORMAL);

[Peter Kasting, 2014/05/07 23:38:29]

Nit: Weird indenting... normally, lines after the first should normally be
indented 4, not even, though in this case I'd probably do:

  return
      ((default_search_provider_source_ == DefaultSearchManager::FROM_USER) ||
       (default_search_provider_source_ ==
           DefaultSearchManager::FROM_FALLBACK)) &&
      (url != GetDefaultSearchProvider()) &&
      url->url_ref().SupportsReplacement() &&
      (url->GetType() == TemplateURL::NORMAL);

[erikwright (departed), 2014/05/08 12:46:24]

Done.

 }
 
 void TemplateURLService::SetUserSelectedDefaultSearchProvider(
     TemplateURL* url) {
-  SetDefaultSearchProvider(url);
-  if (default_search_manager_) {
-    if (url)
-      default_search_manager_->SetUserSelectedDefaultSearchEngine(url->data());
-    else
-      default_search_manager_->ClearUserSelectedDefaultSearchEngine();
-  }
+  // Omnibox keywords cannot be made default. Extension-controlled search
+  // engines can be made default only by the extension itself because they
+  // aren't persisted.
+  DCHECK(!url || (url->GetType() == TemplateURL::NORMAL));
+  // We rely on the DefaultSearchManager to push this back to us if, in fact,
+  // the effective DSE changes.
+  if (url)
+    default_search_manager_.SetUserSelectedDefaultSearchEngine(url->data());
+  else
+    default_search_manager_.ClearUserSelectedDefaultSearchEngine();
 }
 
 TemplateURL* TemplateURLService::GetDefaultSearchProvider() {
-  return (loaded_ && !load_failed_) ?
+  return loaded_ ?
       default_search_provider_ : initial_default_search_provider_.get();
 }
 
@@ -871,59 +681,28 @@ bool TemplateURLService::IsSearchResultsPageFromDefaultSearchProvider(
 }
 
 bool TemplateURLService::IsExtensionControlledDefaultSearch() {
-  const TemplateURL* default_provider = GetDefaultSearchProvider();
-  return default_provider && (default_provider->GetType() ==
-      TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
-}
-
-TemplateURL* TemplateURLService::FindNewDefaultSearchProvider() {
-  // See if the prepopulated default still exists.
-  scoped_ptr<TemplateURLData> prepopulated_default =
-      TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(GetPrefs());
-
-  for (TemplateURLVector::iterator i = template_urls_.begin();
-       i != template_urls_.end(); ++i) {
-    if ((*i)->prepopulate_id() == prepopulated_default->prepopulate_id)
-      return *i;
-  }
-  // If not, use the first non-extension keyword of the templates that supports
-  // search term replacement.
-  if (processing_syncer_changes_) {
-    UMA_HISTOGRAM_ENUMERATION(
-        kFirstPotentialEngineHistogramName,
-        FIRST_POTENTIAL_CALLSITE_FIND_NEW_DSP_PROCESSING_SYNC_CHANGES,
-        FIRST_POTENTIAL_CALLSITE_MAX);
-  } else {
-    if (sync_processor_.get()) {
-      // We're not currently in a sync cycle, but we're syncing.
-      UMA_HISTOGRAM_ENUMERATION(kFirstPotentialEngineHistogramName,
-                                FIRST_POTENTIAL_CALLSITE_FIND_NEW_DSP_SYNCING,
-                                FIRST_POTENTIAL_CALLSITE_MAX);
-    } else {
-      // We're not syncing at all.
-      UMA_HISTOGRAM_ENUMERATION(
-          kFirstPotentialEngineHistogramName,
-          FIRST_POTENTIAL_CALLSITE_FIND_NEW_DSP_NOT_SYNCING,
-          FIRST_POTENTIAL_CALLSITE_MAX);
-    }
-  }
-  return FirstPotentialDefaultEngine(template_urls_);
+  return default_search_provider_source_ ==
+         DefaultSearchManager::FROM_EXTENSION;

[Peter Kasting, 2014/05/07 23:38:29]

Nit: This should be indented 4, not 7

[erikwright (departed), 2014/05/08 12:46:24]

Done.

 }
 
 void TemplateURLService::RepairPrepopulatedSearchEngines() {
   // Can't clean DB if it hasn't been loaded.
   DCHECK(loaded());
 
+  if (default_search_provider_source_ == DefaultSearchManager::FROM_USER ||
+      default_search_provider_source_ == DefaultSearchManager::FROM_FALLBACK) {

[Peter Kasting, 2014/05/07 23:38:29]

Nit: Parens around binary subexprs (file style)  (many places)

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+    // Clear this so that we can remove what it points to (if it's a
+    // prepopulated engine). No matter what we will reset it at the end anyway.

[Peter Kasting, 2014/05/07 23:38:29]

Nit: How about:

Clear |default_search_provider_| in case we want to remove the engine it points
to.  This will get reset at the end of the function anyway.

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+    default_search_provider_ = NULL;
+  }
+
   size_t default_search_provider_index = 0;
   ScopedVector<TemplateURLData> prepopulated_urls =
       TemplateURLPrepopulateData::GetPrepopulatedEngines(
           GetPrefs(), &default_search_provider_index);
   DCHECK(!prepopulated_urls.empty());
-  int default_search_engine_id =
-      prepopulated_urls[default_search_provider_index]->prepopulate_id;
-  TemplateURL* current_dse = default_search_provider_;
   ActionsFromPrepopulateData actions(CreateActionsFromCurrentPrepopulateData(
-      &prepopulated_urls, template_urls_, current_dse));
+      &prepopulated_urls, template_urls_, default_search_provider_));
 
   WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
 
@@ -936,8 +715,7 @@ void TemplateURLService::RepairPrepopulatedSearchEngines() {
   for (EditedEngines::iterator i(actions.edited_engines.begin());
        i < actions.edited_engines.end(); ++i) {
     UIThreadSearchTermsData search_terms_data(profile());
-    TemplateURL new_values(profile(), i->second);
-    UpdateNoNotify(i->first, new_values, search_terms_data);
+    UpdateNoNotify(i->first, i->second, search_terms_data);
   }
 
   // Add items.
@@ -948,16 +726,17 @@ void TemplateURLService::RepairPrepopulatedSearchEngines() {
     AddNoNotify(new TemplateURL(profile_, *i), true);
   }
 
-  // Change the DSE.
-  TemplateURL* new_dse = FindURLByPrepopulateID(template_urls_,
-                                                default_search_engine_id);
-  DCHECK(new_dse);
-  if (CanMakeDefault(new_dse)) {
-    base::AutoReset<DefaultSearchChangeOrigin> change_origin(
-        &dsp_change_origin_, DSP_CHANGE_PROFILE_RESET);
-    SetDefaultSearchProviderNoNotify(new_dse);
+  default_search_manager_.ClearUserSelectedDefaultSearchEngine();
+
+  if (!default_search_provider_) {
+    DefaultSearchManager::Source source;
+    const TemplateURLData* new_dse =
+        default_search_manager_.GetDefaultSearchEngine(&source);
+    // ApplyDefaultSearchChange will notify observers once it is done.
+    ApplyDefaultSearchChange(new_dse, source);
+  } else {
+    NotifyObservers();
   }
-  NotifyObservers();
 }
 
 void TemplateURLService::AddObserver(TemplateURLServiceObserver* observer) {
@@ -975,12 +754,10 @@ void TemplateURLService::Load() {
   if (!service_)
     service_ = WebDataService::FromBrowserContext(profile_);
 
-  if (service_) {
+  if (service_)
     load_handle_ = service_->GetKeywords(this);
-  } else {
-    ChangeToLoadedState();
-    on_loaded_callbacks_.Notify();
-  }
+  else
+    OnFailedLoad();

[Peter Kasting, 2014/05/07 23:38:29]

This should no longer cause a load failure (see latest version of my patch).

[erikwright (departed), 2014/05/08 12:46:24]

Done.

 }
 
 scoped_ptr<TemplateURLService::Subscription>
@@ -1001,30 +778,30 @@ void TemplateURLService::OnWebDataServiceRequestDone(
   if (!result) {
     // Results are null if the database went away or (most likely) wasn't
     // loaded.
-    load_failed_ = true;
-    service_ = NULL;
-    ChangeToLoadedState();
-    on_loaded_callbacks_.Notify();
+    OnFailedLoad();
     return;
   }
 
-  // initial_default_search_provider_ is only needed before we've finished
-  // loading. Now that we've loaded we can nuke it.
-  initial_default_search_provider_.reset();
-
   TemplateURLVector template_urls;
-  TemplateURL* default_search_provider = NULL;
   int new_resource_keyword_version = 0;
-  GetSearchProvidersUsingKeywordResult(*result, service_.get(), profile_,
-      &template_urls, &default_search_provider, &new_resource_keyword_version,
+  GetSearchProvidersUsingKeywordResult(
+      *result,
+      service_.get(),
+      profile_,
+      &template_urls,
+      (default_search_provider_source_ == DefaultSearchManager::FROM_USER) ?
+          initial_default_search_provider_.get() : NULL,
+      &new_resource_keyword_version,
       &pre_sync_deletes_);
 
   WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
 
-  AddTemplateURLsAndSetupDefaultEngine(&template_urls, default_search_provider);
+  PatchMissingSyncGUIDs(&template_urls);
+  SetTemplateURLs(&template_urls);
 
   // This initializes provider_map_ which should be done before
   // calling UpdateKeywordSearchTermsForURL.
+  // This also calls NotifyObservers.
   ChangeToLoadedState();
 
   // Index any visits that occurred before we finished loading.
@@ -1034,11 +811,6 @@ void TemplateURLService::OnWebDataServiceRequestDone(
 
   if (new_resource_keyword_version)
     service_->SetBuiltinKeywordVersion(new_resource_keyword_version);
-
-  EnsureDefaultSearchProviderExists();
-
-  NotifyObservers();
-  on_loaded_callbacks_.Notify();
 }
 
 base::string16 TemplateURLService::GetKeywordShortName(
@@ -1066,12 +838,6 @@ void TemplateURLService::Observe(int type,
       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 {
     DCHECK_EQ(chrome::NOTIFICATION_GOOGLE_URL_UPDATED, type);
     if (loaded_) {
@@ -1092,27 +858,6 @@ void TemplateURLService::Shutdown() {
   service_ = NULL;
 }
 
-void TemplateURLService::OnSyncedDefaultSearchProviderGUIDChanged() {
-  // Listen for changes to the default search from Sync.
-  PrefService* prefs = GetPrefs();
-  TemplateURL* new_default_search = GetTemplateURLForGUID(
-      prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID));
-  if (new_default_search && !is_default_search_managed_) {
-    if (new_default_search != GetDefaultSearchProvider()) {
-      base::AutoReset<DefaultSearchChangeOrigin> change_origin(
-          &dsp_change_origin_, DSP_CHANGE_SYNC_PREF);
-      SetUserSelectedDefaultSearchProvider(new_default_search);
-      pending_synced_default_search_ = false;
-    }
-  } else {
-    // If it's not there, or if default search is currently managed, set a
-    // flag to indicate that we waiting on the search engine entry to come
-    // in through Sync.
-    pending_synced_default_search_ = true;
-  }
-  UpdateDefaultSearch();
-}
-
 syncer::SyncDataList TemplateURLService::GetAllSyncData(
     syncer::ModelType type) const {
   DCHECK_EQ(syncer::SEARCH_ENGINES, type);
@@ -1146,12 +891,6 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
 
   base::AutoReset<bool> processing_changes(&processing_syncer_changes_, true);
 
-  // We've started syncing, so set our origin member to the base Sync value.
-  // As we move through Sync Code, we may set this to increasingly specific
-  // origins so we can tell what exactly caused a DSP change.
-  base::AutoReset<DefaultSearchChangeOrigin> change_origin(&dsp_change_origin_,
-      DSP_CHANGE_SYNC_UNINTENTIONAL);
-
   WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
 
   syncer::SyncChangeList new_changes;
@@ -1200,16 +939,16 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
         base::string16 updated_keyword = UniquifyKeyword(*existing_turl, true);
         TemplateURLData data(existing_turl->data());
         data.SetKeyword(updated_keyword);
-        TemplateURL new_turl(existing_turl->profile(), data);
         UIThreadSearchTermsData search_terms_data(existing_turl->profile());
-        if (UpdateNoNotify(existing_turl, new_turl, search_terms_data))
+        if (UpdateNoNotify(existing_turl, data, search_terms_data))
           NotifyObservers();
 
-        syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(new_turl);
+        syncer::SyncData sync_data =
+            CreateSyncDataFromTemplateURL(*existing_turl);
         new_changes.push_back(syncer::SyncChange(FROM_HERE,
                                                  syncer::SyncChange::ACTION_ADD,
                                                  sync_data));
-        // Ignore the delete attempt. This means we never end up reseting the
+        // Ignore the delete attempt. This means we never end up resetting the
         // default search provider due to an ACTION_DELETE from sync.
         continue;
       }
@@ -1231,10 +970,9 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
       // 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);
+      TemplateURL* added = new TemplateURL(profile_, data);
+      if (Add(added))
+        MaybeUpdateDSEAfterSync(added);
     } else if (iter->change_type() == syncer::SyncChange::ACTION_UPDATE) {
       if (!existing_turl) {
         error = sync_error_factory_->CreateAndUploadError(
@@ -1249,8 +987,10 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
                                    &new_changes);
       }
       UIThreadSearchTermsData search_terms_data(existing_turl->profile());
-      if (UpdateNoNotify(existing_turl, *turl, search_terms_data))
+      if (UpdateNoNotify(existing_turl, turl->data(), search_terms_data)) {
         NotifyObservers();
+        MaybeUpdateDSEAfterSync(existing_turl);
+      }
     } else {
       // We've unexpectedly received an ACTION_INVALID.
       error = sync_error_factory_->CreateAndUploadError(
@@ -1292,28 +1032,10 @@ syncer::SyncMergeResult TemplateURLService::MergeDataAndStartSyncing(
   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()) {
-    std::string default_guid = GetPrefs()->GetString(
-        prefs::kSyncedDefaultSearchProviderGUID);
-    const TemplateURL* current_default = GetDefaultSearchProvider();
-
-    if (!default_guid.empty() &&
-        (!current_default || current_default->sync_guid() != default_guid))
-      pending_synced_default_search_ = true;
-  }
-
   // We do a lot of calls to Add/Remove/ResetTemplateURL here, so ensure we
   // don't step on our own toes.
   base::AutoReset<bool> processing_changes(&processing_syncer_changes_, true);
 
-  // We've started syncing, so set our origin member to the base Sync value.
-  // As we move through Sync Code, we may set this to increasingly specific
-  // origins so we can tell what exactly caused a DSP change.
-  base::AutoReset<DefaultSearchChangeOrigin> change_origin(&dsp_change_origin_,
-      DSP_CHANGE_SYNC_UNINTENTIONAL);
-
   syncer::SyncChangeList new_changes;
 
   // Build maps of our sync GUIDs to syncer::SyncData.
@@ -1359,7 +1081,7 @@ syncer::SyncMergeResult TemplateURLService::MergeDataAndStartSyncing(
         // also makes the local data's last_modified timestamp equal to Sync's,
         // avoiding an Update on the next MergeData call.
         UIThreadSearchTermsData search_terms_data(local_turl->profile());
-        if (UpdateNoNotify(local_turl, *sync_turl, search_terms_data))
+        if (UpdateNoNotify(local_turl, sync_turl->data(), search_terms_data))
           NotifyObservers();
         merge_result.set_num_items_modified(
             merge_result.num_items_modified() + 1);
@@ -1382,15 +1104,6 @@ syncer::SyncMergeResult TemplateURLService::MergeDataAndStartSyncing(
     }
   }
 
-  // If there is a pending synced default search provider that was processed
-  // above, set it now.
-  TemplateURL* pending_default = GetPendingSyncedDefaultSearchProvider();
-  if (pending_default) {
-    base::AutoReset<DefaultSearchChangeOrigin> change_origin(
-        &dsp_change_origin_, DSP_CHANGE_SYNC_ADD);
-    SetUserSelectedDefaultSearchProvider(pending_default);
-  }
-
   // 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();
@@ -1624,6 +1337,14 @@ void TemplateURLService::SetKeywordSearchTermsForURL(
 
 void TemplateURLService::Init(const Initializer* initializers,
                               int num_initializers) {
+  if (GetPrefs()) {
+    pref_change_registrar_.Init(GetPrefs());
+    pref_change_registrar_.Add(
+        prefs::kSyncedDefaultSearchProviderGUID,
+        base::Bind(&TemplateURLService::OnDSEGUIDPrefChanged,
+                   base::Unretained(this)));
+  }
+
   // Register for notifications.
   if (profile_) {
     // TODO(sky): bug 1166191. The keywords should be moved into the history
@@ -1635,16 +1356,12 @@ void TemplateURLService::Init(const Initializer* initializers,
                                 profile_source);
     notification_registrar_.Add(this, chrome::NOTIFICATION_GOOGLE_URL_UPDATED,
                                 profile_source);
-    pref_change_registrar_.Init(GetPrefs());
-    pref_change_registrar_.Add(
-        prefs::kSyncedDefaultSearchProviderGUID,
-        base::Bind(
-            &TemplateURLService::OnSyncedDefaultSearchProviderGUIDChanged,
-            base::Unretained(this)));
   }
-  notification_registrar_.Add(
-      this, chrome::NOTIFICATION_DEFAULT_SEARCH_POLICY_CHANGED,
-      content::NotificationService::AllSources());
+
+  DefaultSearchManager::Source source = DefaultSearchManager::FROM_USER;
+  TemplateURLData* dse =
+      default_search_manager_.GetDefaultSearchEngine(&source);
+  ApplyDefaultSearchChange(dse, source);
 
   if (num_initializers > 0) {
     // This path is only hit by test code and is used to simulate a loaded
@@ -1669,17 +1386,15 @@ void TemplateURLService::Init(const Initializer* initializers,
 
       // Set the first provided identifier to be the default.
       if (i == 0)
-        SetDefaultSearchProviderNoNotify(template_url);
+        default_search_manager_.SetUserSelectedDefaultSearchEngine(data);
     }
   }
 
-  // Initialize default search.
-  UpdateDefaultSearch();
-
   // Request a server check for the correct Google URL if Google is the
   // default search engine and not in headless mode.
-  if (profile_ && initial_default_search_provider_.get() &&
-      initial_default_search_provider_->url_ref().HasGoogleBaseURLs()) {
+  TemplateURL* default_search_provider = GetDefaultSearchProvider();
+  if (profile_ && default_search_provider &&
+      default_search_provider->url_ref().HasGoogleBaseURLs()) {
     scoped_ptr<base::Environment> env(base::Environment::Create());
     if (!env->HasVar(env_vars::kHeadless))
       GoogleURLTracker::RequestServerCheck(profile_, false);
@@ -1792,16 +1507,22 @@ void TemplateURLService::ChangeToLoadedState() {
   UIThreadSearchTermsData search_terms_data(profile_);
   provider_map_->Init(template_urls_, search_terms_data);
   loaded_ = true;
+
+  // This will cause a call to NotifyObservers().
+  ApplyDefaultSearchChange(initial_default_search_provider_
+                               ? &initial_default_search_provider_->data()
+                               : NULL,

[Peter Kasting, 2014/05/07 23:38:29]

Nit: Format as:

  ApplyDefaultSearchChange(
      initial_default_search_provider_ ?
          initial_default_search_provider_->data() : NULL,
      default_search_provider_source_);

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+                           default_search_provider_source_);
+  initial_default_search_provider_.reset();
+  on_loaded_callbacks_.Notify();
 }
 
-void TemplateURLService::ClearDefaultProviderFromPrefs() {
-  // We overwrite user preferences. If the default search engine is managed,
-  // there is no effect.
-  SaveDefaultSearchProviderToPrefs(NULL, GetPrefs());
-  // Default value for kDefaultSearchProviderEnabled is true.
-  PrefService* prefs = GetPrefs();
-  if (prefs)
-    prefs->SetBoolean(prefs::kDefaultSearchProviderEnabled, true);
+void TemplateURLService::OnFailedLoad() {
+  load_failed_ = true;
+  service_ = NULL;
+  // Have to call this before GetDefaultSearchProvider(), or that function will
+  // just return |initial_default_search_provider_|.

[Peter Kasting, 2014/05/07 23:38:29]

This comment is no longer meaningful.

This whole function can go away, see latest version of my patch.

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+  ChangeToLoadedState();
 }
 
 bool TemplateURLService::CanReplaceKeywordForHost(
@@ -1844,7 +1565,7 @@ TemplateURL* TemplateURLService::FindNonExtensionTemplateURLForKeyword(
 
 bool TemplateURLService::UpdateNoNotify(
     TemplateURL* existing_turl,
-    const TemplateURL& new_values,
+    const TemplateURLData& new_values,
     const SearchTermsData& old_search_terms_data) {
   DCHECK(loaded_);
   DCHECK(existing_turl);
@@ -1898,13 +1619,14 @@ bool TemplateURLService::UpdateNoNotify(
     service_->UpdateKeyword(existing_turl->data());
 
   // Inform sync of the update.
-  ProcessTemplateURLChange(FROM_HERE,
-                           existing_turl,
-                           syncer::SyncChange::ACTION_UPDATE);
+  ProcessTemplateURLChange(
+      FROM_HERE, existing_turl, syncer::SyncChange::ACTION_UPDATE);
 
-  if (default_search_provider_ == existing_turl) {
-    bool success = SetDefaultSearchProviderNoNotify(existing_turl);
-    DCHECK(success);
+  if (default_search_provider_ == existing_turl &&
+             default_search_provider_source_ ==

[Peter Kasting, 2014/05/07 23:38:29]

Nit: Crazy indenting!  (11 instead of 4?)

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+                 DefaultSearchManager::FROM_USER) {
+    default_search_manager_.SetUserSelectedDefaultSearchEngine(
+        default_search_provider_->data());
   }
   return true;
 }
@@ -1925,11 +1647,20 @@ void TemplateURLService::UpdateTemplateURLIfPrepopulated(
   for (size_t i = 0; i < prepopulated_urls.size(); ++i) {
     if (prepopulated_urls[i]->prepopulate_id == prepopulate_id) {
       MergeIntoPrepopulatedEngineData(template_url, prepopulated_urls[i]);
-      template_url->CopyFrom(TemplateURL(profile, *prepopulated_urls[i]));
+      template_url->CopyFrom(*prepopulated_urls[i]);
     }
   }
 }
 
+void TemplateURLService::MaybeUpdateDSEAfterSync(TemplateURL* synced_turl) {
+  if (GetPrefs() &&
+      (synced_turl->sync_guid() ==
+       GetPrefs()->GetString(prefs::kSyncedDefaultSearchProviderGUID))) {

[Peter Kasting, 2014/05/07 23:38:29]

Nit: Indent an additional 4, not 1

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+        default_search_manager_.SetUserSelectedDefaultSearchEngine(

[Peter Kasting, 2014/05/07 23:38:29]

Nit: Indent 4 from "if", not 6

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+            synced_turl->data());
+  }
+}
+
 PrefService* TemplateURLService::GetPrefs() {
   return profile_ ? profile_->GetPrefs() : NULL;
 }
@@ -2020,7 +1751,7 @@ void TemplateURLService::GoogleBaseURLChanged(const GURL& old_base_url) {
       // This will send the keyword change to sync.  Note that other clients
       // need to reset the keyword to an appropriate local value when this
       // change arrives; see CreateTemplateURLFromTemplateURLAndSyncData().
-      UpdateNoNotify(t_url, updated_turl,
+      UpdateNoNotify(t_url, updated_turl.data(),
           OldBaseURLSearchTermsData(t_url->profile(), old_base_url.spec()));
     }
   }
@@ -2028,205 +1759,146 @@ void TemplateURLService::GoogleBaseURLChanged(const GURL& old_base_url) {
     NotifyObservers();
 }
 
-void TemplateURLService::UpdateDefaultSearch() {
-  if (!loaded_) {
-    // Set |initial_default_search_provider_| from the preferences.  We use this
-    // value for default search provider until the database has been loaded.
-    scoped_ptr<TemplateURLData> data;
-    if (!LoadDefaultSearchProviderFromPrefs(
-            GetPrefs(), &data, &is_default_search_managed_)) {
-      // Prefs does not specify, so rely on the prepopulated engines.  This
-      // should happen only the first time Chrome is started.
-      data =
-          TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(GetPrefs());
-      is_default_search_managed_ = false;
-    }
+void TemplateURLService::OnDSEGUIDPrefChanged() {
+  std::string new_guid =
+      GetPrefs()->GetString(prefs::kSyncedDefaultSearchProviderGUID);
+  if (new_guid.empty()) {
+    default_search_manager_.ClearUserSelectedDefaultSearchEngine();
+    return;
+  }
 
+  TemplateURL* turl = GetTemplateURLForGUID(new_guid);
+  if (turl)
+    default_search_manager_.SetUserSelectedDefaultSearchEngine(turl->data());
+}
+
+void TemplateURLService::OnDefaultSearchChange(
+    const TemplateURLData* data,
+    DefaultSearchManager::Source source) {
+  if (GetPrefs() && (source == DefaultSearchManager::FROM_USER) &&
+      ((source != default_search_provider_source_) ||
+       !IdenticalSyncGUIDs(data, GetDefaultSearchProvider()))) {
+    GetPrefs()->SetString(prefs::kSyncedDefaultSearchProviderGUID,
+                          data->sync_guid);
+  }
+  ApplyDefaultSearchChange(data, source);
+}
+
+void TemplateURLService::ApplyDefaultSearchChange(
+    const TemplateURLData* data,
+    DefaultSearchManager::Source source) {
+  if (!loaded_) {
+    // Set |initial_default_search_provider_| from the preferences. This is
+    // mainly so we can hold ownership until we get to the point where the list
+    // of keywords from Web Data is the owner of everything including the
+    // default.
     initial_default_search_provider_.reset(
         data ? new TemplateURL(profile_, *data) : NULL);
+    default_search_provider_source_ = source;
+    return;
+  }
 
+  // Prevent recursion if we update the value stored in default_search_manager_.
+  // Note that we exclude if data == NULL because that could cause a false

[Peter Kasting, 2014/05/07 23:38:29]

Nit: if -> the case of

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+  // positive for recursion when the initial_default_search_provider_ is NULL
+  // due to policy. No recursion case could lead to recursion with NULL.

[Peter Kasting, 2014/05/07 23:38:29]

Nit: "...We'll never actually get recursion with data == NULL." instead?

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+  if (source == default_search_provider_source_ && data != NULL &&
+      TemplateURLMatchesData(default_search_provider_, data))
     return;
+
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
+  if (default_search_provider_source_ == DefaultSearchManager::FROM_POLICY ||
+      source == DefaultSearchManager::FROM_POLICY) {
+    // We do this both to clear up removed policy-defined DSE as well as to add

[Peter Kasting, 2014/05/07 23:38:29]

Nit: clear up removed -> remove any no-longer-applicable?

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+    // the new one, if appropriate.
+    UpdateProvidersCreatedByPolicy(
+        &template_urls_,
+        source == DefaultSearchManager::FROM_POLICY ? data : NULL);
   }
-  // Load the default search specified in prefs.
-  scoped_ptr<TemplateURLData> new_default_from_prefs;
-  bool new_is_default_managed = false;
-  // Load the default from prefs.  It's possible that it won't succeed
-  // because we are in the middle of doing SaveDefaultSearchProviderToPrefs()
-  // and all the preference items have not been saved.  In that case, we
-  // don't have yet a default.  It would be much better if we could save
-  // preferences in batches and trigger notifications at the end.
-  LoadDefaultSearchProviderFromPrefs(
-      GetPrefs(), &new_default_from_prefs, &new_is_default_managed);
-  if (!is_default_search_managed_ && !new_is_default_managed) {
-    // We're not interested in cases where the default was and remains
-    // unmanaged.  In that case, preferences have no impact on the default.
+
+  if (!data) {
+    default_search_provider_ = NULL;
+    default_search_provider_source_ = source;
+    NotifyObservers();
     return;
   }
-  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
-  if (is_default_search_managed_ && new_is_default_managed) {
-    // The default was managed and remains managed.  Update the default only
-    // if it has changed; we don't want to respond to changes triggered by
-    // SaveDefaultSearchProviderToPrefs.
-    if (TemplateURLMatchesData(default_search_provider_,
-                               new_default_from_prefs.get()))
-      return;
-    if (!new_default_from_prefs) {
-      // default_search_provider_ can't be NULL otherwise
-      // TemplateURLMatchesData would have returned true.  Remove this now
-      // invalid value.
-      TemplateURL* old_default = default_search_provider_;
-      bool success = SetDefaultSearchProviderNoNotify(NULL);
-      DCHECK(success);
-      RemoveNoNotify(old_default);
-    } else if (default_search_provider_) {
-      new_default_from_prefs->created_by_policy = true;
-      TemplateURL new_values(profile_, *new_default_from_prefs);
+
+  if (source == DefaultSearchManager::FROM_EXTENSION) {

[Peter Kasting, 2014/05/07 23:38:29]

Nit: Rather than several successive "if (source == ...)", it might make sense to
have a switch statement here, to make it clear to the reader that these are all
parallel, mutually exclusive handler blocks.

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+    default_search_provider_ = FindMatchingExtensionTemplateURL(
+        *data, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+  }
+
+  if (source == DefaultSearchManager::FROM_FALLBACK) {
+    default_search_provider_ =
+        FindPrepopulatedTemplateURL(data->prepopulate_id);
+    if (default_search_provider_) {
+      TemplateURLData update_data(*data);
+      update_data.sync_guid = default_search_provider_->sync_guid();
+      if (!default_search_provider_->safe_for_autoreplace()) {
+        update_data.safe_for_autoreplace = false;
+        update_data.SetKeyword(default_search_provider_->keyword());
+        update_data.short_name = default_search_provider_->short_name();
+      }
       UIThreadSearchTermsData search_terms_data(
           default_search_provider_->profile());
-      UpdateNoNotify(default_search_provider_, new_values, search_terms_data);
+      UpdateNoNotify(default_search_provider_, update_data, search_terms_data);
     } else {
-      TemplateURL* new_template = NULL;
-      if (new_default_from_prefs) {
-        new_default_from_prefs->created_by_policy = true;
-        new_template = new TemplateURL(profile_, *new_default_from_prefs);
-        if (!AddNoNotify(new_template, true))
-          return;
-      }
-      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) {
-      new_default_from_prefs->created_by_policy = true;
-      new_template = new TemplateURL(profile_, *new_default_from_prefs);
-      if (!AddNoNotify(new_template, true))
-        return;
+      // Normally the prepopulated fallback should be present in
+      // |template_urls_|, but in a few cases it might not be:
+      // (1) Tests that initialize the TemplateURLService in peculiar ways.
+      // (2) If the user deleted the pre-populated default and we subsequently
+      // lost their user-selected value.
+      TemplateURL* new_dse = new TemplateURL(profile_, *data);
+      if (AddNoNotify(new_dse, true))
+        default_search_provider_ = new_dse;
+      else
+        delete new_dse;

[Peter Kasting, 2014/05/07 23:38:29]

This isn't right.  AddNoNotify() always takes ownership of its arg, even when
addition fails.  You're double-freeing.  (3 places)

[erikwright (departed), 2014/05/08 12:46:24]

That's disconcerting. I already noticed this myself and I'm sure I fixed it. I
must be losing my mind.

     }
-    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);
+  }
+  if (source == DefaultSearchManager::FROM_USER) {
+    default_search_provider_ = GetTemplateURLForGUID(data->sync_guid);
+    if (!default_search_provider_ && data->prepopulate_id) {
+      default_search_provider_ =
+          FindPrepopulatedTemplateURL(data->prepopulate_id);
     }
-
-    // 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;
-
-      base::AutoReset<DefaultSearchChangeOrigin> change_origin(
-          &dsp_change_origin_, DSP_CHANGE_SYNC_NOT_MANAGED);
-      SetDefaultSearchProviderNoNotify(synced_default);
+    if (default_search_provider_) {
+      TemplateURLData new_data(*data);
+      new_data.show_in_default_list = true;
+      UIThreadSearchTermsData search_terms_data(
+          default_search_provider_->profile());
+      UpdateNoNotify(default_search_provider_, new_data, search_terms_data);
     } else {
-      SetDefaultSearchProviderNoNotify(FindNewDefaultSearchProvider());
+      TemplateURL* new_dse = new TemplateURL(profile_, *data);
+      new_dse->data_.show_in_default_list = true;
+      new_dse->data_.id = kInvalidTemplateURLID;

[Peter Kasting, 2014/05/07 23:38:29]

Nit: Since TemplateURLData is supposed to basically be immutable once inside
TemplateURLs, a more kosher way of doing this would probably be:

  TemplateURLData new_data(*data);
  new_data.show_in_default_list = true;
  new_data.id = kInvalidTemplateURLID;
  TemplateURL* new_dse = new TemplateURL(profile_, new_data);

Then these first two lines could be pulled out of both arms and moved above the
conditional.

[erikwright (departed), 2014/05/08 12:46:24]

Done.

+      if (!AddNoNotify(new_dse, true))
+        delete new_dse;
+      else
+        default_search_provider_ = new_dse;
     }
-  }
-  NotifyObservers();
-}
-
-void TemplateURLService::SetDefaultSearchProvider(
-    TemplateURL* url) {
-  DCHECK(!is_default_search_managed_);
-  // Omnibox keywords cannot be made default. Extension-controlled search
-  // engines can be made default only by the extension itself because they
-  // aren't persisted.
-  DCHECK(!url || (url->GetType() == TemplateURL::NORMAL));
-
-  // Always persist the setting in the database, that way if the backup
-  // signature has changed out from under us it gets reset correctly.
-  if (SetDefaultSearchProviderNoNotify(url))
-    NotifyObservers();
-}
-
-bool TemplateURLService::SetDefaultSearchProviderNoNotify(TemplateURL* url) {
-  if (url) {
-    if (std::find(template_urls_.begin(), template_urls_.end(), url) ==
-        template_urls_.end())
-      return false;
-    // Omnibox keywords cannot be made default.
-    DCHECK_NE(TemplateURL::OMNIBOX_API_EXTENSION, url->GetType());
-  }
-
-  // Only bother reassigning |url| if it has changed. Notice that we don't just
-  // early exit if they are equal, because |url| may have had its fields
-  // changed, and needs to be persisted below (for example, when this is called
-  // from UpdateNoNotify).
-  if (default_search_provider_ != url) {
-    // Engines set by policy override extension-controlled engines, which
-    // override other engines.
-    DCHECK(!is_default_search_managed() || !url ||
-           (url->GetType() == TemplateURL::NORMAL));
-    if (is_default_search_managed() || !default_search_provider_ ||
-        (default_search_provider_->GetType() == TemplateURL::NORMAL) ||
-        (url &&
-         (url->GetType() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION))){
-      UMA_HISTOGRAM_ENUMERATION("Search.DefaultSearchChangeOrigin",
-                                dsp_change_origin_, DSP_CHANGE_MAX);
-      default_search_provider_ = url;
+    if (default_search_provider_ && GetPrefs()) {
+      GetPrefs()->SetString(
+          prefs::kSyncedDefaultSearchProviderGUID,
+          default_search_provider_->sync_guid());
     }
+
   }
 
-  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_ && (url->GetType() == TemplateURL::NORMAL))
-      service_->UpdateKeyword(url->data());
+  default_search_provider_source_ = source;
 
-    if (url->url_ref().HasGoogleBaseURLs()) {
+  if (default_search_provider_ &&
+      default_search_provider_->url_ref().HasGoogleBaseURLs()) {
+    if (profile_)
       GoogleURLTracker::RequestServerCheck(profile_, false);
 #if defined(ENABLE_RLZ)
-      RLZTracker::RecordProductEvent(rlz_lib::CHROME,
-                                     RLZTracker::CHROME_OMNIBOX,
-                                     rlz_lib::SET_TO_GOOGLE);
+    // TODO(erikwright): should this only be if user-selected?

[Peter Kasting, 2014/05/07 23:38:29]

Don't ask me -- but do be sure to get the RLZ pings right, we care a great deal
about them.  I don't know who owns this stuff, cpu might know.

[erikwright (departed), 2014/05/08 12:46:24]

I verified the prior implementation as well as checking with the relevant
people.

+    RLZTracker::RecordProductEvent(rlz_lib::CHROME,
+                                   RLZTracker::CHROME_OMNIBOX,
+                                   rlz_lib::SET_TO_GOOGLE);
 #endif
-    }
-  }
-
-  // Extension-controlled search engines shouldn't be persisted anywhere.
-  if (url && (url->GetType() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION))
-    return true;
-
-  if (!is_default_search_managed_) {
-    SaveDefaultSearchProviderToPrefs(url, GetPrefs());
-
-    // If we are syncing, we want to set the synced pref that will notify other
-    // instances to change their default to this new search provider.
-    // Note: we don't update the pref if we're currently in the middle of
-    // handling a sync operation. Sync operations from other clients are not
-    // guaranteed to arrive together, and any client that deletes the default
-    // needs to set a new default as well. If we update the default here, we're
-    // likely to race with the update from the other client, resulting in
-    // a possibly random default search provider.
-    if (sync_processor_.get() && url && !url->sync_guid().empty() &&
-        GetPrefs() && !processing_syncer_changes_) {
-      GetPrefs()->SetString(prefs::kSyncedDefaultSearchProviderGUID,
-                            url->sync_guid());
-    }
   }
 
-  if (service_)
-    service_->SetDefaultSearchProviderID(url ? url->id() : 0);
-
-  // Inform sync the change to the show_in_default_list flag.
-  if (url)
-    ProcessTemplateURLChange(FROM_HERE,
-                             url,
-                             syncer::SyncChange::ACTION_UPDATE);
-  return true;
+  NotifyObservers();
 }
 
 bool TemplateURLService::AddNoNotify(TemplateURL* template_url,
@@ -2331,41 +2003,35 @@ void TemplateURLService::NotifyObservers() {
 }
 
 // |template_urls| are the TemplateURLs loaded from the database.
-// |default_search_provider| points to one of them, if it was set in the db.
-// |default_from_prefs| is the default search provider from the preferences.
-// Check |is_default_search_managed_| to determine if it was set by policy.
+// |default_from_prefs| is the default search provider from the preferences, or
+// NULL if the DSE is not policy-defined.
 //
 // This function removes from the vector and the database all the TemplateURLs
-// that were set by policy, unless it is the current default search provider
-// and matches what is set by a managed preference.
-void TemplateURLService::RemoveProvidersCreatedByPolicy(
+// that were set by policy, unless it is the current default search provider, in
+// which case it is updated with the data from prefs.
+void TemplateURLService::UpdateProvidersCreatedByPolicy(
     TemplateURLVector* template_urls,
-    TemplateURL** default_search_provider,
-    TemplateURLData* default_from_prefs) {
+    const TemplateURLData* default_from_prefs) {
   DCHECK(template_urls);
-  DCHECK(default_search_provider);
+
   for (TemplateURLVector::iterator i = template_urls->begin();
        i != template_urls->end(); ) {
     TemplateURL* template_url = *i;
     if (template_url->created_by_policy()) {
-      if (template_url == *default_search_provider &&
-          is_default_search_managed_ &&
+      if (default_from_prefs &&
           TemplateURLMatchesData(template_url, default_from_prefs)) {
         // If the database specified a default search provider that was set
         // by policy, and the default search provider from the preferences
         // is also set by policy and they are the same, keep the entry in the
         // database and the |default_search_provider|.
+        default_search_provider_ = template_url;
+        // Prevent us from saving any other entries, or creating a new one.
+        default_from_prefs = NULL;
         ++i;
         continue;
       }
 
-      // The database loaded a managed |default_search_provider|, but it has
-      // been updated in the prefs. Remove it from the database, and update the
-      // |default_search_provider| pointer here.
-      if (*default_search_provider &&
-          (*default_search_provider)->id() == template_url->id())
-        *default_search_provider = NULL;
-
+      RemoveFromMaps(template_url);
       i = template_urls->erase(i);
       if (service_)
         service_->RemoveKeyword(template_url->id());
@@ -2374,6 +2040,18 @@ void TemplateURLService::RemoveProvidersCreatedByPolicy(
       ++i;
     }
   }
+
+  if (default_from_prefs) {
+    default_search_provider_ = NULL;
+    default_search_provider_source_ = DefaultSearchManager::FROM_POLICY;
+    TemplateURLData new_data(*default_from_prefs);
+    new_data.created_by_policy = true;
+    TemplateURL* new_dse = new TemplateURL(profile_, new_data);
+    if (!AddNoNotify(new_dse, true))
+      delete new_dse;
+    else
+      default_search_provider_ = new_dse;
+  }
 }
 
 void TemplateURLService::ResetTemplateURLGUID(TemplateURL* url,
@@ -2383,9 +2061,8 @@ void TemplateURLService::ResetTemplateURLGUID(TemplateURL* url,
 
   TemplateURLData data(url->data());
   data.sync_guid = guid;
-  TemplateURL new_url(url->profile(), data);
   UIThreadSearchTermsData search_terms_data(url->profile());
-  UpdateNoNotify(url, new_url, search_terms_data);
+  UpdateNoNotify(url, data, search_terms_data);
 }
 
 base::string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl,
@@ -2456,9 +2133,8 @@ void TemplateURLService::ResolveSyncKeywordConflict(
     // Update |applied_sync_turl| in the local model with the new keyword.
     TemplateURLData data(applied_sync_turl->data());
     data.SetKeyword(new_keyword);
-    TemplateURL new_turl(applied_sync_turl->profile(), data);
     UIThreadSearchTermsData search_terms_data(applied_sync_turl->profile());
-    if (UpdateNoNotify(applied_sync_turl, new_turl, search_terms_data))
+    if (UpdateNoNotify(applied_sync_turl, data, search_terms_data))
       NotifyObservers();
   }
   // The losing TemplateURL should have their keyword updated. Send a change to
@@ -2507,18 +2183,6 @@ void TemplateURLService::MergeInSyncTemplateURL(
             CreateSyncDataFromTemplateURL(*conflicting_turl);
         change_list->push_back(syncer::SyncChange(
             FROM_HERE, syncer::SyncChange::ACTION_UPDATE, sync_data));
-        if (conflicting_turl == GetDefaultSearchProvider() &&
-            !pending_synced_default_search_) {
-          // If we're not waiting for the Synced default to come in, we should
-          // override the pref with our new GUID. If we are waiting for the
-          // arrival of a synced default, setting the pref here would cause us
-          // to lose the GUID we are waiting on.
-          PrefService* prefs = GetPrefs();
-          if (prefs) {
-            prefs->SetString(prefs::kSyncedDefaultSearchProviderGUID,
-                             conflicting_turl->sync_guid());
-          }
-        }
         // Note that in this case we do not add the Sync TemplateURL to the
         // local model, since we've effectively "merged" it in by updating the
         // local conflicting entry with its sync_guid.
@@ -2543,43 +2207,14 @@ void TemplateURLService::MergeInSyncTemplateURL(
     // Force the local ID to kInvalidTemplateURLID so we can add it.
     TemplateURLData data(sync_turl->data());
     data.id = kInvalidTemplateURLID;
-    Add(new TemplateURL(profile_, data));
+    TemplateURL* added = new TemplateURL(profile_, data);
+    if (Add(added))
+      MaybeUpdateDSEAfterSync(added);
     merge_result->set_num_items_added(
         merge_result->num_items_added() + 1);
   }
 }
 
-void TemplateURLService::SetDefaultSearchProviderIfNewlySynced(
-    const std::string& guid) {
-  // If we're not syncing or if default search is managed by policy, ignore.
-  if (!sync_processor_.get() || is_default_search_managed_)
-    return;
-
-  PrefService* prefs = GetPrefs();
-  if (prefs && pending_synced_default_search_ &&
-      prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID) == guid) {
-    // Make sure this actually exists. We should not be calling this unless we
-    // really just added this TemplateURL.
-    TemplateURL* turl_from_sync = GetTemplateURLForGUID(guid);
-    if (turl_from_sync && turl_from_sync->SupportsReplacement()) {
-      base::AutoReset<DefaultSearchChangeOrigin> change_origin(
-          &dsp_change_origin_, DSP_CHANGE_SYNC_ADD);
-      SetDefaultSearchProvider(turl_from_sync);
-    }
-    pending_synced_default_search_ = false;
-  }
-}
-
-TemplateURL* TemplateURLService::GetPendingSyncedDefaultSearchProvider() {
-  PrefService* prefs = GetPrefs();
-  if (!prefs || !pending_synced_default_search_)
-    return NULL;
-
-  // Could be NULL if no such thing exists.
-  return GetTemplateURLForGUID(
-      prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID));
-}
-
 void TemplateURLService::PatchMissingSyncGUIDs(
     TemplateURLVector* template_urls) {
   DCHECK(template_urls);
@@ -2597,116 +2232,19 @@ void TemplateURLService::PatchMissingSyncGUIDs(
   }
 }
 
-void TemplateURLService::AddTemplateURLsAndSetupDefaultEngine(
-    TemplateURLVector* template_urls,
-    TemplateURL* default_search_provider) {
-  DCHECK(template_urls);
-  is_default_search_managed_ = false;
-  bool database_specified_a_default = (default_search_provider != NULL);
-
-  // Check if default search provider is now managed.
-  scoped_ptr<TemplateURLData> default_from_prefs;
-  LoadDefaultSearchProviderFromPrefs(
-      GetPrefs(), &default_from_prefs, &is_default_search_managed_);
-
-  // Remove entries that were created because of policy as they may have
-  // changed since the database was saved.
-  RemoveProvidersCreatedByPolicy(template_urls,
-                                 &default_search_provider,
-                                 default_from_prefs.get());
-
-  PatchMissingSyncGUIDs(template_urls);
-
-  if (is_default_search_managed_) {
-    SetTemplateURLs(template_urls);
-
-    if (TemplateURLMatchesData(default_search_provider,
-                               default_from_prefs.get())) {
-      // 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) {
-        default_from_prefs->created_by_policy = true;
-        default_from_prefs->id = kInvalidTemplateURLID;
-        default_search_provider =
-            new TemplateURL(profile_, *default_from_prefs);
-        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->GetType() !=
-            TemplateURL::OMNIBOX_API_EXTENSION) &&
-         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) {
-      UMA_HISTOGRAM_ENUMERATION(kFirstPotentialEngineHistogramName,
-                                FIRST_POTENTIAL_CALLSITE_ON_LOAD,
-                                FIRST_POTENTIAL_CALLSITE_MAX);
-      default_search_provider = FirstPotentialDefaultEngine(*template_urls);
-    }
-
-    // If the default search provider existed previously, then just
-    // set the member variable. Otherwise, we'll set it using the method
-    // to ensure that it is saved properly after its id is set.
-    if (default_search_provider &&
-        (default_search_provider->id() != kInvalidTemplateURLID)) {
-      default_search_provider_ = default_search_provider;
-      default_search_provider = NULL;
-    }
-    SetTemplateURLs(template_urls);
-
-    if (default_search_provider) {
-      base::AutoReset<DefaultSearchChangeOrigin> change_origin(
-          &dsp_change_origin_, default_from_prefs ?
-              dsp_change_origin_ : DSP_CHANGE_NEW_ENGINE_NO_PREFS);
-      // Note that this saves the default search provider to prefs.
-      SetDefaultSearchProvider(default_search_provider);
-    } else {
-      // Always save the default search provider to prefs. That way we don't
-      // have to worry about it being out of sync.
-      if (default_search_provider_)
-        SaveDefaultSearchProviderToPrefs(default_search_provider_, GetPrefs());
-    }
+TemplateURL* TemplateURLService::FindPrepopulatedTemplateURL(
+    int prepopulated_id) {
+  for (TemplateURLVector::const_iterator i = template_urls_.begin();
+       i != template_urls_.end(); ++i) {
+    if ((*i)->prepopulate_id() == prepopulated_id)
+      return *i;
   }
-}
 
-void TemplateURLService::EnsureDefaultSearchProviderExists() {
-  if (!is_default_search_managed()) {
-    bool has_default_search_provider = default_search_provider_ &&
-        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) {
-      bool success =
-          SetDefaultSearchProviderNoNotify(FindNewDefaultSearchProvider());
-      DCHECK(success);
-    }
-    // Don't log anything if the user has a NULL default search provider.
-    if (default_search_provider_) {
-      UMA_HISTOGRAM_ENUMERATION("Search.DefaultSearchProviderType",
-          TemplateURLPrepopulateData::GetEngineType(*default_search_provider_),
-          SEARCH_ENGINE_MAX);
-    }
-  }
+  return NULL;
 }
 
 TemplateURL* TemplateURLService::CreateTemplateURLForExtension(
-    const ExtensionKeyword& extension_keyword) const {
+    const ExtensionKeyword& extension_keyword) {
   TemplateURLData data;
   data.short_name = base::UTF8ToUTF16(extension_keyword.extension_name);
   data.SetKeyword(base::UTF8ToUTF16(extension_keyword.extension_keyword));
@@ -2719,7 +2257,7 @@ TemplateURL* TemplateURLService::CreateTemplateURLForExtension(
 
 TemplateURL* TemplateURLService::FindTemplateURLForExtension(
     const std::string& extension_id,
-    TemplateURL::Type type) const {
+    TemplateURL::Type type) {
   DCHECK_NE(TemplateURL::NORMAL, type);
   for (TemplateURLVector::const_iterator i = template_urls_.begin();
        i != template_urls_.end(); ++i) {
@@ -2731,7 +2269,20 @@ TemplateURL* TemplateURLService::FindTemplateURLForExtension(
   return NULL;
 }
 
-TemplateURL* TemplateURLService::FindExtensionDefaultSearchEngine() const {
+TemplateURL* TemplateURLService::FindMatchingExtensionTemplateURL(
+    const TemplateURLData& data,
+    TemplateURL::Type type) {
+  DCHECK_NE(TemplateURL::NORMAL, type);
+  for (TemplateURLVector::const_iterator i = template_urls_.begin();
+       i != template_urls_.end(); ++i) {
+    if ((*i)->GetType() == type && TemplateURLMatchesData(*i, &data))
+      return *i;
+  }
+
+  return NULL;
+}
+
+void TemplateURLService::UpdateExtensionDefaultSearchEngine() {
   TemplateURL* most_recently_intalled_default = NULL;
   for (TemplateURLVector::const_iterator i = template_urls_.begin();
        i != template_urls_.end(); ++i) {
@@ -2744,29 +2295,10 @@ TemplateURL* TemplateURLService::FindExtensionDefaultSearchEngine() const {
       most_recently_intalled_default = *i;
   }
 
-  return most_recently_intalled_default;
-}
-
-void TemplateURLService::
-    SetDefaultSearchProviderAfterRemovingDefaultExtension() {
-  DCHECK(!is_default_search_managed());
-  TemplateURL* new_dse = FindExtensionDefaultSearchEngine();
-  if (!new_dse) {
-    scoped_ptr<TemplateURLData> default_provider;
-    bool is_managed;
-    if (LoadDefaultSearchProviderFromPrefs(
-            GetPrefs(), &default_provider, &is_managed) &&
-        default_provider) {
-      for (TemplateURLVector::const_iterator i = template_urls_.begin();
-           i != template_urls_.end(); ++i) {
-        if ((*i)->id() == default_provider->id) {
-          new_dse = *i;
-          break;
-        }
-      }
-    }
+  if (most_recently_intalled_default) {
+    default_search_manager_.SetExtensionControlledDefaultSearchEngine(
+        most_recently_intalled_default->data());
+  } else {
+    default_search_manager_.ClearExtensionControlledDefaultSearchEngine();
   }
-  if (!new_dse)
-    new_dse = FindNewDefaultSearchProvider();
-  SetDefaultSearchProviderNoNotify(new_dse);
 }