Add ability to change default pref values, and use in BrowserInstantController.

Fix prefs registration in BrowserInstantController.

Registration should happen prior to PrefService construction, so accepting a PrefService parameter to RegisterUserPrefs is a no-no.

To keep the existing semantics, this change adds a PrefService::SetDefaultPrefValue method that allows you to change the default value for a pref.

As BrowserInstantController was the last method called from chrome::RegisterUserPrefs that required a PrefService pointer, this also removes the PrefService parameter from the latter function and updates callers.

TBR=ben@chromium.org
BUG=155525
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185712

[Jói, 2013-02-26 22:11:49]

Mattias,

Let me know what you think. An alternative would be to leave the public
interface of PrefRegistry unchanged, and instead have a private GetDefaultStore
method and make PrefService a friend.

I was ambivalent because I figured we were already allowing begin() and end()
via PrefRegistry, and by adding SetDefaultPrefValue the only method we would be
hiding by taking the friend approach would be
DefaultPrefStore::RemoveDefaultValue. On the other hand,
PrefService::SetDefaultPrefValue has an extra check that
DefaultPrefStore::SetDefaultValue doesn't (of checking whether the pref is
already registered) so perhaps it's worth it.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2013-02-27 10:50:52]

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

https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_registry.h#ne...
base/prefs/pref_registry.h:36: scoped_refptr<DefaultPrefStore> defaults();
I think we should rather not expose the DefaultPrefStore type here, but just add
a function to PrefRegistry:

void SetDefaultPrefValue(const char* pref_name, const Value* value);

That keeps the registration functions off limits, but still allows defaults to
be changed.

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

https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_service.cc#ne...
base/prefs/pref_service.cc:281:
pref_registry_->defaults()->SetDefaultValue(path, value);
We should CHECK() that the type doesn't change here, and make sure that |value|
!= NULL

https://codereview.chromium.org/12315116/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12315116/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:101: // BrowserInstantController
constructor in this case.
Maybe we can move all default setting to the ctor for simplicity now?

[Jói, 2013-02-27 11:06:59]

> I think we should rather not expose the DefaultPrefStore type here, but
> just add a function to PrefRegistry:
>
> void SetDefaultPrefValue(const char* pref_name, const Value* value);

That seems fine.  However, note in the long term if we manage to do
all registrations up front everywhere (including tests) then keeping
the PrefRegistry object around after PrefService construction time
will become unnecessary.  I guess we can switch to something different
if and when that happens.

Cheers,
Jói


On Wed, Feb 27, 2013 at 10:50 AM,  <mnissler@chromium.org> wrote:
>
> https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_registry.h
> File base/prefs/pref_registry.h (right):
>
>
https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_registry.h#ne...
> base/prefs/pref_registry.h:36: scoped_refptr<DefaultPrefStore>
> defaults();
> I think we should rather not expose the DefaultPrefStore type here, but
> just add a function to PrefRegistry:
>
> void SetDefaultPrefValue(const char* pref_name, const Value* value);
>
> That keeps the registration functions off limits, but still allows
> defaults to be changed.
>
> https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_service.cc
> File base/prefs/pref_service.cc (right):
>
>
https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_service.cc#ne...
> base/prefs/pref_service.cc:281:
> pref_registry_->defaults()->SetDefaultValue(path, value);
> We should CHECK() that the type doesn't change here, and make sure that
> |value| != NULL
>
>
https://codereview.chromium.org/12315116/diff/1/chrome/browser/ui/browser_ins...
> File chrome/browser/ui/browser_instant_controller.cc (right):
>
>
https://codereview.chromium.org/12315116/diff/1/chrome/browser/ui/browser_ins...
> chrome/browser/ui/browser_instant_controller.cc:101: //
>
> BrowserInstantController constructor in this case.
> Maybe we can move all default setting to the ctor for simplicity now?
>
> https://codereview.chromium.org/12315116/

[Mattias Nissler (ping if slow), 2013-02-27 12:30:29]

On Wed, Feb 27, 2013 at 12:06 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> > I think we should rather not expose the DefaultPrefStore type here, but
> > just add a function to PrefRegistry:
> >
> > void SetDefaultPrefValue(const char* pref_name, const Value* value);
>
> That seems fine.  However, note in the long term if we manage to do
> all registrations up front everywhere (including tests) then keeping
> the PrefRegistry object around after PrefService construction time
> will become unnecessary.  I guess we can switch to something different
> if and when that happens.
>

True. On the other hand, keeping the PrefRegistry object doesn't hurt much
though, it's only a couple bytes.


