Create a DefaultPrefStore to hold registered application-default preference v...

Create a DefaultPrefStore to hold registered application-default preference values.

Also rework preference notifications so they're not sent when a pref store rewrites the same value it already had,
to avoid infinite recursion in places that set a pref in response to a notification that that pref has changed.

This allows notifications to be sent properly when another PrefStore takes control from a default value.

BUG=52719, 54950
TEST=covered by unit tests (PrefValueStoreTest.*, PrefNotifierTest.*, and ExtensionPrefStoreTest.*)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=59345

[Pam (message me for reviews), 2010-09-10 10:22:27]

Here's an expansion of the previous patch, including removing old_value from
PrefHasChanged() and making that logic more accurate. Thanks for the review.

- Pam

[Mattias Nissler (ping if slow), 2010-09-10 11:49:41]

I've put some comments that suggest to move the whole change detection and
notification business into pref_value_store. I'm not sure this is really
possible, I'm happy to talk about this more if you think that's a good idea and
want to discuss.

http://codereview.chromium.org/3323022/diff/17001/18002
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/3323022/diff/17001/18002#newcode105
chrome/browser/extensions/extension_pref_store.cc:105:
my_old_value.reset(my_old_value_ptr->DeepCopy());
If I read the scoped_ptr header correctly, you replace this block with the
shorter and maybe more understandable:

scoped_ptr<Value> my_old_value;
prefs_->Get(path, &*my_old_value);

Taking the address of the reference is a bit awkward, your call.

http://codereview.chromium.org/3323022/diff/17001/18006
File chrome/browser/prefs/default_pref_store.h (right):

http://codereview.chromium.org/3323022/diff/17001/18006#newcode12
chrome/browser/prefs/default_pref_store.h:12: 
Remove one of the two line breaks?

http://codereview.chromium.org/3323022/diff/17001/18007
File chrome/browser/prefs/pref_notifier.cc (right):

http://codereview.chromium.org/3323022/diff/17001/18007#newcode45
chrome/browser/prefs/pref_notifier.cc:45: }
Any reason for adding the braces?

http://codereview.chromium.org/3323022/diff/17001/18008
File chrome/browser/prefs/pref_notifier.h (right):

http://codereview.chromium.org/3323022/diff/17001/18008#newcode62
chrome/browser/prefs/pref_notifier.h:62: // before).
Why do we need value_changed then? Doesn't that mean the value as observed by
client code doesn't change? So can we remove the third parameter and not call
OnPreferenceSet if the value didn't change?

http://codereview.chromium.org/3323022/diff/17001/18010
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/3323022/diff/17001/18010#newcode406
chrome/browser/prefs/pref_service.cc:406:
pref_notifier_->OnUserPreferenceSet(path, had_old_value);
Shouldn't the pref_value_store_ be responsible for generating this notification?
This would likely also save the second lookup, since RemoveUserPrefValue
probably has all need information.

http://codereview.chromium.org/3323022/diff/17001/18010#newcode441
chrome/browser/prefs/pref_service.cc:441:
pref_notifier_->OnUserPreferenceSet(path, value_changed);
Same comment as above.

http://codereview.chromium.org/3323022/diff/17001/18010#newcode557
chrome/browser/prefs/pref_service.cc:557: void
PrefService::SetUserPrefValue(const char* path, Value* new_value) {
Consolidating the setters is a really good thing to do!

http://codereview.chromium.org/3323022/diff/17001/18010#newcode565
chrome/browser/prefs/pref_service.cc:565: if (pref->IsManaged()) {
We talked about removing this check a few times, maybe now it's time to do it?

http://codereview.chromium.org/3323022/diff/17001/18010#newcode580
chrome/browser/prefs/pref_service.cc:580:
pref_notifier_->OnUserPreferenceSet(path, value_changed);
Again, wouldn't it be easier to handle the change detection in PrefValueStore?

http://codereview.chromium.org/3323022/diff/17001/18014
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/3323022/diff/17001/18014#newcode79
chrome/browser/prefs/pref_value_store.h:79: // the pref has changed. Virtual so
it can be mocked for a unit test.
It's not virtual?

[Pam (message me for reviews), 2010-09-12 14:03:08]

Requested changes made (except as noted below). Please take another look.

- Pam

http://codereview.chromium.org/3323022/diff/17001/18002
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/3323022/diff/17001/18002#newcode105
chrome/browser/extensions/extension_pref_store.cc:105:
my_old_value.reset(my_old_value_ptr->DeepCopy());
On 2010/09/10 11:49:41, Mattias Nissler wrote:
> If I read the scoped_ptr header correctly, you replace this block with the
> shorter and maybe more understandable:
> 
> scoped_ptr<Value> my_old_value;
> prefs_->Get(path, &*my_old_value);
> 
> Taking the address of the reference is a bit awkward, your call.

I agree, it's ugly. But *my_old_value is a Value, not a Value*. We need the
equivalent of &(my_old_value.get()), but my_old_value.get() by itself isn't an
l-value. I can't find a simpler way to accomplish it.

http://codereview.chromium.org/3323022/diff/17001/18007
File chrome/browser/prefs/pref_notifier.cc (right):

http://codereview.chromium.org/3323022/diff/17001/18007#newcode45
chrome/browser/prefs/pref_notifier.cc:45: }
On 2010/09/10 11:49:41, Mattias Nissler wrote:
> Any reason for adding the braces?

At one point I used a longer argument name that extended onto a second line, in
which case it was easier to read with braces.

Well, you asked. :)

