Add more per-tab preferences.

Add more per-tab preferences.

intl.global.charset_default
webkit.webprefs.global.standard_font_family
webkit.webprefs.global.fixed_font_family
webkit.webprefs.global.serif_font_family
webkit.webprefs.global.sansserif_font_family
webkit.webprefs.global.cursive_font_family
webkit.webprefs.global.fantasy_font_family
webkit.webprefs.global.default_font_size
webkit.webprefs.global.default_fixed_font_size
webkit.webprefs.global.minimum_font_size
webkit.webprefs.global.minimum_logical_font_size
webkit.webprefs.global.javascript_can_open_windows_automatically
webkit.webprefs.global.loads_images_automatically
webkit.webprefs.global.plugins_enabled

BUG=105537
TEST=PrefsTabHelperTest*,PrefsTabHelperBrowserTest*

For files that has only prefs names changes and require OWNERS approval:

TBR=erikwright@chromium.org,gene@chromium.org,csilv@chromium.org,atwilson@chromium.org,mirandac@chromium.org

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

[mnaganov (inactive), 2011-12-08 17:00:17]

Hi Mattias,

I'm adding more per-tab overridable preferences. Changes are mostly mechanical.

[Mattias Nissler (ping if slow), 2011-12-09 09:54:44]

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/net/chrome_ur...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.cc:284:
prefs->GetString(prefs::kGlobalDefaultCharset);
Are you OK with the request context reporting the charset from the global pref,
even if there's an override in place? Potentially a bug, just want to make sure
you thought about this.

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/prefs/per_tab...
File chrome/browser/prefs/per_tab_user_pref_store.cc (right):

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/prefs/per_tab...
chrome/browser/prefs/per_tab_user_pref_store.cc:19:
prefs::kWebKitPluginsEnabled, prefs::kWebKitGlobalPluginsEnabled);
nit: I'd just make all of these use two lines to give the function a consistent
look (much easier to scan over for me). Your call.

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper_unittest.cc (right):

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper_unittest.cc:160:
TestBooleanPreference(prefs::kWebKitJavascriptEnabled);
Basically, you're doing value-parameterized tests here, which googletest has
some nice support for:
http://code.google.com/p/googletest/wiki/AdvancedGuide#How_to_Write_Value-Par...

So you might want to give that a shot. I'm fine either way, so if you like the
version you have better, that's cool with me.

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/toolbar/en...
File chrome/browser/ui/toolbar/encoding_menu_controller.cc (right):

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/toolbar/en...
chrome/browser/ui/toolbar/encoding_menu_controller.cc:90:
browser_profile->GetPrefs()->GetString(prefs::kGlobalDefaultCharset);
This one is interesting. Shouldn't we actually grab the effective value here?
Also, changing the setting from the menu won't make a difference, so maybe we
need to disable the menu entirely? Thoughts?

http://codereview.chromium.org/8879016/diff/4001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8879016/diff/4001/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:115: "webkit.webprefs.global.standard_font_family";
How do we handle migration of existing prefs?

[mnaganov (inactive), 2011-12-12 18:19:03]

Thanks, Mattias! Your concern about preferences migration requires some
additional work to be done. I will resolve it and will ping you back.

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/net/chrome_ur...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.cc:284:
prefs->GetString(prefs::kGlobalDefaultCharset);
On 2011/12/09 09:54:44, Mattias Nissler wrote:
> Are you OK with the request context reporting the charset from the global
pref,
> even if there's an override in place? Potentially a bug, just want to make
sure
> you thought about this.

A good catch---that's an issue. Currently request context is owned by a profile,
so it seems logical to deal only with the global property here. But I think I
will need to fix this in the near future.

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/prefs/per_tab...
File chrome/browser/prefs/per_tab_user_pref_store.cc (right):

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/prefs/per_tab...
chrome/browser/prefs/per_tab_user_pref_store.cc:19:
prefs::kWebKitPluginsEnabled, prefs::kWebKitGlobalPluginsEnabled);
On 2011/12/09 09:54:44, Mattias Nissler wrote:
> nit: I'd just make all of these use two lines to give the function a
consistent
> look (much easier to scan over for me). Your call.

