Add TrackedPreferenceValidationDelegate

Add TrackedPreferenceValidationDelegate, by which the creator of a
profile pref store may observe the validation of tracked preferences.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272880

[grt (UTC plus 2), 2014-04-30 17:10:58]

erikwright: primary review
gab: OWNERS review

Formatting courtesy of git cl format.

[erikwright (departed), 2014-05-14 00:39:25]

LGTM with some nits.

Please augment the comments everywhere there is an Observer pointer parameter
indicating that it is optional.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:435:
scoped_ptr<TrackedPreferenceValidationObserver>())),
I gather the actual implementation will be supplied in a separate CL?

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
File chrome/browser/prefs/mock_validation_observer.cc (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.cc:7: #include "base/logging.h"
needed?

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
File chrome/browser/prefs/mock_validation_observer.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:19: class ValidationData :
public base::RefCounted<ValidationData> {
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.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:31: std::string pref_path;
#include pref_hash_store_transaction, tracked_preference_helper

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:62:
DISALLOW_COPY_AND_ASSIGN(ValidationData);
base/macros.h

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:70: const std::string&
pref_path,
// ValidationObserver implementation

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:73:
TrackedPreferenceHelper::ResetAction reset_action) OVERRIDE;
base/compiler_specific.h

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:347:
mock_validation_observer.PassAs<TrackedPreferenceValidationObserver>(),
I assume you decided it was clearer not to inline the initialization. I'll trust
your judgement.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:543:
validated_split_pref->strategy);
Can you add at least one check somewhere that an atomic value was correctly
reported?

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_atomic_preference.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_atomic_preference.h:32:
TrackedPreferenceValidationObserver* observer) const OVERRIDE;
This could probably be a constructor parameter / member, right? It doesn't
change from call to call?

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preference.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preference.h:31: // store.
If it won't be a constructor parameter (detail of the concrete class) please add
a comment here describing |observer|.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preference_validation_observer.h
(right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preference_validation_observer.h:27: //
desired based on |value_state| and the enforcement level in place.
is desired -> will occur.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preference_validation_observer.h:36: //
reset of |value_keys| is desired based on |value_state| and the enforcement
ditto.

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

patch set 6 is a sync. patch set 7 contains most of the requested changes.
please take another look. thanks.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:435:
scoped_ptr<TrackedPreferenceValidationObserver>())),
On 2014/05/14 00:39:26, erikwright wrote:
> I gather the actual implementation will be supplied in a separate CL?

Correct.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
File chrome/browser/prefs/mock_validation_observer.cc (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.cc:7: #include "base/logging.h"
On 2014/05/14 00:39:26, erikwright wrote:
> needed?

Not any more. Thanks.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
File chrome/browser/prefs/mock_validation_observer.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:19: class ValidationData :
public base::RefCounted<ValidationData> {
On 2014/05/14 00:39:26, erikwright wrote:
> 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.

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

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:31: std::string pref_path;
On 2014/05/14 00:39:26, erikwright wrote:
> #include pref_hash_store_transaction, tracked_preference_helper

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?

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:62:
DISALLOW_COPY_AND_ASSIGN(ValidationData);
On 2014/05/14 00:39:26, erikwright wrote:
> base/macros.h

Done.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:70: const std::string&
pref_path,
On 2014/05/14 00:39:26, erikwright wrote:
> // ValidationObserver implementation

Done.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:73:
TrackedPreferenceHelper::ResetAction reset_action) OVERRIDE;
On 2014/05/14 00:39:26, erikwright wrote:
> base/compiler_specific.h

Done.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:347:
mock_validation_observer.PassAs<TrackedPreferenceValidationObserver>(),
On 2014/05/14 00:39:26, erikwright wrote:
> I assume you decided it was clearer not to inline the initialization. I'll
trust
> your judgement.

Not really. I thought PassAs was required (which would have made for a long
line), but it's not. Thanks.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:543:
validated_split_pref->strategy);
On 2014/05/14 00:39:26, erikwright wrote:
> Can you add at least one check somewhere that an atomic value was correctly
> reported?

