Provide access to recommended pref values

PrefValueStore and PrefService expose the value of a pref returned by the
highest-priority PrefStore only. Recommended values are interesting even
if they are currently being overridden by a higher-priority source.

This CL adds a getter for the recommended value and makes pref change
notifications be sent on all value changes, even if the change occurs in
a PrefStore that is currently overridden. This will allow the settings UI
to adapt when recommended values change.

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

[bartfab (slow), 2012-07-26 19:48:33]

Mattias, could you have a look? I want to add a few more unit tests but in
general, this is what I want to do with prefs.

[Mattias Nissler (ping if slow), 2012-07-27 07:26:11]

http://codereview.chromium.org/10821047/diff/1/chrome/browser/prefs/pref_valu...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/10821047/diff/1/chrome/browser/prefs/pref_valu...
chrome/browser/prefs/pref_value_store.cc:131: }
indentation

http://codereview.chromium.org/10821047/diff/1/chrome/browser/prefs/pref_valu...
chrome/browser/prefs/pref_value_store.cc:132: return true;
seems like the code in this function is essentially to loop body of GetValue()
above. Share?

http://codereview.chromium.org/10821047/diff/1/chrome/browser/prefs/pref_valu...
chrome/browser/prefs/pref_value_store.cc:148: // effective value is unchanged.
As discussed offline, maybe we want to just drop all the filtering. Can you
instrument a build and count the number of pref change notifications sent out w/
vs. w/out the filtering? I'd like to see some number on how much impact this
has.

[bartfab (slow), 2012-07-27 12:14:49]

https://chromiumcodereview.appspot.com/10821047/diff/1/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_value_store.cc (right):

https://chromiumcodereview.appspot.com/10821047/diff/1/chrome/browser/prefs/p...
chrome/browser/prefs/pref_value_store.cc:131: }
On 2012/07/27 07:26:11, Mattias Nissler wrote:
> indentation

Done.

https://chromiumcodereview.appspot.com/10821047/diff/1/chrome/browser/prefs/p...
chrome/browser/prefs/pref_value_store.cc:132: return true;
On 2012/07/27 07:26:11, Mattias Nissler wrote:
> seems like the code in this function is essentially to loop body of GetValue()
> above. Share?

Done.

https://chromiumcodereview.appspot.com/10821047/diff/1/chrome/browser/prefs/p...
chrome/browser/prefs/pref_value_store.cc:148: // effective value is unchanged.
Fresh profile: Start browser, visit google.com, shut down: 1489 pref change
notifications, 0 of these get filtered.

Existing profile: Start browser, visit google.com, visit settings page, shut
down: 1407 pref change notifications, 72 of these get filtered.

[Mattias Nissler (ping if slow), 2012-07-27 12:21:58]

https://chromiumcodereview.appspot.com/10821047/diff/6001/chrome/browser/pref...
File chrome/browser/prefs/pref_value_store.cc (right):

https://chromiumcodereview.appspot.com/10821047/diff/6001/chrome/browser/pref...
chrome/browser/prefs/pref_value_store.cc:248: i <=
static_cast<size_t>(last_checked_store); ++i) {
I'd rather put the loop into GetValue(), and make this a
GetValueFromStoreWithType() that you can call from there and from
GetRecommendedValue().

https://chromiumcodereview.appspot.com/10821047/diff/6001/chrome/browser/pref...
File chrome/browser/prefs/pref_value_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10821047/diff/6001/chrome/browser/pref...
chrome/browser/prefs/pref_value_store_unittest.cc:347: 
remove excess whitespace

[bartfab (slow), 2012-07-27 13:12:08]

https://chromiumcodereview.appspot.com/10821047/diff/6001/chrome/browser/pref...
File chrome/browser/prefs/pref_value_store.cc (right):

https://chromiumcodereview.appspot.com/10821047/diff/6001/chrome/browser/pref...
chrome/browser/prefs/pref_value_store.cc:248: i <=
static_cast<size_t>(last_checked_store); ++i) {
On 2012/07/27 12:21:58, Mattias Nissler wrote:
> I'd rather put the loop into GetValue(), and make this a
> GetValueFromStoreWithType() that you can call from there and from
> GetRecommendedValue().

Done.

https://chromiumcodereview.appspot.com/10821047/diff/6001/chrome/browser/pref...
File chrome/browser/prefs/pref_value_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10821047/diff/6001/chrome/browser/pref...
chrome/browser/prefs/pref_value_store_unittest.cc:347: 
On 2012/07/27 12:21:58, Mattias Nissler wrote:
> remove excess whitespace

Done.

[Mattias Nissler (ping if slow), 2012-07-27 13:21:45]

LGTM with nits.

https://chromiumcodereview.appspot.com/10821047/diff/3003/chrome/browser/pref...
File chrome/browser/prefs/pref_value_store.cc (right):

https://chromiumcodereview.appspot.com/10821047/diff/3003/chrome/browser/pref...
chrome/browser/prefs/pref_value_store.cc:107: if
(GetValueFromStoreWithType(name.c_str(),
nit: In function calls, you may put multiple arguments in one line, which I
would personally prefer here. Your call though.

https://chromiumcodereview.appspot.com/10821047/diff/3003/chrome/browser/pref...
chrome/browser/prefs/pref_value_store.cc:119: return
GetValueFromStoreWithType(name.c_str(),
same here.

https://chromiumcodereview.appspot.com/10821047/diff/3003/chrome/browser/pref...
File chrome/browser/prefs/pref_value_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10821047/diff/3003/chrome/browser/pref...
chrome/browser/prefs/pref_value_store_unittest.cc:337: ASSERT_TRUE(value ==
NULL);
nit: ASSERT_FALSE(value) (here and elsewhere in this file)

[bartfab (slow), 2012-07-27 13:24:42]

https://chromiumcodereview.appspot.com/10821047/diff/3003/chrome/browser/pref...
File chrome/browser/prefs/pref_value_store.cc (right):

https://chromiumcodereview.appspot.com/10821047/diff/3003/chrome/browser/pref...
chrome/browser/prefs/pref_value_store.cc:107: if
(GetValueFromStoreWithType(name.c_str(),
On 2012/07/27 13:21:45, Mattias Nissler wrote:
> nit: In function calls, you may put multiple arguments in one line, which I
> would personally prefer here. Your call though.

Done.

https://chromiumcodereview.appspot.com/10821047/diff/3003/chrome/browser/pref...
chrome/browser/prefs/pref_value_store.cc:119: return
GetValueFromStoreWithType(name.c_str(),
On 2012/07/27 13:21:45, Mattias Nissler wrote:
> same here.

Done.

https://chromiumcodereview.appspot.com/10821047/diff/3003/chrome/browser/pref...
File chrome/browser/prefs/pref_value_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10821047/diff/3003/chrome/browser/pref...
chrome/browser/prefs/pref_value_store_unittest.cc:337: ASSERT_TRUE(value ==
NULL);
I inherited that from the code that was in the file already. Didn't even notice
it :). Done.

[commit-bot: I haz the power, 2012-07-27 17:03:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/10821047/4008

[commit-bot: I haz the power, 2012-07-27 19:46:37]

Try job failure for 10821047-4008 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-07-30 08:11:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/10821047/4008

[commit-bot: I haz the power, 2012-07-30 10:14:04]

Change committed as 148939