Separate storage for protected preferences into Protected Preferences file.

chrome/browser/prefs/profile_pref_store_manager.cc

 // 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.
 
 #include "chrome/browser/prefs/profile_pref_store_manager.h"
 
 #include "base/file_util.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
@@ skipping 140 matching lines @@
   return profile_path.Append(chrome::kPreferencesFilename);
 }
 
 // static
 void ProfilePrefStoreManager::ResetAllPrefHashStores(PrefService* local_state) {
   PrefServiceHashStoreContents::ResetAllPrefHashStores(local_state);
 }
 
 //  static
 base::Time ProfilePrefStoreManager::GetResetTime(PrefService* pref_service) {
+  // It's a bit of a coincidence that this (and ClearResetTime) work(s). The

[gab, 2014/04/01 18:55:06]

:( -- to avoid all of this madness how about we use PrefService to Set/Get (we
can't do this directly at the time of Set() since the PrefService doesn't exist
yet, but we could post a task to have an anonymous method kick in to do this
properly when the message loop starts running).

Feels cleaner than depending so tightly on the implementation for this to work.

[erikwright (departed), 2014/04/01 19:46:08]

The current implementation is cleaner, easier to understand, and has less moving
parts than what you propose.

If I were really worried I would just expose the name of the pref from
PrefHashFilter and then add it to the list of selected prefs that I pass to the
segregated pref store. Then I would know for certain that it was stored in the
right place.

But it's not really tightly coupled to anything too far removed. Remember that
it is _this_ class (ProfilePrefStoreManager) that is responsible for doing the
configuration - so if the behaviour changes, this class will be part of the
change.

Finally, there are unit tests that prevent unintended regressions of this
functionality.

[gab, 2014/04/02 17:02:03]

Okay, makes sense, I agree with you now that I understand the
SegregatedPrefStore (I hadn't reviewed that change yet when I wrote this comment
originally).

This comment might sound more alarming than it needs to be, but that's okay.

+  // PrefHashFilter attached to the protected pref store will store the reset
+  // time directly in the protected pref store without going through the
+  // SegregatedPrefStore.
+
+  // PrefHashFilter::GetResetTime will read the value through the pref service,
+  // and thus through the SegregatedPrefStore. Even though it's not listed as
+  // "protected" it will be read from the protected store prefentially to the

[gab, 2014/04/01 18:55:06]

s/prefentially/preferentially

+  // (NULL) value in the unprotected pref store.
   return PrefHashFilter::GetResetTime(pref_service);
 }
 
 // static
 void ProfilePrefStoreManager::ClearResetTime(PrefService* pref_service) {
+  // PrefHashFilter::ClearResetTime will clear the value through the pref
+  // service, and thus through the SegregatedPrefStore. Since it's not listed as
+  // "protected" it will be migrated from the protected store to the unprotected
+  // pref store before being deleted from the latter.
   PrefHashFilter::ClearResetTime(pref_service);
 }
 
 void ProfilePrefStoreManager::ResetPrefHashStore() {
   if (kPlatformSupportsPreferenceTracking)
     GetPrefHashStoreImpl()->Reset();
 }
 
 PersistentPrefStore* ProfilePrefStoreManager::CreateProfilePrefStore(
     const scoped_refptr<base::SequencedTaskRunner>& io_task_runner) {
@@ skipping 66 matching lines @@
 
 scoped_ptr<PrefHashStoreImpl> ProfilePrefStoreManager::GetPrefHashStoreImpl() {
   DCHECK(kPlatformSupportsPreferenceTracking);
 
   return make_scoped_ptr(new PrefHashStoreImpl(
       seed_,
       device_id_,
       scoped_ptr<HashStoreContents>(new PrefServiceHashStoreContents(
           profile_path_.AsUTF8Unsafe(), local_state_))));
 }