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

chrome/browser/search_engines/default_search_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/search_engines/default_search_manager.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 12 matching lines @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_prepopulate_data.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/util.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 
 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";
 const char DefaultSearchManager::kPrepopulateID[] = "prepopulate_id";
 const char DefaultSearchManager::kSyncGUID[] = "synced_guid";
 
 const char DefaultSearchManager::kURL[] = "url";
 const char DefaultSearchManager::kSuggestionsURL[] = "suggestions_url";
 const char DefaultSearchManager::kInstantURL[] = "instant_url";
 const char DefaultSearchManager::kImageURL[] = "image_url";
@@ skipping 16 matching lines @@
 const char DefaultSearchManager::kDateCreated[] = "date_created";
 const char DefaultSearchManager::kLastModified[] = "last_modified";
 
 const char DefaultSearchManager::kUsageCount[] = "usage_count";
 const char DefaultSearchManager::kAlternateURLs[] = "alternate_urls";
 const char DefaultSearchManager::kSearchTermsReplacementKey[] =
     "search_terms_replacement_key";
 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() {
 }
 
 // static
 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);
+  pref_value_map->SetValue(kDefaultSearchProviderDataPrefName, 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;
+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 (source)
+      *source = FROM_EXTENSION;
+    return extension_default_search_.get();
+  }
+  if (prefs_default_search_) {
+    if (source)
+      *source = FROM_USER;
+    return prefs_default_search_.get();
+  }
 
-  return FROM_FALLBACK;
-}
-
-TemplateURLData* DefaultSearchManager::GetDefaultSearchEngine() const {
-  if (default_search_controlled_by_policy_)
-    return prefs_default_search_.get();
-  if (extension_default_search_)
-    return extension_default_search_.get();
-  if (prefs_default_search_)
-    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);
   url_dict.SetString(kKeyword, data.keyword());
   url_dict.SetInteger(kPrepopulateID, data.prepopulate_id);
   url_dict.SetString(kSyncGUID, data.sync_guid);
 
   url_dict.SetString(kURL, data.url());
   url_dict.SetString(kSuggestionsURL, data.suggestions_url);
   url_dict.SetString(kInstantURL, data.instant_url);
@@ skipping 10 matching lines @@
 
   url_dict.SetBoolean(kSafeForAutoReplace, data.safe_for_autoreplace);
 
   url_dict.SetDouble(kDateCreated, data.date_created.ToInternalValue());
   url_dict.SetDouble(kLastModified, data.last_modified.ToInternalValue());
   url_dict.SetInteger(kUsageCount, data.usage_count);
 
   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());
 
   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());
 
   url_dict.SetString(kSearchTermsReplacementKey,
                      data.search_terms_replacement_key);
 
   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() {
   if (!prefs_default_search_ || !prefs_default_search_->prepopulate_id)
     return;
 
   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 ==
         prefs_default_search_->prepopulate_id) {
       if (!prefs_default_search_->safe_for_autoreplace) {
         prepopulated_urls[i]->safe_for_autoreplace = false;
         prepopulated_urls[i]->SetKeyword(prefs_default_search_->keyword());
         prepopulated_urls[i]->short_name = prefs_default_search_->short_name;
       }
       prepopulated_urls[i]->id = prefs_default_search_->id;
       prepopulated_urls[i]->sync_guid = prefs_default_search_->sync_guid;
       prepopulated_urls[i]->date_created = prefs_default_search_->date_created;
       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) {
+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(kDefaultSearchProviderData);
+      pref_service_->GetDictionary(kDefaultSearchProviderDataPrefName);
+  if (url_dict->empty())
+    return;
 
-  if (url_dict->empty())
-    return false;
-
-  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);
+  prefs_default_search_->date_created =
+      base::Time::FromInternalValue(date_created);
+  prefs_default_search_->last_modified =
+      base::Time::FromInternalValue(last_modified);
 
-  url_dict->GetInteger(kUsageCount, &data->usage_count);
+  url_dict->GetInteger(kUsageCount, &prefs_default_search_->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);
+  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, &data->created_by_policy);
+  url_dict->GetBoolean(kCreatedByPolicy,
+                       &prefs_default_search_->created_by_policy);
 
-  data->show_in_default_list = true;
-  *dse_data = data;
-  return true;
+  prefs_default_search_->show_in_default_list = true;
+  MergePrefsDataWithPrepopulated();
 }
+
+void DefaultSearchManager::LoadPrepopulatedDefaultSearch() {
+  scoped_ptr<TemplateURLData> data =
+     TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(pref_service_);
+  fallback_default_search_ = data.Pass();
+  MergePrefsDataWithPrepopulated();
+}
+
+void DefaultSearchManager::NotifyObserver() {
+  if (!change_observer_.is_null()) {
+    Source source = FROM_FALLBACK;
+    TemplateURLData* data = GetDefaultSearchEngine(&source);
+    change_observer_.Run(data, source);
+  }
+}