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

chrome/browser/search_engines/default_search_manager.cc

Index: chrome/browser/search_engines/default_search_manager.cc
diff --git a/chrome/browser/search_engines/default_search_manager.cc b/chrome/browser/search_engines/default_search_manager.cc
index d905a42d293f81e54889dfffe0a638b8e007438f..6566b3b7ebfcb286989edf6f71f84dc2ca4458b3 100644
--- a/chrome/browser/search_engines/default_search_manager.cc
+++ b/chrome/browser/search_engines/default_search_manager.cc
@@ -30,14 +30,21 @@
 
 namespace {
 
+bool IsDefaultSearchProviderManaged(PrefService* pref_service) {
+  const PrefService::Preference* pref = pref_service->FindPreference(
+      DefaultSearchManager::kDefaultSearchProviderDataPrefName);
+  DCHECK(pref);
+  return pref->IsManaged();
+}
+
+}  // namespace
+
 // A dictionary to hold all data related to the Default Search Engine.
 // Eventually, this should replace all the data stored in the
 // default_search_provider.* prefs.
-const char kDefaultSearchProviderData[] =
+const char DefaultSearchManager::kDefaultSearchProviderDataPrefName[] =
     "default_search_provider_data.template_url_data";
 
-}  // namespace
-
 const char DefaultSearchManager::kID[] = "id";
 const char DefaultSearchManager::kShortName[] = "short_name";
 const char DefaultSearchManager::kKeyword[] = "keyword";
@@ -74,20 +81,25 @@ const char DefaultSearchManager::kSearchTermsReplacementKey[] =
 const char DefaultSearchManager::kCreatedByPolicy[] = "created_by_policy";
 const char DefaultSearchManager::kDisabledByPolicy[] = "disabled_by_policy";
 
