Add TrackedPreferenceValidationDelegate

chrome/browser/prefs/mock_validation_observer.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_MOCK_VALIDATION_OBSERVER_H_
+#define CHROME_BROWSER_PREFS_MOCK_VALIDATION_OBSERVER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "chrome/browser/prefs/pref_hash_filter.h"
+#include "chrome/browser/prefs/tracked/tracked_preference_validation_observer.h"
+
+// A mock tracked preference validation observer for use by tests.
+class MockValidationObserver : public TrackedPreferenceValidationObserver {
+ public:
+  // A container of observed validation events.
+  class ValidationData : public base::RefCounted<ValidationData> {

[erikwright (departed), 2014/05/14 00:39:26]

Personally I would just make this a regular class, have an instance owned by the
test case (as a member) and pass a raw pointer to the mock object. There should
be a strong guarantee that the mock object is destroyed when the pref hash
filter or pref hash store is destroyed, and the test suite should ensure that
that happens before the test's own destruction. So there is no life-cycle
ambiguity.

If you want to simplify things further you could even do away with the Data
object, just own (and share a raw pointer to) a vector of events, and put the
utility functions in the test files that use them (it seems there is limited
overlap).

I don't feel strongly about either of those things being essential.

[grt (UTC plus 2), 2014/05/14 18:57:22]

If you don't mind, I'm inclined to leave this as-is.

+   public:
+    struct ValidationEvent {
+      ValidationEvent(const std::string& path,
+                      PrefHashStoreTransaction::ValueState state,
+                      TrackedPreferenceHelper::ResetAction action,
+                      PrefHashFilter::PrefTrackingStrategy tracking_strategy)
+          : pref_path(path),
+            value_state(state),
+            reset_action(action),
+            strategy(tracking_strategy) {}
+
+      std::string pref_path;

[erikwright (departed), 2014/05/14 00:39:26]

#include pref_hash_store_transaction, tracked_preference_helper

[grt (UTC plus 2), 2014/05/14 18:57:22]

Since these types are used here as a consequence of implementing
TrackedPreferenceValidationObserver (which includes the corresponding headers by
necessity), I didn't think there was a need to re-include those headers here.
Are there rules for this sort of thing?

[erikwright (departed), 2014/05/14 21:24:06]

My rule of thumb is that you don't need to include anything that is only
referenced in the declaration of an OVERRIDE. But if your implementation uses
it, you should include it, even if it is also used in an OVERRIDE.

Rationale: a method could be removed from the implemented pure-virtual base
class without necessarily removing the implementation's dependency on one of the
types it referenced. According to my interpretation, you only need to review
includes when you change the implementation.

Further rationale: In some cases a base class can go from #including a
dependency to forward declaring it. If the implementation is depending on that
#include it will break.

[grt (UTC plus 2), 2014/05/15 01:43:11]

That makes sense, and is consistent with the name of Include What You Use since
the implementation is using the stuff.
Counter argument: by virtue of OVERRIDE, the classes extending the base will
have to be modified if the method requiring the types is removed from the base
class. It's trivial and obvious to add required #includes to extending classes
if needed (you'll end up with a compile error should you forget). In the
opposite case (the headers were #included by the extending class but are no
longer needed following a change to the base) you have to do manual work that
won't be caught by tooling to remove the no-longer-needed #includes.

I'm not convinced that it's better to
include-what-isn't-required-in-case-it-might-be-some-day, but your rule of thumb
makes as much sense to me as anything else, and is easy to remember/apply, so
okay.
Good point. Doesn't apply in this case since they can't be forward declared, but
it's a good general policy.

Done.

[erikwright (departed), 2014/05/15 13:49:02]

You often won't end up with a compile error as the types might already be
transitively included/declared. The number of missing includes/declarations in
the files I edit contradicts the triviality/obviousness of this step.
class Ifc {public: virtual void mth(OtherType a) = 0; };
...
class Impl : public Ifc {public virtual void mth(OtherType a) OVERRIDE; };


If OtherType is only referenced to declare the Impl::mth I do not propose
including or forward-declaring it in impl.h. It's only if Impl also declares
other non-override methods that reference OtherType, or members of type
OtherType, that I believe it's appropriate.

So simply removing or changing a method in the base class will never require you
to review the includes. But any time you change the declaration or
implementation of the subclass you will have to consider it.

Of course it is possible those two events will go together (i.e., if a type is
no longer referenced by a method it might also no longer be used as a member or
constructor parameter).
IWYU wouldn't exist except to insist on including what isn't strictly required
but might be some day. After all, we don't need IWYU to tell us to include
anything without which we won't compile.

I agree that the point is to choose a simple rule that is easy to
remember/apply.

+      PrefHashStoreTransaction::ValueState value_state;
+      TrackedPreferenceHelper::ResetAction reset_action;
+      PrefHashFilter::PrefTrackingStrategy strategy;
+    };
+    ValidationData();
+
+    // Adds a new validation event.
+    void RecordValidation(const std::string& pref_path,
+                          PrefHashStoreTransaction::ValueState value_state,
+                          TrackedPreferenceHelper::ResetAction reset_action,
+                          PrefHashFilter::PrefTrackingStrategy strategy);
+
+    // Returns the number of recorded validations.
+    size_t recorded_validations_count() const { return validations_.size(); }
+
+    // Returns the number of validations of a given value state.
+    size_t CountValidationsOfState(
+        PrefHashStoreTransaction::ValueState value_state) const;
+
+    // Returns the event for the preference with a given path.
+    const ValidationEvent* GetEventForPath(const std::string& pref_path) const;
+
+    // Returns all recorded validation.
+    void GetAllEvents(std::vector<ValidationEvent>* validations) const;
+
+   private:
+    friend class base::RefCounted<ValidationData>;
+    ~ValidationData();
+
+    std::vector<ValidationEvent> validations_;
+    DISALLOW_COPY_AND_ASSIGN(ValidationData);

[erikwright (departed), 2014/05/14 00:39:26]

base/macros.h

[grt (UTC plus 2), 2014/05/14 18:57:22]

Done.

+  };
+
+  explicit MockValidationObserver(
+      const scoped_refptr<ValidationData>& validation_data);
+  virtual ~MockValidationObserver();
+
+  virtual void OnAtomicPreferenceValidation(
+      const std::string& pref_path,

[erikwright (departed), 2014/05/14 00:39:26]

// ValidationObserver implementation

[grt (UTC plus 2), 2014/05/14 18:57:22]

Done.

+      const base::Value* value,
+      PrefHashStoreTransaction::ValueState value_state,
+      TrackedPreferenceHelper::ResetAction reset_action) OVERRIDE;

[erikwright (departed), 2014/05/14 00:39:26]

base/compiler_specific.h

[grt (UTC plus 2), 2014/05/14 18:57:22]

Done.

+  virtual void OnSplitPreferenceValidation(
+      const std::string& pref_path,
+      const base::DictionaryValue* dict_value,
+      const std::vector<std::string>& invalid_keys,
+      PrefHashStoreTransaction::ValueState value_state,
+      TrackedPreferenceHelper::ResetAction reset_action) OVERRIDE;
+
+ private:
+  scoped_refptr<ValidationData> validation_data_;
+  DISALLOW_COPY_AND_ASSIGN(MockValidationObserver);
+};
+
+#endif  // CHROME_BROWSER_PREFS_MOCK_VALIDATION_OBSERVER_H_