Fix a race condition in preference metric reporting.

Fix a race condition in preference metric reporting.

Introduces:
- PrefFilter: An interface to intercept preference values as they are loaded from disk, before any changes are possible.
- PrefHashFilter: An implementation that verifies preference values against hashes in a PrefHashStore.
- PrefHashStore(Impl): An interface and implementation for storing and verifying hashes of preferences.
- PrefHashCalculator: A utility for calculating preference value hashes.

TBR=brettw (base/base.gyp), scottbyer (chrome/service/service_process_prefs.cc)
BUG=321680
NOTRY=True

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

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

[erikwright (departed), 2013-11-27 04:36:35]

FYI for now, in case you have comments on the general design.

Unit tests will follow. I will also separate IWYU changes into a separate
follow-up CL.

[erikwright (departed), 2013-11-27 22:09:13]

Unit tests updated.

[erikwright (departed), 2013-11-27 22:11:49]

Upload is failing for some reason. I'll let you know when it's working.

[gab, 2013-11-27 23:43:26]

Some generic notes after a first pass:

I feel like PrefHashStore and PrefHashTracker should actually be the same
object. The only reason I see for having a separate "Tracker" object is the
after "initialization" methods are done in the "Store", only "update" methods
should be called, but I think this can be simply enforced via contract (i.e.,
DCHECKs and is_initialized member variable) or via inheritance (i.e. an open and
a more restrained interface to the same underlying impl); having 2 separate
objects with a complex lifetime (i.e., one that creates the other and then
deletes itself yet having that "creation" be requested by an outsider which must
know to only  request it after initialization is complete) appears to increase
cognitive load to the reader for no obvious gain.

PrefStoreObserver is an observer which forwards observations to yet another
"observer", i.e. PrefHashTracker, is the only need for the split into two
objects here for testability (this is a valid reason, but I'd like to understand
design choices).

Overall I like the split of concerns in this implementation, but at first it
seems a bit overkill for some cases. Feels like many of these objects are
designed to work together and not really with anything else; this is nice for
testability, but otherwise makes it harder to reason about the whole design in a
single read. Either fold them into a simpler design or have a detailed comment
somewhere that explains their relationships with one another.

Also consider using something more generic than PrefHashStore, i.e., it would be
nice to eventually augment this to store other things (e.g., backup values).

More comments below written as I read it; some may be voided by my above
post-review analysis.

Cheers!
Gab

PS: For design related stuff, let's discuss in person tomorrow instead of
getting into a long back-and-forth over email..!

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.cc
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.cc...
base/prefs/json_pref_store.cc:181: std::find(owned_observers_.begin(),
owned_observers_.end(), observer);
When would you want to remove an owned observer? Isn't it the whole point that
no one has a pointer to that observer anyways..? Maybe I'll find out as I keep
reading this CL...

Ah I see.. it's because you need to call it in ~JsonPrefStore... see my comment
there.

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.cc...
base/prefs/json_pref_store.cc:337: RemoveObserver(*it);
This is operating on owned_observers_ while iterating over it, in particular
this will make |it| invalid and ++it is undefined after this operation I
believe.

How about keeping a simple RemoveObserver method that only affects |observers_|
and then either explicitly calling owned_observers_.clear() after this loop or
letting the ScopedVector do its work as the object is destroyed.

In fact, do we even need to remove the observers at all? They shouldn't be used
after this object is destroyed. The implicit end of this destructor taking care
of this object's members will actually clear out the ScopedVector et al.

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.h
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.h#...
base/prefs/json_pref_store.h:57: void
AddOwnedObserver(scoped_ptr<PrefStore::Observer> observer);
#include base/prefs/pref_store.h

