Split PrefService into PrefService, PrefServiceSimple and PrefServiceSyncable.

chrome/browser/prefs/chrome_pref_service_factory.cc

 // Copyright (c) 2012 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/prefs/chrome_pref_service_builder.h"
+#include "chrome/browser/prefs/chrome_pref_service_factory.h"
 
 #include "base/bind.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/default_pref_store.h"
 #include "base/prefs/json_pref_store.h"
 #include "chrome/browser/policy/configuration_policy_pref_store.h"
 #include "chrome/browser/prefs/command_line_pref_store.h"
 #include "chrome/browser/prefs/pref_model_associator.h"
 #include "chrome/browser/prefs/pref_notifier_impl.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/pref_value_store.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/profile_error_dialog.h"
 #include "content/public/browser/browser_context.h"
 #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;
 
 namespace {
 
 // Forwards a notification after a PostMessage so that we can wait for the
 // MessageLoop to run.
 void NotifyReadError(int message_id) {
   ShowProfileErrorDialog(message_id);
@@ skipping 21 matching lines @@
   }
 }
 
 }  // namespace
 
 // TODO(joi): Find a better home for this.
 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,
     PrefStore* extension_prefs,
     bool async) {
 #if defined(OS_LINUX)
   // We'd like to see what fraction of our users have the preferences
   // stored on a network file system, as we've had no end of troubles
   // with NFS/AFS.
   // TODO(evanm): remove this once we've collected state.
   file_util::FileSystemType fstype;
   if (file_util::GetFileSystemType(pref_filename.DirName(), &fstype)) {
     UMA_HISTOGRAM_ENUMERATION("PrefService.FileSystemType",
                               static_cast<int>(fstype),
                               file_util::FILE_SYSTEM_TYPE_COUNT);
   }
 #endif
 
 #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));
 }