Add an ExtensionPrefStore, layered between the user prefs nad the managed pre...

Add an ExtensionPrefStore, layered between the user prefs and the managed prefs, to manage preferences set by extensions.
Update various callers of the PrefValueStore constructor accordingly.

The initial user will be the proxy extension API.

BUG=266
TEST=covered by unit tests

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

[Pam (message me for reviews), 2010-06-30 14:02:01]

It's not as bad as it looks -- most of the changes are minor updates to the
PrefValueStore() called from unit tests.

Thanks,
- Pam

[Pam (message me for reviews), 2010-06-30 15:03:56]

All the files should be in now. Sorry about that.

- Pam

[Bernhard Bauer, 2010-07-05 15:58:45]

In addition to my comments below, please fix the lint errors, and don't set
svn:eol-style.

http://codereview.chromium.org/2823037/diff/10005/28
File chrome/browser/dom_ui/new_tab_ui_uitest.cc (right):

http://codereview.chromium.org/2823037/diff/10005/28#newcode88
chrome/browser/dom_ui/new_tab_ui_uitest.cc:88: NULL /* no advised prefs */));
We really should create a convenience method for this, as it's used all the time
in tests.

http://codereview.chromium.org/2823037/diff/10005/30
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/2823037/diff/10005/30#newcode26
chrome/browser/extensions/extension_pref_store.cc:26:
pref_service_->FindPreference(path)->GetValue()->DeepCopy());
Nit: This line is indented five space w/r/t the previous line instead of four.

http://codereview.chromium.org/2823037/diff/10005/30#newcode70
chrome/browser/extensions/extension_pref_store.cc:70: pref_values = new
PrefValueMap();
Nit: This line is indented three spaces w/r/t the previous line instead of two.

http://codereview.chromium.org/2823037/diff/10005/41
File chrome/browser/pref_service_unittest.cc (right):

