Fix a race condition in preference metric reporting.

base/prefs/pref_filter.h

+// Copyright (c) 2013 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 BASE_PREFS_PREF_FILTER_H_
+#define BASE_PREFS_PREF_FILTER_H_
+
+#include <string>
+#include "base/prefs/base_prefs_export.h"
+
+class PersistentPrefStore;
+
+// Filters preferences as they are loaded from disk or updated at runtime.
+// Currently supported only by JsonPrefStore.
+class BASE_PREFS_EXPORT PrefFilter {
+ public:
+  virtual ~PrefFilter() {}
+
+  // Receives notification when the pref store data has been loaded but before
+  // any other clients are notified.

[Bernhard Bauer, 2013/12/10 16:52:07]

Description nit: There are no clients of a JsonPrefStore, only observers.

[erikwright (departed), 2013/12/12 15:53:30]

Done.

+  // Changes made by a PrefFilter during FilterOnLoad do not result in
+  // notifications to |PrefStore::Observer|s.
+  virtual void FilterOnLoad(PersistentPrefStore* pref_store) = 0;

[Bernhard Bauer, 2013/12/10 16:52:07]

I think it would be somewhat more intuitive if this method simply received a
DictionaryValue with the contents of the Preferences file, and would return
another DictionaryValue with the filtered contents. That way, the filter
wouldn't need to go through the PersistentPrefStore API at all (so we wouldn't
need the code to ignore the pref changes).

[erikwright (departed), 2013/12/10 18:57:29]

I agree, with a couple caveats:
(1) It requires the PrefFilter to know that preference names map to children of
that dictionary - not a big surprise but arguably an implementation detail.
(2) In the same vein, it would be awkward for pref stores that _didn't_ store as
a JSON dictionary (i.e., they would have to synthesize this structure
unnecessarily and then sync back from it to the internal structure). For
example, LevelDbPrefStore, at some point.

Would "FilterUpdate" do the same, then? In order to do so, I guess its API would
be something like:

bool FilterUpdate(const std::string& path, base::Value* new_value)

That has the advantage of being a more restricted API than the current one.

[Bernhard Bauer, 2013/12/10 23:31:43]

Well, it's only used by the JsonPrefStore anyway ;-)
I'm tempted to say, let's cross that bridge when we come to it. If we end up
having only LevelDbPrefStore use the PrefFilter, then I would not have a huge
problem with changing the interface so that it filters key by key. Sure, the
fact that the interface looks like this in one situation and like that in
another one leaks some implementation details of the PrefStore, but in practice
I think that happens anyway when you have only one implementation of an
interface.
Yes, I like that interface! What is the return value for though?

[erikwright (departed), 2013/12/11 14:33:06]

The theory is that FilterUpdate might reject an update, but that's not something
we need right now so we can have a void return.

+
+  // Receives notification when a pref store value is changed, before any other
+  // clients are notified.

[Bernhard Bauer, 2013/12/10 16:52:07]

Same comment about clients as above; also: is it really important that this
happens before the observers are notified?

[erikwright (departed), 2013/12/10 18:57:29]

For consistency with FilterOnLoad, I think so. For my specific scenario, no.

Also, my code doesn't use this to make changes, but if you did, you would want
it to occur before others had observed the original change.

[Bernhard Bauer, 2013/12/10 23:31:43]

Urr, can we just say that modifying the pref store during a callback from it is
not a good idea? (See also: the reentrancy issues that we have when loading.) I
also don't think that this is something we'll ever need to do for pref
verification, as we know for "live" pref changes that they were done by the
user, right?

[erikwright (departed), 2013/12/11 14:33:06]

Sure, because we know how all of this is tied together it's apparent to you and
I that changing a value during an Observer callback is a bad idea. But that
might not stop others from doing so, and if they did it would be much easier to
maintain consistency if the verification observed the updates first.

+  virtual void FilterUpdate(PersistentPrefStore* pref_store,
+                            const std::string& path) = 0;
+};
+
+#endif  // BASE_PREFS_PREF_FILTER_H_