Remove JsonPrefStore pruning of empty values on write.

Remove JsonPrefStore pruning of empty values on write.

Written from https://codereview.chromium.org/12092021

The entire source code was surveyed for existing list/dict prefs for which this change could be problematic, it doesn't look like anything is relying on this internal detail of the API, see this for details: https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AtwXJ4IPPZBAdG9rX3RTc3k5Z1pyN3U4b3d4Tkota3c#gid=0

TBR=joth (for minor android_webview side-effects)
BUG=323346
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237473

[gab, 2013-11-22 04:03:55]

Merged https://codereview.chromium.org/12092021 all the way to up to ToT.

Will try to wrap my head around all of this more tomorrow, but in the mean time
what do you think is left to do here?

[Mattias Nissler (ping if slow), 2013-11-22 08:03:38]

Looks good mostly, just a couple things to clean up.

Regarding what's left to be done: When I wrote the original change (mostly as a
POC), I didn't feel confident this doesn't break anything. I was meaning to
gather a list of all list and dict prefs that we have and go through them one by
one to check whether the code that handled them would behave equivalently now
that we store and return empty values instead of returning the default.

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

https://codereview.chromium.org/81183005/diff/1/base/prefs/json_pref_store_un...
base/prefs/json_pref_store_unittest.cc:222: // Test that the persistent value
can be loaded.
I think this comment is stale.

https://codereview.chromium.org/81183005/diff/1/base/prefs/json_pref_store_un...
base/prefs/json_pref_store_unittest.cc:224: new JsonPrefStore(pref_file,
message_loop_.message_loop_proxy());
Looks like there's a merge problem here, the original test set some empty values
in |pref_store|. In fact, I believe what's here now should fail in lines
232-235.

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

https://codereview.chromium.org/81183005/diff/1/base/prefs/pref_service.cc#ne...
base/prefs/pref_service.cc:348: //REMOVE?
Yes, as well as PrefService::AddInitialPreferences and
PrefRegistry::SetRegistrationCallback.

https://codereview.chromium.org/81183005/diff/1/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/81183005/diff/1/base/values.cc#newcode772
base/values.cc:772: Remove(subdict_path, NULL);
RemoveWithoutPathExpansion for clarity.

[gab, 2013-11-22 23:24:21]

Comments addressed, I've grepped the entire source for
Register(List|Dictionary)Pref calls and found 152 prefs needing to be surveyed.

I put them up at
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AtwXJ4IPPZBAdG9rX...
and will use that to survey every single one of them -- help is welcome :)!

To make sure I don't miss any that would be committed later I will scan the tree
for any addition of Register(List|Dictionary)Pref calls made after r236815
before/after this lands.

Thanks!
Gab

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

https://codereview.chromium.org/81183005/diff/1/base/prefs/json_pref_store_un...
base/prefs/json_pref_store_unittest.cc:222: // Test that the persistent value
can be loaded.
On 2013/11/22 08:03:38, Mattias Nissler wrote:
> I think this comment is stale.

Removed.

https://codereview.chromium.org/81183005/diff/1/base/prefs/json_pref_store_un...
base/prefs/json_pref_store_unittest.cc:224: new JsonPrefStore(pref_file,
message_loop_.message_loop_proxy());
On 2013/11/22 08:03:38, Mattias Nissler wrote:
> Looks like there's a merge problem here, the original test set some empty
values
> in |pref_store|. In fact, I believe what's here now should fail in lines
> 232-235.

Oops, merge problem indeed, fixed to be as in
https://codereview.chromium.org/12092021/diff/4001/base/prefs/json_pref_store...

I moved it up one block in this CL to have a clearer diff (the diff tool
otherwise tries to make a reasonable diff from the test being deleted to the new
test which aren't really the same at all...).

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

https://codereview.chromium.org/81183005/diff/1/base/prefs/pref_service.cc#ne...
base/prefs/pref_service.cc:348: //REMOVE?
On 2013/11/22 08:03:38, Mattias Nissler wrote:
> Yes, as well as PrefService::AddInitialPreferences and
> PrefRegistry::SetRegistrationCallback.

Hmmm interesting that I didn't get a PRESUBMIT check when uploading this... I
usually put comments with no space as notes to myself to be handled before
upload... somehow this made it up despite my intention of cleaning this up after
merging.

Thanks for clarifying what to remove, done.

https://codereview.chromium.org/81183005/diff/1/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/81183005/diff/1/base/values.cc#newcode772
base/values.cc:772: Remove(subdict_path, NULL);
On 2013/11/22 08:03:38, Mattias Nissler wrote:
> RemoveWithoutPathExpansion for clarity.

Done.

[gab, 2013-11-22 23:45:31]

On 2013/11/22 23:24:21, gab wrote:
> Comments addressed, I've grepped the entire source for
> Register(List|Dictionary)Pref calls and found 152 prefs needing to be
surveyed.
> 
> I put them up at
>
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AtwXJ4IPPZBAdG9rX...
> and will use that to survey every single one of them -- help is welcome :)!

