Move creation of the PrefStores into the PrefValueStore, to reduce the knowle...

Move creation of the PrefStores into the PrefValueStore, to reduce the knowledge the PrefService has of its two-levels-deep implementation.

Create a TestingPrefService::TestingPrefValueStore to allow tests to set the PrefStores directly, as they used to be able to do.

BUG=50722
TEST=covered by unit tests

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

[Mattias Nissler (ping if slow), 2010-08-05 15:07:25]

Overall, this LGTM. I've put a comment concerning the PrefValueStore
constructor, but I don't feel strong about it. If you decide you like your
current approach, go for it.

http://codereview.chromium.org/3032058/diff/26003/31007
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/3032058/diff/26003/31007#newcode52
chrome/browser/pref_value_store.h:52: bool user_only);
Hm, I'm not so sure about the constructor change, the SetPrefStores() method and
the TestingPrefService subclass. How about keeping the constructor that takes
all the stores, and providing a factory method for creating the regular
PrefValueStore? That way you'd also get rid of the SetPrefStores() function,
which seems a bit fishy to me, since you may only call it for tests and then
only when initializing, since it doesn't take care to generate notifications if
the contents of the stores change.

http://codereview.chromium.org/3032058/diff/26003/31008
File chrome/browser/pref_value_store_unittest.cc (right):

http://codereview.chromium.org/3032058/diff/26003/31008#newcode9
chrome/browser/pref_value_store_unittest.cc:9: #include
"chrome/browser/chrome_thread.h"
why?

[Pam (message me for reviews), 2010-08-05 20:18:08]

http://codereview.chromium.org/3032058/diff/26003/31007
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/3032058/diff/26003/31007#newcode52
chrome/browser/pref_value_store.h:52: bool user_only);
On 2010/08/05 15:07:26, mnissler wrote:
> Hm, I'm not so sure about the constructor change, the SetPrefStores() method
and
> the TestingPrefService subclass. How about keeping the constructor that takes
> all the stores, and providing a factory method for creating the regular
> PrefValueStore? That way you'd also get rid of the SetPrefStores() function,
> which seems a bit fishy to me, since you may only call it for tests and then
> only when initializing, since it doesn't take care to generate notifications
if
> the contents of the stores change.

OK, I turned SetPrefStores() into a constructor, but left it protected. Still
nobody but a unit test should be calling it, but this ensures it's only used for
initialization. So I'm leaving the TestingPrefService subclass as the safe way
to expose the protected constructor to tests, rather than letting just anybody
mess with it.

http://codereview.chromium.org/3032058/diff/26003/31008
File chrome/browser/pref_value_store_unittest.cc (right):

http://codereview.chromium.org/3032058/diff/26003/31008#newcode9
chrome/browser/pref_value_store_unittest.cc:9: #include
"chrome/browser/chrome_thread.h"
On 2010/08/05 15:07:26, mnissler wrote:
> why?

Danno's previous patch added a dependency on ChromeThread. It was pulled in
indirectly already, but it's good to make the dependency obvious.

[Mattias Nissler (ping if slow), 2010-08-06 07:29:41]

LGTM with two little nits.

http://codereview.chromium.org/3032058/diff/14002/21006
File chrome/browser/pref_value_store.cc (right):

http://codereview.chromium.org/3032058/diff/14002/21006#newcode247
chrome/browser/pref_value_store.cc:247: }
nit: Order of definitions doesn't correspond to order of declarations in the
header.

http://codereview.chromium.org/3032058/diff/14002/21007
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/3032058/diff/14002/21007#newcode139
chrome/browser/pref_value_store.h:139: // This constructor should only be used
by subclasses in testing. The usual
nit: It's also used for creating the actual PrefValueStore.