Done.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_atomic_preference.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_atomic_preference.h:32:
TrackedPreferenceValidationObserver* observer) const OVERRIDE;
On 2014/05/14 00:39:26, erikwright wrote:
> This could probably be a constructor parameter / member, right? It doesn't
> change from call to call?

I chose to make it a parameter here to avoid ownership issues. As a ctor
parameter, it either needs to be a naked pointer documented as being owned by
the TrackedPreference's owner, or it needs to be refcounted. Is there another
option I'm missing? Do you prefer one of those?

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preference.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preference.h:31: // store.
On 2014/05/14 00:39:26, erikwright wrote:
> If it won't be a constructor parameter (detail of the concrete class) please
add
> a comment here describing |observer|.

Done.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preference_validation_observer.h
(right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preference_validation_observer.h:27: //
desired based on |value_state| and the enforcement level in place.
On 2014/05/14 00:39:26, erikwright wrote:
> is desired -> will occur.

Done.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preference_validation_observer.h:36: //
reset of |value_keys| is desired based on |value_state| and the enforcement
On 2014/05/14 00:39:26, erikwright wrote:
> ditto.

Done.

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

patch set 6 is a sync. patch set 7 contains most of the requested changes.
please take another look. thanks.

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

SLGTM. I do prefer the method->constructor parameter change to be implemented.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
File chrome/browser/prefs/mock_validation_observer.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:31: std::string pref_path;
On 2014/05/14 18:57:22, grt wrote:
> On 2014/05/14 00:39:26, erikwright wrote:
> > #include pref_hash_store_transaction, tracked_preference_helper
> 
> 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?

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.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_atomic_preference.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_atomic_preference.h:32:
TrackedPreferenceValidationObserver* observer) const OVERRIDE;
On 2014/05/14 18:57:22, grt wrote:
> On 2014/05/14 00:39:26, erikwright wrote:
> > This could probably be a constructor parameter / member, right? It doesn't
> > change from call to call?
> 
> I chose to make it a parameter here to avoid ownership issues. As a ctor
> parameter, it either needs to be a naked pointer documented as being owned by
> the TrackedPreference's owner, or it needs to be refcounted. Is there another
> option I'm missing? Do you prefer one of those?

I prefer it being a raw pointer because there is no case for using a different
observer in different invocations. You can mention that it must outlive the
TrackedPreference if you like. But the TrackedPreference is owned by the
PrefHashFilter, so the lifecycle relationship is unambiguous.

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

Thanks. PTAL.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
File chrome/browser/prefs/mock_validation_observer.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:31: std::string pref_path;
On 2014/05/14 21:24:06, erikwright wrote:
> On 2014/05/14 18:57:22, grt wrote:
> > On 2014/05/14 00:39:26, erikwright wrote:
> > > #include pref_hash_store_transaction, tracked_preference_helper
> > 
> > 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?
> 
> 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.

That makes sense, and is consistent with the name of Include What You Use since
the implementation is using the stuff.

> 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.

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.

> 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.

Good point. Doesn't apply in this case since they can't be forward declared, but
it's a good general policy.

Done.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_atomic_preference.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_atomic_preference.h:32:
TrackedPreferenceValidationObserver* observer) const OVERRIDE;
On 2014/05/14 21:24:06, erikwright wrote:
> On 2014/05/14 18:57:22, grt wrote:
> > On 2014/05/14 00:39:26, erikwright wrote:
> > > This could probably be a constructor parameter / member, right? It doesn't
> > > change from call to call?
> > 
> > I chose to make it a parameter here to avoid ownership issues. As a ctor
> > parameter, it either needs to be a naked pointer documented as being owned
by
> > the TrackedPreference's owner, or it needs to be refcounted. Is there
another
> > option I'm missing? Do you prefer one of those?
> 
> I prefer it being a raw pointer because there is no case for using a different
> observer in different invocations. You can mention that it must outlive the
> TrackedPreference if you like. But the TrackedPreference is owned by the
> PrefHashFilter, so the lifecycle relationship is unambiguous.