Started a surveying bit to see how it feels to go through these and it got me
thinking, I think we also want the following:

1) DictionaryValue::Remove and DictionaryValue::RemovePath to be merged into a
single method.

2) Should ListValue::Remove(int index) clear the list or leave it empty if the
call makes the list empty? I'm guessing that all lists that manage individual
items in their lists have a default value of empty list anyways, so it probably
doesn't matter... but I was wondering what you thought?

Cheers,
Gab

> 
> To make sure I don't miss any that would be committed later I will scan the
tree
> for any addition of Register(List|Dictionary)Pref calls made after r236815
> before/after this lands.
> 
> Thanks!
> Gab

[Mattias Nissler (ping if slow), 2013-11-25 10:35:13]

Patch Set 3 LGTM pending as soon as we establish reasonable confidence we're not
breaking consumers (green tryjobs are a good start!).

On 2013/11/22 23:45:31, gab wrote:
> On 2013/11/22 23:24:21, gab wrote:
> > Comments addressed, I've grepped the entire source for
> > Register(List|Dictionary)Pref calls and found 152 prefs needing to be
> surveyed.
> > 
> > I put them up at
> >
>
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AtwXJ4IPPZBAdG9rX...
> > and will use that to survey every single one of them -- help is welcome :)!

Put a comment on the doc. 

> 
> Started a surveying bit to see how it feels to go through these and it got me
> thinking, I think we also want the following:
> 
> 1) DictionaryValue::Remove and DictionaryValue::RemovePath to be merged into a
> single method.

Potentially yes. That opens another can of worms regarding surveying code though
- let's cover this in a separate discussion and review.

> 
> 2) Should ListValue::Remove(int index) clear the list or leave it empty if the
> call makes the list empty? I'm guessing that all lists that manage individual
> items in their lists have a default value of empty list anyways, so it
probably
> doesn't matter... but I was wondering what you thought?

I don't follow - list.Remove(0) one a single-element list results in an empty
list.

> 
> Cheers,
> Gab
> 
> > 
> > To make sure I don't miss any that would be committed later I will scan the
> tree
> > for any addition of Register(List|Dictionary)Pref calls made after r236815
> > before/after this lands.
> > 
> > Thanks!
> > Gab

[gab, 2013-11-25 12:57:08]

On 2013/11/25 10:35:13, Mattias Nissler wrote:
> Patch Set 3 LGTM pending as soon as we establish reasonable confidence we're
not
> breaking consumers (green tryjobs are a good start!).
> 
> On 2013/11/22 23:45:31, gab wrote:
> > On 2013/11/22 23:24:21, gab wrote:
> > > Comments addressed, I've grepped the entire source for
> > > Register(List|Dictionary)Pref calls and found 152 prefs needing to be
> > surveyed.
> > > 
> > > I put them up at
> > >
> >
>
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AtwXJ4IPPZBAdG9rX...
> > > and will use that to survey every single one of them -- help is welcome
:)!
> 
> Put a comment on the doc. 
> 
> > 
> > Started a surveying bit to see how it feels to go through these and it got
me
> > thinking, I think we also want the following:
> > 
> > 1) DictionaryValue::Remove and DictionaryValue::RemovePath to be merged into
a
> > single method.
> 
> Potentially yes. That opens another can of worms regarding surveying code
though
> - let's cover this in a separate discussion and review.

I just don't see a case where we won't want to port all DictionaryValue::Remove
calls to DictionaryValue::RemovePath calls.

Effectively, DictionaryValue::Remove does the same thing as
DictionaryValue::RemovePath without cleaning up empty dictionaries resulting of
the removal.

If we agree that DictionaryValue::Remove should just become
DictionaryValue::RemovePath than that's easier to do than porting all of the
existing calls over to DictionaryValue::RemovePath.

