Split PrefService into PrefService, PrefServiceSimple and PrefServiceSyncable.

chrome/browser/prefs/chrome_pref_service_factory.cc

Index: chrome/browser/prefs/chrome_pref_service_factory.cc
diff --git a/chrome/browser/prefs/chrome_pref_service_builder.cc b/chrome/browser/prefs/chrome_pref_service_factory.cc
similarity index 69%
rename from chrome/browser/prefs/chrome_pref_service_builder.cc
rename to chrome/browser/prefs/chrome_pref_service_factory.cc
index d3fddeacd31a2732113ef2b4ef542fbd6b66d66f..6188641c812944d25f312c1ab3eb5ea28627b4e5 100644
--- a/chrome/browser/prefs/chrome_pref_service_builder.cc
+++ b/chrome/browser/prefs/chrome_pref_service_factory.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/prefs/chrome_pref_service_builder.h"
+#include "chrome/browser/prefs/chrome_pref_service_factory.h"
 
 #include "base/bind.h"
 #include "base/file_path.h"
@@ -22,7 +22,6 @@
 #include "content/public/browser/browser_thread.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
-#include "ui/base/l10n/l10n_util.h"
 
 using content::BrowserContext;
 using content::BrowserThread;
@@ -64,14 +63,50 @@ PrefServiceBase* PrefServiceBase::FromBrowserContext(BrowserContext* context) {
   return static_cast<Profile*>(context)->GetPrefs();
 }
 
-ChromePrefServiceBuilder::ChromePrefServiceBuilder() {
+ChromePrefServiceFactory::ChromePrefServiceFactory() {
   ResetDefaultState();
 }
 
-ChromePrefServiceBuilder::~ChromePrefServiceBuilder() {
+ChromePrefServiceFactory::~ChromePrefServiceFactory() {
 }
 
-PrefService* ChromePrefServiceBuilder::CreateChromePrefs(
+PrefServiceSimple* ChromePrefServiceFactory::CreateLocalState(
+    const FilePath& pref_filename,
+    base::SequencedTaskRunner* pref_io_task_runner,
+    policy::PolicyService* policy_service,
+    PrefStore* extension_prefs,
+    bool async) {
+  PrepareBuilder(pref_filename,
+                 pref_io_task_runner,
+                 policy_service,
+                 extension_prefs,
+                 async);
+
+  PrefServiceSimple* pref_service = new PrefServiceSimple();

[Mattias Nissler (ping if slow), 2012/12/20 13:41:52]

I think I posted this somewhere already, but to reiterate: The builder should
really call the constructor, I think we want separate PrefServiceBuilder
subclasses for the separate PrefService derivatives. I'd rather not see
half-initialized PrefService instances.

[Jói, 2012/12/20 16:30:31]

We addressed that by making the constructor private, and making
ChromePrefServiceFactory a friend.  We can switch back to using the constructor
if you still prefer that approach, but the current approach does avoid any
half-initialized PrefService objects.

[Mattias Nissler (ping if slow), 2012/12/21 13:09:31]

I would really prefer the ctor approach, which would be a clean implementation
of the builder pattern that people are much more likely to understand than the
convoluted protected-ctor and friended-factory using a builder internally
approach.

Bonus: With a clean builder pattern, it'll be easy to move standalone-usable
core implementations of PrefService and its associated PrefServiceBuilder into
base/prefs, while chrome/browser/prefs can provide an extended builder that can
also construct syncable PrefService instances (possibly instantiating a
different PrefService subclass). The approach you have requires a friended
factory for every PrefService subclass, which mostly annihilates the usefulness
of PrefServiceBuilder since you cannot use it directly to construct special
PrefService instances any longer.

Finally, here's a consistency argument: PrefServiceMockBuilder implements
exactly what I proposed above, why should we do it differently (through the
factory) for production PrefService instances? How about we rename
PrefServiceMockBuilder to ChromePrefServiceBuilder (the CreateSimple() and
CreateSyncable() functions are already there!) and get rid of
ChromePrefServiceFactory entirely?

+  builder_.Build(pref_service);
+  ResetDefaultState();
+  return pref_service;
+}
+
+PrefServiceSyncable* ChromePrefServiceFactory::CreateProfilePrefs(
+    const FilePath& pref_filename,
+    base::SequencedTaskRunner* pref_io_task_runner,
+    policy::PolicyService* policy_service,
+    PrefStore* extension_prefs,
+    bool async) {
+  PrepareBuilder(pref_filename,
+                 pref_io_task_runner,
+                 policy_service,
+                 extension_prefs,
+                 async);
+
+  PrefServiceSyncable* pref_service = new PrefServiceSyncable();
+  builder_.Build(pref_service);
+  ResetDefaultState();
+  return pref_service;
+}
+
+void ChromePrefServiceFactory::PrepareBuilder(
     const FilePath& pref_filename,
     base::SequencedTaskRunner* pref_io_task_runner,
     policy::PolicyService* policy_service,
@@ -92,26 +127,20 @@ PrefService* ChromePrefServiceBuilder::CreateChromePrefs(
 
 #if defined(ENABLE_CONFIGURATION_POLICY)
   using policy::ConfigurationPolicyPrefStore;
-  WithManagedPrefs(
+  builder_.WithManagedPrefs(
       ConfigurationPolicyPrefStore::CreateMandatoryPolicyPrefStore(
           policy_service));
-  WithRecommendedPrefs(
+  builder_.WithRecommendedPrefs(
       ConfigurationPolicyPrefStore::CreateRecommendedPolicyPrefStore(
           policy_service));
 #endif  // ENABLE_CONFIGURATION_POLICY
 
-  WithAsync(async);
-  WithCommandLinePrefs(
+  builder_.WithAsync(async);
+  builder_.WithCommandLinePrefs(
       new CommandLinePrefStore(CommandLine::ForCurrentProcess()));
-  WithUserPrefs(new JsonPrefStore(pref_filename, pref_io_task_runner));
-
-  PrefService* pref_service = Create();
-  ResetDefaultState();
-  return pref_service;
+  builder_.WithUserPrefs(new JsonPrefStore(pref_filename, pref_io_task_runner));
 }
 
-void ChromePrefServiceBuilder::ResetDefaultState() {
-  WithLocalizedStringMethod(base::Bind(&l10n_util::GetStringUTF8));
-  WithReadErrorCallback(base::Bind(&HandleReadError));
-  WithSyncAssociator(new PrefModelAssociator());
+void ChromePrefServiceFactory::ResetDefaultState() {
+  builder_.WithReadErrorCallback(base::Bind(&HandleReadError));
 }