Add TrackedPreferenceValidationDelegate

chrome/browser/prefs/pref_hash_filter.h

Index: chrome/browser/prefs/pref_hash_filter.h
diff --git a/chrome/browser/prefs/pref_hash_filter.h b/chrome/browser/prefs/pref_hash_filter.h
index cb795b53becf21365cffc8572e05544aa5e1bb39..a491427623182c69d9fbc25ee017800047360e23 100644
--- a/chrome/browser/prefs/pref_hash_filter.h
+++ b/chrome/browser/prefs/pref_hash_filter.h
@@ -18,6 +18,7 @@
 
 class PrefService;
 class PrefStore;
+class TrackedPreferenceValidationObserver;
 
 namespace base {
 class DictionaryValue;
@@ -56,12 +57,14 @@ class PrefHashFilter : public InterceptablePrefFilter {
   };
 
   // Constructs a PrefHashFilter tracking the specified |tracked_preferences|
-  // using |pref_hash_store| to check/store hashes.
+  // using |pref_hash_store| to check/store hashes. An optional |observer| is
+  // notified of the status of each preference as it is checked.
   // |reporting_ids_count| is the count of all possible IDs (possibly greater
   // than |tracked_preferences.size()|).
   PrefHashFilter(
       scoped_ptr<PrefHashStore> pref_hash_store,
       const std::vector<TrackedPreferenceMetadata>& tracked_preferences,
+      scoped_ptr<TrackedPreferenceValidationObserver> observer,

[Mattias Nissler (ping if slow), 2014/05/15 14:32:25]

I may be lacking context, but it's generally rather hard to work with an
interface where the Observer is owned by the event source. The usual pattern is
that something creates and handles ownership for the event source
(PrefHashFilter in this case) and registers an observer on it. Now if the source
owns the observer, you'll get a cyclic reference in your ownership graph.

[erikwright (departed), 2014/05/15 14:44:51]

In this scenario, we are augmenting the functionality of a pref store. The
objects that implement that functionality must have a lifetime that is similar
to that of the pref store. As the pref store is ref counted, the only reliable
way to do that is to have ownership in this form.

Even without the refcounting, I believe this is a better approach in general for
a number of reasons:
(1) Adding/removing observers is hard to implement correctly, especially when no
one really ever wants to remove one or add more than one.
(2) When the observer is not attached to a top-level object but, rather, to a
sub-object that is indirectly owned by some top-level object, detaching it
requires holding onto that embedded object, and for no other purpose.
(3) If the client removes their observer, the client must have some guarantees
to be called before the observed object is destroyed.
(4) Observers that are not provided at constructor time and kept until
destruction have potential blindspots where they don't observe very early/very
late events unless the person that adds/removes the observer is also the person
that destroys the observed object.

[Mattias Nissler (ping if slow), 2014/05/15 14:55:08]

Can you fill me on what will be implementing TrackedPreferenceValidationObserver
in production code?
The PrefStore is ref-counted so it can be shared across multiple PrefServices.
Your persistent profile pref store still has a logical life time that
corresponds to that of the Profile - so IMHO it's perfectly fine to make the
observer's live time tied to Profile.
I'll respond to these points once I know what the entity is that does the actual
observing :)

[grt (UTC plus 2), 2014/05/15 18:21:35]

This observation mechanism is for a feature of safe browsing. Individual
observer instances will have a weak pointer to a service in the safe browsing
world that has a lifetime on par with the browser process. To be clear:
TrackedPreferenceValidationObserver will be implemented in a class specifically
designed to be instantiated multiple times and funnel events into a long-running
service in safe browsing.

[Mattias Nissler (ping if slow), 2014/05/16 10:34:47]

What you write suggests that the safe browsing service lifetime covers the
lifetime of all observers (which is certainly true for all Profile PrefService
instances, is this work relevant also for the local_state instance?). Given
that, why doesn't the safe browsing service just implement the observer
interface directly, or own the object that implements the observer interface?
Then you could just pass a plain observer pointer at PrefService construction
time.

[Mattias Nissler (ping if slow), 2014/05/16 10:34:47]

One thing that is not clear to me: Do you need the observers to have strictly
similar lifetime to the PrefStore, or does the observer lifespan just need to
cover that of the PrefStore? In other words, do you need the ctor/dtor of the
Observer run at roughly the same point in time as those of the PrefStore?
What you describe here is actually closer to the pattern that we usually name
Delegate in the chrome code base. In my experience, Observers are generally
expected to live shorter than the observed object, and there's a dependency
arrow from Observer to observed object. Delegates on the other hand typically
get passed to the hosting object as a dependency in the constructor, and the
delegate is assumed to outlive the hosting object. It sounds like the delegate
pattern is closer to what you actually need here? If you agree, let's rename the
class for clarity.
I agree we don't want to detach the observer/delegate explicitly in this case.
As mentioned above, Observers are generally expected to live shorter than the
object they observe, hence it makes sense for them to deregister themselves. If
that's not the case, that likely indicates some deficiency in the
dependency/ownership graph. In the case at hand, you solved that by tying
observer/delegate lifetime to the observed object. I'm saying that you can just
as well extend ownership of the delegate by hooking it up to something
browser-global to cover that of the observed objects. That way you get what you
need without special code to handle the awkward ownership situation.
This point again suggests we're really talking about a delegate.

[erikwright (departed), 2014/05/26 16:38:54]

Do you propose adding a profile keyed service for the purpose of removing this
observer?

       size_t reporting_ids_count);
 
   virtual ~PrefHashFilter();
@@ -108,6 +111,8 @@ class PrefHashFilter : public InterceptablePrefFilter {
 
   scoped_ptr<PrefHashStore> pref_hash_store_;
 
+  scoped_ptr<TrackedPreferenceValidationObserver> observer_;
+
   TrackedPreferencesMap tracked_paths_;
 
   // The set of all paths whose value has changed since the last call to