Store default search provider data in dictionary pref.

chrome/browser/search_engines/default_search_manager.h

+// 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.
+
+#ifndef CHROME_BROWSER_SEARCH_ENGINES_DEFAULT_SEARCH_MANAGER_H_
+#define CHROME_BROWSER_SEARCH_ENGINES_DEFAULT_SEARCH_MANAGER_H_
+
+#include "base/basictypes.h"
+
+class PrefService;
+class Profile;
+struct TemplateURLData;
+
+class DefaultSearchManager {
+ public:
+  DefaultSearchManager();

[erikwright (departed), 2014/04/23 14:18:01]

I think this constructor should take a PrefService*. It doesn't make sense to
maintain state if we are not tied to a single PrefService (what would it mean to
receive different PS pointers to subsequent calls to the various methods?).

[Cait (Slow), 2014/04/23 19:10:05]

Done.

+
+  // Sets |default_search_provider_| and updates Prefs.
+  void SetUserSelectedDefaultSearchProvider(PrefService* prefs,
+                                            TemplateURLData* url);

[erikwright (departed), 2014/04/23 14:18:01]

const ref TUData?

[Cait (Slow), 2014/04/23 19:10:05]

Done.

+
+  // Returns the default search provider.
+  TemplateURLData* GetDefaultSearchProvider();

[erikwright (departed), 2014/04/23 14:18:01]

return const-ref?

[Cait (Slow), 2014/04/23 19:10:05]

Done.

+
+  // Sets |default_search_provider_|.
+  void SetDefaultSearchProvider(TemplateURLData* url);

[erikwright (departed), 2014/04/23 14:18:01]

const-ref?

[Cait (Slow), 2014/04/23 19:10:05]

Done.

+
+  // Read default search provider data from the pref service provided.
+  bool ReadFromPrefService(PrefService* prefs, TemplateURLData* url);

[erikwright (departed), 2014/04/23 14:18:01]

With the existing implementation there is a concept of an "explicitly NULL" DSE.
i.e., the user has said there is none, so the default should not be used.

To support that, presumably this method could return true while leaving url
NULL, right? If so, please document.

Also note in comments that |url| will (presumably) remain owned by this class
and may be invalidated by subsequent changes in the pref service.

[erikwright (departed), 2014/04/23 14:18:01]

I don't think there's a good reason to have separate methods for doing this.
Just take the PS in the constructor, and every Get/Set will be from the PS.

It's no big deal to construct one of these and then use it just to do a single
read. One can even do:

  bool result = DefaultSearchManager(pref_service).ReadFromPrefService(&url);

[Cait (Slow), 2014/04/23 17:26:30]

Hmm.. I'm not sure what the best way to handle the "DSE was explicitly set to
NULL" case is if we're reading from prefs every time (can Prefs differentiate
between "this value was never set" and "this value was explicitly set to null"?)


Also, if we're using a const ref to pass the TUData to this class to save the
data to prefs, we *can't* pass a null ref.

Do we really think there is a need to support setting the DSE to NULL? Certainly
in the UI at least, there is no way to explicitly do this. In the existing code,
it seems to be used primarily as a signal that we need to find a new DSE from
somewhere. WDYT?

+
+  // Write default search provider data to the pref service provided.
+  void SaveToPrefService(PrefService* prefs, const TemplateURLData* data);

[erikwright (departed), 2014/04/23 14:18:01]

const-ref?

[Cait (Slow), 2014/04/23 19:10:05]

Done.

+
+ private:
+  TemplateURLData* default_search_provider_;

[erikwright (departed), 2014/04/23 14:18:01]

scoped_ptr?

[Cait (Slow), 2014/04/23 19:10:05]

Removing this altogether until there is an explicit need for it.

+
+  DISALLOW_COPY_AND_ASSIGN(DefaultSearchManager);
+};
+
+#endif  // CHROME_BROWSER_SEARCH_ENGINES_DEFAULT_SEARCH_MANAGER_H_