for PrefStore::Observer

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/chrome_p...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/chrome_p...
chrome/browser/prefs/chrome_pref_service_factory.cc:126:
ReportPrefStoreChangesToUMA(pref_store.get(), pref_hash_store.Pass());
Consider a more generic name; this could later be augmented to do more than
simple reporting.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/chrome_p...
chrome/browser/prefs/chrome_pref_service_factory.cc:128:
builder->WithUserPrefs(pref_store.get());
FYI, looks like this changed in https://codereview.chromium.org/64193003

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_cha...
File chrome/browser/prefs/pref_change_reporter.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_cha...
chrome/browser/prefs/pref_change_reporter.cc:39: kUnregisteredPreference,
Looks like you forgot to bring over the definition of this constant.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_cha...
chrome/browser/prefs/pref_change_reporter.cc:91: void
PrefStoreObserver::OnInitializationCompleted(bool succeeded) {
I'm not convinced that doing this OnInitializationCompleted() is early enough;
other observers could already have been notified and have started reading
preferences right?

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_cha...
chrome/browser/prefs/pref_change_reporter.cc:98: // in order to skip things that
aren't registered.
This matters, in particular to handle kUnregisteredPreference on some OSes; we
could find a better way to do this than rely on checking whether it's registered
though.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_cha...
File chrome/browser/prefs/pref_change_reporter.h (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_cha...
chrome/browser/prefs/pref_change_reporter.h:17: void
ReportPrefStoreChangesToUMA(JsonPrefStore* pref_store,
Change this name to something more generic as this could be used in the future
to do more than UMA.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
File chrome/browser/prefs/pref_hash_calculator.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_calculator.cc:20: namespace {
nit: +empty line above

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_calculator.cc:92:
components.insert(components.begin(), path);
components.push_back(path); would be simpler here.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_calculator.cc:128: return i == 0 ? VALID :
VALID_LEGACY;
I love the CalculateFunctor approach, but it seems a bit weird to loop over
"all" yet have a binary return value based on if its the first one or second
one.

What about:

COMPILE_ASSERT(arraysize(kCalculators) == 2, unhandled_calculator);
if (hash == kCalculators[0](seed_, path, value))
  return VALID;
else if (hash == kCalculators[1](seed_, path, value))
  return VALID_LEGACY;

return INVALID;

----------------------

And someone can make this a loop one day if more calculators get added and it
makes sense then.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store.h:37: INITIALIZED
nit: Add trailing comma (no trailing comma usually indicates that no new values
should be appended after that one (i.e., the should be added before if
anything); e.g. for enum values like FOO_COUNT).

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store.h:43: virtual InitializationResult
InitializeTrackedValue(
This may eventually do more than just "initialize" and "report", but I don't
really have a better name to propose so feel free to ignore this :)!

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store.h:55: // be destroyed immediately after
this method is called.
Lifetime overly complex?

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store_impl.cc:33: // Updates the stored hash for
|path|, if |path| has been configured for
I think this sentence flows better without the comma.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store_impl.cc:35: // |path|.
"no value *at* |path|?"

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store_impl.cc:57: tracked_paths_.insert(path);
Why not use the PrefChangeRegistrar for this? Is it not an option for the entire
lifetime of this object?

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store_impl.cc:68: // Get the dictionary
corresponding to the profile name,
s/profile name/hash store id

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store_impl.cc:92: if (!pref_hash_tracker_.get())
Why handle this case with silent success? The contract (i.e., via DCHECK) is
that this should always be called before the |pref_hash_tracker_| is passed
over, right? And thus it should never be NULL.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
File chrome/browser/prefs/pref_hash_store_impl.h (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store_impl.h:39: class PrefHashTrackerImpl;
Does this need to be a private class? Could it just be a standalone class in the
unnamed namespace of pref_hash_store_impl.cc?

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_met...
File chrome/browser/prefs/pref_metrics_service.cc (left):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_met...
chrome/browser/prefs/pref_metrics_service.cc:100:
extensions::api::DeviceId::GetDeviceId(
Let's be aware that not using device_id on Mac will force a spike in "Changed"
reports (i.e., every user will be migrated from "with device_id" to "without
device_id", but we have no way to tell this is a migration (since we don't have
the device_id or we'd use it..) and will report "Changed").

[robertshield, 2013-11-28 02:54:25]

just a few extra comments from me, Gab has already brought the rain :-)

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.cc
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.cc...
base/prefs/json_pref_store.cc:337: RemoveObserver(*it);
doesn't this call owned_observers_.erase() invalidating the iterator?  It could
call observers_.RemoveObserver(*it); directly, then call owned_observers.clear()
after the loop instead.

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.h
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.h#...
base/prefs/json_pref_store.h:57: void
AddOwnedObserver(scoped_ptr<PrefStore::Observer> observer);
AddObserver takes a raw pointer, any reason not to make this do so as well?

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store.h:51: scoped_ptr<PrefHashStore> builder);
Since this accepts an instance of the class it is defined in, why not make it an
instance method rather than a static method?

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store.h:55: // be destroyed immediately after
this method is called.
I agree, it's not clear to me how the current instance of PrefHashStore would be
destroyed when this returns.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store_impl.cc:22: // Values are stored at
kProfilePreferenceHashes / |hash_store_id| / path,
nit: should this be prefs::kProfilePreferenceHashes

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store_impl.cc:24: // |hash_store_id| is not
expanded but path and kProfilePreferenceHashes
please elaborate slightly on what "expanded" means in this context.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_impl.cc:457: scoped_ptr<PrefHashStore>(new
PrefHashStoreImpl(
Is there a benefit to wrapping this in a scoped pointer? It does make ownership
slightly clearer, but at the cost of a fair bit more boilerplate throughout the
places this is passed through.

[gab, 2013-11-28 04:12:03]

https://codereview.chromium.org/90563003/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_impl.cc:457: scoped_ptr<PrefHashStore>(new
PrefHashStoreImpl(
On 2013/11/28 02:54:26, robertshield wrote:
> Is there a benefit to wrapping this in a scoped pointer? It does make
ownership
> slightly clearer, but at the cost of a fair bit more boilerplate throughout
the
> places this is passed through.

FWIW, I like the paradigm of passing via scoped_ptr when transferring ownership.
Being explicit about memory management is more important than less boilerplate
in this case IMO.

[erikwright (departed), 2013-11-28 17:48:07]

Some responses to your questions, but I'm going to update this before getting
more feedback.

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.cc
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.cc...
base/prefs/json_pref_store.cc:337: RemoveObserver(*it);
On 2013/11/28 02:54:26, robertshield wrote:
> doesn't this call owned_observers_.erase() invalidating the iterator?  It
could
> call observers_.RemoveObserver(*it); directly, then call
owned_observers.clear()
> after the loop instead.

You're both right, it's a mistake that this affects owned_observers_ while
iterating over it.

I don't care about affecting owned_observers_ here but I do want to affect it if
a client calls RemoveObserver during the lifetime of the object.

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.h
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/90563003/diff/1/base/prefs/json_pref_store.h#...
base/prefs/json_pref_store.h:57: void
AddOwnedObserver(scoped_ptr<PrefStore::Observer> observer);
On 2013/11/28 02:54:26, robertshield wrote:
> AddObserver takes a raw pointer, any reason not to make this do so as well?

Because there is a transfer of ownership.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_cha...
File chrome/browser/prefs/pref_change_reporter.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_cha...
chrome/browser/prefs/pref_change_reporter.cc:91: void
PrefStoreObserver::OnInitializationCompleted(bool succeeded) {
On 2013/11/27 23:43:27, gab wrote:
> I'm not convinced that doing this OnInitializationCompleted() is early enough;
> other observers could already have been notified and have started reading
> preferences right?

In this case, chrome_pref_service_factory explicitly added us first, so we _are_
the first to hear about changes.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
File chrome/browser/prefs/pref_hash_calculator.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_calculator.cc:128: return i == 0 ? VALID :
VALID_LEGACY;
On 2013/11/27 23:43:27, gab wrote:
> I love the CalculateFunctor approach, but it seems a bit weird to loop over
> "all" yet have a binary return value based on if its the first one or second
> one.
> 
> What about:
> 
> COMPILE_ASSERT(arraysize(kCalculators) == 2, unhandled_calculator);
> if (hash == kCalculators[0](seed_, path, value))
>   return VALID;
> else if (hash == kCalculators[1](seed_, path, value))
>   return VALID_LEGACY;
> 
> return INVALID;
> 
> ----------------------
> 
> And someone can make this a loop one day if more calculators get added and it
> makes sense then.

Actually already refactored this in the patchset that failed to upload.

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/prefs/pref_has...
chrome/browser/prefs/pref_hash_store_impl.cc:57: tracked_paths_.insert(path);
On 2013/11/27 23:43:27, gab wrote:
> Why not use the PrefChangeRegistrar for this? Is it not an option for the
entire
> lifetime of this object?

We're dealing with a PrefStore, not a PrefService.

(1) There is no added value to the Registrar approach, just added complexity
(because we have to use both the PrefStore (for initialization) and the
PrefService (for updates) APIs. You have to make an assumption that the two
objects you have are the correct pref store and pref service.
(2) It may actually be harmful to track the PrefService and not the user prefs
PrefStore. After all, the value of a preference can change for other reasons
than a user pref change(i.e. managed prefs or extensions pref stores could
change a value at runtime, and we don't want to track a hash of that).

https://codereview.chromium.org/90563003/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/90563003/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_impl.cc:457: scoped_ptr<PrefHashStore>(new
PrefHashStoreImpl(
On 2013/11/28 04:12:03, gab wrote:
> On 2013/11/28 02:54:26, robertshield wrote:
> > Is there a benefit to wrapping this in a scoped pointer? It does make
> ownership
> > slightly clearer, but at the cost of a fair bit more boilerplate throughout
> the
> > places this is passed through.
> 
> FWIW, I like the paradigm of passing via scoped_ptr when transferring
ownership.
> Being explicit about memory management is more important than less boilerplate
> in this case IMO.

AFAIU, this is the new gold standard for documenting and enforcing ownership
transfer. And I, for one, welcome our movable overlords.

[erikwright (departed), 2013-11-28 21:44:14]

I've changed the approach a bit. I don't have a chance to respond to all your
comments now, but new feedback especially on the structure would be great.

[gab, 2013-11-29 04:05:51]

Thoughts (or essay rather...) on the latest design below.

Cheers!
Gab

https://codereview.chromium.org/90563003/diff/70001/chrome/browser/prefs/hash...
File chrome/browser/prefs/hashed_pref_store.h (right):

https://codereview.chromium.org/90563003/diff/70001/chrome/browser/prefs/hash...
chrome/browser/prefs/hashed_pref_store.h:32: virtual bool GetMutableValue(const
std::string& key,
Having to override all of these methods just to forward the calls to the
wrapped_pref_store_ feels to me like its calling out that this is the wrong
design (and I'm sorry I didn't realize that when we discussed earlier today..).
The possibility of someone extending this "helper" class to do much more than
simply forward these calls scares me. It is too big and strong for what it needs
to do.

I'm leaning more and more towards a VerifierDelegate design where a
JsonPrefStore could be given a VerifierDelegate which, if set, gets to verify
(and change if necessary) the state of the pref store BEFORE pref changes start
generating notifications to observers.

I don't feel this would be intrusive in the JsonPrefStore code as it only
requires:
1) a member/setter for the delegate
2) a call to the delegate that it should do its thing (right before calling
OnInitializationComplete())
3) a member variable in the pref store to know not to notify observers of
changes done by the delegate

Then we are guaranteed that when the store is handed off to the world and its
real observers, it is done in a verified state.

And the same delegate is also informed of all further changes.

This is akin to being the first observer as in your first design, but is much
more explicit about it and also has state to avoid notifying other observers
when doing the initial verification-based updates.

The store is handed ownership of its delegate.

The only downside to this I see is that it might be slightly harder to force
future implementations of PersistentPrefStore to do the same... here are
solutions I can think of:

1) It might already be taken care of simply by the fact that if someone wants to
trade a JsonPrefStore object for a FooPrefStore object, the FooPrefStore object
will have to handle set_verifier_delegate() or the calling code won't compile
and I don't think anyone sane (and their reviewers) would just blindly remove
the call without understanding what it does...

2) We could make PersistentPrefStore implement handling the delegate (I don't
know whether we care that it remains pure-virtual or if it being virtual is good
enough... actually it has a destructor defined so it is not pure virtual,
right?); then the delegate itself would simply have to DCHECK that it's being
told about initialization before any other pref change observation event.

3) Just add a pure-virtual SetVerifierDelegate() method to PersistentPrefStore
and assume new implementations will do the right thing.
At the end of the day, there are always things that new implementations are
implicitly expected to do which are only enforced by common sense (e.g.,
notifying observers added via AddObserver() on modification).

https://codereview.chromium.org/90563003/diff/70001/chrome/browser/prefs/hash...
chrome/browser/prefs/hashed_pref_store.h:74: ObserverList<PrefStore::Observer,
true>& observers() {
I don't think we allow return by non-const ref in Chromium

[gab, 2013-11-29 04:17:10]

Another idea below!

https://codereview.chromium.org/90563003/diff/70001/chrome/browser/prefs/hash...
File chrome/browser/prefs/hashed_pref_store.h (right):

https://codereview.chromium.org/90563003/diff/70001/chrome/browser/prefs/hash...
chrome/browser/prefs/hashed_pref_store.h:32: virtual bool GetMutableValue(const
std::string& key,
On 2013/11/29 04:05:51, gab wrote:
> Having to override all of these methods just to forward the calls to the
> wrapped_pref_store_ feels to me like its calling out that this is the wrong
> design (and I'm sorry I didn't realize that when we discussed earlier
today..).
> The possibility of someone extending this "helper" class to do much more than
> simply forward these calls scares me. It is too big and strong for what it
needs
> to do.
> 
> I'm leaning more and more towards a VerifierDelegate design where a
> JsonPrefStore could be given a VerifierDelegate which, if set, gets to verify
> (and change if necessary) the state of the pref store BEFORE pref changes
start
> generating notifications to observers.
> 
> I don't feel this would be intrusive in the JsonPrefStore code as it only
> requires:
> 1) a member/setter for the delegate
> 2) a call to the delegate that it should do its thing (right before calling
> OnInitializationComplete())
> 3) a member variable in the pref store to know not to notify observers of
> changes done by the delegate
> 
> Then we are guaranteed that when the store is handed off to the world and its
> real observers, it is done in a verified state.
> 
> And the same delegate is also informed of all further changes.
> 
> This is akin to being the first observer as in your first design, but is much
> more explicit about it and also has state to avoid notifying other observers
> when doing the initial verification-based updates.
> 
> The store is handed ownership of its delegate.
> 
> The only downside to this I see is that it might be slightly harder to force
> future implementations of PersistentPrefStore to do the same... here are
> solutions I can think of:
> 
> 1) It might already be taken care of simply by the fact that if someone wants
to
> trade a JsonPrefStore object for a FooPrefStore object, the FooPrefStore
object
> will have to handle set_verifier_delegate() or the calling code won't compile
> and I don't think anyone sane (and their reviewers) would just blindly remove
> the call without understanding what it does...
> 
> 2) We could make PersistentPrefStore implement handling the delegate (I don't
> know whether we care that it remains pure-virtual or if it being virtual is
good
> enough... actually it has a destructor defined so it is not pure virtual,
> right?); then the delegate itself would simply have to DCHECK that it's being
> told about initialization before any other pref change observation event.
> 
> 3) Just add a pure-virtual SetVerifierDelegate() method to PersistentPrefStore
> and assume new implementations will do the right thing.
> At the end of the day, there are always things that new implementations are
> implicitly expected to do which are only enforced by common sense (e.g.,
> notifying observers added via AddObserver() on modification).

Actually, even simpler than a delegate would be to hand a Callback over to the
pref store, to be called once: right before calling other observers (and
disabling observer notifications about change for the duration of the callback).

Than we can just be a regular observer for the remainder of the store's lifetime
(perhaps an owned observer, otherwise I'm not sure who would own it).

[erikwright (departed), 2013-12-04 20:11:59]

PTAL. Filter based approach.

[gab, 2013-12-04 22:22:30]

See comments below; overall I am proposing changing the design from:
JsonPrefStore->PrefFilter->PrefHashFilter->PrefStore->PrefStoreImpl
to:
JsonPrefStore->PrefVerificationDelegate->PrefHashVerificator

where: 
 - PrefVerificationDelegate is the new name for the PrefFilter interface.
 - PrefHashVerificator is the implementation of the delegate which has the
verification and storing logic.

I think this is nicer as:
 1) It avoids the cognitive overload of multiple layers (which aren't necessary
at this point; you could argue that there could be other implementations of this
delegate, but that's probably not so likely if it wasn't needed all this
time...).
 2) Having the verification and storing logic is nice as you don't need an
interface to report the result from one to the other solely for the purpose of
UMA; you also avoid having clients think about who should create the store, who
should create the delegate, how to connect them, etc.

Haven't looked at the tests yet; waiting for agreement on design first.

Also, as I mentioned below, consider syncing as it looks like the
pref_service_builder.cc, which is at the top of this design chain, has been
refactored recently in http://crrev.com/235913.

Cheers!
Gab

https://codereview.chromium.org/90563003/diff/110001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/90563003/diff/110001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:293: FOR_EACH_OBSERVER(PrefStore::Observer,
What about in this case? Should we not do/report something if the profile-dir
was deleted altogether?

https://codereview.chromium.org/90563003/diff/110001/base/prefs/pref_filter.h
File base/prefs/pref_filter.h (right):

https://codereview.chromium.org/90563003/diff/110001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:15: class BASE_PREFS_EXPORT PrefFilter {
I don't really like the term "filter"; to me a filter selects/discards
individual items passed through it; not verifies/modifies all items attached to
a global bucket passed to it.

On the FilterUpdate case, it kind of acts as a filter (but more as an observer),
but on the FilterOnLoad case it definitely is much more than a filter IMO.

I prefer
PrefVerificator/PrefController/PrefInterceptor/PrefVerificationDelegate.

I also don't think we need a generic interface for this delegate and a more
specific impl; in particular given there is only one and this will always be the
one (and should that change we can reconsider -- no need to be too general yet).

https://codereview.chromium.org/90563003/diff/110001/base/prefs/pref_service_...
File base/prefs/pref_service_builder.cc (right):

https://codereview.chromium.org/90563003/diff/110001/base/prefs/pref_service_...
base/prefs/pref_service_builder.cc:70: PrefServiceBuilder&
PrefServiceBuilder::WithUserFilePrefs(
This method no longer exists on trunk, consider syncing.

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:62:
pref_hash_store_->InitializeTrackedValue(kTrackedPrefs[i], value);
How does this work in a world where we do more than reporting? Is
InitializeTrackedValue() expected to do the fixups and return an even broader
enum to describe what it did?

I think it will be simpler in the long run if PrefHashFilter and PrefHashStore
are folded in the same class.

Currently PrefHashFilter is pretty much a middle-man class for which most of the
logic just forwarding calls to PrefHashStore and reporting back on findings
(which PrefHashStore could easily do directly).

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:33:
DISALLOW_COPY_AND_ASSIGN(PrefHashFilter);
nit: I think we always have an empty line between variables and
DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.h (right):

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.h:25: class PrefHashStoreImpl : public
PrefHashStore {
I think it's overkill to have a public interface and an impl in this case.

I could see it being useful though if the interface was directly the delegate
interfacing with JsonPrefStore as suggested above.

[erikwright (departed), 2013-12-05 23:47:59]

PTAL.

I should still write a test for PrefHashFilter. But if you like what is here
I'll send it to bbauer for review in the meantime.

https://codereview.chromium.org/90563003/diff/110001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/90563003/diff/110001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:293: FOR_EACH_OBSERVER(PrefStore::Observer,
On 2013/12/04 22:22:30, gab wrote:
> What about in this case? Should we not do/report something if the profile-dir
> was deleted altogether?

If the store is missing but the directory is there, a new store is created. But
this case is when the entire profile has been deleted. I don't think it's the
job of this class to react to that situation. In fact, I'd assume that someone
should fail in that case before we even get here.

https://codereview.chromium.org/90563003/diff/110001/base/prefs/pref_filter.h
File base/prefs/pref_filter.h (right):

https://codereview.chromium.org/90563003/diff/110001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:15: class BASE_PREFS_EXPORT PrefFilter {
On 2013/12/04 22:22:30, gab wrote:
> I don't really like the term "filter"; to me a filter selects/discards
> individual items passed through it; not verifies/modifies all items attached
to
> a global bucket passed to it.

I'm happy to change it. Rather than do that now I'll let bbauer select a name.

> On the FilterUpdate case, it kind of acts as a filter (but more as an
observer),
> but on the FilterOnLoad case it definitely is much more than a filter IMO.
> 
> I prefer
> PrefVerificator/PrefController/PrefInterceptor/PrefVerificationDelegate.
> 
> I also don't think we need a generic interface for this delegate and a more
> specific impl; in particular given there is only one and this will always be
the
> one (and should that change we can reconsider -- no need to be too general
yet).

Needed because the implementation should not be in base/.

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:62:
pref_hash_store_->InitializeTrackedValue(kTrackedPrefs[i], value);
On 2013/12/04 22:22:30, gab wrote:
> How does this work in a world where we do more than reporting? Is
> InitializeTrackedValue() expected to do the fixups and return an even broader
> enum to describe what it did?
> 
> I think it will be simpler in the long run if PrefHashFilter and PrefHashStore
> are folded in the same class.
> 
> Currently PrefHashFilter is pretty much a middle-man class for which most of
the
> logic just forwarding calls to PrefHashStore and reporting back on findings
> (which PrefHashStore could easily do directly).

See my comments elsewhere on the motivation for this factorization. But I've
shifted the responsibility for storing the value from PrefHashStore to
PrefHashFilter.

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:33:
DISALLOW_COPY_AND_ASSIGN(PrefHashFilter);
On 2013/12/04 22:22:30, gab wrote:
> nit: I think we always have an empty line between variables and
> DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.h (right):

https://codereview.chromium.org/90563003/diff/110001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.h:25: class PrefHashStoreImpl : public
PrefHashStore {
On 2013/12/04 22:22:30, gab wrote:
> I think it's overkill to have a public interface and an impl in this case.
> 
> I could see it being useful though if the interface was directly the delegate
> interfacing with JsonPrefStore as suggested above.

The current factoring makes it very easy to test how we react to mismatches
without having to muck about with the values in local state. And it makes it
very easy to test the storage and verification of hashes in local state without
having to set up and tear down the user prefs over and over again. See for
example that the new tests are a lot simpler than the old tests.

[gab, 2013-12-06 17:23:37]

Looks very good!

I suggest the following things before sending this to Bernhard:

1) Sync
2) Update CL description to explain what each new component does (both to avoid
the same comments I had and for future readers' sake).

Cheers!
Gab

https://codereview.chromium.org/90563003/diff/150001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/90563003/diff/150001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:274: // We don't notify observers about changes
during initialization (i.e., due to
Remove "We"

https://codereview.chromium.org/90563003/diff/150001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:278: pref_filter_->FilterUpdate(this, key);
To continue my point about naming; to me here FilterUpdate feels like a method
which would return a boolean and basically tell you whether to continue with the
update or not.

https://codereview.chromium.org/90563003/diff/150001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/90563003/diff/150001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:21: class PrefFilter;
Don't you need the full type here since you declare a scoped_ptr to one below?
Or is it sufficient that the full type only exist where the destructor is
defined?

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_calculator.h (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:15: // This class is intentionally
copy-constructable.
The only reason for this I believe is because it's copied in the constructor.

I actually think a better design would be for the PrefHashStoreImpl to take the
seed/device_id as parameters and construct the PrefHashCalculator itself in its
constructor.

The calculator is really an implementation detail of the impl and it doesn't
make sense to force users of the API to know about it and build one for us.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:21: VALID_LEGACY
nit: add trailing comma

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:26: //
nit: I don't think an empty line is required here.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:30: // Calculates a hash value for
the supplied preference path and value. |value|
Put path and value in ||.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:32: std::string Calculate(const
std::string& path, const base::Value* value);
Make this method and other methods of this class const.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_calculator_unittest.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator_unittest.cc:70:
calc1.Calculate("pref_path", NULL));
What's the value of checking the same call twice here?

I think ASSERT_FALSE(calc1.Calculate("pref_path", NULL).empty()); is a good test
here to make sure the hash is calculated AND not empty.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator_unittest.cc:71: }
I would also like to see tests with hardcoded hashes for each Value type to
ensure future modifications of the algorithm do not start generating different
hashes.

Ideally these would re-use hashes that were hardcoded in the old tests to
confirm that this new implementation also doesn't start generating new hashes.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator_unittest.cc:80: // 32 NULL bytes is
the seed that was used to generate the legacy hash.
Why 32 NULL bytes?! Won't an empty string do the same?

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator_unittest.cc:86:
"23C069909933F362022B1F187E73E9A2"));
I would prefer having this all on one line; it wasn't obvious to me at first
that this was the continuation of the above string (i.e., I thought "why 2
hashes?").

Please put it in a static const variable above.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:44: tracked_paths_(kTrackedPrefs,
kTrackedPrefs + arraysize(kTrackedPrefs)) {}
This works?! i.e., "const char*" can act as an iterator?! Cool, didn't know that
:)!

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:60: if (kTrackedPrefs[i] == NULL)
Checking for NULL here is no longer correct; please use the set for this logic
now (and for the looping).

This makes me think that we may want to have the kTrackedPrefs defined as a
static const[] in the local-scope of the constructor? The only downside I guess
is that we can't use the initializer list if we do this :(.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:88: default:
Remove default block here as well (ref comment I made further below).

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:28: virtual void
FilterOnLoad(PersistentPrefStore* pref_store) OVERRIDE;
Should we keep the "filter" term, I think what bugs me most is that this is
called "Filter" while it filters more than one item at once; how about
"FilterAllOnLoad"?

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:29: virtual void
FilterUpdate(PersistentPrefStore* pref_store,
Add comments to these methods (here on in the impl) saying what this specific
impl does in both scenarios).

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store.h:24: virtual ~PrefHashStore() {}
I forget the C++ details, can you justify why it's better to explicitly specify
a public virtual destructor here rather than keeping it as a pure-virtual
interface?

Is this required for owners of a PrefHashStore to be able to delete it via
virtual dispatch to the impl's destructor?

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store.h:37: UNKNOWN_VALUE
nit: add trailing comma

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:49: default:
Remove the default block; you're already handling all enum values above and not
having a default value will force anyone adding a new
PrefHashCalculator::ValidationResult to change this switch to compile (whereas a
default block wouldn't).

If VS2010 isn't smart enough to compile this (i.e. complains about a path with
no return), I suggest having the NOTREACHED() at the end of the method outside
of this switch.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:62: // which may have a '.'
Feels like more of this comment can fit on the first line.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.h (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.h:35: PrefService* local_state,
out-params last (this is not really an out-param, but it's not const so rule
still applies I think).

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.h:50: PrefHashCalculator
pref_hash_calculator_;
Same order here as you have in the constructor

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl_unittest.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl_unittest.cc:34:
pref_hash_store.CheckValue("path1", &string_1));
Same check twice above, typo?

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service.cc (left):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:19: // http://crbug.com/276485
Perhaps include this bug in this CL's description for context for anyone looking
at this later?

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.h:30: class Profile;
nit: Pro after Pre

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service_unittest.cc (left):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service_unittest.cc:443:
EXPECT_EQ("A50FE7EB31BFBC32B8A27E71730AF15421178A9B5815644ACE174B18966735B9",
As mentioned in the calculator tests; I would like to see some of these hashes
there to confirm that the changes in this CL aren't causing different hashes for
some reason.

[gab, 2013-12-06 18:38:07]

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:60: if (kTrackedPrefs[i] == NULL)
On 2013/12/06 17:23:38, gab wrote:
> Checking for NULL here is no longer correct; please use the set for this logic
> now (and for the looping).
> 
> This makes me think that we may want to have the kTrackedPrefs defined as a
> static const[] in the local-scope of the constructor? The only downside I
guess
> is that we can't use the initializer list if we do this :(.

In fact this makes me think that a pref_hash_filter_unittest.cc is probably
called for to avoid bugs like this one :)!

[erikwright (departed), 2013-12-06 19:00:46]

Responses to some of your comments. I'll update the code to reflect your other
requests now.

https://codereview.chromium.org/90563003/diff/150001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/90563003/diff/150001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:21: class PrefFilter;
On 2013/12/06 17:23:38, gab wrote:
> Don't you need the full type here since you declare a scoped_ptr to one below?
> Or is it sufficient that the full type only exist where the destructor is
> defined?

The latter. It's the call to scoped_ptr<T>::~scoped_ptr<T>() that instantiates
the method definition, and it's the instantiation of the method definition that
calls T::~T() (and thus needs to know if it is virtual). And it's
JsonPrefStore::~JsonPrefStore() that implicitly includes the calls to the
destructors of all JsonPrefStore members.

Since the latter occurs in the .cc file, all the other stuff also implicitly
occurs there.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_calculator_unittest.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator_unittest.cc:80: // 32 NULL bytes is
the seed that was used to generate the legacy hash.
On 2013/12/06 17:23:38, gab wrote:
> Why 32 NULL bytes?! Won't an empty string do the same?

Like the comment says. That is what the old test used. In order to prove that
the algorithm hasn't changed and generates the same hash with the same seed, I
am using the same seed.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:44: tracked_paths_(kTrackedPrefs,
kTrackedPrefs + arraysize(kTrackedPrefs)) {}
On 2013/12/06 17:23:38, gab wrote:
> This works?! i.e., "const char*" can act as an iterator?! Cool, didn't know
that
> :)!

An iterator is basically anything that supports operator++(), operator*(),
operator==().

STL collection iterator range constructors look roughly like this:

template <typename Value> class collection {
 public:
  ..
  template <typename Iterator> collection(Iterator begin, Iterator end);
  ..
}

And they only require that Iterator::operator*() returns a type that can be
passed to a single-argument constructor for Value.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:60: if (kTrackedPrefs[i] == NULL)
On 2013/12/06 18:38:07, gab wrote:
> On 2013/12/06 17:23:38, gab wrote:
> > Checking for NULL here is no longer correct; please use the set for this
logic
> > now (and for the looping).
> > 
> > This makes me think that we may want to have the kTrackedPrefs defined as a
> > static const[] in the local-scope of the constructor? The only downside I
> guess
> > is that we can't use the initializer list if we do this :(.
> 
> In fact this makes me think that a pref_hash_filter_unittest.cc is probably
> called for to avoid bugs like this one :)!