> 
> > 
> > 2) Should ListValue::Remove(int index) clear the list or leave it empty if
the
> > call makes the list empty? I'm guessing that all lists that manage
individual
> > items in their lists have a default value of empty list anyways, so it
> probably
> > doesn't matter... but I was wondering what you thought?
> 
> I don't follow - list.Remove(0) one a single-element list results in an empty
> list.

Yes, but on write we currently prune empty lists out, now that we no longer
will, should a Remove(i) resulting in an empty list also get rid of the list
(i.e., to let the recommended/default value take over).

I think it probably makes sense to keep the empty list, but was curious to know
what you guys who have a deeper knowledge of the pref system thought?

> 
> > 
> > Cheers,
> > Gab
> > 
> > > 
> > > To make sure I don't miss any that would be committed later I will scan
the
> > tree
> > > for any addition of Register(List|Dictionary)Pref calls made after r236815
> > > before/after this lands.
> > > 
> > > Thanks!
> > > Gab

[Mattias Nissler (ping if slow), 2013-11-25 13:13:47]

On 2013/11/25 12:57:08, gab wrote:
> On 2013/11/25 10:35:13, Mattias Nissler wrote:
> > Patch Set 3 LGTM pending as soon as we establish reasonable confidence we're
> not
> > breaking consumers (green tryjobs are a good start!).
> > 
> > On 2013/11/22 23:45:31, gab wrote:
> > > On 2013/11/22 23:24:21, gab wrote:
> > > > Comments addressed, I've grepped the entire source for
> > > > Register(List|Dictionary)Pref calls and found 152 prefs needing to be
> > > surveyed.
> > > > 
> > > > I put them up at
> > > >
> > >
> >
>
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AtwXJ4IPPZBAdG9rX...
> > > > and will use that to survey every single one of them -- help is welcome
> :)!
> > 
> > Put a comment on the doc. 
> > 
> > > 
> > > Started a surveying bit to see how it feels to go through these and it got
> me
> > > thinking, I think we also want the following:
> > > 
> > > 1) DictionaryValue::Remove and DictionaryValue::RemovePath to be merged
into
> a
> > > single method.
> > 
> > Potentially yes. That opens another can of worms regarding surveying code
> though
> > - let's cover this in a separate discussion and review.
> 
> I just don't see a case where we won't want to port all
DictionaryValue::Remove
> calls to DictionaryValue::RemovePath calls.

Maybe that's the case, maybe not. We'd have to check all calling code ;)

> 
> Effectively, DictionaryValue::Remove does the same thing as
> DictionaryValue::RemovePath without cleaning up empty dictionaries resulting
of
> the removal.

Which some callers may want to keep intentionally, you never know.

> 
> If we agree that DictionaryValue::Remove should just become
> DictionaryValue::RemovePath than that's easier to do than porting all of the
> existing calls over to DictionaryValue::RemovePath.

Right, but we can still make that switch easily in a separate step that changes
Remove() to do what RemovePath() now does and switching the few RemovePath()
calls over to Remove().

I must admit that I'm a bit hesitant to do so due to the concern that Remove()
now is intuitive (it does exactly what you'd expect), whereas RemovePath()
performs some non-obvious extra work, which the more complex name nicely
reflects :)

> 
> > 
> > > 
> > > 2) Should ListValue::Remove(int index) clear the list or leave it empty if
> the
> > > call makes the list empty? I'm guessing that all lists that manage
> individual
> > > items in their lists have a default value of empty list anyways, so it
> > probably
> > > doesn't matter... but I was wondering what you thought?
> > 
> > I don't follow - list.Remove(0) one a single-element list results in an
empty
> > list.
> 
> Yes, but on write we currently prune empty lists out, now that we no longer
> will, should a Remove(i) resulting in an empty list also get rid of the list
> (i.e., to let the recommended/default value take over).

Are you talking about lists that are members of dictionaries and nested lists?
Or are you talking about ListValue-typed preferences? If I have a single
ListValue object and I do a Remove(0), there's no way to prune that list - you
can't just destroy it while the caller owns it.

> 
> I think it probably makes sense to keep the empty list, but was curious to
know
> what you guys who have a deeper knowledge of the pref system thought?

As mentioned elsewhere in this discussion, I think empty list pruning is an
artifact of the desire to keep cruft from accumulating in the Preferences file.
On the PrefService API level, I think we should use the principle of least
surprise guide ourselves, and therefore behave consistent with other types, i.e.
leave trivial values in place (after all, we also store boolean false and
integer 0 explicitly).

