Refactor TestingPrefStore to make it useful to some tests of load errors and asynchrony.

Refactor TestingPrefStore to make it useful to some tests of load errors and asynchrony.

BUG=349158
R=mnissler@chromium.org

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

[erikwright (departed), 2014-03-27 19:22:04]

mnissler: PTAL.

[erikwright (departed), 2014-03-27 19:22:27]

On 2014/03/27 19:22:04, erikwright wrote:
> mnissler: PTAL.

This is used by https://codereview.chromium.org/205813002/

[Mattias Nissler (ping if slow), 2014-03-27 20:28:05]

https://codereview.chromium.org/210063003/diff/140001/base/prefs/overlay_user...
File base/prefs/overlay_user_pref_store_unittest.cc (right):

https://codereview.chromium.org/210063003/diff/140001/base/prefs/overlay_user...
base/prefs/overlay_user_pref_store_unittest.cc:29: void
ExpectAndPopOneValue(const std::string& value,
It'd probably make sense to turn this into:

void PrefStoreObserverMock::VerifyChangedKeyAndReset(const std::string& key);

That way, you could also use it for the other test.

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
File base/prefs/testing_pref_store.cc (right):

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
base/prefs/testing_pref_store.cc:100: if (read_success_ && read_error_ !=
PREF_READ_ERROR_NONE && error_delegate_)
nit: I'd drop the read_error_ != PREF_READ_ERROR_NONE clause here, which would
leave tests fully in control of the returned error code.

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
File base/prefs/testing_pref_store.h (right):

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
base/prefs/testing_pref_store.h:103: scoped_ptr<ReadErrorDelegate>
error_delegate_;
Why is this a scoped_ptr? In production PrefStore implementations it's a weak
pointer, and I deviating here probably just causes nasty surprises for users of
TestingPrefStore.

https://codereview.chromium.org/210063003/diff/140001/components/policy/core/...
File components/policy/core/browser/configuration_policy_pref_store_unittest.cc
(right):

https://codereview.chromium.org/210063003/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_pref_store_unittest.cc:189:
observer_.changed_keys.clear();
Let's add a Reset() member function to PrefStoreObserverMock that clears
internal state.

[erikwright (departed), 2014-03-28 12:29:22]

Based on these comments and my responses, the only change I will implement is
the VerifyChangedKeyAndReset. Let me know if you disagree.

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
File base/prefs/testing_pref_store.cc (right):

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
base/prefs/testing_pref_store.cc:100: if (read_success_ && read_error_ !=
PREF_READ_ERROR_NONE && error_delegate_)
On 2014/03/27 20:28:05, Mattias Nissler wrote:
> nit: I'd drop the read_error_ != PREF_READ_ERROR_NONE clause here, which would
> leave tests fully in control of the returned error code.

This logic models the behavior of JsonPrefStore, which is the only actual
implementation of PersistentPrefStore.

To remove the logic would require me to add another bit of state, which is
whether or not to actually call the error delegate.

While the API of JsonPrefStore (and thus PersistentPrefStore) is a bit awkward,
I think for now it makes sense to have this mock object facilitate testing
something as similar as possible to it.

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
File base/prefs/testing_pref_store.h (right):

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
base/prefs/testing_pref_store.h:103: scoped_ptr<ReadErrorDelegate>
error_delegate_;
On 2014/03/27 20:28:05, Mattias Nissler wrote:
> Why is this a scoped_ptr? In production PrefStore implementations it's a weak
> pointer, and I deviating here probably just causes nasty surprises for users
of
> TestingPrefStore.

No, see JsonPrefStore. scoped_ptr, held for the lifetime of the
PersistentPrefStore, as strange as I agree that is.

https://codereview.chromium.org/210063003/diff/140001/components/policy/core/...
File components/policy/core/browser/configuration_policy_pref_store_unittest.cc
(right):

https://codereview.chromium.org/210063003/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_pref_store_unittest.cc:189:
observer_.changed_keys.clear();
On 2014/03/27 20:28:05, Mattias Nissler wrote:
> Let's add a Reset() member function to PrefStoreObserverMock that clears
> internal state.

I assume you mean the VerifyChangedKeyAndReset(), which I think would only reset
the changed_keys, not the initialized state. It makes sense for the observer to
signal a failure if it is unexpectedly initialized again.

[Mattias Nissler (ping if slow), 2014-03-28 12:32:52]

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
File base/prefs/testing_pref_store.cc (right):

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
base/prefs/testing_pref_store.cc:100: if (read_success_ && read_error_ !=
PREF_READ_ERROR_NONE && error_delegate_)
On 2014/03/28 12:29:22, erikwright wrote:
> On 2014/03/27 20:28:05, Mattias Nissler wrote:
> > nit: I'd drop the read_error_ != PREF_READ_ERROR_NONE clause here, which
would
> > leave tests fully in control of the returned error code.
> 
> This logic models the behavior of JsonPrefStore, which is the only actual
> implementation of PersistentPrefStore.
> 
> To remove the logic would require me to add another bit of state, which is
> whether or not to actually call the error delegate.

Don't you already have that bit in |read_success_| ?

> 
> While the API of JsonPrefStore (and thus PersistentPrefStore) is a bit
awkward,
> I think for now it makes sense to have this mock object facilitate testing
> something as similar as possible to it.

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
File base/prefs/testing_pref_store.h (right):

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
base/prefs/testing_pref_store.h:103: scoped_ptr<ReadErrorDelegate>
error_delegate_;
On 2014/03/28 12:29:22, erikwright wrote:
> On 2014/03/27 20:28:05, Mattias Nissler wrote:
> > Why is this a scoped_ptr? In production PrefStore implementations it's a
weak
> > pointer, and I deviating here probably just causes nasty surprises for users
> of
> > TestingPrefStore.
> 
> No, see JsonPrefStore. scoped_ptr, held for the lifetime of the
> PersistentPrefStore, as strange as I agree that is.

Ah, right, my bad.

https://codereview.chromium.org/210063003/diff/140001/components/policy/core/...
File components/policy/core/browser/configuration_policy_pref_store_unittest.cc
(right):

https://codereview.chromium.org/210063003/diff/140001/components/policy/core/...
components/policy/core/browser/configuration_policy_pref_store_unittest.cc:189:
observer_.changed_keys.clear();
On 2014/03/28 12:29:22, erikwright wrote:
> On 2014/03/27 20:28:05, Mattias Nissler wrote:
> > Let's add a Reset() member function to PrefStoreObserverMock that clears
> > internal state.
> 
> I assume you mean the VerifyChangedKeyAndReset(), which I think would only
reset
> the changed_keys, not the initialized state. It makes sense for the observer
to
> signal a failure if it is unexpectedly initialized again.

Yes, sounds good.

[erikwright (departed), 2014-03-28 12:49:55]

PTAL.

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
File base/prefs/testing_pref_store.cc (right):

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
base/prefs/testing_pref_store.cc:100: if (read_success_ && read_error_ !=
PREF_READ_ERROR_NONE && error_delegate_)
On 2014/03/28 12:32:53, Mattias Nissler wrote:
> On 2014/03/28 12:29:22, erikwright wrote:
> > On 2014/03/27 20:28:05, Mattias Nissler wrote:
> > > nit: I'd drop the read_error_ != PREF_READ_ERROR_NONE clause here, which
> would
> > > leave tests fully in control of the returned error code.
> > 
> > This logic models the behavior of JsonPrefStore, which is the only actual
> > implementation of PersistentPrefStore.
> > 
> > To remove the logic would require me to add another bit of state, which is
> > whether or not to actually call the error delegate.
> 
> Don't you already have that bit in |read_success_| ?
> 
> > 
> > While the API of JsonPrefStore (and thus PersistentPrefStore) is a bit
> awkward,
> > I think for now it makes sense to have this mock object facilitate testing
> > something as similar as possible to it.
> 

No, in JsonPrefStore, read_success_ can be true while read_error is not NONE.

Strangely, it's only not "success" if there is no directory. If there is a
directory, even if we can't for some reason access the file (which causes a
non-NONE read error) we report success.