As mentioned, the test for this class is clearly required, but seeking feedback
on the design from you before writing it.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store.h:24: virtual ~PrefHashStore() {}
On 2013/12/06 17:23:38, gab wrote:
> I forget the C++ details, can you justify why it's better to explicitly
specify
> a public virtual destructor here rather than keeping it as a pure-virtual
> interface?
> 
> Is this required for owners of a PrefHashStore to be able to delete it via
> virtual dispatch to the impl's destructor?

All classes in the class hierarchy must have destructors. A subclass's
destructor will always implicitly invoke its parent class destructor. So if this
were pure-virtual (" = 0;") with no definition anywhere you would not be able to
link due to a missing symbol PrefHashStore::~PrefHashStore referenced by
PrefHashStoreImpl::~PrefHashStoreImpl

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:49: default:
On 2013/12/06 17:23:38, gab wrote:
> Remove the default block; you're already handling all enum values above and
not
> having a default value will force anyone adding a new
> PrefHashCalculator::ValidationResult to change this switch to compile (whereas
a
> default block wouldn't).
> 
> If VS2010 isn't smart enough to compile this (i.e. complains about a path with
> no return), I suggest having the NOTREACHED() at the end of the method outside
> of this switch.

It's impossible for the compiler to assert that there is no path where
validation_result will not equal one of those 'case' covered values. Remember
that at runtime the enum is really just an int, it could have any value at all.