> 
> > 
> > > 
> > > Cheers,
> > > Gab
> > > 
> > > > 
> > > > To make sure I don't miss any that would be committed later I will scan
> the
> > > tree
> > > > for any addition of Register(List|Dictionary)Pref calls made after
r236815
> > > > before/after this lands.
> > > > 
> > > > Thanks!
> > > > Gab

[gab, 2013-11-25 13:24:18]

On 2013/11/25 13:13:47, Mattias Nissler wrote:
> On 2013/11/25 12:57:08, gab wrote:
> > On 2013/11/25 10:35:13, Mattias Nissler wrote:
> > > Patch Set 3 LGTM pending as soon as we establish reasonable confidence
we're
> > not
> > > breaking consumers (green tryjobs are a good start!).
> > > 
> > > On 2013/11/22 23:45:31, gab wrote:
> > > > On 2013/11/22 23:24:21, gab wrote:
> > > > > Comments addressed, I've grepped the entire source for
> > > > > Register(List|Dictionary)Pref calls and found 152 prefs needing to be
> > > > surveyed.
> > > > > 
> > > > > I put them up at
> > > > >
> > > >
> > >
> >
>
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AtwXJ4IPPZBAdG9rX...
> > > > > and will use that to survey every single one of them -- help is
welcome
> > :)!
> > > 
> > > Put a comment on the doc. 
> > > 
> > > > 
> > > > Started a surveying bit to see how it feels to go through these and it
got
> > me
> > > > thinking, I think we also want the following:
> > > > 
> > > > 1) DictionaryValue::Remove and DictionaryValue::RemovePath to be merged
> into
> > a
> > > > single method.
> > > 
> > > Potentially yes. That opens another can of worms regarding surveying code
> > though
> > > - let's cover this in a separate discussion and review.
> > 
> > I just don't see a case where we won't want to port all
> DictionaryValue::Remove
> > calls to DictionaryValue::RemovePath calls.
> 
> Maybe that's the case, maybe not. We'd have to check all calling code ;)
> 
> > 
> > Effectively, DictionaryValue::Remove does the same thing as
> > DictionaryValue::RemovePath without cleaning up empty dictionaries resulting
> of
> > the removal.
> 
> Which some callers may want to keep intentionally, you never know.
> 
> > 
> > If we agree that DictionaryValue::Remove should just become
> > DictionaryValue::RemovePath than that's easier to do than porting all of the
> > existing calls over to DictionaryValue::RemovePath.
> 
> Right, but we can still make that switch easily in a separate step that
changes
> Remove() to do what RemovePath() now does and switching the few RemovePath()
> calls over to Remove().
> 
> I must admit that I'm a bit hesitant to do so due to the concern that Remove()
> now is intuitive (it does exactly what you'd expect), whereas RemovePath()
> performs some non-obvious extra work, which the more complex name nicely
> reflects :)


Sure, I'll keep an eye out for such cases while doing the survey.

I think it's fine as an intermediary step to keep Remove() and not switch all
callers to RemovePath(), the worst case is only that it will leave a few empty
dictionaries around.

I understand the naming argument; in which case perhaps we should just get rid
of Remove() and move everybody over to RemovePath() -- if the survey proves this
to be the right move.



> 
> > 
> > > 
> > > > 
> > > > 2) Should ListValue::Remove(int index) clear the list or leave it empty
if
> > the
> > > > call makes the list empty? I'm guessing that all lists that manage
> > individual
> > > > items in their lists have a default value of empty list anyways, so it
> > > probably
> > > > doesn't matter... but I was wondering what you thought?
> > > 
> > > I don't follow - list.Remove(0) one a single-element list results in an
> empty
> > > list.
> > 
> > Yes, but on write we currently prune empty lists out, now that we no longer
> > will, should a Remove(i) resulting in an empty list also get rid of the list
> > (i.e., to let the recommended/default value take over).
> 
> Are you talking about lists that are members of dictionaries and nested lists?
> Or are you talking about ListValue-typed preferences? If I have a single
> ListValue object and I do a Remove(0), there's no way to prune that list - you
> can't just destroy it while the caller owns it.
> 
> > 
> > I think it probably makes sense to keep the empty list, but was curious to
> know
> > what you guys who have a deeper knowledge of the pref system thought?
> 
> As mentioned elsewhere in this discussion, I think empty list pruning is an
> artifact of the desire to keep cruft from accumulating in the Preferences
file.
> On the PrefService API level, I think we should use the principle of least
> surprise guide ourselves, and therefore behave consistent with other types,
i.e.
> leave trivial values in place (after all, we also store boolean false and
> integer 0 explicitly).