sgtm. Done.

[grt (UTC plus 2), 2014-05-15 02:07:00]

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:347:
mock_validation_observer.PassAs<TrackedPreferenceValidationObserver>(),
On 2014/05/14 18:57:22, grt wrote:
> On 2014/05/14 00:39:26, erikwright wrote:
> > I assume you decided it was clearer not to inline the initialization. I'll
> trust
> > your judgement.
> 
> Not really. I thought PassAs was required (which would have made for a long
> line), but it's not. Thanks.

Well, as it turns out PassAs would be required in order to use make_scoped_ptr,
so I've switched to wrapping.

[erikwright (departed), 2014-05-15 13:33:32]

LGTM.

[grt (UTC plus 2), 2014-05-15 13:36:17]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-05-15 13:36:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grt@chromium.org/266553002/180001

[grt (UTC plus 2), 2014-05-15 13:47:15]

+mnissler for OWNERS review (and -gab since he's away). Thanks.

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

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
File chrome/browser/prefs/mock_validation_observer.h (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/moc...
chrome/browser/prefs/mock_validation_observer.h:31: std::string pref_path;
> > 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.
> 
> 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

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.

> 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.

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).

> 
> 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.

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.

> > 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.
> 
> Good point. Doesn't apply in this case since they can't be forward declared,
but
> it's a good general policy.
> 
> Done.

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/266553002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:347:
mock_validation_observer.PassAs<TrackedPreferenceValidationObserver>(),
On 2014/05/15 02:07:00, grt wrote:
> On 2014/05/14 18:57:22, grt wrote:
> > On 2014/05/14 00:39:26, erikwright wrote:
> > > I assume you decided it was clearer not to inline the initialization. I'll
> > trust
> > > your judgement.
> > 
> > Not really. I thought PassAs was required (which would have made for a long
> > line), but it's not. Thanks.
> 
> Well, as it turns out PassAs would be required in order to use
make_scoped_ptr,
> so I've switched to wrapping.

Ah, yes, make_scoped_ptr infers the type from its parameter. And while a
temporary ScopedPtr _can_ be directly passed if its type matches, it may not if
the type doesn't match.

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

The code looks mostly good, except for the ownership weirdness I've pointed out
in the comments. I also couldn't find the code that wires this up in an actual
running browser (as opposed to unit tests), am I missing something or is that
part of a separate CL? That context would help understand the ownership
decisions you made I guess.

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/mo...
File chrome/browser/prefs/mock_validation_observer.h (right):

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/mo...
chrome/browser/prefs/mock_validation_observer.h:23: class ValidationData :
public base::RefCounted<ValidationData> {
Is there a good reason to split the observer implementation and the validation
data container into two different classes?

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:67:
scoped_ptr<TrackedPreferenceValidationObserver> observer,
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.

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
File chrome/browser/prefs/profile_pref_store_manager.cc (right):

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
chrome/browser/prefs/profile_pref_store_manager.cc:199: class
DelegatingValidationObserver
Ah, so this is where the ownership awkwardness happens get handled. If the
PrefHashFilter wouldn't own the observer, you could just pass the same pointer
to both instances, no?

[commit-bot: I haz the power, 2014-05-15 14:34:59]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-15 14:39:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-15 14:39:05]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[erikwright (departed), 2014-05-15 14:44:50]

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:67:
scoped_ptr<TrackedPreferenceValidationObserver> observer,
On 2014/05/15 14:32:25, Mattias Nissler wrote:
> 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.

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:07]

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:67:
scoped_ptr<TrackedPreferenceValidationObserver> observer,
On 2014/05/15 14:44:51, erikwright wrote:
> On 2014/05/15 14:32:25, Mattias Nissler wrote:
> > 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.
> 
> In this scenario, we are augmenting the functionality of a pref store. 

Can you fill me on what will be implementing TrackedPreferenceValidationObserver
in production code?