http://codereview.chromium.org/3323022/diff/17001/18010
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/3323022/diff/17001/18010#newcode565
chrome/browser/prefs/pref_service.cc:565: if (pref->IsManaged()) {
On 2010/09/10 11:49:41, Mattias Nissler wrote:
> We talked about removing this check a few times, maybe now it's time to do it?

I'd like to leave that to another patch, since it's conceptually a different
change.

[Mattias Nissler (ping if slow), 2010-09-13 12:17:47]

I like the value_changed removal. It feels like we're now getting much closer to
the point where we can actually stop refactoring prefs :)

http://codereview.chromium.org/3323022/diff/17001/18002
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/3323022/diff/17001/18002#newcode105
chrome/browser/extensions/extension_pref_store.cc:105:
my_old_value.reset(my_old_value_ptr->DeepCopy());
On 2010/09/12 14:03:08, Pam wrote:
> On 2010/09/10 11:49:41, Mattias Nissler wrote:
> > If I read the scoped_ptr header correctly, you replace this block with the
> > shorter and maybe more understandable:
> > 
> > scoped_ptr<Value> my_old_value;
> > prefs_->Get(path, &*my_old_value);
> > 
> > Taking the address of the reference is a bit awkward, your call.
> 
> I agree, it's ugly. But *my_old_value is a Value, not a Value*. We need the
> equivalent of &(my_old_value.get()), but my_old_value.get() by itself isn't an
> l-value. I can't find a simpler way to accomplish it.

Right, I don't see a better way either.

http://codereview.chromium.org/3323022/diff/17001/18010
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/3323022/diff/17001/18010#newcode565
chrome/browser/prefs/pref_service.cc:565: if (pref->IsManaged()) {
On 2010/09/12 14:03:08, Pam wrote:
> On 2010/09/10 11:49:41, Mattias Nissler wrote:
> > We talked about removing this check a few times, maybe now it's time to do
it?
> 
> I'd like to leave that to another patch, since it's conceptually a different
> change.

OK.

[Mattias Nissler (ping if slow), 2010-09-13 12:17:59]

Oh, and LGTM :)

[Nico, 2010-09-15 17:20:53]

This probably caused http://code.google.com/p/chromium/issues/detail?id=55720

[jeanluc1, 2010-09-21 04:16:39]

http://codereview.chromium.org/3323022/diff/36002/42013
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/3323022/diff/36002/42013#newcode96
chrome/browser/prefs/pref_value_store.cc:96: return rv &&
!PrefValueFromDefaultStore(path);
This change modifies the behavior of
TemplateURLModel::LoadDefaultSearchProviderFromPrefs.  I'll fix that in another
CL.

[jeanluc1, 2010-09-21 04:27:11]

Follow up to the previous message...

For the current implementation of
TemplateURLModel::LoadDefaultSearchProviderFromPrefs, there's a difference
between not finding prefs::kDefaultSearchProviderSearchURL and finding it but
with the default value of blank.  For the former, it means that no default
search have been set and we rely on built-in defaults.  For the latter, it means
that the user has chosen to not have any defaults.

This CL breaks this.

