Fix a race condition in preference metric reporting.

chrome/browser/prefs/pref_hash_store_impl.h

+// Copyright 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 CHROME_BROWSER_PREFS_PREF_HASH_STORE_IMPL_H_
+#define CHROME_BROWSER_PREFS_PREF_HASH_STORE_IMPL_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/prefs/pref_hash_calculator.h"
+#include "chrome/browser/prefs/pref_hash_store.h"
+
+class PrefRegistrySimple;
+class PrefService;
+
+namespace base {
+class Value;
+}  // namespace base
+
+// Implements PrefHashStoreImpl by storing preference hashes in a PrefService.
+class PrefHashStoreImpl : public PrefHashStore {

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

Are you sure that it's worth using an abstract interface for this class? AFAICS,
there is no other implementation of PrefHashStore (not even in a test), and it's
not like this class has a much larger API surface than the PrefHashStore. 

If you keep the abstract interface, consider moving this class into an anonymous
namespace in pref_hash_store.cc and moving the static methods to PrefHashStore,
like content/ does.

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

There is another implementation, in the newly added pref_hash_filter_unittest.cc

I prefer to be able to test the PrefHashFilter without having to properly set up
the contents of local_state etc.

Making this an interface rather than directly accessing the concrete
implementation permits that.
I'm not sure if your suggestion still holds if we do have a Mock implementation.
In any case, I think the pattern used in content/ serves a different purpose. In
the cases that I am familiar with, the interface is defined in content/public
but the implementation is defined elsewhere. The interface declares but does not
define a static factory method. This method is then defined alongside the
definition of the interface subclass implementation.

The purpose of that pattern is to provide static access to a class of unknown
name. In this case, no static access is required because the dependency on
PrefHashStore is injected from ProfileImpl into chrome_pref_service_factory.cc
(and subsequently to PrefHashFilter).

[Bernhard Bauer, 2013/12/13 19:35:50]

FWIW, you could use a TestingPrefStore, which is fairly easy.
 
What I like about the content/ approach is that it has a clear separation of who
is allowed to know about an Impl class and who isn't: if you're inside content/,
you know about Impl (and you're even encouraged to use FooImpl over Foo!), if
you're outside, you don't know about it. If the interface and the implementation
are in the same component, that distinction gets a bit muddled, and hiding the
Impl class in an anonymous namespace would make it a bit clearer again. But
that's just a suggestion anyway.

+ public:
+  // Constructs a PrefHashStoreImpl that calculates hashes using
+  // |seed| and |device_id| and stores them in |local_state|. Multiple hash
+  // stores can use the same |local_state| with distinct |hash_store_id|s.
+  //
+  // The same |seed|, |device_id|, and |hash_store_id| must be used to load and
+  // validate previously stored hashes in |local_state|.
+  //
+  // |local_state| must have previously been passed to |RegisterPrefs|.
+  PrefHashStoreImpl(const std::string& hash_store_id,
+                    const std::string& seed,
+                    const std::string& device_id,
+                    PrefService* local_state);
+
+  // Registers required local state preferences.
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
+  // PrefHashStore implementation.
+  virtual ValueState CheckValue(const std::string& path,
+                                const base::Value* value) const OVERRIDE;
+  virtual void StoreValue(const std::string& path,
+                          const base::Value* value) OVERRIDE;
+
+ private:
+  std::string hash_store_id_;
+  PrefHashCalculator pref_hash_calculator_;
+  PrefService* local_state_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrefHashStoreImpl);
+};
+
+#endif  // CHROME_BROWSER_PREFS_PREF_HASH_STORE_IMPL_H_