> 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.

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.

> 
> 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.

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:34]

Thanks for the comments, Mattias.

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/mo...
File chrome/browser/prefs/mock_validation_observer.h (right):

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/mo...
chrome/browser/prefs/mock_validation_observer.h:23: class ValidationData :
public base::RefCounted<ValidationData> {
On 2014/05/15 14:32:25, Mattias Nissler wrote:
> Is there a good reason to split the observer implementation and the validation
> data container into two different classes?

This is a consequence of the decision to have ownership of the observer
transferred to the PersistentPrefStore. The data may need to be accessed in the
test after the observer has been destroyed. Erik has suggested a simplification
to this split that I'm happy to implement if you feel it's important.

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:67:
scoped_ptr<TrackedPreferenceValidationObserver> observer,
On 2014/05/15 14:55:08, Mattias Nissler wrote:
> On 2014/05/15 14:44:51, erikwright wrote:
> > On 2014/05/15 14:32:25, Mattias Nissler wrote:
> > > 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.
> > 
> > In this scenario, we are augmenting the functionality of a pref store. 
> 
> Can you fill me on what will be implementing
TrackedPreferenceValidationObserver
> in production code?

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.

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
File chrome/browser/prefs/profile_pref_store_manager.cc (right):

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
chrome/browser/prefs/profile_pref_store_manager.cc:199: class
DelegatingValidationObserver
On 2014/05/15 14:32:25, Mattias Nissler wrote:
> Ah, so this is where the ownership awkwardness happens get handled. If the
> PrefHashFilter wouldn't own the observer, you could just pass the same pointer
> to both instances, no?

Yes. This introduces other issues that Erik has already pointed out.

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

I still find it hard to reason about this without having seen the code that
implements TrackedPreferenceValidationObserver, but I think we're getting
somewhere.

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:67:
scoped_ptr<TrackedPreferenceValidationObserver> observer,
On 2014/05/15 18:21:35, grt wrote:
> On 2014/05/15 14:55:08, Mattias Nissler wrote:
> > On 2014/05/15 14:44:51, erikwright wrote:
> > > On 2014/05/15 14:32:25, Mattias Nissler wrote:
> > > > 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.
> > > 
> > > In this scenario, we are augmenting the functionality of a pref store. 
> > 
> > Can you fill me on what will be implementing
> TrackedPreferenceValidationObserver
> > in production code?
> 
> 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.

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.

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:67:
scoped_ptr<TrackedPreferenceValidationObserver> observer,
On 2014/05/15 14:44:51, erikwright wrote:
> On 2014/05/15 14:32:25, Mattias Nissler wrote:
> > 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.
> 
> 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.

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?

> 
> 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.

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.

> (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.

I agree we don't want to detach the observer/delegate explicitly in this case.

> (3) If the client removes their observer, the client must have some guarantees
> to be called before the observed object is destroyed.

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.

> (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.

This point again suggests we're really talking about a delegate.

[grt (UTC plus 2), 2014-05-16 20:35:44]

As it happens, the safe browsing service is shut down before pref hash filters
are destroyed. Even renaming it from Observer to Delegate, I'll need a way to
disconnect the delegate/observer (weak pointer, as I was headed, or an explicit
SetDelegate(NULL)/RemoveObserver).

[Mattias Nissler (ping if slow), 2014-05-21 08:36:37]

On 2014/05/16 20:35:44, grt wrote:
> As it happens, the safe browsing service is shut down before pref hash filters
> are destroyed. Even renaming it from Observer to Delegate, I'll need a way to
> disconnect the delegate/observer (weak pointer, as I was headed, or an
explicit
> SetDelegate(NULL)/RemoveObserver).

I think a long-lived observer implementation that outlives all relevant
PrefService instances and gets a Shutdown() call before the safe browsing
service goes down should be a simple and easy-to-understand solution that solves
the problem?

[grt (UTC plus 2), 2014-05-25 17:24:23]

Hi Mattias and Erik. I've switched from Observer to Delegate, where the Delegate
is owned by ProfileImpl and is guaranteed to outlive the pref store. PTAL.

[Mattias Nissler (ping if slow), 2014-05-26 07:01:14]

LGTM w/ nits.

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/prefs/mo...
File chrome/browser/prefs/mock_validation_delegate.h (right):

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/prefs/mo...
chrome/browser/prefs/mock_validation_delegate.h:70: std::vector<ValidationEvent>
validations_;
nit: I think the majority of our files put a blank line here.

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:730: // CHANGED 2
Stray edit?

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.h:248:
scoped_ptr<TrackedPreferenceValidationDelegate> pref_validation_delegate_;
nit: blank line

[grt (UTC plus 2), 2014-05-26 13:28:15]

Thanks.

+noms for chrome/profiles OWNERS review

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/prefs/mo...
File chrome/browser/prefs/mock_validation_delegate.h (right):

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/prefs/mo...
chrome/browser/prefs/mock_validation_delegate.h:70: std::vector<ValidationEvent>
validations_;
On 2014/05/26 07:01:15, Mattias Nissler wrote:
> nit: I think the majority of our files put a blank line here.

Done.

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:730: // CHANGED 2
On 2014/05/26 07:01:15, Mattias Nissler wrote:
> Stray edit?

Done.

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/266553002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.h:248:
scoped_ptr<TrackedPreferenceValidationDelegate> pref_validation_delegate_;
On 2014/05/26 07:01:15, Mattias Nissler wrote:
> nit: blank line

Done.

[grt (UTC plus 2), 2014-05-26 13:28:51]

On 2014/05/26 13:28:15, grt wrote:
> +noms for chrome/profiles OWNERS review

make that chrome/browser/profiles. thanks.

[noms (inactive), 2014-05-26 13:38:20]

profiles lgtm with a small formatting nit.

https://codereview.chromium.org/266553002/diff/300001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/266553002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:476: sequenced_task_runner,
I think the indenting is off on this. It should only be 4 spaces.

[noms (inactive), 2014-05-26 13:39:03]

I think your CL title is cut off too :)

On 2014/05/26 13:38:20, Monica Dinculescu wrote:
> profiles lgtm with a small formatting nit.
> 
>
https://codereview.chromium.org/266553002/diff/300001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
https://codereview.chromium.org/266553002/diff/300001/chrome/browser/profiles...
> chrome/browser/profiles/profile_impl.cc:476: sequenced_task_runner,
> I think the indenting is off on this. It should only be 4 spaces.

[grt (UTC plus 2), 2014-05-26 14:30:20]

Thanks. Also shortened the text for the issue's "Subject" for your reading
pleasure, although this has no impact on the commit message itself. Cheers.

https://codereview.chromium.org/266553002/diff/300001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/266553002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:476: sequenced_task_runner,
On 2014/05/26 13:38:21, Monica Dinculescu wrote:
> I think the indenting is off on this. It should only be 4 spaces.

"git cl format" did this for me. I've switched it back to the way it was.
Thanks.

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

LGTM.

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:67:
scoped_ptr<TrackedPreferenceValidationObserver> observer,
On 2014/05/15 14:55:08, Mattias Nissler wrote:
> On 2014/05/15 14:44:51, erikwright wrote:
> > On 2014/05/15 14:32:25, Mattias Nissler wrote:
> > > 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.
> > 
> > In this scenario, we are augmenting the functionality of a pref store. 
> 
> Can you fill me on what will be implementing
TrackedPreferenceValidationObserver
> in production code?
> 
> > 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.
> 
> 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.

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

> > 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.
> 
> I'll respond to these points once I know what the entity is that does the
actual
> observing :)

[Mattias Nissler (ping if slow), 2014-05-26 16:46:50]

On 2014/05/26 16:38:54, erikwright wrote:
> LGTM.
> 
>
https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
> File chrome/browser/prefs/pref_hash_filter.h (right):
> 
>
https://codereview.chromium.org/266553002/diff/180001/chrome/browser/prefs/pr...
> chrome/browser/prefs/pref_hash_filter.h:67:
> scoped_ptr<TrackedPreferenceValidationObserver> observer,
> On 2014/05/15 14:55:08, Mattias Nissler wrote:
> > On 2014/05/15 14:44:51, erikwright wrote:
> > > On 2014/05/15 14:32:25, Mattias Nissler wrote:
> > > > 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.
> > > 
> > > In this scenario, we are augmenting the functionality of a pref store. 
> > 
> > Can you fill me on what will be implementing
> TrackedPreferenceValidationObserver
> > in production code?
> > 
> > > 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.
> > 
> > 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.
> 
> Do you propose adding a profile keyed service for the purpose of removing this
> observer?

The observer is now a delegate, so it doesn't need to be removed. However, the
standard way to tie object lifetime to a profile is to make it a profile keyed
service, so that'd be the best solution for the delegate as well. At the moment,
that's not gonna work though because PrefService is not a profile keyed service
itself. Thus, Greg's code is the best way to do this at present.

> 
> > > 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.
> > 
> > I'll respond to these points once I know what the entity is that does the
> actual
> > observing :)