There will either be a NOTREACHED in the default or following the switch
statement. The current structure protects against two things:

(1) Each case must have a return value. Else a path without a return.
(2) NOTREACHED if someone adds a new enum and forgets to update this.

The NOTREACHED has a very clear comment that is guaranteed to be correct.
Whereas if we put it at the bottom, and then someone forgets to add a return
statement in one of the cases, the log message will be misleading.

[gab, 2013-12-06 23:22:37]

Thanks for the replies, re-reply below to something we discussed offline.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:49: default:
On 2013/12/06 19:00:47, erikwright wrote:
> On 2013/12/06 17:23:38, gab wrote:
> > Remove the default block; you're already handling all enum values above and
> not
> > having a default value will force anyone adding a new
> > PrefHashCalculator::ValidationResult to change this switch to compile
(whereas
> a
> > default block wouldn't).
> > 
> > If VS2010 isn't smart enough to compile this (i.e. complains about a path
with
> > no return), I suggest having the NOTREACHED() at the end of the method
outside
> > of this switch.
> 
> It's impossible for the compiler to assert that there is no path where
> validation_result will not equal one of those 'case' covered values. Remember
> that at runtime the enum is really just an int, it could have any value at
all.
> 
> There will either be a NOTREACHED in the default or following the switch
> statement. The current structure protects against two things:
> 
> (1) Each case must have a return value. Else a path without a return.
> (2) NOTREACHED if someone adds a new enum and forgets to update this.
> 
> The NOTREACHED has a very clear comment that is guaranteed to be correct.
> Whereas if we put it at the bottom, and then someone forgets to add a return
> statement in one of the cases, the log message will be misleading.

We clarified this offline already, but for the sake of the public discussion I
just stumbled upon an example of what I'm suggesting:
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_serv...

