Make a JsonPrefStore readable while mid-interception.

Make a JsonPrefStore readable while mid-interception.

BUG=372547

[erikwright (departed), 2014-06-11 18:42:46]

gab: PTAL. I still need to add a comment per your request.

[gab, 2014-06-11 21:12:56]

+Bernhard FYI


Please add a link to the main CL, https://codereview.chromium.org/324493002/, to
justify this change (which still hurts my soul but I'm willing to let through
for the sake of velocity on obtaining data on the improvement impact of having
the MACs in the same store as the values they protect).

Please augment the CL description to contain something as such:

"""
This CL is temporarily necessary to allow the PrefHashFilter/PrefHashStore to
access MACs stored in the same JsonPrefStore as their values.

This change introduces undesired ownership and lifetime requirements which were
already respected by the code as it is today, but which we don't want to enforce
on the API and have to keep around long term.

This is simpler as a first step as PrefHashFilter/PrefHashStore can just keep
operating the way they currently do without having to know that the
PrefHashStore is actually now backed by the same store as the values it is
protecting. 

The ownership model prior to this CL will be restored in a follow-up CL with
extra API changes allowing PrefHashFilter to be aware of the fact that the MACs
are stored alongside the values which will simplify the APIs and abstraction
layers and get rid of a circular dependency we temporarily have to introduce in
https://codereview.chromium.org/324493002/.
"""

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

https://codereview.chromium.org/329173002/diff/1/base/prefs/json_pref_store.c...
base/prefs/json_pref_store.cc:345: &JsonPrefStore::FinalizeFileRead, this,
initialization_successful));
For context for Bernhard:

this CL is okay in the sense that it won't cause issues short term. However it
forces this callback to hold a reference to the JsonPrefStore which we wanted to
get rid of.

This was discussed in lengths @
https://codereview.chromium.org/324493002/diff/40001/base/prefs/json_pref_sto...

Erik will add a TODO for now explaining this so that we can proceed with a very
important change for M37 (allowing the MACs to be in the same JsonPrefStore as
their protected values: https://codereview.chromium.org/324493002) and I will
follow-up with a change to clean this up, restoring the ownership dance of the
|prefs_| and allowing us to drop the refcounting here and on the
FileThreadDeserializer.

https://codereview.chromium.org/329173002/diff/1/base/prefs/json_pref_store_u...
File base/prefs/json_pref_store_unittest.cc (right):

https://codereview.chromium.org/329173002/diff/1/base/prefs/json_pref_store_u...
base/prefs/json_pref_store_unittest.cc:68: void
InterceptingPrefFilter::ReleasePrefs() {
"ReleasePrefs" is technically wrong now that this object is no longer holding on
to prefs..

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

https://codereview.chromium.org/329173002/diff/1/base/prefs/pref_filter.h#new...
base/prefs/pref_filter.h:32: // disk
nit: fix early line break

https://codereview.chromium.org/329173002/diff/1/chrome/browser/prefs/pref_ha...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/329173002/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_filter_unittest.cc:372: void GetPrefsBack(bool
expected_schedule_write,
The name of this method doesn't make sense anymore.

https://codereview.chromium.org/329173002/diff/1/chrome/browser/prefs/tracked...
File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc
(left):

https://codereview.chromium.org/329173002/diff/1/chrome/browser/prefs/tracked...
chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:198: bool
HasPrefs(MockPrefStoreID store_id) {
See my previous comment on the main CL; we need something to replace HasPrefs()
(as a verifier that the TrackedPreferencesMigrator was handed the prefs).

https://codereview.chromium.org/329173002/diff/1/chrome/browser/prefs/tracked...
File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc
(right):

https://codereview.chromium.org/329173002/diff/1/chrome/browser/prefs/tracked...
chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:241: void
GetPrefsBack(MockPrefStoreID store_id,
The name of this method no longer makes sense without ownership transfer.

[gab, 2014-06-13 02:00:02]

not lgtm given we managed to go straight to the better design in
https://codereview.chromium.org/324493002 and won't need this intermediate step

Closing this.

Thanks again for bearing with me!
Gab