[grt (UTC plus 2), 2014-05-26 17:03:34]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-05-26 17:03:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grt@chromium.org/266553002/320001

[commit-bot: I haz the power, 2014-05-26 18:48:01]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-26 23:43:12]

Change committed as 272880

[gab, 2014-06-10 16:22:09]

Took a sneak peek at this on my way back from leave.

Mostly nits, but also one question about the
TrackedPreferenceValidationDelegate's interface.

Cheers!
Gab

https://codereview.chromium.org/266553002/diff/320001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/266553002/diff/320001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:567: const
MockValidationDelegate::ValidationEvent* validated_atomic_pref =
In all of these tests, the "atomic" part of the test always comes before the
"split" equivalent part of the test.

Swap the last test here to keep this consistency.

https://codereview.chromium.org/266553002/diff/320001/chrome/browser/prefs/pr...
File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right):

https://codereview.chromium.org/266553002/diff/320001/chrome/browser/prefs/pr...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:240: if
(!ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking)
You could test this too here, change this method's to:

// Expect a validation event unless this platform doesn't support preference
tracking.
EXPECT_EQ(ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking,
          mock_validation_delegate_.GetEventForPath(pref_path) != NULL) <<
"...";

https://codereview.chromium.org/266553002/diff/320001/chrome/browser/prefs/pr...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:472:
ExpectValidationObserved(kProtectedAtomic);
You do this verification often after InitializePrefs() in many tests which is
redundant. This verification would also often be more relevant later in the
test.