Agree---a consistent layout is easier to parse. Changed to 3 lines per property.

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper_unittest.cc (right):

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper_unittest.cc:160:
TestBooleanPreference(prefs::kWebKitJavascriptEnabled);
On 2011/12/09 09:54:44, Mattias Nissler wrote:
> Basically, you're doing value-parameterized tests here, which googletest has
> some nice support for:
>
http://code.google.com/p/googletest/wiki/AdvancedGuide#How_to_Write_Value-Par...
> 
> So you might want to give that a shot. I'm fine either way, so if you like the
> version you have better, that's cool with me.

That's a good thing to know about, thanks! But in our case we have properties
with different types, so I would end up creating 3 versions of the test class
for bool, int and string types (yes, I can create a template, but as PrefService
isn't templatized, this will not generalize anything), and then parametrize each
other by value. I think currently we don't have that much preferences to justify
applying this machinery.

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/toolbar/en...
File chrome/browser/ui/toolbar/encoding_menu_controller.cc (right):

http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/toolbar/en...
chrome/browser/ui/toolbar/encoding_menu_controller.cc:90:
browser_profile->GetPrefs()->GetString(prefs::kGlobalDefaultCharset);
On 2011/12/09 09:54:44, Mattias Nissler wrote:
> This one is interesting. Shouldn't we actually grab the effective value here?
> Also, changing the setting from the menu won't make a difference, so maybe we
> need to disable the menu entirely? Thoughts?

Currently we only plan to set per-tab settings programmatically, and there will
be no Chrome UI there.

http://codereview.chromium.org/8879016/diff/4001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8879016/diff/4001/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:115: "webkit.webprefs.global.standard_font_family";
On 2011/12/09 09:54:44, Mattias Nissler wrote:
> How do we handle migration of existing prefs?

A good question, indeed! I've found this code dialing with preferences
migration: http://codereview.chromium.org/5915006/, I think I will apply a
similar approach.

[mnaganov (inactive), 2011-12-15 15:05:03]

Matias, I've implemented preferences migration. Please take a look!

On 2011/12/12 18:19:03, Mikhail Naganov (Chromium) wrote:
> Thanks, Mattias! Your concern about preferences migration requires some
> additional work to be done. I will resolve it and will ping you back.
> 
>
http://codereview.chromium.org/8879016/diff/4001/chrome/browser/net/chrome_ur...
> File chrome/browser/net/chrome_url_request_context.cc (right):
> 
>
http://codereview.chromium.org/8879016/diff/4001/chrome/browser/net/chrome_ur...
> chrome/browser/net/chrome_url_request_context.cc:284:
> prefs->GetString(prefs::kGlobalDefaultCharset);
> On 2011/12/09 09:54:44, Mattias Nissler wrote:
> > Are you OK with the request context reporting the charset from the global
> pref,
> > even if there's an override in place? Potentially a bug, just want to make
> sure
> > you thought about this.
> 
> A good catch---that's an issue. Currently request context is owned by a
profile,
> so it seems logical to deal only with the global property here. But I think I
> will need to fix this in the near future.
> 
>
http://codereview.chromium.org/8879016/diff/4001/chrome/browser/prefs/per_tab...
> File chrome/browser/prefs/per_tab_user_pref_store.cc (right):
> 
>
http://codereview.chromium.org/8879016/diff/4001/chrome/browser/prefs/per_tab...
> chrome/browser/prefs/per_tab_user_pref_store.cc:19:
> prefs::kWebKitPluginsEnabled, prefs::kWebKitGlobalPluginsEnabled);
> On 2011/12/09 09:54:44, Mattias Nissler wrote:
> > nit: I'd just make all of these use two lines to give the function a
> consistent
> > look (much easier to scan over for me). Your call.
> 
> Agree---a consistent layout is easier to parse. Changed to 3 lines per
property.
> 
>
http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/prefs/pref...
> File chrome/browser/ui/prefs/prefs_tab_helper_unittest.cc (right):
> 
>
http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/prefs/pref...
> chrome/browser/ui/prefs/prefs_tab_helper_unittest.cc:160:
> TestBooleanPreference(prefs::kWebKitJavascriptEnabled);
> On 2011/12/09 09:54:44, Mattias Nissler wrote:
> > Basically, you're doing value-parameterized tests here, which googletest has
> > some nice support for:
> >
>
http://code.google.com/p/googletest/wiki/AdvancedGuide#How_to_Write_Value-Par...
> > 
> > So you might want to give that a shot. I'm fine either way, so if you like
the
> > version you have better, that's cool with me.
> 
> That's a good thing to know about, thanks! But in our case we have properties
> with different types, so I would end up creating 3 versions of the test class
> for bool, int and string types (yes, I can create a template, but as
PrefService
> isn't templatized, this will not generalize anything), and then parametrize
each
> other by value. I think currently we don't have that much preferences to
justify
> applying this machinery.
> 
>
http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/toolbar/en...
> File chrome/browser/ui/toolbar/encoding_menu_controller.cc (right):
> 
>
http://codereview.chromium.org/8879016/diff/4001/chrome/browser/ui/toolbar/en...
> chrome/browser/ui/toolbar/encoding_menu_controller.cc:90:
> browser_profile->GetPrefs()->GetString(prefs::kGlobalDefaultCharset);
> On 2011/12/09 09:54:44, Mattias Nissler wrote:
> > This one is interesting. Shouldn't we actually grab the effective value
here?
> > Also, changing the setting from the menu won't make a difference, so maybe
we
> > need to disable the menu entirely? Thoughts?
> 
> Currently we only plan to set per-tab settings programmatically, and there
will
> be no Chrome UI there.
> 
> http://codereview.chromium.org/8879016/diff/4001/chrome/common/pref_names.cc
> File chrome/common/pref_names.cc (right):
> 
>
http://codereview.chromium.org/8879016/diff/4001/chrome/common/pref_names.cc#...
> chrome/common/pref_names.cc:115:
"webkit.webprefs.global.standard_font_family";
> On 2011/12/09 09:54:44, Mattias Nissler wrote:
> > How do we handle migration of existing prefs?
> 
> A good question, indeed! I've found this code dialing with preferences
> migration: http://codereview.chromium.org/5915006/, I think I will apply a
> similar approach.

[Mattias Nissler (ping if slow), 2011-12-15 15:37:29]

Couple of comments. Note that I'll be on vacation from later today till after
Christmas and will not be able to do further reviews. Bernhard, can you take
over?

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:572: if
(prefs->GetBoolean(prefs::kWebKitSettingsMigratedToGlobal)) {
no need for curlies.

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:576: DCHECK(kPrefsToMigrateLength ==
arraysize(kPrefNamesToMigrateTo));
It seems like you should rather use an anonymous struct here:

const struct {
  const char* from;
  const char* to;
} kPrefNamesToMigrate[] = {
  { prefs::kWebkit, prefs::kWebkitGlobal },
};

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:583:
prefs->ClearPref(kPrefNamesToMigrateFrom[i]);
Maybe we can instead use the signal "all old prefs clear" in order to make the
decision whether we want to migrate? That way, you'd avoid introducing that
boolean migration flag.

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc (right):

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc:17: public:
can be protected.

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc:48: public:
remove

[mnaganov (inactive), 2011-12-15 16:45:17]

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:572: if
(prefs->GetBoolean(prefs::kWebKitSettingsMigratedToGlobal)) {
On 2011/12/15 15:37:29, Mattias Nissler wrote:
> no need for curlies.

The code is now gone.

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:576: DCHECK(kPrefsToMigrateLength ==
arraysize(kPrefNamesToMigrateTo));
On 2011/12/15 15:37:29, Mattias Nissler wrote:
> It seems like you should rather use an anonymous struct here:
> 
> const struct {
>   const char* from;
>   const char* to;
> } kPrefNamesToMigrate[] = {
>   { prefs::kWebkit, prefs::kWebkitGlobal },
> };

Done.

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:583:
prefs->ClearPref(kPrefNamesToMigrateFrom[i]);
On 2011/12/15 15:37:29, Mattias Nissler wrote:
> Maybe we can instead use the signal "all old prefs clear" in order to make the
> decision whether we want to migrate? That way, you'd avoid introducing that
> boolean migration flag.

Good idea! I have realized that we don't even need to make a "decision" -- just
migrate all existing old names into new ones.

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc (right):

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc:17: public:
On 2011/12/15 15:37:29, Mattias Nissler wrote:
> can be protected.

Done.

http://codereview.chromium.org/8879016/diff/8001/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc:48: public:
On 2011/12/15 15:37:29, Mattias Nissler wrote:
> remove

Done.

[Bernhard Bauer, 2011-12-21 16:41:32]

Can you CC me on the bug for context? Thanks!

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:543: } kPrefNamesToMigrate[] = {
Put this into an anonymous namespace and move to the top of the file.

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:568: const int kPrefsToMigrateLength
= ARRAYSIZE_UNSAFE(kPrefNamesToMigrate);
Can you use arraysize here or does it something like creating a static
initializer?

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc (right):

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc:16: class
PrefsTabHelperUITest : public UITest {
If you make this a browser test instead of a UI test, you can directly ask the
PrefService instead of having to go through the prefs file.

[mnaganov (inactive), 2011-12-21 18:43:33]

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:543: } kPrefNamesToMigrate[] = {
On 2011/12/21 16:41:32, Bernhard Bauer wrote:
> Put this into an anonymous namespace and move to the top of the file.

Oh, right. There is an anonymous n/s already. Moved there.

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper.cc:568: const int kPrefsToMigrateLength
= ARRAYSIZE_UNSAFE(kPrefNamesToMigrate);
On 2011/12/21 16:41:32, Bernhard Bauer wrote:
> Can you use arraysize here or does it something like creating a static
> initializer?

arraysize doesn't work for anonymous structs.

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/basictypes.h&q=a...

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc (right):

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc:16: class
PrefsTabHelperUITest : public UITest {
On 2011/12/21 16:41:32, Bernhard Bauer wrote:
> If you make this a browser test instead of a UI test, you can directly ask the
> PrefService instead of having to go through the prefs file.

What I'm doing here is:

1. Copying a hand-crafted Preferences file from test data into temp profile dir.

2. Launching Chrome to migrate preferences. I need to terminate it to make sure
that the profile file is saved to disk.

3. Verifying that in the saved file preferences have been migrated.

I suppose this approach gives me more guarantees than just querying them from
PrefService. What do you think?

[Bernhard Bauer, 2011-12-22 15:09:00]

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/prefs/per_tab...
File chrome/browser/prefs/per_tab_user_pref_store.cc (right):

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/prefs/per_tab...
chrome/browser/prefs/per_tab_user_pref_store.cc:10: RegisterOverlayProperty(
I don't think we actually need a separate subclass here that just registers the
overlay properties (why 'property' and not 'pref', BTW?). You could just use a
plain OverlayUserPrefStore and call |RegisterOverlayProperty| on it. That way we
can keep the code in prefs/ independent of the actual set of prefs stored in the
overlay.

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
File chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc (right):

http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc:16: class
PrefsTabHelperUITest : public UITest {
On 2011/12/21 18:43:33, Mikhail Naganov (Chromium) wrote:
> On 2011/12/21 16:41:32, Bernhard Bauer wrote:
> > If you make this a browser test instead of a UI test, you can directly ask
the
> > PrefService instead of having to go through the prefs file.
> 
> What I'm doing here is:
> 
> 1. Copying a hand-crafted Preferences file from test data into temp profile
dir.
> 
> 2. Launching Chrome to migrate preferences. I need to terminate it to make
sure
> that the profile file is saved to disk.
> 
> 3. Verifying that in the saved file preferences have been migrated.
> 
> I suppose this approach gives me more guarantees than just querying them from
> PrefService. What do you think?

Does it? Assuming that serialization/deserialization works correctly (which we
have separate unit tests for), doesn't this test basically just verify that a
PrefService correctly migrates preferences (which you could also check
in-browser)?

I guess what I want to say is that in general I'm not a fan of UI tests ;-) That
being said, if you want to do a UI test in this case, I'm fine with it.

[mnaganov (inactive), 2011-12-28 12:40:33]

On 2011/12/22 15:09:00, Bernhard Bauer wrote:
>
http://codereview.chromium.org/8879016/diff/9025/chrome/browser/prefs/per_tab...
> File chrome/browser/prefs/per_tab_user_pref_store.cc (right):
> 
>
http://codereview.chromium.org/8879016/diff/9025/chrome/browser/prefs/per_tab...
> chrome/browser/prefs/per_tab_user_pref_store.cc:10: RegisterOverlayProperty(
> I don't think we actually need a separate subclass here that just registers
the
> overlay properties (why 'property' and not 'pref', BTW?). You could just use a
> plain OverlayUserPrefStore and call |RegisterOverlayProperty| on it. That way
we
> can keep the code in prefs/ independent of the actual set of prefs stored in
the
> overlay.
>

Historically, there was IncognitoUserPrefStore which was encapsulating prefs
overriding and was defining a set of "special" pref names. I generalized it into
OverlayUserPrefStore and made IncognitoUserPrefStore a trivial subclass that
only defines the "special" pref. Then I added PerTabUserPrefStore, which is also
trivial. So we can get rid of these two trivial classes, but I'd better make
this a separate change.
 
>
http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
> File chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc (right):
> 
>
http://codereview.chromium.org/8879016/diff/9025/chrome/browser/ui/prefs/pref...
> chrome/browser/ui/prefs/prefs_tab_helper_uitest.cc:16: class
> PrefsTabHelperUITest : public UITest {
> On 2011/12/21 18:43:33, Mikhail Naganov (Chromium) wrote:
> > On 2011/12/21 16:41:32, Bernhard Bauer wrote:
> > > If you make this a browser test instead of a UI test, you can directly ask
> the
> > > PrefService instead of having to go through the prefs file.
> > 
> > What I'm doing here is:
> > 
> > 1. Copying a hand-crafted Preferences file from test data into temp profile
> dir.
> > 
> > 2. Launching Chrome to migrate preferences. I need to terminate it to make
> sure
> > that the profile file is saved to disk.
> > 
> > 3. Verifying that in the saved file preferences have been migrated.
> > 
> > I suppose this approach gives me more guarantees than just querying them
from
> > PrefService. What do you think?
> 
> Does it? Assuming that serialization/deserialization works correctly (which we
> have separate unit tests for), doesn't this test basically just verify that a
> PrefService correctly migrates preferences (which you could also check
> in-browser)?
> 
> I guess what I want to say is that in general I'm not a fan of UI tests ;-)
That
> being said, if you want to do a UI test in this case, I'm fine with it.

I've reimplemented this test as a browser test, and it has become shorter,
thanks to your suggestion. I have also discovered that PrefService was missing a
real "de-registration", so I've added it.

[Bernhard Bauer, 2011-12-28 13:33:41]

http://codereview.chromium.org/8879016/diff/22001/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/8879016/diff/22001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:380:
CHECK(synced_preferences_.count(name) == 0);
Make this a DCHECK? And should this be |registered_preferences_|?

[mnaganov (inactive), 2011-12-28 13:41:39]

http://codereview.chromium.org/8879016/diff/22001/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/8879016/diff/22001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:380:
CHECK(synced_preferences_.count(name) == 0);
On 2011/12/28 13:33:42, Bernhard Bauer wrote:
> Make this a DCHECK? And should this be |registered_preferences_|?

Of course, DCHECK. Fixed.

No. If a property isn't contained in registered_preferences_, it means it was
never registered. Thus, it makes no point in unregistering it.
|synced_preferences_| is a set that is filled during preference syncing. The
comment for it says "Note: this set never decreases", so I want to make sure
that unregistering happens before any syncing, as I don't know what will happen
if I violate the "never decreases" assumption by removing a preference from the
set.

[Bernhard Bauer, 2011-12-28 22:35:30]

LGTM once you have uploaded the fixed version :)

http://codereview.chromium.org/8879016/diff/22001/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/8879016/diff/22001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:380:
CHECK(synced_preferences_.count(name) == 0);
On 2011/12/28 13:41:40, Mikhail Naganov (Chromium) wrote:
> On 2011/12/28 13:33:42, Bernhard Bauer wrote:
> > Make this a DCHECK? And should this be |registered_preferences_|?
> 
> Of course, DCHECK. Fixed.
> 
> No. If a property isn't contained in registered_preferences_, it means it was
> never registered. Thus, it makes no point in unregistering it.
> |synced_preferences_| is a set that is filled during preference syncing. The
> comment for it says "Note: this set never decreases", so I want to make sure
> that unregistering happens before any syncing, as I don't know what will
happen
> if I violate the "never decreases" assumption by removing a preference from
the
> set.

OK, that makes sense.

[mnaganov (inactive), 2011-12-29 10:14:00]

Uploaded. Sorry for the delay.

On 2011/12/28 22:35:30, Bernhard Bauer wrote:
> LGTM once you have uploaded the fixed version :)
> 
>
http://codereview.chromium.org/8879016/diff/22001/chrome/browser/prefs/pref_m...
> File chrome/browser/prefs/pref_model_associator.cc (right):
> 
>
http://codereview.chromium.org/8879016/diff/22001/chrome/browser/prefs/pref_m...
> chrome/browser/prefs/pref_model_associator.cc:380:
> CHECK(synced_preferences_.count(name) == 0);
> On 2011/12/28 13:41:40, Mikhail Naganov (Chromium) wrote:
> > On 2011/12/28 13:33:42, Bernhard Bauer wrote:
> > > Make this a DCHECK? And should this be |registered_preferences_|?
> > 
> > Of course, DCHECK. Fixed.
> > 
> > No. If a property isn't contained in registered_preferences_, it means it
was
> > never registered. Thus, it makes no point in unregistering it.
> > |synced_preferences_| is a set that is filled during preference syncing. The
> > comment for it says "Note: this set never decreases", so I want to make sure
> > that unregistering happens before any syncing, as I don't know what will
> happen
> > if I violate the "never decreases" assumption by removing a preference from
> the
> > set.
> 
> OK, that makes sense.

[mnaganov (inactive), 2011-12-29 10:35:05]

Adding OWNERS of files that has only preference names changes and require
approval.

[Miranda Callahan, 2011-12-29 14:41:23]

On 2011/12/29 10:35:05, Mikhail Naganov (Chromium) wrote:
> Adding OWNERS of files that has only preference names changes and require
> approval.

profiles/* LGTM.

[Tyler Breisacher (Chromium), 2012-01-03 23:12:22]

I think the changes under options/ need to be copied into options2/ -- see
http://codereview.chromium.org/9071026/

[James Hawkins, 2012-01-03 23:23:52]

Please do not TBR changes that are non-trivial. Yes it's a judgement call, but
I'd certainly call this non-trivial. As Tyler noted, this caused a regression.

[mnaganov (inactive), 2012-01-05 11:00:22]

On 2012/01/03 23:23:52, James Hawkins wrote:
> Please do not TBR changes that are non-trivial. Yes it's a judgement call, but
> I'd certainly call this non-trivial. As Tyler noted, this caused a regression.

I'm sorry for creating a regression, and thank you for fixing it! The patch was
properly reviewed, I TBR'd people from OWNERS files because:

- changes in their files were really trivial and mechanical -- only properties
names change;

- waiting for too long for such kind of a change will add more and more
regressions as people are adding new stuff; this new options2 UI has been added
during the time I was working on this patch;

- it was a pre-NY week, everybody were having vacations, so I had no chance
getting approvals from so many people in time;