Perfect, this was my intuition too, but just wanted to confirm with you (and you
bring up a good point about ownership which strengthens that no pruning should
occur as a result of ListValue::Remove(i)).

Thanks,
Gab

> 
> > 
> > > 
> > > > 
> > > > Cheers,
> > > > Gab
> > > > 
> > > > > 
> > > > > To make sure I don't miss any that would be committed later I will
scan
> > the
> > > > tree
> > > > > for any addition of Register(List|Dictionary)Pref calls made after
> r236815
> > > > > before/after this lands.
> > > > > 
> > > > > Thanks!
> > > > > Gab

[gab, 2013-11-25 22:45:25]

Okay, so I just went over a bunch of them today (less than half to go) @
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AtwXJ4IPPZBAdG9rX...

Everything looks good, a few minor issues (and ideas I have to fix them):

1) DictionaryValue::Remove/Clear/RemoveWithoutPathExpansion can't cleanup if the
call makes their underlying dictionary empty (ownership problem).

Solution: All of those operations are typically made inside a
DictionaryPrefUpdate which itself could cleanup.

2) PrefService::ClearPref() used to do a simple prefs_->Remove() in the
user_prefs_store_ and the pruning would happen on write; it now needs to clean
further.

Solution: You had already fixed that one by changing
JsonPrefStore::RemoveValue() to use RemovePath().

3) Users of DictionaryValue::GetDictionaryWithoutPathExpansion() should always
check the return value for success (as the dictionary could have gone away if
emptied by another call; a few call sites could make assumptions about the way
they handle things, but it just feels safer to always check).

Solution: Add WARN_UNUSED_RESULT to this method's declaration.

4) There a few HasPrefPath() calls; none of which are very alarming, but keeping
the behavior consistent as far as trying our best to prune out empty
dictionaries would be nice to avoid inconsistent behavior here.

Solution: Implement solutions to #1/2.

5) Open-ended question: When pruning empty dicts on Remove, etc.: should
observers of paths being pruned out also be notified or are we happy only
notifying observers of the item being removed at the top of the hierarchy?

That's it for now!

Cheers,
Gab

[Mattias Nissler (ping if slow), 2013-11-26 09:07:14]

On 2013/11/25 22:45:25, gab wrote:
> Okay, so I just went over a bunch of them today (less than half to go) @
>
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AtwXJ4IPPZBAdG9rX...

Thanks for taking on that work!

> 
> Everything looks good, a few minor issues (and ideas I have to fix them):
> 
> 1) DictionaryValue::Remove/Clear/RemoveWithoutPathExpansion can't cleanup if
the
> call makes their underlying dictionary empty (ownership problem).
> 
> Solution: All of those operations are typically made inside a
> DictionaryPrefUpdate which itself could cleanup.

I think as long as the empty lists/dictionaries are within a subtree of the JSON
that's below a registered pref path, we're actually good. It's up to the code
wrangling that pref to keep its dictionary/list in reasonable shape. Have you
encountered this case when checking existing code where code assumed the
implicit pruning would happen? I'd expect that this is very rare and code that
needs to have pruning can easily do it for nested dictionaries/lists or call
PrefService::ClearPref() if required. So I'm not convinced we actually need to
do something about this.

> 
> 2) PrefService::ClearPref() used to do a simple prefs_->Remove() in the
> user_prefs_store_ and the pruning would happen on write; it now needs to clean
> further.
> 
> Solution: You had already fixed that one by changing
> JsonPrefStore::RemoveValue() to use RemovePath().
> 
> 3) Users of DictionaryValue::GetDictionaryWithoutPathExpansion() should always
> check the return value for success (as the dictionary could have gone away if
> emptied by another call; a few call sites could make assumptions about the way
> they handle things, but it just feels safer to always check).
> 
> Solution: Add WARN_UNUSED_RESULT to this method's declaration.

This is definitely a good idea. Whether it'll fly will depend on how many legal
cases of not checking we'd complicate by doing this, I can't tell at this point
given that DictionaryValue is used across the entire code base and for non-pref
things. In any case, this change is orthogonal and doesn't block this CL.

> 
> 4) There a few HasPrefPath() calls; none of which are very alarming, but
keeping
> the behavior consistent as far as trying our best to prune out empty
> dictionaries would be nice to avoid inconsistent behavior here.
> 
> Solution: Implement solutions to #1/2.

