Separate storage for protected preferences into Protected Preferences file.

chrome/browser/prefs/profile_pref_store_manager.cc

Index: chrome/browser/prefs/profile_pref_store_manager.cc
diff --git a/chrome/browser/prefs/profile_pref_store_manager.cc b/chrome/browser/prefs/profile_pref_store_manager.cc
index fa6af810d4dba7d1c9864349f7670439db2bc01e..9e57a464675dffd7b52f81328947809058a52867 100644
--- a/chrome/browser/prefs/profile_pref_store_manager.cc
+++ b/chrome/browser/prefs/profile_pref_store_manager.cc
@@ -13,12 +13,102 @@
 #include "base/prefs/pref_registry_simple.h"
 #include "chrome/browser/prefs/pref_hash_store_impl.h"
 #include "chrome/browser/prefs/tracked/pref_service_hash_store_contents.h"
+#include "chrome/browser/prefs/tracked/protected_pref_store.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 
 namespace {
 
+// An adaptor that allows a PrefHashStoreImpl to access a preference store
+// directly as a dictionary. Uses an equivalent layout to
+// PrefStoreHashStoreContents.
+class DictionaryHashStoreContents : public HashStoreContents {
+ public:
+  // Instantiates a HashStoreContents that is a copy of |to_copy|. The copy is
+  // mutable but does not affect the original, nor is it persisted to disk in
+  // any other way.
+  explicit DictionaryHashStoreContents(const HashStoreContents& to_copy)
+      : hash_store_id_(to_copy.hash_store_id()),
+        super_mac_(to_copy.GetSuperMac()),
+        dictionary_(to_copy.IsInitialized()
+                        ? to_copy.GetContents()->DeepCopy()
+                        : static_cast<base::DictionaryValue*>(NULL)) {
+    int version = 0;
+    if (to_copy.GetVersion(&version))
+      version_.reset(new int(version));
+  }
+
+  // HashStoreContents implementation
+  virtual std::string hash_store_id() const OVERRIDE { return hash_store_id_; }
+
+  virtual void Reset() OVERRIDE {
+    dictionary_.reset();
+    super_mac_ = "";
+    version_.reset();
+  }
+
+  virtual bool IsInitialized() const OVERRIDE {
+    return dictionary_;
+  }
+
+  virtual const base::DictionaryValue* GetContents() const OVERRIDE{
+    return dictionary_.get();
+  }
+
+  virtual scoped_ptr<MutableDictionary> GetMutableContents() OVERRIDE {
+    return scoped_ptr<MutableDictionary>(
+        new SimpleMutableDictionary(this));
+  }
+
+  virtual std::string GetSuperMac() const  OVERRIDE {

[robertshield, 2014/03/25 03:05:12]

nit: double space

[erikwright (departed), 2014/03/25 20:28:26]

Done.

+    return super_mac_;
+  }
+
+  virtual void SetSuperMac(const std::string& super_mac) OVERRIDE {

[robertshield, 2014/03/25 03:05:12]

please update hash_store_contents.h with a description of what a "super mac" is.

[erikwright (departed), 2014/03/25 20:28:26]

Done here:
https://codereview.chromium.org/205563009/

+    super_mac_ = super_mac;
+  }
+
+  virtual bool GetVersion(int* version) const  OVERRIDE {

[robertshield, 2014/03/25 03:05:12]

nit: double space

[erikwright (departed), 2014/03/25 20:28:26]

Done.

+    if (!version_)
+      return false;
+    *version = *version_;
+    return true;
+  }
+
+  virtual void SetVersion(int version) OVERRIDE {
+    version_.reset(new int(version));
+  }
+
+ private:
+  class SimpleMutableDictionary
+      : public HashStoreContents::MutableDictionary {
+   public:
+    explicit SimpleMutableDictionary(DictionaryHashStoreContents* outer)
+        : outer_(outer) {}
+
+    virtual ~SimpleMutableDictionary() {}
+
+    // MutableDictionary implementation
+    virtual base::DictionaryValue* operator->() OVERRIDE{
+      if (!outer_->dictionary_)
+        outer_->dictionary_.reset(new base::DictionaryValue);
+      return outer_->dictionary_.get();
+    }
+
+   private:
+    DictionaryHashStoreContents* outer_;
+    DISALLOW_COPY_AND_ASSIGN(SimpleMutableDictionary);
+  };
+
+  const std::string hash_store_id_;
+  std::string super_mac_;
+  scoped_ptr<int> version_;

[robertshield, 2014/03/25 03:05:12]

I wish HashStoreContents had

const int kInvalidVersion = -1;

which would remove the need for using a NULL-valued scoped_ptr to serve as an "I
don't have a version" marker.

[erikwright (departed), 2014/03/25 20:28:26]

The version number will go away in a little while.

This could be a bool and an int. Do you like that better?

[Bernhard Bauer, 2014/03/26 15:05:50]

FWIW, I would prefer a negative value for "no version" as well (I was going to
write a comment to that effect, before I found this one).

+  scoped_ptr<base::DictionaryValue> dictionary_;
+
+  DISALLOW_COPY_AND_ASSIGN(DictionaryHashStoreContents);
+};
+
 // An in-memory PrefStore backed by an immutable DictionaryValue.
 class DictionaryPrefStore : public PrefStore {
  public:
@@ -174,15 +264,55 @@ void ProfilePrefStoreManager::ResetPrefHashStore() {
 PersistentPrefStore* ProfilePrefStoreManager::CreateProfilePrefStore(
     const scoped_refptr<base::SequencedTaskRunner>& io_task_runner) {
   scoped_ptr<PrefFilter> pref_filter;
-  if (kPlatformSupportsPreferenceTracking) {
-    pref_filter.reset(
-        new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
-                           tracking_configuration_,
-                           reporting_ids_count_));
+  if (!kPlatformSupportsPreferenceTracking) {
+    return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
+                             io_task_runner,
+                             scoped_ptr<PrefFilter>());
   }
-  return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
-                           io_task_runner,
-                           pref_filter.Pass());
+
+  std::vector<PrefHashFilter::TrackedPreferenceMetadata>
+      unprotected_configuration;
+  std::vector<PrefHashFilter::TrackedPreferenceMetadata>
+      protected_configuration;
+  std::set<std::string> protected_pref_names;
+  for (size_t i = 0; i < tracking_configuration_.size(); ++i) {
+    bool is_protected = tracking_configuration_[i].enforcement_level >
+                        PrefHashFilter::NO_ENFORCEMENT;
+    (is_protected ? protected_configuration : unprotected_configuration)
+        .push_back(tracking_configuration_[i]);
+    if (is_protected)
+      protected_pref_names.insert(tracking_configuration_[i].name);

[robertshield, 2014/03/25 03:05:12]

I find the following easier to read:

if (is_protected) {
  protected_configuration.push_back(tracking_configuration_[i]);
  protected_pref_names.insert(tracking_configuration_[i].name);
} else {
  unprotected_configuration.push_back(tracking_configuration_[i]);
}

  

[erikwright (departed), 2014/03/25 20:28:26]

Done.

+  }
+
+  scoped_ptr<PrefHashFilter> unprotected_pref_hash_filter(
+      new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+                         unprotected_configuration,
+                         reporting_ids_count_));
+  scoped_ptr<PrefHashFilter> protected_pref_hash_filter(
+      new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+                         protected_configuration,
+                         reporting_ids_count_));
+
+  scoped_refptr<PersistentPrefStore> unprotected_pref_store(
+      new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
+                        io_task_runner,
+                        unprotected_pref_hash_filter.PassAs<PrefFilter>()));
+  scoped_refptr<PersistentPrefStore> protected_pref_store(new JsonPrefStore(
+      profile_path_.Append(chrome::kProtectedPreferencesFilename),
+      io_task_runner,
+      protected_pref_hash_filter.PassAs<PrefFilter>()));
+
+  return new ProtectedPrefStore(
+      unprotected_pref_store,
+      protected_pref_store,
+      protected_pref_names,
+      base::Bind(&PrefHashFilter::MigrateValues,
+                 base::Owned(new PrefHashFilter(
+                     CopyPrefHashStore(),
+                     protected_configuration,
+                     reporting_ids_count_)),
+                 unprotected_pref_store,
+                 protected_pref_store));
 }
 
 void ProfilePrefStoreManager::UpdateProfileHashStoreIfRequired(
@@ -218,6 +348,8 @@ bool ProfilePrefStoreManager::InitializePrefsFromMasterPrefs(
   if (!base::CreateDirectory(profile_path_))
     return false;
 
+  // This will actually write out to a single combined file but it will be
+  // immediately migrated to two files on load.

[robertshield, 2014/03/25 03:05:12]

Suggestion: "This will write out to a single combined file which will be
immediately migrated into two files on load."

[erikwright (departed), 2014/03/25 20:28:26]

Done.

   JSONFileValueSerializer serializer(
       GetPrefFilePathFromProfilePath(profile_path_));
 
@@ -240,6 +372,23 @@ bool ProfilePrefStoreManager::InitializePrefsFromMasterPrefs(
   return success;
 }
 
+PersistentPrefStore*
+ProfilePrefStoreManager::CreateDeprecatedCombinedProfilePrefStore(
+    const scoped_refptr<base::SequencedTaskRunner>& io_task_runner) {
+  if (!kPlatformSupportsPreferenceTracking) {
+    return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
+                             io_task_runner,
+                             scoped_ptr<PrefFilter>());
+  }
+
+  return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
+                           io_task_runner,
+                           scoped_ptr<PrefFilter>(new PrefHashFilter(
+                               GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+                               tracking_configuration_,
+                               reporting_ids_count_)));

[robertshield, 2014/03/25 03:05:12]

Can the above be replaced by:

scoped_ptr<PrefFilter> pref_filter;
if (kPlatformSupportsPreferenceTracking) {
  pref_filter.reset(new PrefHashFilter(
      GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
      tracking_configuration_, reporting_ids_count_))
}
 
return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
                         io_task_runner, pref_filter);

[erikwright (departed), 2014/03/25 20:28:26]

Done.

+}
+
 scoped_ptr<PrefHashStoreImpl> ProfilePrefStoreManager::GetPrefHashStoreImpl() {
   DCHECK(kPlatformSupportsPreferenceTracking);
 
@@ -249,3 +398,15 @@ scoped_ptr<PrefHashStoreImpl> ProfilePrefStoreManager::GetPrefHashStoreImpl() {
       scoped_ptr<HashStoreContents>(new PrefServiceHashStoreContents(
           profile_path_.AsUTF8Unsafe(), local_state_))));
 }
+
+scoped_ptr<PrefHashStore> ProfilePrefStoreManager::CopyPrefHashStore() {
+  DCHECK(kPlatformSupportsPreferenceTracking);
+
+  PrefServiceHashStoreContents real_contents(profile_path_.AsUTF8Unsafe(),
+                                             local_state_);
+  return scoped_ptr<PrefHashStore>(new PrefHashStoreImpl(
+      seed_,
+      device_id_,
+      scoped_ptr<HashStoreContents>(
+          new DictionaryHashStoreContents(real_contents))));
+}