Separate storage for protected preferences into Protected Preferences file.

chrome/browser/prefs/tracked/protected_pref_store.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_PREFS_TRACKED_PROTECTED_PREF_STORE_H_
+#define CHROME_BROWSER_PREFS_TRACKED_PROTECTED_PREF_STORE_H_
+
+#include <set>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/compiler_specific.h"
+#include "base/macros.h"
+#include "base/observer_list.h"
+#include "base/prefs/persistent_pref_store.h"
+#include "base/strings/string_piece.h"
+#include "chrome/browser/prefs/pref_hash_filter.h"
+
+class PrefHashStore;
+
+
+// Provides a unified PersistentPrefStore implementation that splits its storage
+// and retrieval between two underlying PersistentPrefStore instances: one each
+// for protected and unprotected preferences.
+class ProtectedPrefStore : public PersistentPrefStore {
+ public:
+  // Creates an instance that delegates to |protected_pref_store| for the
+  // preferences named in |protected_pref_names| and to |unprotected_pref_store|
+  // for all others. If an unprotected preference is present in
+  // |protected_pref_store| (i.e. because it was previously protected) it will
+  // be migrated back to |unprotected_pref_store| upon access.
+  // |on_initialization| will be invoked when both stores have been initialized,
+  // before observers of the combined store are notified.
+  ProtectedPrefStore(
+      const scoped_refptr<PersistentPrefStore>& unprotected_pref_store,
+      const scoped_refptr<PersistentPrefStore>& protected_pref_store,
+      const std::set<std::string>& protected_pref_names,
+      const base::Closure& on_initialization);
+
+  // PrefStore implementation
+  virtual void AddObserver(Observer* observer) OVERRIDE;
+  virtual void RemoveObserver(Observer* observer) OVERRIDE;
+  virtual bool HasObservers() const OVERRIDE;
+  virtual bool IsInitializationComplete() const OVERRIDE;
+  virtual bool GetValue(const std::string& key,
+                        const base::Value** result) const OVERRIDE;
+
+  // WriteablePrefStore implementation
+  virtual void SetValue(const std::string& key, base::Value* value) OVERRIDE;
+  virtual void RemoveValue(const std::string& key) OVERRIDE;
+
+  // PersistentPrefStore implementation
+  virtual bool GetMutableValue(const std::string& key,
+                               base::Value** result) OVERRIDE;
+  virtual void ReportValueChanged(const std::string& key) OVERRIDE;
+  virtual void SetValueSilently(const std::string& key,
+                                base::Value* value) OVERRIDE;
+  virtual bool ReadOnly() const OVERRIDE;
+  virtual PrefReadError GetReadError() const OVERRIDE;
+  virtual PrefReadError ReadPrefs() OVERRIDE;
+  virtual void ReadPrefsAsync(ReadErrorDelegate* error_delegate) OVERRIDE;
+  virtual void CommitPendingWrite() OVERRIDE;
+
+ private:
+  // Combines events from the underlying stores and reports them to the
+  // observers of the combined store.

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

Also document how this invokes the on_initialization callback.

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

Done.

+  class TeeObserver : public PrefStore::Observer {

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

Naming is hard. NotificationAggregator maybe? Maybe not, your name is fine too.

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

In Unix there is a utility named tee that takes input from one thing and outputs
it to two things. Although this is the reverse, it was my naming inspiration. I
don't mind changing it.

There will be a reviewer from Munich and I'll get them to weigh in before I go
to the trouble.

+   public:
+    explicit TeeObserver(ProtectedPrefStore* outer);

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

Should pass just the |observers_| list and the |on_initialization_| closure here
instead of the whole outer class. 

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

There are three members of the outer class that are accessed by the inner class.
Two of them are only accessed by the inner class (read_error_delegate_ and
on_initialization_). observers_ is accessed by three methods of the outer class
(AddObserver, RemoveObserver, and HasObservers). Forwarding those three methods
to the inner class is possible but cumbersome and also turns the Observer
implementation into something more than an observer implementation.

So we are left with the fact that there _will_ be shared state (i.e., at a
minimum, TeeObserver must have a pointer to observers_ as well as the two other
values, which it could be the owner of).

I don't really mind, but truth be told I think that the current design is
correct and good. The inner class is part of the implementation of the outer
class, it's OK and normal for them to share state.

I'll upload another patchset with things the way you suggested, and you/someone
in Munich can compare the two approaches.

[erikwright (departed), 2014/03/25 20:41:11]

Also, in the current patchset we now access outer_->GetReadError() from the
inner class. So we are not going to avoid having a pointer to the outer class.

With that in mind, I think the current design is now definitively better. An
alternative would be to have only a pointer to outer_, and simply forward all of
the calls that access outer_'s private state to private methods of outer_. But
that doesn't seem different or better to me.

+
+    // PrefStore::Observer implementation
+    virtual void OnPrefValueChanged(const std::string& key) OVERRIDE;
+    virtual void OnInitializationCompleted(bool succeeded) OVERRIDE;
+
+   private:
+    ProtectedPrefStore* outer_;
+    uint8 failed_sub_initializations_;
+    uint8 successful_sub_initializations_;
+
+    DISALLOW_COPY_AND_ASSIGN(TeeObserver);
+  };
+
+  virtual ~ProtectedPrefStore();
+
+  // Returns |protected_pref_store| if |key| is protected or a value is present
+  // in |protected_pref_store|. This could happen if |key| was previously
+  // protected.
+  const PersistentPrefStore* StoreForKey(const std::string& key) const;
+
+  // Returns |protected_pref_store| if |key| is protected. If |key| is
+  // unprotected but has a value in |protected_pref_store|, moves the value to
+  // |unprotected_pref_store| before returning |unprotected_pref_store|.
+  PersistentPrefStore* StoreForKey(const std::string& key);
+
+  scoped_refptr<PersistentPrefStore> unprotected_pref_store_;
+  scoped_refptr<PersistentPrefStore> protected_pref_store_;
+  std::set<std::string> protected_preference_names_;
+  base::Closure on_initialization_;
+
+  ObserverList<PrefStore::Observer, true> observers_;
+  TeeObserver tee_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProtectedPrefStore);
+};
+
+#endif  // CHROME_BROWSER_PREFS_TRACKED_PROTECTED_PREF_STORE_H_