http://codereview.chromium.org/2823037/diff/10005/41#newcode152
chrome/browser/pref_service_unittest.cc:152: PrefService prefs(new
PrefValueStore(NULL, NULL, new DummyPrefStore(),
Shouldn't this be |pref_store|?

[Pam (message me for reviews), 2010-07-12 08:35:22]

All fixed, except the svn:eol-style. That's required (see
http://dev.chromium.org/developers/coding-style#TOC-Subversion-properties and
the link to
http://src.chromium.org/viewvc/chrome/trunk/tools/buildbot/slave/config) and
enforced by the presubmit checks.

Please take another look.

- Pam

[Bernhard Bauer, 2010-07-12 09:05:32]

http://codereview.chromium.org/2823037/diff/101001/102013
File chrome/browser/pref_service.h (right):

http://codereview.chromium.org/2823037/diff/101001/102013#newcode81
chrome/browser/pref_service.h:81: explicit PrefService(const FilePath&
user_pref_file);
Could you add a comment (either here or above) explaining how this PrefService
differs from one created by CreatePrefService?

Also, is overloading the constructor allowed?

http://codereview.chromium.org/2823037/diff/101001/102016
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/2823037/diff/101001/102016#newcode88
chrome/browser/pref_value_store.h:88: scoped_ptr<PrefStore> recommended_prefs_;
I think now that we've broken the rule of only having three PrefStores, we could
simply have a list of them. What was danno's opinion here?

[Bernhard Bauer, 2010-07-12 09:06:49]

On 2010/07/12 08:35:22, Pam wrote:
> All fixed, except the svn:eol-style. That's required (see
> http://dev.chromium.org/developers/coding-style#TOC-Subversion-properties and
> the link to
> http://src.chromium.org/viewvc/chrome/trunk/tools/buildbot/slave/config) and
> enforced by the presubmit checks.

Hm, does this mean that it's already set on the other files? I can't remember
seeing svn:eol-style in my CL's, and I didn't have any problems with the
presubmit checks.

> 
> Please take another look.
> 
> - Pam

[Pam (message me for reviews), 2010-07-12 11:55:47]

On 2010/07/12 09:06:49, Bernhard Bauer wrote:
> On 2010/07/12 08:35:22, Pam wrote:
> > All fixed, except the svn:eol-style. That's required (see
> > http://dev.chromium.org/developers/coding-style#TOC-Subversion-properties
and
> > the link to
> > http://src.chromium.org/viewvc/chrome/trunk/tools/buildbot/slave/config) and
> > enforced by the presubmit checks.
> 
> Hm, does this mean that it's already set on the other files? I can't remember
> seeing svn:eol-style in my CL's, and I didn't have any problems with the
> presubmit checks.

Yes, it's set on most files. Some slip through, maybe from people whose git or
svn aren't configured with the auto-props. I seem to recall that git-cl doesn't
run the presubmit checks that gcl does, but I'm not an expert on that.

- Pam

[Pam (message me for reviews), 2010-07-12 13:49:38]

All fixed again. Please take another look.

I don't know of any problems with overloading constructors.

- Pam

[Bernhard Bauer, 2010-07-12 14:03:30]

Small question below, and extension_pref_store* is missing newlines at end of
file, otherwise LGTM.

http://codereview.chromium.org/2823037/diff/66025/110018
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/2823037/diff/66025/110018#newcode98
chrome/browser/pref_value_store.h:98: scoped_ptr<PrefStore>
pref_stores_[PREF_STORE_TYPE_MAX + 1];
Why don't you simply use something like NUM_PREF_STORE_TYPES here?

[Pam (message me for reviews), 2010-07-12 14:39:45]

I've checked several times, and those files all have ending newlines on my hard
drive. I don't know what's going on with the upload, patch, or linter.

http://codereview.chromium.org/2823037/diff/66025/110018
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/2823037/diff/66025/110018#newcode98
chrome/browser/pref_value_store.h:98: scoped_ptr<PrefStore>
pref_stores_[PREF_STORE_TYPE_MAX + 1];
On 2010/07/12 14:03:30, Bernhard Bauer wrote:
> Why don't you simply use something like NUM_PREF_STORE_TYPES here?

Habit, really. This way makes the special entry stick out more, reducing the
chance that someone puts another one after it. Also, it's more resilient to
silly things like

enum Foo {
  FIRST = 0,
  SECOND,
  THIRD = 20,
  NUM_FOO
};

int foo_array[NUM_FOO];  // Oops!

(which is the big risk with using enums as array indices at all, of course).
Finally, if you have gcc warn when an enum value isn't in a switch case, the
extra value will give false warnings.

I'm not attached to it, though, if you find it harder to read.

[Bernhard Bauer, 2010-07-12 14:44:09]

On 2010/07/12 13:49:38, Pam wrote:
> All fixed again. Please take another look.
> 
> I don't know of any problems with overloading constructors.

Well, function overloading in general is frowned upon in the style guide if the
respective methods do different things.

[Bernhard Bauer, 2010-07-12 15:34:13]

On 2010/07/12 14:39:45, Pam wrote:
> I've checked several times, and those files all have ending newlines on my
hard
> drive. I don't know what's going on with the upload, patch, or linter.

Maybe it's tripped up by the svn attribute changes in the patch file.

> 
> http://codereview.chromium.org/2823037/diff/66025/110018
> File chrome/browser/pref_value_store.h (right):
> 
> http://codereview.chromium.org/2823037/diff/66025/110018#newcode98
> chrome/browser/pref_value_store.h:98: scoped_ptr<PrefStore>
> pref_stores_[PREF_STORE_TYPE_MAX + 1];
> On 2010/07/12 14:03:30, Bernhard Bauer wrote:
> > Why don't you simply use something like NUM_PREF_STORE_TYPES here?
> 
> Habit, really. This way makes the special entry stick out more, reducing the
> chance that someone puts another one after it. Also, it's more resilient to
> silly things like
> 
> enum Foo {
>   FIRST = 0,
>   SECOND,
>   THIRD = 20,
>   NUM_FOO
> };
> 
> int foo_array[NUM_FOO];  // Oops!
> 
> (which is the big risk with using enums as array indices at all, of course).
> Finally, if you have gcc warn when an enum value isn't in a switch case, the
> extra value will give false warnings.
> 
> I'm not attached to it, though, if you find it harder to read.

I'm also not really passionate about it (especially at this temperature...)

[Pam (message me for reviews), 2010-07-12 15:44:22]

> Well, function overloading in general is frowned upon in the style guide if
the
> respective methods do different things.

Right, then, off it goes. Another look please?

- Pam

[Bernhard Bauer, 2010-07-12 16:00:34]

On 2010/07/12 15:44:22, Pam wrote:
> > Well, function overloading in general is frowned upon in the style guide if
> the
> > respective methods do different things.
> 
> Right, then, off it goes. Another look please?
> 
> - Pam

LGTM.