This has the benefit of forcing the switch to handle each possible enum case (or
gcc/clang will warn on compile -- VS2010 doesn't do it unfortunately).

[erikwright (departed), 2013-12-09 17:59:39]

Gab: PTAL. I'll resync now.

https://codereview.chromium.org/90563003/diff/150001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/90563003/diff/150001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:274: // We don't notify observers about changes
during initialization (i.e., due to
On 2013/12/06 17:23:38, gab wrote:
> Remove "We"

Done.

https://codereview.chromium.org/90563003/diff/150001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:278: pref_filter_->FilterUpdate(this, key);
On 2013/12/06 17:23:38, gab wrote:
> To continue my point about naming; to me here FilterUpdate feels like a method
> which would return a boolean and basically tell you whether to continue with
the
> update or not.

I thought about that.

(1) It's not necessary now.
(2) It's clear what the right behaviour would be. If someone uses a PrefValue to
change a preference, and we refuse it, do we still need to fire an Update event
to let them know that the value is not what they expect it is? Etc.

Anyway, as previously mentioned, I'm indifferent to the name but rather than
change it twice I'll take Bernhard's input.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_calculator.h (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:15: // This class is intentionally
copy-constructable.
On 2013/12/06 17:23:38, gab wrote:
> The only reason for this I believe is because it's copied in the constructor.
> 
> I actually think a better design would be for the PrefHashStoreImpl to take
the
> seed/device_id as parameters and construct the PrefHashCalculator itself in
its
> constructor.
> 
> The calculator is really an implementation detail of the impl and it doesn't
> make sense to force users of the API to know about it and build one for us.

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:21: VALID_LEGACY
On 2013/12/06 17:23:38, gab wrote:
> nit: add trailing comma

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:26: //
On 2013/12/06 17:23:38, gab wrote:
> nit: I don't think an empty line is required here.

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:30: // Calculates a hash value for
the supplied preference path and value. |value|
On 2013/12/06 17:23:38, gab wrote:
> Put path and value in ||.

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:32: std::string Calculate(const
std::string& path, const base::Value* value);
On 2013/12/06 17:23:38, gab wrote:
> Make this method and other methods of this class const.

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_calculator_unittest.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator_unittest.cc:70:
calc1.Calculate("pref_path", NULL));
On 2013/12/06 17:23:38, gab wrote:
> What's the value of checking the same call twice here?
> 
> I think ASSERT_FALSE(calc1.Calculate("pref_path", NULL).empty()); is a good
test
> here to make sure the hash is calculated AND not empty.

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator_unittest.cc:71: }
On 2013/12/06 17:23:38, gab wrote:
> I would also like to see tests with hardcoded hashes for each Value type to
> ensure future modifications of the algorithm do not start generating different
> hashes.
> 
> Ideally these would re-use hashes that were hardcoded in the old tests to
> confirm that this new implementation also doesn't start generating new hashes.

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator_unittest.cc:86:
"23C069909933F362022B1F187E73E9A2"));
On 2013/12/06 17:23:38, gab wrote:
> I would prefer having this all on one line; it wasn't obvious to me at first
> that this was the continuation of the above string (i.e., I thought "why 2
> hashes?").
> 
> Please put it in a static const variable above.

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:88: default:
On 2013/12/06 17:23:38, gab wrote:
> Remove default block here as well (ref comment I made further below).

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:29: virtual void
FilterUpdate(PersistentPrefStore* pref_store,
On 2013/12/06 17:23:38, gab wrote:
> Add comments to these methods (here on in the impl) saying what this specific
> impl does in both scenarios).

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store.h:37: UNKNOWN_VALUE
On 2013/12/06 17:23:38, gab wrote:
> nit: add trailing comma

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:49: default:
On 2013/12/06 23:22:38, gab wrote:
> On 2013/12/06 19:00:47, erikwright wrote:
> > On 2013/12/06 17:23:38, gab wrote:
> > > Remove the default block; you're already handling all enum values above
and
> > not
> > > having a default value will force anyone adding a new
> > > PrefHashCalculator::ValidationResult to change this switch to compile
> (whereas
> > a
> > > default block wouldn't).
> > > 
> > > If VS2010 isn't smart enough to compile this (i.e. complains about a path
> with
> > > no return), I suggest having the NOTREACHED() at the end of the method
> outside
> > > of this switch.
> > 
> > It's impossible for the compiler to assert that there is no path where
> > validation_result will not equal one of those 'case' covered values.
Remember
> > that at runtime the enum is really just an int, it could have any value at
> all.
> > 
> > There will either be a NOTREACHED in the default or following the switch
> > statement. The current structure protects against two things:
> > 
> > (1) Each case must have a return value. Else a path without a return.
> > (2) NOTREACHED if someone adds a new enum and forgets to update this.
> > 
> > The NOTREACHED has a very clear comment that is guaranteed to be correct.
> > Whereas if we put it at the bottom, and then someone forgets to add a return
> > statement in one of the cases, the log message will be misleading.
> 
> We clarified this offline already, but for the sake of the public discussion I
> just stumbled upon an example of what I'm suggesting:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_serv...
> 
> This has the benefit of forcing the switch to handle each possible enum case
(or
> gcc/clang will warn on compile -- VS2010 doesn't do it unfortunately).

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.h (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.h:35: PrefService* local_state,
On 2013/12/06 17:23:38, gab wrote:
> out-params last (this is not really an out-param, but it's not const so rule
> still applies I think).

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.h:50: PrefHashCalculator
pref_hash_calculator_;
On 2013/12/06 17:23:38, gab wrote:
> Same order here as you have in the constructor

Done.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl_unittest.cc (right):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl_unittest.cc:34:
pref_hash_store.CheckValue("path1", &string_1));
On 2013/12/06 17:23:38, gab wrote:
> Same check twice above, typo?

Previously it was a mutating operation. But with our API changes it isn't
anymore.

I've made CheckValue 'const' to make this explicit.

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service.cc (left):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:19: // http://crbug.com/276485
On 2013/12/06 17:23:38, gab wrote:
> Perhaps include this bug in this CL's description for context for anyone
looking
> at this later?

I migrated the bug link into profile_impl.cc (where we now load the device ID).

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service_unittest.cc (left):

https://codereview.chromium.org/90563003/diff/150001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service_unittest.cc:443:
EXPECT_EQ("A50FE7EB31BFBC32B8A27E71730AF15421178A9B5815644ACE174B18966735B9",
On 2013/12/06 17:23:38, gab wrote:
> As mentioned in the calculator tests; I would like to see some of these hashes
> there to confirm that the changes in this CL aren't causing different hashes
for
> some reason.

Done.

[gab, 2013-12-09 20:30:14]

lgtm w/ comments below + pref_hash_filter_unittest.cc + sync to ToT + updated CL
description.

Cheers!
Gab

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_calculator_unittest.cc (right):

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator_unittest.cc:76: const char*
kExpectedValue =
nit: Make this static (see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/wrNhX...
for details)

and use char[] instead of char*

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator_unittest.cc:91: const char*
kExpectedValue =
Same here.

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:90: if (kTrackedPrefs[i][0] == '\0')
This feels hacky... why not just loop over the set and check for empty() here?

[erikwright (departed), 2013-12-10 16:06:18]

bauerb: PTAL.

Please consider more appropriate names for PrefFilter.

I am currently adding a unit test for pref_hash_filter.cc

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:90: if (kTrackedPrefs[i][0] == '\0')
On 2013/12/09 20:30:15, gab wrote:
> This feels hacky... why not just loop over the set and check for empty() here?

That would involve constructing std::string unnecessarily. Given that we are
looping over a char* array I think it's pretty clear that a '\0' means an empty
string.

[Bernhard Bauer, 2013-12-10 16:52:06]

I don't think PrefFilter is too bad a name. The only alternative that comes to
mind is some kind of delegate, but TBH I like filter better.

https://codereview.chromium.org/90563003/diff/190001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/90563003/diff/190001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:273: if (pref_filter_.get())
Nit: scoped_ptr has a bool() operator, so the .get() is unnecessary.

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h
File base/prefs/pref_filter.h (right):

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:20: // any other clients are notified.
Description nit: There are no clients of a JsonPrefStore, only observers.

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:23: virtual void FilterOnLoad(PersistentPrefStore*
pref_store) = 0;
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).

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:26: // clients are notified.
Same comment about clients as above; also: is it really important that this
happens before the observers are notified?

https://codereview.chromium.org/90563003/diff/190001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/190001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:35: "",
Could you use NULL here instead?

https://codereview.chromium.org/90563003/diff/190001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.h (right):

https://codereview.chromium.org/90563003/diff/190001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.h:24: class PrefHashStoreImpl : public
PrefHashStore {
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-10 18:57:28]

Please see my responses to your questions.

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h
File base/prefs/pref_filter.h (right):

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:23: virtual void FilterOnLoad(PersistentPrefStore*
pref_store) = 0;
On 2013/12/10 16:52:07, Bernhard Bauer wrote:
> 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).

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.

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:26: // clients are notified.
On 2013/12/10 16:52:07, Bernhard Bauer wrote:
> Same comment about clients as above; also: is it really important that this
> happens before the observers are notified?

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.

https://codereview.chromium.org/90563003/diff/190001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/190001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:35: "",
On 2013/12/10 16:52:07, Bernhard Bauer wrote:
> Could you use NULL here instead?

If I did, I wouldn't be able to use the set<std::string>::set(const char* begin,
const char* end) constructor on line 72.

[gab, 2013-12-10 20:00:30]

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:90: if (kTrackedPrefs[i][0] == '\0')
On 2013/12/10 16:06:19, erikwright wrote:
> On 2013/12/09 20:30:15, gab wrote:
> > This feels hacky... why not just loop over the set and check for empty()
here?
> 
> That would involve constructing std::string unnecessarily. Given that we are
> looping over a char* array I think it's pretty clear that a '\0' means an
empty
> string.

It wouldn't involve building a string; you would simply iterate over the set (in
which the strings already exist) with a const_iterator; and check for empty()
here.

This also allows you to hide kTrackedPrefs away in the constructor's scope;
having both kTrackedPrefs and tracked_paths_ in scope here makes things
confusing IMO.

[Bernhard Bauer, 2013-12-10 23:31:42]

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h
File base/prefs/pref_filter.h (right):

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:23: virtual void FilterOnLoad(PersistentPrefStore*
pref_store) = 0;
On 2013/12/10 18:57:29, erikwright wrote:
> On 2013/12/10 16:52:07, Bernhard Bauer wrote:
> > 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).
> 
> 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.

Well, it's only used by the JsonPrefStore anyway ;-)

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

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.

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

Yes, I like that interface! What is the return value for though?

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

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:26: // clients are notified.
On 2013/12/10 18:57:29, erikwright wrote:
> On 2013/12/10 16:52:07, Bernhard Bauer wrote:
> > Same comment about clients as above; also: is it really important that this
> > happens before the observers are notified?
> 
> 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.

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

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h
File base/prefs/pref_filter.h (right):

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:23: virtual void FilterOnLoad(PersistentPrefStore*
pref_store) = 0;
On 2013/12/10 23:31:43, Bernhard Bauer wrote:
> On 2013/12/10 18:57:29, erikwright wrote:
> > On 2013/12/10 16:52:07, Bernhard Bauer wrote:
> > > 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).
> > 
> > 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.
> 
> Well, it's only used by the JsonPrefStore anyway ;-)
> 
> > (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.
> 
> 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.
> 
> > 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)
> 
> Yes, I like that interface! What is the return value for though?

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.

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

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:26: // clients are notified.
On 2013/12/10 23:31:43, Bernhard Bauer wrote:
> On 2013/12/10 18:57:29, erikwright wrote:
> > On 2013/12/10 16:52:07, Bernhard Bauer wrote:
> > > Same comment about clients as above; also: is it really important that
this
> > > happens before the observers are notified?
> > 
> > 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.
> 
> 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?

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.

[gab, 2013-12-11 18:37:14]

lg, comments below, mostly about the new test.

Cheers,
Gab

https://codereview.chromium.org/90563003/diff/270001/base/prefs/pref_filter.h
File base/prefs/pref_filter.h (right):