As surprising to me as it is to you, but that's what I'm working with, for now.

[Mattias Nissler (ping if slow), 2014-03-28 12:57:41]

LGTM

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
File base/prefs/testing_pref_store.cc (right):

https://codereview.chromium.org/210063003/diff/140001/base/prefs/testing_pref...
base/prefs/testing_pref_store.cc:100: if (read_success_ && read_error_ !=
PREF_READ_ERROR_NONE && error_delegate_)
On 2014/03/28 12:49:55, erikwright wrote:
> On 2014/03/28 12:32:53, Mattias Nissler wrote:
> > On 2014/03/28 12:29:22, erikwright wrote:
> > > On 2014/03/27 20:28:05, Mattias Nissler wrote:
> > > > nit: I'd drop the read_error_ != PREF_READ_ERROR_NONE clause here, which
> > would
> > > > leave tests fully in control of the returned error code.
> > > 
> > > This logic models the behavior of JsonPrefStore, which is the only actual
> > > implementation of PersistentPrefStore.
> > > 
> > > To remove the logic would require me to add another bit of state, which is
> > > whether or not to actually call the error delegate.
> > 
> > Don't you already have that bit in |read_success_| ?
> > 
> > > 
> > > While the API of JsonPrefStore (and thus PersistentPrefStore) is a bit
> > awkward,
> > > I think for now it makes sense to have this mock object facilitate testing
> > > something as similar as possible to it.
> > 
> 
> No, in JsonPrefStore, read_success_ can be true while read_error is not NONE.
> 
> Strangely, it's only not "success" if there is no directory. If there is a
> directory, even if we can't for some reason access the file (which causes a
> non-NONE read error) we report success.
> 
> As surprising to me as it is to you, but that's what I'm working with, for
now.

OK, makes sense.

[erikwright (departed), 2014-03-28 12:58:27]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-03-28 12:59:22]

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

[erikwright (departed), 2014-03-28 16:17:49]

Committed patchset #8 manually as r260150 (presubmit successful).