>
> Cheers,
> Jói
>
>
> On Wed, Feb 27, 2013 at 10:50 AM,  <mnissler@chromium.org> wrote:
> >
> >
> https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_registry.h
> > File base/prefs/pref_registry.h (right):
> >
> >
>
https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_registry.h#ne...
> > base/prefs/pref_registry.h:36: scoped_refptr<DefaultPrefStore>
> > defaults();
> > I think we should rather not expose the DefaultPrefStore type here, but
> > just add a function to PrefRegistry:
> >
> > void SetDefaultPrefValue(const char* pref_name, const Value* value);
> >
> > That keeps the registration functions off limits, but still allows
> > defaults to be changed.
> >
> >
> https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_service.cc
> > File base/prefs/pref_service.cc (right):
> >
> >
>
https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_service.cc#ne...
> > base/prefs/pref_service.cc:281:
> > pref_registry_->defaults()->SetDefaultValue(path, value);
> > We should CHECK() that the type doesn't change here, and make sure that
> > |value| != NULL
> >
> >
>
https://codereview.chromium.org/12315116/diff/1/chrome/browser/ui/browser_ins...
> > File chrome/browser/ui/browser_instant_controller.cc (right):
> >
> >
>
https://codereview.chromium.org/12315116/diff/1/chrome/browser/ui/browser_ins...
> > chrome/browser/ui/browser_instant_controller.cc:101: //
> >
> > BrowserInstantController constructor in this case.
> > Maybe we can move all default setting to the ctor for simplicity now?
> >
> > https://codereview.chromium.org/12315116/
>

[Jói, 2013-02-27 16:30:56]

Addressed review comments, and added a bit of documentation on kInstantEnabled
vs. kInstantExtendedEnabled in BrowserInstantController.

Sreeram, perhaps you could look at the added documentation in
browser_instant_controller.cc?  Mattias, the rest is for you.

Cheers,
Jói

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

https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_registry.h#ne...
base/prefs/pref_registry.h:36: scoped_refptr<DefaultPrefStore> defaults();
On 2013/02/27 10:50:52, Mattias Nissler wrote:
> I think we should rather not expose the DefaultPrefStore type here, but just
add
> a function to PrefRegistry:
> 
> void SetDefaultPrefValue(const char* pref_name, const Value* value);
> 
> That keeps the registration functions off limits, but still allows defaults to
> be changed.

Done.

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

https://codereview.chromium.org/12315116/diff/1/base/prefs/pref_service.cc#ne...
base/prefs/pref_service.cc:281:
pref_registry_->defaults()->SetDefaultValue(path, value);
On 2013/02/27 10:50:52, Mattias Nissler wrote:
> We should CHECK() that the type doesn't change here, and make sure that
|value|
> != NULL

Done.

https://codereview.chromium.org/12315116/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12315116/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:101: // BrowserInstantController
constructor in this case.
On 2013/02/27 10:50:52, Mattias Nissler wrote:
> Maybe we can move all default setting to the ctor for simplicity now?

Done.

[sreeram, 2013-02-27 16:35:27]

browser_instant_controller.* lgtm

[Jói, 2013-02-28 14:46:40]

mnissler: Gentle ping.

[Mattias Nissler (ping if slow), 2013-02-28 15:46:25]

LGTM with a nit.

Sorry for the delay, my Inbox grew faster the last 24 hours than I could process
it :-/

https://codereview.chromium.org/12315116/diff/12001/base/prefs/pref_service.h
File base/prefs/pref_service.h (right):

https://codereview.chromium.org/12315116/diff/12001/base/prefs/pref_service.h...
base/prefs/pref_service.h:223: // Changes the default value for a preference.
Takes ownership of |value|.
nit: add a line saying that this will fire pref change notifications if the
effective value changed.

[Jói, 2013-02-28 16:14:11]

Thanks Mattias, and no worries on the delay - it was just I thought this
one might have slipped through because I sent you a different change at
around the same time as I updated this change yesterday.

I will update the documentation as you suggested before I land.

Cheers,
Jói
 On Feb 28, 2013 3:46 PM, <mnissler@chromium.org> wrote:

> LGTM with a nit.
>
> Sorry for the delay, my Inbox grew faster the last 24 hours than I could
> process
> it :-/
>
>
> https://codereview.chromium.**org/12315116/diff/12001/base/**
>
prefs/pref_service.h<https://codereview.chromium.org/12315116/diff/12001/base/prefs/pref_service.h>
> File base/prefs/pref_service.h (right):
>
> https://codereview.chromium.**org/12315116/diff/12001/base/**
>
prefs/pref_service.h#**newcode223<https://codereview.chromium.org/12315116/diff/12001/base/prefs/pref_service.h#newcode223>
> base/prefs/pref_service.h:223: // Changes the default value for a
> preference. Takes ownership of |value|.
> nit: add a line saying that this will fire pref change notifications if
> the effective value changed.
>
>
https://codereview.chromium.**org/12315116/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-03-01 12:45:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12315116/17001

[commit-bot: I haz the power, 2013-03-01 12:45:52]

Presubmit check for 12315116-17001 failed and returned exit status 1.

INFO:root:Found 14 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/base/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/profiles/profile_impl.cc
    chrome/browser/ui/browser_instant_controller.cc
    chrome/browser/ui/browser_instant_controller.h
    chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
    chrome/browser/extensions/extension_service_unittest.cc
    chrome/test/base/testing_profile.cc

Presubmit checks took 1.5s to calculate.

[Jói, 2013-03-01 12:47:32]

TBR=ben@chromium.org for trivial usage updates of changed Prefs API.

Cheers,
Jói

[commit-bot: I haz the power, 2013-03-01 12:47:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12315116/17001

[commit-bot: I haz the power, 2013-03-02 16:07:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12315116/17001

[commit-bot: I haz the power, 2013-03-02 17:02:27]

Change committed as 185712