https://codereview.chromium.org/90563003/diff/270001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:9: #include "base/prefs/base_prefs_export.h"
nit: +empty line above

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

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:11: #include
"base/basictypes.h"
nit: +empty line above

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:17: 
nit: remove empty line

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:57: check_results_;
nit: doesn't this fit above?

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:94: scoped_ptr<PrefHashFilter>
CreatePrefHashFilter(
+Comment (here and on methods above in general).

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:104: class PrefHashFilterTest
: public testing::Test {
I've generally seen ::testing::Test instead of testing::Test and think you
should change this here for consistency.

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:115: tracked_path_ =
*temp_hash_store->checked_paths().begin();
I find this overly complex just to get the name of a tracked_path :(.

Could we perhaps expose kTrackedPrefs in an internals:: namespace?

Feels to would be useful to have better tests below (i.e., ASSERT exacts sizes
on checks instead of "greater than 0"...).

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:138: ASSERT_EQ(std::string(),
mock_pref_hash_store_->checked_value(tracked_path_));
It is important that there be a difference between NULL and empty string; please
make |tracked_path_| a pointer to able to check this.

[gab, 2013-12-11 19:06:51]

FYI, this CL makes all browser_tests fail currently with, e.g.:

        memcmp [0x6B03C8CC+2524]
        memcmp [0x6B03C386+1174]
        memcmp [0x6B03C08C+412]
        std::char_traits<char>::compare [0x00735FB4+20] (c:\program files
(x86)\microsoft visual studio 10.0\vc\include\iosfwd:486)
        std::basic_string<char,std::char_traits<char>,std::allocator<char>
>::compare [0x00735F15+149] (c:\program files (x86)\microsoft visual studio
10.0\vc\include\xstring:1899)
        std::basic_string<char,std::char_traits<char>,std::allocator<char>
>::compare [0x0075AF61+49] (c:\program files (x86)\microsoft visual studio
10.0\vc\include\xstring:1854)
        std::operator<<char,std::char_traits<char>,std::allocator<char> >
[0x007E011F+15] (c:\program files (x86)\microsoft visual studio
10.0\vc\include\string:227)
       
std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> >
>::operator() [0x007DBC1B+27] (c:\program files (x86)\microsoft visual studio
10.0\vc\include\xfunctional:125)
       
std::_Debug_lt_pred<std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> >,std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::basic_string<char,std::char_traits<char>,std::allocator<char> > >
[0x007E0C84+20] (c:\program files (x86)\microsoft
isual studio 10.0\vc\include\xutility:674)
       
std::_Tree<std::_Tset_traits<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> >
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<
[0x00D57D7F+111] (c:\program files
x86)\microsoft visual studio 10.0\vc\include\xtree:1746)
       
std::_Tree<std::_Tset_traits<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> >
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<
[0x00D57CCE+46] (c:\program files (
86)\microsoft visual studio 10.0\vc\include\xtree:1450)
       
std::_Tree<std::_Tset_traits<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> >
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<
[0x00D57A91+81] (c:\program files (
86)\microsoft visual studio 10.0\vc\include\xtree:1424)
        PrefHashFilter::FilterUpdate [0x02446931+113]
(d:\src\chrome\src\chrome\browser\prefs\pref_hash_filter.cc:79)
        JsonPrefStore::ReportValueChanged [0x0EE31301+161]
(d:\src\chrome\src\base\prefs\json_pref_store.cc:273)
        JsonPrefStore::SetValue [0x0EE2FFC9+425]
(d:\src\chrome\src\base\prefs\json_pref_store.cc:205)
        PrefService::SetUserPrefValue [0x0EE63F4D+973]
(d:\src\chrome\src\base\prefs\pref_service.cc:454)
        PrefService::SetInt64 [0x0EE626EF+143]
(d:\src\chrome\src\base\prefs\pref_service.cc:360)
        ChromeBrowserMainParts::PreCreateThreadsImpl [0x0191F567+5303]
(d:\src\chrome\src\chrome\browser\chrome_browser_main.cc:994)
        ChromeBrowserMainParts::PreCreateThreads [0x0191DD35+261]
(d:\src\chrome\src\chrome\browser\chrome_browser_main.cc:774)
        ChromeBrowserMainPartsWin::PreCreateThreads [0x0192BB16+86]
(d:\src\chrome\src\chrome\browser\chrome_browser_main_win.cc:217)
        content::BrowserMainLoop::PreCreateThreads [0x1061AD8A+298]
(d:\src\chrome\src\content\browser\browser_main_loop.cc:546)
        base::internal::RunnableAdapter<int (__thiscall
content::BrowserMainLoop::*)(void)>::Run [0x1062C91B+27]
(d:\src\chrome\src\base\bind_internal.h:134)
        base::internal::InvokeHelper<0,int,base::internal::RunnableAdapter<int
(__thiscall content::BrowserMainLoop::*)(void)>,void
__cdecl(content::BrowserMainLoop *)>::MakeItSo [0x1062C7EA+26]
(d:\src\chrome\src\base\bind_internal.h:864)
       
base::internal::Invoker<1,base::internal::BindState<base::internal::RunnableAdapter<int
(__thiscall content::BrowserMainLoop::*)(void)>,int
__cdecl(content::BrowserMainLoop *),void
__cdecl(base::internal::UnretainedWrapper<content::BrowserMainLoop>)>,int 
[0x1062C53A+74] (d:\src\chrome\src\
ase\bind_internal.h:1169)
        base::Callback<int __cdecl(void)>::Run [0x10CB6EFF+47]
(d:\src\chrome\src\base\callback.h:401)
        content::StartupTaskRunner::RunAllTasksNow [0x110B7568+184]
(d:\src\chrome\src\content\browser\startup_task_runner.cc:45)
        content::BrowserMainLoop::CreateStartupTasks [0x1061B3F1+993]
(d:\src\chrome\src\content\browser\browser_main_loop.cc:617)
        content::BrowserMainRunnerImpl::Initialize [0x1062EE2C+908]
(d:\src\chrome\src\content\browser\browser_main_runner.cc:111)
        content::BrowserMain [0x1061566A+250]
(d:\src\chrome\src\content\browser\browser_main.cc:22)
        content::BrowserTestBase::SetUp [0x064C2FFE+1614]
(d:\src\chrome\src\content\public\test\browser_test_base.cc:235)
        InProcessBrowserTest::SetUp [0x0382FFE4+1716]
(d:\src\chrome\src\chrome\test\base\in_process_browser_test.cc:210)
       
testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void>
[0x03A97E2F+319] (d:\src\chrome\src\testing\gtest\src\gtest.cc:2045)
        testing::Test::Run [0x03A8170B+107]
(d:\src\chrome\src\testing\gtest\src\gtest.cc:2057)
        testing::TestInfo::Run [0x03A823DD+221]
(d:\src\chrome\src\testing\gtest\src\gtest.cc:2241)
        testing::TestCase::Run [0x03A82D5F+239]
(d:\src\chrome\src\testing\gtest\src\gtest.cc:2345)
        testing::internal::UnitTestImpl::RunAllTests [0x03A8AF8D+701]
(d:\src\chrome\src\testing\gtest\src\gtest.cc:4066)
       
testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
[0x03A98A77+327] (d:\src\chrome\src\testing\gtest\src\gtest.cc:2045)
        testing::UnitTest::Run [0x03A89300+192]
(d:\src\chrome\src\testing\gtest\src\gtest.cc:3700)
        RUN_ALL_TESTS [0x038DF17F+15]
(d:\src\chrome\src\testing\gtest\include\gtest\gtest.h:2232)
        base::TestSuite::Run [0x038DF089+233]
(d:\src\chrome\src\base\test\test_suite.cc:213)
        `anonymous namespace'::ChromeTestLauncherDelegate::RunTestSuite
[0x0382C195+101] (d:\src\chrome\src\chrome\test\base\chrome_test_launcher.cc:65)
        content::LaunchTests [0x06586D82+1122]
(d:\src\chrome\src\content\public\test\test_launcher.cc:499)
        LaunchChromeTests [0x0382C00A+90]
(d:\src\chrome\src\chrome\test\base\chrome_test_launcher.cc:147)
        main [0x01502390+96]
(d:\src\chrome\src\chrome\test\base\browser_tests_main.cc:10)
        __tmainCRTStartup [0x0741C0BF+447]
(f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c:555)
        mainCRTStartup [0x0741BEEF+15]
(f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c:371)
        BaseThreadInitThunk [0x7609850D+14]
        RtlInitializeExceptionChain [0x775CBF39+133]
        RtlInitializeExceptionChain [0x775CBF0C+88]
[1/2] AppListStartPageWebUITest.Basic (2855 ms)

[erikwright (departed), 2013-12-12 15:53:29]

bauerb: PTAL.

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:90: if (kTrackedPrefs[i][0] == '\0')
On 2013/12/10 20:00:31, gab wrote:
> On 2013/12/10 16:06:19, erikwright wrote:
> > On 2013/12/09 20:30:15, gab wrote:
> > > This feels hacky... why not just loop over the set and check for empty()
> here?
> > 
> > That would involve constructing std::string unnecessarily. Given that we are
> > looping over a char* array I think it's pretty clear that a '\0' means an
> empty
> > string.
> 
> It wouldn't involve building a string; you would simply iterate over the set
(in
> which the strings already exist) with a const_iterator; and check for empty()
> here.
> 
> This also allows you to hide kTrackedPrefs away in the constructor's scope;
> having both kTrackedPrefs and tracked_paths_ in scope here makes things
> confusing IMO.

I can't iterate over the set; I need the index for reporting to UMA.

I could store a vector instead, but then FilterUpdate would need to O(N) search
the vector before calling StoreValue.

I could use both a vector _and_ a set, but then we are in the same situation of
having two in-scope copies of the same data (plus the hidden kTrackedPrefs).

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h
File base/prefs/pref_filter.h (right):

https://codereview.chromium.org/90563003/diff/190001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:20: // any other clients are notified.
On 2013/12/10 16:52:07, Bernhard Bauer wrote:
> Description nit: There are no clients of a JsonPrefStore, only observers.

Done.

https://codereview.chromium.org/90563003/diff/190001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.h (right):

https://codereview.chromium.org/90563003/diff/190001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.h:24: class PrefHashStoreImpl : public
PrefHashStore {
On 2013/12/10 16:52:07, Bernhard Bauer wrote:
> 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. 

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.

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

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

[gab, 2013-12-12 17:20:57]

Some comments based on my recent interaction with this CL while writing a CL
based on top of it.

Cheers,
Gab

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/170001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:90: if (kTrackedPrefs[i][0] == '\0')
On 2013/12/12 15:53:30, erikwright wrote:
> On 2013/12/10 20:00:31, gab wrote:
> > On 2013/12/10 16:06:19, erikwright wrote:
> > > On 2013/12/09 20:30:15, gab wrote:
> > > > This feels hacky... why not just loop over the set and check for empty()
> > here?
> > > 
> > > That would involve constructing std::string unnecessarily. Given that we
are
> > > looping over a char* array I think it's pretty clear that a '\0' means an
> > empty
> > > string.
> > 
> > It wouldn't involve building a string; you would simply iterate over the set
> (in
> > which the strings already exist) with a const_iterator; and check for
empty()
> > here.
> > 
> > This also allows you to hide kTrackedPrefs away in the constructor's scope;
> > having both kTrackedPrefs and tracked_paths_ in scope here makes things
> > confusing IMO.
> 
> I can't iterate over the set; I need the index for reporting to UMA.

Right, I just realized that while working with this CL; I will change the
approach in an upcoming CL, but I'm fine with you keeping it like this for now.

See https://codereview.chromium.org/114223002/ for a skeleton of my approach.

> 
> I could store a vector instead, but then FilterUpdate would need to O(N)
search
> the vector before calling StoreValue.
> 
> I could use both a vector _and_ a set, but then we are in the same situation
of
> having two in-scope copies of the same data (plus the hidden kTrackedPrefs).
>

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:39: std::set<std::string>
tracked_paths_;
std::set<const std::string>

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store.h:45: virtual void StoreValue(const
std::string& path,
Consider a different name (e.g. "UpdateHash") given this doesn't actually store
the value itself (and update the comment above).

[erikwright (departed), 2013-12-12 22:02:56]

gab: Responded to your comments.

https://codereview.chromium.org/90563003/diff/270001/base/prefs/pref_filter.h
File base/prefs/pref_filter.h (right):

https://codereview.chromium.org/90563003/diff/270001/base/prefs/pref_filter.h...
base/prefs/pref_filter.h:9: #include "base/prefs/base_prefs_export.h"
On 2013/12/11 18:37:14, gab wrote:
> nit: +empty line above

Done.

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:39: std::set<std::string>
tracked_paths_;
On 2013/12/12 17:20:58, gab wrote:
> std::set<const std::string>

Discussed offline. This would have no impact. Dereferencing an iterator already
returns a const reference.

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

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:11: #include
"base/basictypes.h"
On 2013/12/11 18:37:14, gab wrote:
> nit: +empty line above

Done.

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:17: 
On 2013/12/11 18:37:14, gab wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:57: check_results_;
On 2013/12/11 18:37:14, gab wrote:
> nit: doesn't this fit above?

Done.

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:94: scoped_ptr<PrefHashFilter>
CreatePrefHashFilter(
On 2013/12/11 18:37:14, gab wrote:
> +Comment (here and on methods above in general).

Done.

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:104: class PrefHashFilterTest
: public testing::Test {
On 2013/12/11 18:37:14, gab wrote:
> I've generally seen ::testing::Test instead of testing::Test and think you
> should change this here for consistency.

I counted. Without :: is more widespread by a factor of 6:1

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:115: tracked_path_ =
*temp_hash_store->checked_paths().begin();
On 2013/12/11 18:37:14, gab wrote:
> I find this overly complex just to get the name of a tracked_path :(.
> 
> Could we perhaps expose kTrackedPrefs in an internals:: namespace?
> 
> Feels to would be useful to have better tests below (i.e., ASSERT exacts sizes
> on checks instead of "greater than 0"...).

I actually prefer this approach because it's not tied to the values that are
actually being tracked. If we so choose, we could explicitly look for some that
we expect to be tracked, but doing so by referencing kTrackedPrefs is a bit
circular.

DCHECKing exact sizes is just a painful thing to manually update when new values
are added.

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:138: ASSERT_EQ(std::string(),
mock_pref_hash_store_->checked_value(tracked_path_));
On 2013/12/11 18:37:14, gab wrote:
> It is important that there be a difference between NULL and empty string;
please
> make |tracked_path_| a pointer to able to check this.

It's a pain for memory safety reasons to maintain the pointers, so I've added a
special value for NULLs.

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store.h:45: virtual void StoreValue(const
std::string& path,
On 2013/12/12 17:20:58, gab wrote:
> Consider a different name (e.g. "UpdateHash") given this doesn't actually
store
> the value itself (and update the comment above).

Done.

[gab, 2013-12-12 22:56:34]

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

https://codereview.chromium.org/90563003/diff/270001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:115: tracked_path_ =
*temp_hash_store->checked_paths().begin();
On 2013/12/12 22:02:57, erikwright wrote:
> On 2013/12/11 18:37:14, gab wrote:
> > I find this overly complex just to get the name of a tracked_path :(.
> > 
> > Could we perhaps expose kTrackedPrefs in an internals:: namespace?
> > 
> > Feels to would be useful to have better tests below (i.e., ASSERT exacts
sizes
> > on checks instead of "greater than 0"...).
> 
> I actually prefer this approach because it's not tied to the values that are
> actually being tracked. If we so choose, we could explicitly look for some
that
> we expect to be tracked, but doing so by referencing kTrackedPrefs is a bit
> circular.
> 
> DCHECKing exact sizes is just a painful thing to manually update when new
values
> are added.

How about adding a test-only constructor that takes in the list of tracked
preference? Then you can pass in whatever you want as a list of test tracked
prefs.

I'm thinking of doing that for the CL I referred to earlier; especially since
the kTrackedPrefs are going to hold metadata in my CL.

(I'm happy with doing it in my CL and letting you commit this as-is FWIW)

[gab, 2013-12-13 12:13:33]

re-lgtm; I'm happy to address my 2 remaining concerns in an upcoming CL of mine
where it'll make even more sense to have the generalizations I was asking for.

@bauerb: all yours :)!

[gab, 2013-12-13 16:54:24]

Still lgtm, but one more thing :)!

https://codereview.chromium.org/90563003/diff/350001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/90563003/diff/350001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store.h:40: // Checks |initial_value| against the
existing stored value hash.
nit: Add mention of |path| in above comment.

https://codereview.chromium.org/90563003/diff/350001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store.h:41: virtual ValueState CheckValue(
How about "CheckHash" here too:
1) To be consistent with StoreHash below.
2) For the upcoming split prefs, "StoreSplitHash" sounds better than
"StoreSplitValue".

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

LGTM

Apologies for the delay, I was on a plane for a while.

https://codereview.chromium.org/90563003/diff/190001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.h (right):

https://codereview.chromium.org/90563003/diff/190001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.h:24: class PrefHashStoreImpl : public
PrefHashStore {
On 2013/12/12 15:53:30, erikwright wrote:
> On 2013/12/10 16:52:07, Bernhard Bauer wrote:
> > 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. 
> 
> 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.

FWIW, you could use a TestingPrefStore, which is fairly easy.
 
> Making this an interface rather than directly accessing the concrete
> implementation permits that.
> 
> > 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.
> 
> 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).

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.

https://codereview.chromium.org/90563003/diff/350001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/90563003/diff/350001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:18: namespace base {
Nit: These forward-declarations aren't really necessary, because they're already
in pref_filter.h.

[erikwright (departed), 2013-12-16 19:38:55]

rlp@ for chrome/browser/profiles/

[erikwright (departed), 2013-12-16 19:40:34]

brettw: for base/base.gypi only.

[rpetterson, 2013-12-16 23:17:01]

c/b/profiles lgtm

[gab, 2013-12-17 00:11:41]

On 2013/12/16 19:40:34, erikwright wrote:
> brettw: for base/base.gypi only.

TBR brettw, let's try to get this in prior to branch cut.

[commit-bot: I haz the power, 2013-12-17 00:17:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/390001

[gab, 2013-12-17 01:44:09]

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

https://codereview.chromium.org/90563003/diff/410002/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:131: pref_filter.reset(new
PrefHashFilter(pref_hash_store.Pass()));
@erikwright: Fixed this for you as discussed, will CQ now.

I think this highlights the fact that it could be better to ask the API's client
to pass a PrefHashFilter in here directly; perhaps even moving the params of
PrefHashStore into PrefHashFilter's constructor, making PrefHashStore an
implementation detail of PrefHashFilter (akin to PrefHashCalculator vs
PrefHashStore).

Let's consider this change for a follow-up CL.

[gab, 2013-12-17 01:45:26]

TBR scottbyer for API changes side-effects on
chrome/service/service_process_prefs.cc

[commit-bot: I haz the power, 2013-12-17 01:54:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/410002

[commit-bot: I haz the power, 2013-12-17 04:00:27]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-17 04:33:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/410002

[commit-bot: I haz the power, 2013-12-17 05:24:57]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-17 12:48:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/410002

[gab, 2013-12-17 12:58:11]

Trying again to get more data running this from ToT, but the latest failures
often include:"FATAL:important_file_writer.cc(143)] Check failed: false.", on
all platforms, which is perhaps a true failure caused by this CL.

[commit-bot: I haz the power, 2013-12-17 13:50:48]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-17 15:19:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/480001

[gab, 2013-12-17 15:31:45]

https://codereview.chromium.org/90563003/diff/480001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/90563003/diff/480001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:295: initialized_ = true;
Why change this back?

Was something else depending on it?

Or simply because the pref_filter_ no longer depends on this as it can now
modify the prefs_ directly without forcing interaction with the observers?

https://codereview.chromium.org/90563003/diff/480001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/90563003/diff/480001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:75:
local_state_->CommitPendingWrite();
Would it make sense to add global shutdown logic for the IO thread to always do
this for each PrefService when it's going away? Feels this is broken at a level
higher than this specific CL.

Some sort of "IO thread shutdown task" posted by each PrefService; not sure how
to structure this in practice..

[commit-bot: I haz the power, 2013-12-17 15:33:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/520001

[erikwright (departed), 2013-12-17 15:43:44]

On 2013/12/17 15:31:45, gab wrote:
>
https://codereview.chromium.org/90563003/diff/480001/base/prefs/json_pref_sto...
> File base/prefs/json_pref_store.cc (right):
> 
>
https://codereview.chromium.org/90563003/diff/480001/base/prefs/json_pref_sto...
> base/prefs/json_pref_store.cc:295: initialized_ = true;
> Why change this back?
> 
> Was something else depending on it?
> 
> Or simply because the pref_filter_ no longer depends on this as it can now
> modify the prefs_ directly without forcing interaction with the observers?

Precisely. I noticed that this now unnecessary change had persisted. I felt it
was best to revert it to reduce confusion.

> 
>
https://codereview.chromium.org/90563003/diff/480001/chrome/browser/prefs/pre...
> File chrome/browser/prefs/pref_hash_store_impl.cc (right):
> 
>
https://codereview.chromium.org/90563003/diff/480001/chrome/browser/prefs/pre...
> chrome/browser/prefs/pref_hash_store_impl.cc:75:
> local_state_->CommitPendingWrite();
> Would it make sense to add global shutdown logic for the IO thread to always
do
> this for each PrefService when it's going away? Feels this is broken at a
level
> higher than this specific CL.
> 
> Some sort of "IO thread shutdown task" posted by each PrefService; not sure
how
> to structure this in practice..

I agree that this is already broken. I'm not sure what the best thing to do is.

[Alexei Svitkine (slow), 2013-12-17 16:01:20]

Histograms look good and it seems much cleaner than before - sweet!

Some misc comments below:

https://codereview.chromium.org/90563003/diff/520001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/90563003/diff/520001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:270: if (pref_filter_) {
Can you add a unit test for this?

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_calculator.cc (right):

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.cc:50: unsigned char
digest[kSHA256DigestSize];
Can you use:

crypto::HMAC hmac(crypto::HMAC::SHA256);
std::vector<uint8> digest(hmac.DigestLength());

(and then use digest.size() instead of arraysize(digest) below).

This way, you don't need to have kSHA256DigestSize hardcoded in this file.

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_calculator.h (right):

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_calculator.h:47: std::string seed_;
Can you document |seed_| somewhere in this header file? I only see "Constructs a
PrefHashCalculator using |seed|", which doesn't tell me what it is.

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:40: void
ReportValidationResult(PrefHashStore::ValueState value_state,
Nit: Add a short comment (especially documenting what |value_index| is).

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:25: // are changed.
I think this description should be more clear about the fact that this class
looks at specific prefs values (rather than being a generic utility class).
Also, mention what type of prefs store this applies to (e.g. profile Prefs file
or Local State or ...)?

[Scott Byer, 2013-12-17 17:08:02]

LGTM for service_process_prefs

[Bernhard Bauer, 2013-12-17 17:21:23]

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:57: const std::string& path, const
base::Value* new_value) {
Nit: I think you can break this after `path,`?

https://codereview.chromium.org/90563003/diff/520001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:74: // IO thread is already gone.
Consider other solutions.
Wait, what IO thread? Or do you mean the SequencedTaskRunner we use for writing
out prefs to disk (which in practice is BrowserThread::GetBlockingPool())?

[commit-bot: I haz the power, 2013-12-17 17:56:10]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-17 18:47:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/520001

[commit-bot: I haz the power, 2013-12-17 21:23:37]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-18 19:26:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/540001

[commit-bot: I haz the power, 2013-12-19 03:47:11]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-12-19 12:21:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/540001

[commit-bot: I haz the power, 2013-12-19 18:27:13]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-12-19 18:43:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/540001

[commit-bot: I haz the power, 2013-12-20 01:51:43]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-12-20 07:18:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/540001

[commit-bot: I haz the power, 2013-12-20 13:37:22]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-12-23 02:07:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/540001

[commit-bot: I haz the power, 2013-12-23 04:14:47]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-23 17:08:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/540001

[commit-bot: I haz the power, 2013-12-23 17:10:05]

Change committed as 242382

[commit-bot: I haz the power, 2013-12-23 20:41:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/850001

[commit-bot: I haz the power, 2013-12-23 20:41:37]

Failed to apply patch for chrome/browser/prefs/pref_service_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/prefs/pref_service_browsertest.cc
  Hunk #4 FAILED at 127.
  Hunk #5 FAILED at 187.
  2 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/prefs/pref_service_browsertest.cc.rej

Patch:       chrome/browser/prefs/pref_service_browsertest.cc
Index: chrome/browser/prefs/pref_service_browsertest.cc
diff --git a/chrome/browser/prefs/pref_service_browsertest.cc
b/chrome/browser/prefs/pref_service_browsertest.cc
index
959e1624d24d6ed82fbc499e77eaa302f19fea38..1964f200a57db8d9a11b08c5b3f9d9b4c83ccd71
100644
--- a/chrome/browser/prefs/pref_service_browsertest.cc
+++ b/chrome/browser/prefs/pref_service_browsertest.cc
@@ -65,9 +65,8 @@ class PreferenceServiceTest : public InProcessBrowserTest {
     base::FilePath user_data_directory;
     PathService::Get(chrome::DIR_USER_DATA, &user_data_directory);
 
-    base::FilePath reference_pref_file;
     if (new_profile_) {
-      reference_pref_file = ui_test_utils::GetTestFilePath(
+      original_pref_file_ = ui_test_utils::GetTestFilePath(
           base::FilePath().AppendASCII("profiles").
                      AppendASCII("window_placement").
                      AppendASCII("Default"),
@@ -77,17 +76,17 @@ class PreferenceServiceTest : public InProcessBrowserTest {
       CHECK(base::CreateDirectory(tmp_pref_file_));
       tmp_pref_file_ = tmp_pref_file_.Append(chrome::kPreferencesFilename);
     } else {
-      reference_pref_file = ui_test_utils::GetTestFilePath(
+      original_pref_file_ = ui_test_utils::GetTestFilePath(
           base::FilePath().AppendASCII("profiles").
                      AppendASCII("window_placement"),
           base::FilePath().Append(chrome::kLocalStateFilename));
       tmp_pref_file_ = user_data_directory.Append(chrome::kLocalStateFilename);
     }
 
-    CHECK(base::PathExists(reference_pref_file));
+    CHECK(base::PathExists(original_pref_file_));
     // Copy only the Preferences file if |new_profile_|, or Local State if not,
     // and the rest will be automatically created.
-    CHECK(base::CopyFile(reference_pref_file, tmp_pref_file_));
+    CHECK(base::CopyFile(original_pref_file_, tmp_pref_file_));
 
 #if defined(OS_WIN)
     // Make the copy writable.  On POSIX we assume the umask allows files
@@ -99,6 +98,7 @@ class PreferenceServiceTest : public InProcessBrowserTest {
   }
 
  protected:
+  base::FilePath original_pref_file_;
   base::FilePath tmp_pref_file_;
 
  private:
@@ -127,7 +127,7 @@ IN_PROC_BROWSER_TEST_F(PreservedWindowPlacementIsLoaded,
Test) {
 
   // The window should open with the new reference profile, with window
   // placement values stored in the user data directory.
-  JSONFileValueSerializer deserializer(tmp_pref_file_);
+  JSONFileValueSerializer deserializer(original_pref_file_);
   scoped_ptr<Value> root(deserializer.Deserialize(NULL, NULL));
 
   ASSERT_TRUE(root.get());
@@ -187,7 +187,7 @@ IN_PROC_BROWSER_TEST_F(PreservedWindowPlacementIsMigrated,
Test) {
   // The window should open with the old reference profile, with window
   // placement values stored in Local State.
 
-  JSONFileValueSerializer deserializer(tmp_pref_file_);
+  JSONFileValueSerializer deserializer(original_pref_file_);
   scoped_ptr<Value> root(deserializer.Deserialize(NULL, NULL));
 
   ASSERT_TRUE(root.get());

[commit-bot: I haz the power, 2013-12-23 21:06:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/900001

[commit-bot: I haz the power, 2013-12-23 21:06:46]

Failed to apply patch for chrome/browser/prefs/pref_service_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/prefs/pref_service_browsertest.cc
  Hunk #4 FAILED at 127.
  Hunk #5 FAILED at 187.
  2 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/prefs/pref_service_browsertest.cc.rej

Patch:       chrome/browser/prefs/pref_service_browsertest.cc
Index: chrome/browser/prefs/pref_service_browsertest.cc
diff --git a/chrome/browser/prefs/pref_service_browsertest.cc
b/chrome/browser/prefs/pref_service_browsertest.cc
index
959e1624d24d6ed82fbc499e77eaa302f19fea38..1964f200a57db8d9a11b08c5b3f9d9b4c83ccd71
100644
--- a/chrome/browser/prefs/pref_service_browsertest.cc
+++ b/chrome/browser/prefs/pref_service_browsertest.cc
@@ -65,9 +65,8 @@ class PreferenceServiceTest : public InProcessBrowserTest {
     base::FilePath user_data_directory;
     PathService::Get(chrome::DIR_USER_DATA, &user_data_directory);
 
-    base::FilePath reference_pref_file;
     if (new_profile_) {
-      reference_pref_file = ui_test_utils::GetTestFilePath(
+      original_pref_file_ = ui_test_utils::GetTestFilePath(
           base::FilePath().AppendASCII("profiles").
                      AppendASCII("window_placement").
                      AppendASCII("Default"),
@@ -77,17 +76,17 @@ class PreferenceServiceTest : public InProcessBrowserTest {
       CHECK(base::CreateDirectory(tmp_pref_file_));
       tmp_pref_file_ = tmp_pref_file_.Append(chrome::kPreferencesFilename);
     } else {
-      reference_pref_file = ui_test_utils::GetTestFilePath(
+      original_pref_file_ = ui_test_utils::GetTestFilePath(
           base::FilePath().AppendASCII("profiles").
                      AppendASCII("window_placement"),
           base::FilePath().Append(chrome::kLocalStateFilename));
       tmp_pref_file_ = user_data_directory.Append(chrome::kLocalStateFilename);
     }
 
-    CHECK(base::PathExists(reference_pref_file));
+    CHECK(base::PathExists(original_pref_file_));
     // Copy only the Preferences file if |new_profile_|, or Local State if not,
     // and the rest will be automatically created.
-    CHECK(base::CopyFile(reference_pref_file, tmp_pref_file_));
+    CHECK(base::CopyFile(original_pref_file_, tmp_pref_file_));
 
 #if defined(OS_WIN)
     // Make the copy writable.  On POSIX we assume the umask allows files
@@ -99,6 +98,7 @@ class PreferenceServiceTest : public InProcessBrowserTest {
   }
 
  protected:
+  base::FilePath original_pref_file_;
   base::FilePath tmp_pref_file_;
 
  private:
@@ -127,7 +127,7 @@ IN_PROC_BROWSER_TEST_F(PreservedWindowPlacementIsLoaded,
Test) {
 
   // The window should open with the new reference profile, with window
   // placement values stored in the user data directory.
-  JSONFileValueSerializer deserializer(tmp_pref_file_);
+  JSONFileValueSerializer deserializer(original_pref_file_);
   scoped_ptr<Value> root(deserializer.Deserialize(NULL, NULL));
 
   ASSERT_TRUE(root.get());
@@ -187,7 +187,7 @@ IN_PROC_BROWSER_TEST_F(PreservedWindowPlacementIsMigrated,
Test) {
   // The window should open with the old reference profile, with window
   // placement values stored in Local State.
 
-  JSONFileValueSerializer deserializer(tmp_pref_file_);
+  JSONFileValueSerializer deserializer(original_pref_file_);
   scoped_ptr<Value> root(deserializer.Deserialize(NULL, NULL));
 
   ASSERT_TRUE(root.get());

[commit-bot: I haz the power, 2013-12-23 21:23:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/90563003/940001

[commit-bot: I haz the power, 2013-12-23 21:24:49]

Change committed as 242407

[erikwright (departed), 2013-12-23 21:30:20]

On 2013/12/23 21:24:49, I haz the power (commit-bot) wrote:
> Change committed as 242407

Oops, forgot to remove NOTRY=True.

I'll watch for build errors.