As explained above, I think it's fair to not prune within registered prefs, i.e.
having HasPrefPath() return true unless you call ClearPref() explicitly. In the
bug you filed you mention that the current code already generates inconsistent
results before and after a store/reload cycle on browser restart, so current
code can probably handle this situation just fine.

> 
> 5) Open-ended question: When pruning empty dicts on Remove, etc.: should
> observers of paths being pruned out also be notified or are we happy only
> notifying observers of the item being removed at the top of the hierarchy?

Note that the pref system is not prepared to cope with overlapping prefs, i.e.
prefs where one pref key is a prefix of another. Granted, there are a few benign
offenders that should be cleaned up, but you can safely assume that the
modification of a pref value only affects a single registered pref key.

In your survey, did you identify any cases that would fail in one of the ways
you describe above? If so, let's discuss them individually.

> 
> That's it for now!
> 
> Cheers,
> Gab

[gab, 2013-11-26 15:48:21]

Thanks for the replies, makes sense.

So our goal is that on ClearPref we do not leave dangling (non-registered and
empty) dictionaries below the cleared pref.

But we don't care about leaving empty dictionaries which are registered and
weren't explicitly removed (in fact the more I think about: it's nice to leave
them there as code that wants to know could differentiate between having
initialized something to an empty dict and never having initialized).

I don't remember seeing any cases which relied on the implicit pruning getting
rid of the empty dictionary on write (which would not be reflected in in-memory
state until the next startup).

Good point regarding observers.

I'll keep digging today, but so far this looks good as-is.

I'll also investigate how many things WARN_UNUSED_RESULT would break and whether
there are legitimate use cases in there.

Cheers!
Gab

[gab, 2013-11-26 20:28:40]

On 2013/11/26 15:48:21, gab wrote:
> Thanks for the replies, makes sense.
> 
> So our goal is that on ClearPref we do not leave dangling (non-registered and
> empty) dictionaries below the cleared pref.
> 
> But we don't care about leaving empty dictionaries which are registered and
> weren't explicitly removed (in fact the more I think about: it's nice to leave
> them there as code that wants to know could differentiate between having
> initialized something to an empty dict and never having initialized).
> 
> I don't remember seeing any cases which relied on the implicit pruning getting
> rid of the empty dictionary on write (which would not be reflected in
in-memory
> state until the next startup).
> 
> Good point regarding observers.
> 
> I'll keep digging today, but so far this looks good as-is.

Nothing particularly worrisome for users of ListValue :).

> 
> I'll also investigate how many things WARN_UNUSED_RESULT would break and
whether
> there are legitimate use cases in there.

This appears to be tricky https://codereview.chromium.org/88703002/, but as you
said I don't think it's a prereq for this CL.

I think we're good to go, please CQ if you agree :)!

Cheers!
Gab

[Mattias Nissler (ping if slow), 2013-11-26 20:46:33]

LGTM, ticking the box.

[commit-bot: I haz the power, 2013-11-26 20:47:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/81183005/570001

[commit-bot: I haz the power, 2013-11-26 21:02:41]

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

[gab, 2013-11-26 22:23:51]

+Nico for base/ rubbertstamp (FYI base/prefs/ was already stamped by Mattias who
owns prefs).

TBR joth for android_webview side-effects

+joi(CC): FYI fixed your TODO to remove PrefService::AddRegisteredPreference :)!

Thanks!
Gab

[Nico, 2013-11-26 22:56:10]

lgtm with comment

https://codereview.chromium.org/81183005/diff/590001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/81183005/diff/590001/base/values.cc#newcode764
base/values.cc:764: } else {
Remove else, dedent body

[gab, 2013-11-26 22:59:48]

Thanks, done!

https://codereview.chromium.org/81183005/diff/590001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/81183005/diff/590001/base/values.cc#newcode764
base/values.cc:764: } else {
On 2013/11/26 22:56:10, Nico wrote:
> Remove else, dedent body

Done.

[commit-bot: I haz the power, 2013-11-26 23:02:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/81183005/610001

[joth, 2013-11-26 23:19:59]

On 2013/11/26 22:23:51, gab wrote:
> +Nico for base/ rubbertstamp (FYI base/prefs/ was already stamped by Mattias
who
> owns prefs).
> 
> TBR joth for android_webview side-effects
> 
lgtm

> +joi(CC): FYI fixed your TODO to remove PrefService::AddRegisteredPreference
:)!
> 
> Thanks!
> Gab

[commit-bot: I haz the power, 2013-11-27 01:38:28]

Change committed as 237473