I suggest:
1) Make the MockTrackedPreferenceValidationDelegate resetable (i.e., allow its
users to clear recorded events; allowing this test to test validation events
multiple times).
2) Move the validation in the generic methods of this test (e.g., in
InitializePrefs() and in LoadExistingPrefs() rather than duplicating this code
inline in every test).

https://codereview.chromium.org/266553002/diff/320001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preference_validation_delegate.h
(right):

https://codereview.chromium.org/266553002/diff/320001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preference_validation_delegate.h:25: //
Notifies observes of the result (|value_state|) of checking the atomic
s/observes/observers ?

https://codereview.chromium.org/266553002/diff/320001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preference_validation_delegate.h:34: //
Notifies observes of the result (|value_state|) of checking the split
s/observes/observers

https://codereview.chromium.org/266553002/diff/320001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preference_validation_delegate.h:43:
TrackedPreferenceHelper::ResetAction reset_action) = 0;
Why would you need to track atomic/split preferences differently?

I'd think the only thing an observer would care about is the |pref_path|,
|value_state|, and |reset_action|.

Why would it need the atomic/split strategy, the |value| and the |invalid_keys|?

At least the current mock tests don't really test |value| and |invalid_keys|
(they do test strategy for the sake of coverage, but don't otherwise indicate a
reason for reporting both differently).