For a different reason, the CL I'm working on adds a boolean
prefs::kDefaultSearchProviderEnabled.  That flag is needed because that's the
way the Windows policy editor can disable a policy.  So I can change
LoadDefaultSearchProviderFromPrefs.  My question is... how can we transition
existing preferences to the new scheme, i.e. if the path is there but empty, set
that boolean to false?
 having the pref of path  and havin


 would distingus

The

Umm, this problem might be a

[Pam (message me for reviews), 2010-09-21 07:25:16]

Previously, PrefValueStore::GetValue() ignored the default value and returned
false if there was no "real" value set. This CL kept the same behavior.

Looking at TemplateURLModel::LoadDefaultSearchProviderFromPrefs, it's using
these APIs in a nonstandard way (the PrefService really wants every preference
to be registered), but it ought to still work. If the pref isn't registered,
HasPrefPath will still return false; if it is registered, it should return the
value set, or the default value of an empty string if nothing is set.

What's it doing instead?

About your second question, we don't currently have any established mechanism
for migrating prefs. As far as I know, any that have needed it have done their
own migration manually at the point of use. 

Rather than continuing discussion here, please file a bug report (or two) so we
can track the problem and its fix.

- Pam

On 2010/09/21 04:27:11, jeanluc1 wrote:
> Follow up to the previous message...
> 
> For the current implementation of
> TemplateURLModel::LoadDefaultSearchProviderFromPrefs, there's a difference
> between not finding prefs::kDefaultSearchProviderSearchURL and finding it but
> with the default value of blank.  For the former, it means that no default
> search have been set and we rely on built-in defaults.  For the latter, it
means
> that the user has chosen to not have any defaults.
> 
> This CL breaks this.
> 
> For a different reason, the CL I'm working on adds a boolean
> prefs::kDefaultSearchProviderEnabled.  That flag is needed because that's the
> way the Windows policy editor can disable a policy.  So I can change
> LoadDefaultSearchProviderFromPrefs.  My question is... how can we transition
> existing preferences to the new scheme, i.e. if the path is there but empty,
set
> that boolean to false?
>  having the pref of path  and havin
> 
> 
>  would distingus
> 
> The
> 
> Umm, this problem might be a

[jeanluc, 2010-09-21 07:42:59]

Never mind, I misunderstood the comment in HasPrefPath.  I'll look into
again in the morning and file a bug if there's still an issue.

Jean-Luc

On Tue, Sep 21, 2010 at 12:25 AM, <pam@chromium.org> wrote:

> Previously, PrefValueStore::GetValue() ignored the default value and
> returned
> false if there was no "real" value set. This CL kept the same behavior.
>
> Looking at TemplateURLModel::LoadDefaultSearchProviderFromPrefs, it's using
> these APIs in a nonstandard way (the PrefService really wants every
> preference
> to be registered), but it ought to still work. If the pref isn't
> registered,
> HasPrefPath will still return false; if it is registered, it should return
> the
> value set, or the default value of an empty string if nothing is set.
>
> What's it doing instead?
>
> About your second question, we don't currently have any established
> mechanism
> for migrating prefs. As far as I know, any that have needed it have done
> their
> own migration manually at the point of use.
>
> Rather than continuing discussion here, please file a bug report (or two)
> so we
> can track the problem and its fix.
>
> - Pam
>
>
> On 2010/09/21 04:27:11, jeanluc1 wrote:
>
>> Follow up to the previous message...
>>
>
>  For the current implementation of
>> TemplateURLModel::LoadDefaultSearchProviderFromPrefs, there's a difference
>> between not finding prefs::kDefaultSearchProviderSearchURL and finding it
>> but
>> with the default value of blank.  For the former, it means that no default
>> search have been set and we rely on built-in defaults.  For the latter, it
>>
> means
>
>> that the user has chosen to not have any defaults.
>>
>
>  This CL breaks this.
>>
>
>  For a different reason, the CL I'm working on adds a boolean
>> prefs::kDefaultSearchProviderEnabled.  That flag is needed because that's
>> the
>> way the Windows policy editor can disable a policy.  So I can change
>> LoadDefaultSearchProviderFromPrefs.  My question is... how can we
>> transition
>> existing preferences to the new scheme, i.e. if the path is there but
>> empty,
>>
> set
>
>> that boolean to false?
>>  having the pref of path  and havin
>>
>
>
>   would distingus
>>
>
>  The
>>
>
>  Umm, this problem might be a
>>
>
>
>
> http://codereview.chromium.org/3323022/show
>