Add a unit test for JsonPrefStore::Remove.

Add a unit test for JsonPrefStore::Remove.

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

[dgrogan, 2014-01-22 02:56:15]

I was slightly surprised by this behavior (the resulting empty dictionary
"long_int" disappears) while working on LevelDBPrefStore, so wanted to document
it via unit test. PTAL?

[gab, 2014-01-22 14:22:25]

On 2014/01/22 02:56:15, dgrogan wrote:
> I was slightly surprised by this behavior (the resulting empty dictionary
> "long_int" disappears) while working on LevelDBPrefStore, so wanted to
document
> it via unit test. PTAL?

Note that this is the behavior if you call RemoveValue because you should only
do this when this is the root registered variable (i.e. it's called by
PrefService::ClearPref()) -- user code shouldn't use this directly.

If your registered variable is "long_int" and you have a "pref" value in the
"long_int" dictionary, you should remove "pref" from "long_int" like this:

DictionaryPrefUpdate update("long_int");
base::DictionaryValue* long_int_dict = update.Get();
long_int_dict->Remove("pref");

The DictionaryPrefUpdate takes care of the update notification for "long_int"
and the Remove() call on the dictionary only removes that value (leaving the
dictionary even if it ends up being empty).

The reason RemoveValue() needs to remove empty dictionaries left behind it is
because if you clear say "foo.bar.baz" and the foo/bar dictionaries only exist
to contain "baz", they should be cleared too.

Perhaps PrefStore::RemoveValue() should be renamed PrefStore::RemovePath() for
consistency with the base::Value::RemovePath() API it's using?
I'm not familiar enough with the API of all other PrefStores to see whether that
name makes sense in all scenarios.

Cheers!
Gab

[gab, 2014-01-22 15:00:39]

On 2014/01/22 14:22:25, gab wrote:
> On 2014/01/22 02:56:15, dgrogan wrote:
> > I was slightly surprised by this behavior (the resulting empty dictionary
> > "long_int" disappears) while working on LevelDBPrefStore, so wanted to
> document
> > it via unit test. PTAL?
> 
> Note that this is the behavior if you call RemoveValue because you should only
> do this when this is the root registered variable (i.e. it's called by
> PrefService::ClearPref()) -- user code shouldn't use this directly.
> 
> If your registered variable is "long_int" and you have a "pref" value in the
> "long_int" dictionary, you should remove "pref" from "long_int" like this:
> 
> DictionaryPrefUpdate update("long_int");
> base::DictionaryValue* long_int_dict = update.Get();
> long_int_dict->Remove("pref");


Also, FTR, DictionaryPrefUpdate can forward calls directly to its underlying
DictionaryValue, so that you could also simply write:
DictionaryPrefUpdate update("long_int");
update->Remove("pref");


> 
> The DictionaryPrefUpdate takes care of the update notification for "long_int"
> and the Remove() call on the dictionary only removes that value (leaving the
> dictionary even if it ends up being empty).
> 
> The reason RemoveValue() needs to remove empty dictionaries left behind it is
> because if you clear say "foo.bar.baz" and the foo/bar dictionaries only exist
> to contain "baz", they should be cleared too.
> 
> Perhaps PrefStore::RemoveValue() should be renamed PrefStore::RemovePath() for
> consistency with the base::Value::RemovePath() API it's using?
> I'm not familiar enough with the API of all other PrefStores to see whether
that
> name makes sense in all scenarios.
> 
> Cheers!
> Gab

[Mattias Nissler (ping if slow), 2014-01-23 09:07:53]

Code comments inline, here's a high-level comment regarding your surprise about
the behavior:

JsonPrefStore uses a hierarchical data structure mostly for historical reasons.
The current PrefStore design assumes that all the keys (i.e. registered pref
names) belong to independent data items. The hierarchical nature of
JsonPrefStore means that we currently have the additional assumption that no
pref name can be a prefix of another pref name. This is not hard enforced though
(but should be arguably) and is violated by a few benign offenders (which should
be fixed).

Thus, when working on a new PrefStore implementation, you don't have to care
about hierarchy, a plain key-value store is good enough (assuming we fix the few
offenders first).

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

https://codereview.chromium.org/144633002/diff/1/base/prefs/json_pref_store_u...
base/prefs/json_pref_store_unittest.cc:271:
ASSERT_FALSE(pref_store->ReadOnly());
Reading this code, I have no idea what the test is supposed to test, can you
improve that?

This would be much clearer if you just constructed the input data
programmatically instead of separating it out into a file and reading it from
there. Another option could be to put a comment here that puts the reader on the
right track.

[dgrogan, 2014-01-29 02:50:46]

Thanks for the comments Gabriel and Mattias, they are helpful for understanding.
I've updated the test to construct the input data programmatically.

[Mattias Nissler (ping if slow), 2014-01-29 08:36:55]

LGTM

[commit-bot: I haz the power, 2014-01-30 01:18:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/144633002/100001

[commit-bot: I haz the power, 2014-01-30 09:22:28]

Change committed as 247915