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

chrome/browser/search_engines/template_url_service.h

Index: chrome/browser/search_engines/template_url_service.h
diff --git a/chrome/browser/search_engines/template_url_service.h b/chrome/browser/search_engines/template_url_service.h
index 654b25c28d443395f5468429635459c1066c5586..44cd99a706a15c3cd86e2b27b0c11757b0868844 100644
--- a/chrome/browser/search_engines/template_url_service.h
+++ b/chrome/browser/search_engines/template_url_service.h
@@ -137,8 +137,8 @@ class TemplateURLService : public WebDataServiceConsumer,
 
   // Saves enough of url to |prefs| so that it can be loaded from preferences on
   // start up.
-  void SaveDefaultSearchProviderToPrefs(const TemplateURL* url,
-                                        PrefService* prefs) const;
+  static void SaveDefaultSearchProviderToPrefs(const TemplateURL* url,

[Peter Kasting, 2014/05/08 22:46:46]

Bad rebase?  This is the same way the code looks on trunk today, I think.

Also, is this function only called by tests now?  May want to note that if so. 
Unless you're just going to delete this in a followup change soon.

[erikwright (departed), 2014/05/09 18:00:22]

In your CL you made it non-static in order to not save when in a failed state.
This CL reverts that since it is now only used externally.

I deferred moving this function to the sole client (a test) in order to
facilitate your review of this CL. You have already LGTM'd the CL that contains
the move (I'll land it ASAP after this CL).

[Peter Kasting, 2014/05/09 19:12:49]

Ah, you're right.  Sorry, at one point I locally reverted and then un-reverted
this change, and I forgot I'd unreverted it.

+                                               PrefService* prefs);
 
   // Returns true if there is no TemplateURL that conflicts with the
   // keyword/url pair, or there is one but it can be replaced. If there is an
@@ -175,7 +175,8 @@ class TemplateURLService : public WebDataServiceConsumer,
 
   // Takes ownership of |template_url| and adds it to this model.  For obvious
   // reasons, it is illegal to Add() the same |template_url| pointer twice.
-  void Add(TemplateURL* template_url);
+  // Returns true if the Add is successful.
+  bool Add(TemplateURL* template_url);
 
   // Like Add(), but overwrites the |template_url|'s values with the provided
   // ones.
@@ -261,7 +262,9 @@ class TemplateURLService : public WebDataServiceConsumer,
   bool IsSearchResultsPageFromDefaultSearchProvider(const GURL& url);
 
   // Returns true if the default search is managed through group policy.
-  bool is_default_search_managed() const { return is_default_search_managed_; }
+  bool is_default_search_managed() const {
+    return default_search_provider_source_ == DefaultSearchManager::FROM_POLICY;
+  }
 
   // Returns true if the default search provider is controlled by an extension.
   bool IsExtensionControlledDefaultSearch();
@@ -384,7 +387,18 @@ class TemplateURLService : public WebDataServiceConsumer,
   static SyncDataMap CreateGUIDToSyncDataMap(
       const syncer::SyncDataList& sync_data);
 
+  // Indicates whether the pre-populated TemplateURLs should be disabled. May
+  // only be true in tests.
+  static bool fallback_search_engines_disabled() {
+    return g_fallback_search_engines_disabled;
+  }
+
 #if defined(UNIT_TEST)
+  // Disables the pre-populated TemplateURLs for testing purposes.
+  static void set_fallback_search_engines_disabled() {
+    g_fallback_search_engines_disabled = true;
+  }
+
   // Set a different time provider function, such as

[Peter Kasting, 2014/05/08 22:46:46]

Nit: While here: Set -> Sets; when -> for

   // base::MockTimeProvider::StaticNow, when testing calls to base::Time::Now.
   void set_time_provider(TimeProvider* time_provider) {
@@ -472,8 +486,15 @@ class TemplateURLService : public WebDataServiceConsumer,
   // Transitions to the loaded state.
   void ChangeToLoadedState();
 
-  // Clears user preferences describing the default search engine.
-  void ClearDefaultProviderFromPrefs();
+  // Called by DefaultSearchManager when the effective Default Search Engine has

[Peter Kasting, 2014/05/08 22:46:46]

Nit: I wouldn't capitalize Default Search Engine

+  // changed.
+  void OnDefaultSearchChange(const TemplateURLData* new_dse_data,
+                             DefaultSearchManager::Source source);
+
+  // Applies a DSE change. May be called at startup or after transitioning to
+  // the loaded state.
+  void ApplyDefaultSearchChange(const TemplateURLData* new_dse_data,
+                                DefaultSearchManager::Source source);
 
   // Returns true if there is no TemplateURL that has a search url with the
   // specified host, or the only TemplateURLs matching the specified host can
@@ -510,6 +531,11 @@ class TemplateURLService : public WebDataServiceConsumer,
   static void UpdateTemplateURLIfPrepopulated(TemplateURL* existing_turl,
                                               Profile* profile);
 
+  // If the TemplateURL's sync GUID matches the kSyncedDefaultSearchProviderGUID
+  // preference it will be used to update the DSE in memory and as persisted in
+  // preferences.
+  void MaybeUpdateDSEAfterSync(TemplateURL* synced_turl);
+
   // Returns the preferences we use.
   PrefService* GetPrefs();
 
@@ -527,26 +553,6 @@ class TemplateURLService : public WebDataServiceConsumer,
   // {google:baseSuggestURL}.
   void GoogleBaseURLChanged(const GURL& old_base_url);
 
-  // Update the default search.  Called at initialization or when a managed
-  // preference has changed.
-  void UpdateDefaultSearch();
-
-  // Set the default search provider.  |url| may be user-selected or
-  // automatically selected and may be null.
-  // This will assert if the default search is managed.
-  void SetDefaultSearchProvider(TemplateURL* url);
-
-  // Set the default search provider even if it is managed. |url| may be null.
-  // Caller is responsible for notifying observers.  Returns whether |url| was
-  // found in |template_urls_|.
-  // If |url| is an extension-controlled search engine then preferences and the
-  // database are left untouched.
-  // If |url| is a normal search engine and the existing default search engine
-  // is controlled by an extension then |url| is propagated to the database and
-  // prefs but the extension-controlled default engine will continue to hide
-  // this value until the extension is uninstalled.
-  bool SetDefaultSearchProviderNoNotify(TemplateURL* url);
-
   // Adds a new TemplateURL to this model. TemplateURLService will own the
   // reference, and delete it when the TemplateURL is removed.
   // If |newly_adding| is false, we assume that this TemplateURL was already
@@ -575,14 +581,12 @@ class TemplateURLService : public WebDataServiceConsumer,
   // model is loaded.
   void NotifyObservers();
 
-  // Removes from the vector any template URL that was created because of
-  // policy.  These TemplateURLs are freed and removed from the database.
-  // Sets default_search_provider to NULL if it was one of them, unless it is
-  // the same as the current default from preferences and it is managed.
-  void RemoveProvidersCreatedByPolicy(
+  // Updates |template_urls| so that the only "created by policy" entry is
+  // |default_from_prefs|. |default_from_prefs| may be NULL if there is no
+  // policy-defined DSE in effect.
+  void UpdateProvidersCreatedByPolicy(
       TemplateURLVector* template_urls,
-      TemplateURL** default_search_provider,
-      TemplateURLData* default_from_prefs);
+      const TemplateURLData* default_from_prefs);
 
   // Resets the sync GUID of the specified TemplateURL and persists the change
   // to the database. This does not notify observers.
@@ -640,14 +644,6 @@ class TemplateURLService : public WebDataServiceConsumer,
                               SyncDataMap* local_data,
                               syncer::SyncMergeResult* merge_result);
 
-  // Checks a newly added TemplateURL from Sync by its sync_guid and sets it as
-  // the default search provider if we were waiting for it.
-  void SetDefaultSearchProviderIfNewlySynced(const std::string& guid);
-
-  // Retrieve the pending default search provider according to Sync. Returns
-  // NULL if there was no pending search provider from Sync.
-  TemplateURL* GetPendingSyncedDefaultSearchProvider();
-
   // Goes through a vector of TemplateURLs and ensure that both the in-memory
   // and database copies have valid sync_guids. This is to fix crbug.com/102038,
   // where old entries were being pushed to Sync without a sync_guid.
@@ -655,40 +651,34 @@ class TemplateURLService : public WebDataServiceConsumer,
 
   void OnSyncedDefaultSearchProviderGUIDChanged();
 
-  // Adds |template_urls| to |template_urls_| and sets up the default search
-  // provider.  If |default_search_provider| is non-NULL, it must refer to one
-  // of the |template_urls|, and will be used as the new default.
+  // Adds |template_urls| to |template_urls_|.
   //
   // This transfers ownership of the elements in |template_urls| to |this|, and
   // may delete some elements, so it's not safe for callers to access any
   // elements after calling; to reinforce this, this function clears
   // |template_urls| on exit.
-  void AddTemplateURLsAndSetupDefaultEngine(
-      TemplateURLVector* template_urls,
-      TemplateURL* default_search_provider);
-
-  // If there is no current default search provider, sets the default to the
-  // result of calling FindNewDefaultSearchProvider().
-  void EnsureDefaultSearchProviderExists();
+  void AddTemplateURLs(TemplateURLVector* template_urls);
 
   // Returns a new TemplateURL for the given extension.
   TemplateURL* CreateTemplateURLForExtension(
       const ExtensionKeyword& extension_keyword);
 
+  // Returns the TemplateURL corresponding to |prepopulated_id|, if any.
+  TemplateURL* FindPrepopulatedTemplateURL(int prepopulated_id);
+
   // Returns the TemplateURL associated with |extension_id|, if any.
   TemplateURL* FindTemplateURLForExtension(const std::string& extension_id,
                                            TemplateURL::Type type);
 
-  // Finds the most recently-installed NORMAL_CONTROLLED_BY_EXTENSION engine
-  // that supports replacement and wants to be default, if any.
-  TemplateURL* FindExtensionDefaultSearchEngine() const;
+  // Finds the extension-supplied TemplateURL that matches |data|, if any.
+  TemplateURL* FindMatchingExtensionTemplateURL(const TemplateURLData& data,
+                                                TemplateURL::Type type);
 
-  // Sets the default search provider to:
-  // (1) BestDefaultExtensionControlledTURL(), if any; or,
-  // (2) LoadDefaultSearchProviderFromPrefs(), if we have a TURL with that ID;
-  // or,
-  // (3) FindNewDefaultSearchProvider().
-  void SetDefaultSearchProviderAfterRemovingDefaultExtension();
+  // Finds the most recently-installed NORMAL_CONTROLLED_BY_EXTENSION engine
+  // that supports replacement and wants to be default, if any. Notifies the
+  // DefaultSearchManager, which might change the effective default search
+  // engine.
+  void UpdateExtensionDefaultSearchEngine();
 
   content::NotificationRegistrar notification_registrar_;
   PrefChangeRegistrar pref_change_registrar_;
@@ -735,12 +725,12 @@ class TemplateURLService : public WebDataServiceConsumer,
   // TemplateURL owned by |template_urls_|.
   TemplateURL* default_search_provider_;
 
-  // The initial search provider extracted from preferences. This is only valid
-  // if we haven't been loaded or loading failed.
+  // A temporary location for the DSE until Web Data has been loaded and it can
+  // be merged into |template_urls_|.
   scoped_ptr<TemplateURL> initial_default_search_provider_;
 
-  // Whether the default search is managed via policy.
-  bool is_default_search_managed_;
+  // Source of the default search provider.
+  DefaultSearchManager::Source default_search_provider_source_;
 
   // ID assigned to next TemplateURL added to this model. This is an ever
   // increasing integer that is initialized from the database.
@@ -765,13 +755,6 @@ class TemplateURLService : public WebDataServiceConsumer,
   // Sync's error handler. We use it to create a sync error.
   scoped_ptr<syncer::SyncErrorFactory> sync_error_factory_;
 
-  // Whether or not we are waiting on the default search provider to come in
-  // from Sync. This is to facilitate the fact that changes to the value of
-  // prefs::kSyncedDefaultSearchProviderGUID do not always come before the
-  // TemplateURL entry it refers to, and to handle the case when we want to use
-  // the Synced default when the default search provider becomes unmanaged.
-  bool pending_synced_default_search_;
-
   // A set of sync GUIDs denoting TemplateURLs that have been removed from this
   // model or the underlying WebDataService prior to MergeDataAndStartSyncing.
   // This set is used to determine what entries from the server we want to
@@ -789,6 +772,9 @@ class TemplateURLService : public WebDataServiceConsumer,
   // Helper class to manage the default search engine.
   DefaultSearchManager default_search_manager_;
 
+  // Used to disable the prepopulated search engines in tests.
+  static bool g_fallback_search_engines_disabled;
+
   DISALLOW_COPY_AND_ASSIGN(TemplateURLService);
 };