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 via a non-const
+  // method.
+  // |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:
+  // Observes events from the underlying stores and synthesizes external events
+  // via |on_initialization|, |read_error_delegate_|, and |observers_|.
+  class TeeObserver : public PrefStore::Observer {

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

Hmm... do you even need this class? The outer class could just as well directly
observe its pref stores. Then you wouldn't need to reach into the |outer_|
object like you do now.

[erikwright (departed), 2014/03/26 15:17:14]

In my personal opinion it breaks encapsulation by exposing the PrefStoreObserver
interface to the client of ProtectedPrefStore.

I believe that the current design is a model for how a class can implement
multiple interfaces while enforcing encapsulation.

[Bernhard Bauer, 2014/03/26 16:41:11]

Hm, I would have been okay with that. Clients do see implementation details of
this class already (like private methods), they just can't access them. I don't
think it's the end of the world if a client could now in theory register this
class as an observer of a different pref store, because there would be no reason
to do so.

If you really don't want to expose the PrefStore::Observer interface, you could
make a private subclass of this class that implements it (with a factory method
in this class).

[erikwright (departed), 2014/03/26 21:08:12]

I don't really agree with the reasoning.

I find the subclass + factory method approach to be the least desirable of
everything proposed.

If you want me to make ProtectedPrefStore implement PrefStore::Observer directly
I will. But I would like to know what the specific objection is to the current
design.

[Bernhard Bauer, 2014/03/27 15:10:45]

Some problems with the current design:
* You have non-zero overhead due to the additional |outer_| member
* You need to reach into |outer_| and access private members
* As previously discussed on the review, the class name isn't very intuitive

Now, granted, none of these are big issues, it's just that exposing the
PrefStore::Observer interface to clients isn't a big issue to me either. In
fact, we have a lot of instances where a class subscribes itself as an observer
(e.g. almost all classes implement NotificationObserver), and if that class is
public, it will expose the observer interface to its clients.

All of that being said, if you want to keep the current approach, I won't block
it; I just wanted to put all the options on the table.

[erikwright (departed), 2014/03/27 19:26:21]

You mean the extra dereference? I suppose so, in theory, but I don't think
that's relevant here.
There will always be the exact same amount of logic sharing state - either in
two nested classes or joined together in a single class. Although there are
other ways of sharing the state (introducing a third class/type to hold the
shared state, sharing pointers/copying the values of the shared state, ...) I
don't believe they are justified here.

Sharing the state and using a nested class helps to decompose the logic of this
class into two related but clearly delineated parts, while also presenting a
robust, provably safe, and clear public API.
We agreed to change this to AggregatingObserver, which narrowly beat out
UpsideDownTeeObserver and EetObserver.
I agree that it is common, but I think that we can also agree that the presence
of a pattern in the Chromium codebase does not, in and of itself, indicate that
it is a good pattern or without tradeoffs. I believe that the approach
demonstrated in this CL represents an improvement over that pattern.
Thank you for the discussion and the flexibility.

+   public:
+    explicit TeeObserver(ProtectedPrefStore* outer);
+
+    // PrefStore::Observer implementation
+    virtual void OnPrefValueChanged(const std::string& key) OVERRIDE;
+    virtual void OnInitializationCompleted(bool succeeded) OVERRIDE;
+
+   private:
+    ProtectedPrefStore* outer_;
+    uint8 failed_sub_initializations_;

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

Nit: The style guide says to only use unsigned types if you really need them
(because you store bit patterns, or need defined overflow behavior), and in
general use plain ints for small integral types.

[erikwright (departed), 2014/03/26 21:08:12]

Done.

+    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_;

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

It's a bit strange that the ProtectedPrefStore has a |protected_pref_store_| ;-)
Should this class maybe be called "UnifiedPrefStore" or "MergedPrefStore" or
something?

[erikwright (departed), 2014/03/26 15:17:14]

We all agree on this.

It's a bit more than a Unified or Merged pref store because it has logic
specific to protection (i.e., it doesn't solve any generalized problem of
aggregating two stores).

PartiallyProtectedPrefStore? No.

How about SegregatedPrefStore? That's really the problem this solves -
segregating a subset of prefs into a separate store. The reason they are
segregated (that they are protected) is not so important.

[Bernhard Bauer, 2014/03/26 16:41:11]

SGTM.

+  std::set<std::string> protected_preference_names_;
+  base::Closure on_initialization_;
+
+  scoped_ptr<PersistentPrefStore::ReadErrorDelegate> read_error_delegate_;
+  ObserverList<PrefStore::Observer, true> observers_;
+  TeeObserver tee_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProtectedPrefStore);
+};
+
+#endif  // CHROME_BROWSER_PREFS_TRACKED_PROTECTED_PREF_STORE_H_