-DefaultSearchManager::DefaultSearchManager(PrefService* pref_service)
-    : pref_service_(pref_service), default_search_controlled_by_policy_(false) {
-  DCHECK(pref_service_);
-  pref_change_registrar_.Init(pref_service_);
-  pref_change_registrar_.Add(
-      kDefaultSearchProviderData,
-      base::Bind(&DefaultSearchManager::OnDefaultSearchPrefChanged,
-                 base::Unretained(this)));
-  pref_change_registrar_.Add(
-      prefs::kSearchProviderOverrides,
-      base::Bind(&DefaultSearchManager::OnOverridesPrefChanged,
-                 base::Unretained(this)));
-  OnDefaultSearchPrefChanged();
-  OnOverridesPrefChanged();
+DefaultSearchManager::DefaultSearchManager(
+    PrefService* pref_service,
+    const ObserverCallback& change_observer)
+    : pref_service_(pref_service),
+      change_observer_(change_observer),
+      default_search_controlled_by_policy_(false) {
+  if (pref_service_) {
+    pref_change_registrar_.Init(pref_service_);
+    pref_change_registrar_.Add(
+        kDefaultSearchProviderDataPrefName,
+        base::Bind(&DefaultSearchManager::OnDefaultSearchPrefChanged,
+                   base::Unretained(this)));
+    pref_change_registrar_.Add(
+        prefs::kSearchProviderOverrides,
+        base::Bind(&DefaultSearchManager::OnOverridesPrefChanged,
+                   base::Unretained(this)));
+  }
+  LoadPrepopulatedDefaultSearch();
+  LoadDefaultSearchEngineFromPrefs();
 }
 
 DefaultSearchManager::~DefaultSearchManager() {
@@ -97,41 +109,48 @@ DefaultSearchManager::~DefaultSearchManager() {
 void DefaultSearchManager::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterDictionaryPref(
-      kDefaultSearchProviderData,
-      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+      kDefaultSearchProviderDataPrefName,
+      user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
 }
 
 // static
 void DefaultSearchManager::AddPrefValueToMap(base::DictionaryValue* value,
                                              PrefValueMap* pref_value_map) {
-  pref_value_map->SetValue(kDefaultSearchProviderData, value);
-}
-
-DefaultSearchManager::Source
-DefaultSearchManager::GetDefaultSearchEngineSource() const {
-  if (default_search_controlled_by_policy_)
-    return FROM_POLICY;
-  if (extension_default_search_)
-    return FROM_EXTENSION;
-  if (prefs_default_search_)
-    return FROM_USER;
-
-  return FROM_FALLBACK;
+  pref_value_map->SetValue(kDefaultSearchProviderDataPrefName, value);
 }
 
-TemplateURLData* DefaultSearchManager::GetDefaultSearchEngine() const {
-  if (default_search_controlled_by_policy_)
+TemplateURLData* DefaultSearchManager::GetDefaultSearchEngine(

[Peter Kasting, 2014/05/03 00:28:18]

Given the number of times you call this just to get the search engine source, it
seems like perhaps leaving things as two separate functions would be better. 
This would make a lot of callers shorter.

You could even leave GetDefaultSearchEngine() as taking an arg, which would mean
the unittest code that really wants this combined form could still use it;  and
the implementation here would basically just be the old version of the code plus
a trailing call to:

  if (source)
    *source = GetDefaultSearchEngineSource();

[erikwright (departed), 2014/05/05 01:49:38]

It makes sense to have a single function calculate both since the algorithm is
identical. Separate functions would end up having (hopefully) duplicate logic.

So I've added a second method that only returns the Source, but delegates to
this one to do so.

+    Source* source) const {
+  if (default_search_controlled_by_policy_) {
+    if (source)
+      *source = FROM_POLICY;
     return prefs_default_search_.get();
-  if (extension_default_search_)
+  }
+  if (extension_default_search_) {
+    if (source)
+      *source = FROM_EXTENSION;
     return extension_default_search_.get();
-  if (prefs_default_search_)
+  }
+  if (prefs_default_search_) {
+    if (source)
+      *source = FROM_USER;
     return prefs_default_search_.get();
+  }
 
+  if (source)
+    *source = FROM_FALLBACK;
   return fallback_default_search_.get();
 }
 
 void DefaultSearchManager::SetUserSelectedDefaultSearchEngine(
     const TemplateURLData& data) {
+  if (!pref_service_) {
+    prefs_default_search_.reset(new TemplateURLData(data));
+    MergePrefsDataWithPrepopulated();
+    NotifyObserver();
+    return;
+  }
+
   base::DictionaryValue url_dict;
   url_dict.SetString(kID, base::Int64ToString(data.id));
   url_dict.SetString(kShortName, data.short_name);
@@ -162,8 +181,7 @@ void DefaultSearchManager::SetUserSelectedDefaultSearchEngine(
   scoped_ptr<base::ListValue> alternate_urls(new base::ListValue);
   for (std::vector<std::string>::const_iterator it =
            data.alternate_urls.begin();
-       it != data.alternate_urls.end();
-       ++it) {
+       it != data.alternate_urls.end(); ++it) {
     alternate_urls->AppendString(*it);
   }
   url_dict.Set(kAlternateURLs, alternate_urls.release());
@@ -171,8 +189,7 @@ void DefaultSearchManager::SetUserSelectedDefaultSearchEngine(
   scoped_ptr<base::ListValue> encodings(new base::ListValue);
   for (std::vector<std::string>::const_iterator it =
            data.input_encodings.begin();
-       it != data.input_encodings.end();
-       ++it) {
+       it != data.input_encodings.end(); ++it) {
     encodings->AppendString(*it);
   }
   url_dict.Set(kInputEncodings, encodings.release());
@@ -182,44 +199,60 @@ void DefaultSearchManager::SetUserSelectedDefaultSearchEngine(
 
   url_dict.SetBoolean(kCreatedByPolicy, data.created_by_policy);
 
-  pref_service_->Set(kDefaultSearchProviderData, url_dict);
+  pref_service_->Set(kDefaultSearchProviderDataPrefName, url_dict);
 }
 
 void DefaultSearchManager::SetExtensionControlledDefaultSearchEngine(
     TemplateURLData* data) {
+  DCHECK(data);
+  Source old_source = FROM_FALLBACK;

[Peter Kasting, 2014/05/03 00:28:18]

Nit: I think initting this before the call, when the call is guaranteed to set
it, is more confusing than helpful.  (I don't _think_ any of our bots ought to
mandate that you do so.)  (3 places)

[erikwright (departed), 2014/05/05 01:49:38]

Done.

+  GetDefaultSearchEngine(&old_source);
   extension_default_search_.reset(data);
+  if (old_source <= FROM_EXTENSION)

[Peter Kasting, 2014/05/03 00:28:18]

This implies that the Source values are in sorted order, which may happen to be
true, but isn't documented or enforced elsewhere.

It seems like a clearer and safer method would be something like:

  extension_default_search_.reset(data);
  Source source;
  GetDefaultSearchEngine(&source);
  if (source == FROM_EXTENSION)
    NotifyObserver();

[erikwright (departed), 2014/05/05 01:49:38]

Done.

+    NotifyObserver();
 }
 
 void DefaultSearchManager::ClearExtensionControlledDefaultSearchEngine() {
+  Source old_source = FROM_FALLBACK;
+  GetDefaultSearchEngine(&old_source);
   extension_default_search_.reset();
+  if (old_source == FROM_EXTENSION)
+    NotifyObserver();
 }
 
 void DefaultSearchManager::ClearUserSelectedDefaultSearchEngine() {
-  pref_service_->ClearPref(kDefaultSearchProviderData);
+  if (pref_service_) {
+    pref_service_->ClearPref(kDefaultSearchProviderDataPrefName);
+  } else {
+    prefs_default_search_.reset();
+    NotifyObserver();
+  }
 }
 
 void DefaultSearchManager::OnDefaultSearchPrefChanged() {
-  TemplateURLData* data;
-  if (ReadDefaultSearchEngineFromPrefs(&data)) {
-    prefs_default_search_.reset(data);
-    MergePrefsDataWithPrepopulated();
+  Source old_source = FROM_FALLBACK;
+  GetDefaultSearchEngine(&old_source);
+  LoadDefaultSearchEngineFromPrefs();
+
+  if (old_source == FROM_USER || old_source == FROM_POLICY) {
+    NotifyObserver();
   } else {
-    prefs_default_search_.reset();
+    Source new_source = FROM_FALLBACK;
+    GetDefaultSearchEngine(&new_source);
+    if (new_source == FROM_USER || new_source == FROM_POLICY)
+      NotifyObserver();
   }

[Peter Kasting, 2014/05/03 00:28:18]

Nit: Simpler:

  Source source;
  GetDefaultSearchEngine(&source);
  LoadDefaultSearchEngineFromPrefs();
  if ((source != FROM_USER) && (source != FROM_POLICY))
    GetDefaultSearchEngine(&source);
  if ((source == FROM_USER) || (source == FROM_POLICY))
    NotifyObserver();

[erikwright (departed), 2014/05/05 01:49:38]

Done.

-  UpdatePolicyStatus();
 }
 
 void DefaultSearchManager::OnOverridesPrefChanged() {
-  scoped_ptr<TemplateURLData> data =
-      TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(pref_service_);
-  fallback_default_search_ = data.Pass();
-  MergePrefsDataWithPrepopulated();
-}
+  LoadPrepopulatedDefaultSearch();
 
-void DefaultSearchManager::UpdatePolicyStatus() {
-  const PrefService::Preference* pref =
-      pref_service_->FindPreference(kDefaultSearchProviderData);
-  default_search_controlled_by_policy_ = pref && pref->IsManaged();
+  TemplateURLData* effective_data = GetDefaultSearchEngine(NULL);
+  if (effective_data && effective_data->prepopulate_id) {
+    // A user-selected, policy-selected or fallback pre-populated engine is
+    // active and may have changed with this event.
+    NotifyObserver();
+  }
 }
 
 void DefaultSearchManager::MergePrefsDataWithPrepopulated() {
@@ -228,8 +261,8 @@ void DefaultSearchManager::MergePrefsDataWithPrepopulated() {
 
   size_t default_search_index;
   ScopedVector<TemplateURLData> prepopulated_urls =
-      TemplateURLPrepopulateData::GetPrepopulatedEngines(pref_service_,
-                                                         &default_search_index);
+      TemplateURLPrepopulateData::GetPrepopulatedEngines(
+         pref_service_, &default_search_index);

[Peter Kasting, 2014/05/03 00:28:18]

Nit: Indenting is wrong (3 instead of 4) -- I would leave things as they were
before, I think that's arguably better and certainly not worse.

[erikwright (departed), 2014/05/05 01:49:38]

Done.

 
   for (size_t i = 0; i < prepopulated_urls.size(); ++i) {
     if (prepopulated_urls[i]->prepopulate_id ==
@@ -245,101 +278,128 @@ void DefaultSearchManager::MergePrefsDataWithPrepopulated() {
       prepopulated_urls[i]->last_modified =
           prefs_default_search_->last_modified;
       prefs_default_search_.reset(prepopulated_urls[i]);
-      prepopulated_urls.weak_erase(prepopulated_urls.begin() + i);

[Peter Kasting, 2014/05/03 00:28:18]

Doesn't removing this line result in the URL being freed on exit from this
function?

       return;
     }
   }
 }
 
-bool DefaultSearchManager::ReadDefaultSearchEngineFromPrefs(
-    TemplateURLData** dse_data) {
-  const base::DictionaryValue* url_dict =
-      pref_service_->GetDictionary(kDefaultSearchProviderData);
+void DefaultSearchManager::LoadDefaultSearchEngineFromPrefs() {
+  if (!pref_service_)
+    return;
 
+  prefs_default_search_.reset();
+  default_search_controlled_by_policy_ =
+      IsDefaultSearchProviderManaged(pref_service_);
+
+  const base::DictionaryValue* url_dict =
+      pref_service_->GetDictionary(kDefaultSearchProviderDataPrefName);
   if (url_dict->empty())
-    return false;
+    return;
 
-  bool disabled_by_policy = false;
-  if (url_dict->GetBoolean(kDisabledByPolicy, &disabled_by_policy) &&
-      disabled_by_policy) {
-    *dse_data = NULL;
-    return true;
+  if (default_search_controlled_by_policy_) {
+    bool disabled_by_policy = false;
+    if (url_dict->GetBoolean(kDisabledByPolicy, &disabled_by_policy) &&
+        disabled_by_policy) {

[Peter Kasting, 2014/05/03 00:28:18]

Nit: No {}

[erikwright (departed), 2014/05/05 01:49:38]

Done.

+      return;
+    }
   }
 
-  TemplateURLData* data = new TemplateURLData();
   std::string search_url;
   base::string16 keyword;
   url_dict->GetString(kURL, &search_url);
   url_dict->GetString(kKeyword, &keyword);
-  if (search_url.empty())
-    return false;
-  if (keyword.empty())
-    keyword = TemplateURLService::GenerateKeyword(GURL(search_url));
-  data->SetKeyword(keyword);
-  data->SetURL(search_url);
+  if (search_url.empty() || keyword.empty())
+    return;
+
+  prefs_default_search_.reset(new TemplateURLData);
+  prefs_default_search_->SetKeyword(keyword);
+  prefs_default_search_->SetURL(search_url);
 
   std::string id;
   url_dict->GetString(kID, &id);
-  base::StringToInt64(id, &data->id);
-  url_dict->GetString(kShortName, &data->short_name);
-  url_dict->GetInteger(kPrepopulateID, &data->prepopulate_id);
-  url_dict->GetString(kSyncGUID, &data->sync_guid);
+  base::StringToInt64(id, &prefs_default_search_->id);
+  url_dict->GetString(kShortName, &prefs_default_search_->short_name);
+  url_dict->GetInteger(kPrepopulateID, &prefs_default_search_->prepopulate_id);
+  url_dict->GetString(kSyncGUID, &prefs_default_search_->sync_guid);
 
-  url_dict->GetString(kSuggestionsURL, &data->suggestions_url);
-  url_dict->GetString(kInstantURL, &data->instant_url);
-  url_dict->GetString(kImageURL, &data->image_url);
-  url_dict->GetString(kNewTabURL, &data->new_tab_url);
+  url_dict->GetString(kSuggestionsURL, &prefs_default_search_->suggestions_url);
+  url_dict->GetString(kInstantURL, &prefs_default_search_->instant_url);
+  url_dict->GetString(kImageURL, &prefs_default_search_->image_url);
+  url_dict->GetString(kNewTabURL, &prefs_default_search_->new_tab_url);
 
   std::string favicon_url;
   std::string originating_url;
   url_dict->GetString(kFaviconURL, &favicon_url);
   url_dict->GetString(kOriginatingURL, &originating_url);
-  data->favicon_url = GURL(favicon_url);
-  data->originating_url = GURL(originating_url);
+  prefs_default_search_->favicon_url = GURL(favicon_url);
+  prefs_default_search_->originating_url = GURL(originating_url);
 
-  url_dict->GetString(kSearchURLPostParams, &data->search_url_post_params);
+  url_dict->GetString(kSearchURLPostParams,
+                      &prefs_default_search_->search_url_post_params);
   url_dict->GetString(kSuggestionsURLPostParams,
-                      &data->suggestions_url_post_params);
-  url_dict->GetString(kInstantURLPostParams, &data->instant_url_post_params);
-  url_dict->GetString(kImageURLPostParams, &data->image_url_post_params);
+                      &prefs_default_search_->suggestions_url_post_params);
+  url_dict->GetString(kInstantURLPostParams,
+                      &prefs_default_search_->instant_url_post_params);
+  url_dict->GetString(kImageURLPostParams,
+                      &prefs_default_search_->image_url_post_params);
 
-  url_dict->GetBoolean(kSafeForAutoReplace, &data->safe_for_autoreplace);
+  url_dict->GetBoolean(kSafeForAutoReplace,
+                       &prefs_default_search_->safe_for_autoreplace);
 
   double date_created = 0.0;
   double last_modified = 0.0;
   url_dict->GetDouble(kDateCreated, &date_created);
   url_dict->GetDouble(kLastModified, &last_modified);
-  data->date_created = base::Time::FromInternalValue(date_created);
-  data->last_modified = base::Time::FromInternalValue(last_modified);
-
-  url_dict->GetInteger(kUsageCount, &data->usage_count);
-
-  const base::ListValue* alternate_urls;
-  url_dict->GetList(kAlternateURLs, &alternate_urls);
-  data->alternate_urls.clear();
-  for (base::ListValue::const_iterator it = alternate_urls->begin();
-       it != alternate_urls->end(); ++it) {
-    std::string alternate_url;
-    if ((*it)->GetAsString(&alternate_url))
-      data->alternate_urls.push_back(alternate_url);
+  prefs_default_search_->date_created =
+      base::Time::FromInternalValue(date_created);
+  prefs_default_search_->last_modified =
+      base::Time::FromInternalValue(last_modified);
+
+  url_dict->GetInteger(kUsageCount, &prefs_default_search_->usage_count);
+
+  const base::ListValue* alternate_urls = NULL;
+  if (url_dict->GetList(kAlternateURLs, &alternate_urls)) {
+    for (base::ListValue::const_iterator it = alternate_urls->begin();
+         it != alternate_urls->end();
+         ++it) {
+      std::string alternate_url;
+      if ((*it)->GetAsString(&alternate_url))
+        prefs_default_search_->alternate_urls.push_back(alternate_url);
+    }
   }
 
-  const base::ListValue* encodings;
-  url_dict->GetList(kInputEncodings, &encodings);
-  data->input_encodings.clear();
-  for (base::ListValue::const_iterator it = encodings->begin();
-       it != encodings->end(); ++it) {
-    std::string encoding;
-    if ((*it)->GetAsString(&encoding))
-      data->input_encodings.push_back(encoding);
+  const base::ListValue* encodings = NULL;
+  if (url_dict->GetList(kInputEncodings, &encodings)) {
+    for (base::ListValue::const_iterator it = encodings->begin();
+         it != encodings->end();
+         ++it) {
+      std::string encoding;
+      if ((*it)->GetAsString(&encoding))
+        prefs_default_search_->input_encodings.push_back(encoding);
+    }
   }
 
   url_dict->GetString(kSearchTermsReplacementKey,
-                      &data->search_terms_replacement_key);
+                      &prefs_default_search_->search_terms_replacement_key);
+
+  url_dict->GetBoolean(kCreatedByPolicy,
+                       &prefs_default_search_->created_by_policy);
+
+  prefs_default_search_->show_in_default_list = true;
+  MergePrefsDataWithPrepopulated();
+}
 
-  url_dict->GetBoolean(kCreatedByPolicy, &data->created_by_policy);
+void DefaultSearchManager::LoadPrepopulatedDefaultSearch() {
+  scoped_ptr<TemplateURLData> data =
+     TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(pref_service_);
+  fallback_default_search_ = data.Pass();
+  MergePrefsDataWithPrepopulated();
+}
 
-  data->show_in_default_list = true;
-  *dse_data = data;
-  return true;
+void DefaultSearchManager::NotifyObserver() {
+  if (!change_observer_.is_null()) {
+    Source source = FROM_FALLBACK;
+    TemplateURLData* data = GetDefaultSearchEngine(&source);
+    change_observer_.Run(data, source);
+  }
 }