Converted IncognitoForced boolean policy into IncognitoModeAvailability enum policy.

Converted IncognitoForced boolean policy into IncognitoModeAvailability enum policy.
Enum may take 3 values:
* ENABLED (default) - both normal and incognito modes are allowed.
* DISABLED - incognito mode cannot be used.
* FORCED - browsing is only possible in incognito mode.

Mapped IncognitoEnabled::false to IncognitoModeAvailability::DISABLED when IncognitoModeAvailability policy/pref is not set.
Removed IncognitoEnabled pref (no longer used).

BUG=69580
TEST=


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

[Mattias Nissler (ping if slow), 2011-07-28 11:08:50]

http://codereview.chromium.org/7520023/diff/1/chrome/app/policy/policy_templa...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7520023/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:270: 'name': 'IncognitoEnabled',
You should mark this policy as deprecated.

http://codereview.chromium.org/7520023/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:312: If 'Disabled' is selected, pages
may not be opened in Incognito mode.
unneeded newline.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:650: if
(prefs_.GetInteger(prefs::kIncognitoModeAvailability, &int_value))
need curlies due to multi-line block below

http://codereview.chromium.org/7520023/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:657: if
(!prefs_.GetBoolean(prefs::kIncognitoEnabled, &obsolete_incognito_enabled))
we shouldn't have that pref any longer. Just remove it. Write an
ApplyIncognito() function that grabs the policy value. Then check here whether
the new policy is present. If not, set the new pref according to what the old
IncognitoEnabled policy value says.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
File chrome/browser/prefs/incognito_mode_availability_prefs.h (right):

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_mode_availability_prefs.h:6: #define
CHROME_BROWSER_PREFS_INCOGNITO_MODE_AVAILABILITY_PREFS_H_
#pragma once

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_mode_availability_prefs.h:8: namespace
IncognitoModeAvailabilityPrefs {
I suggest you make this a class, rename the enum to something shorter, and
provide static functions getting/setting the value given a PrefService. Also,
moving the registration of the pref here would be nice to have everything in one
place. Check out chrome/browser/prefs/session_startup_pref.cc for a similar
situation.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_mode_availability_prefs.h:29: }  // namespace
IncognitoModeAvailability
incorrect name.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7520023/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:2259:
prefs->RegisterIntegerPref(prefs::kIncognitoModeAvailability,
As mentioned, this should be moved to your helper file.

[Bernhard Bauer, 2011-07-28 11:34:02]

http://codereview.chromium.org/7520023/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:651:
IncognitoModeAvailabilityPrefs::IntToIngocnitoModeAvailability(
To be very thorough, you should also check the return value of this method.
Also, I think you should initialize the values above, otherwise coverity might
complain.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/command_li...
File chrome/browser/prefs/command_line_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/command_li...
chrome/browser/prefs/command_line_pref_store.cc:132:
Value::CreateIntegerValue(IncognitoModeAvailabilityPrefs::FORCED));
The --incognito flag opens the first window in incognito mode, but doesn't force
other windows into incognito mode. Here you're changing that behavior.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
File chrome/browser/prefs/incognito_mode_availability_prefs.h (right):

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_mode_availability_prefs.h:8: namespace
IncognitoModeAvailabilityPrefs {
On 2011/07/28 11:08:50, Mattias Nissler wrote:
> I suggest you make this a class, rename the enum to something shorter, and
> provide static functions getting/setting the value given a PrefService. Also,
> moving the registration of the pref here would be nice to have everything in
one
> place. Check out chrome/browser/prefs/session_startup_pref.cc for a similar
> situation.

What Mattias said. How about naming the class just IncognitoModePrefs and the
enum Availability?

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_mode_availability_prefs.h:18: DISABLED = 1,
Why do you explicitly list the enum values?

[rustema, 2011-07-29 06:49:23]

http://codereview.chromium.org/7520023/diff/1/chrome/app/policy/policy_templa...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7520023/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:270: 'name': 'IncognitoEnabled',
On 2011/07/28 11:08:50, Mattias Nissler wrote:
> You should mark this policy as deprecated.

Done.

http://codereview.chromium.org/7520023/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:312: If 'Disabled' is selected, pages
may not be opened in Incognito mode.
On 2011/07/28 11:08:50, Mattias Nissler wrote:
> unneeded newline.

It appears to me that display is a bit broken here. In fact there's only one
empty line between lines 312 and 314.
If you meant deleting line 313, I will do it (although other policy descriptions
are formatted this way).

http://codereview.chromium.org/7520023/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:650: if
(prefs_.GetInteger(prefs::kIncognitoModeAvailability, &int_value))
On 2011/07/28 11:08:50, Mattias Nissler wrote:
> need curlies due to multi-line block below

Done.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:651:
IncognitoModeAvailabilityPrefs::IntToIngocnitoModeAvailability(
On 2011/07/28 11:34:03, Bernhard Bauer wrote:
> To be very thorough, you should also check the return value of this method.
> Also, I think you should initialize the values above, otherwise coverity might
> complain.

Done.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:657: if
(!prefs_.GetBoolean(prefs::kIncognitoEnabled, &obsolete_incognito_enabled))
On 2011/07/28 11:08:50, Mattias Nissler wrote:
> we shouldn't have that pref any longer. Just remove it. Write an
> ApplyIncognito() function that grabs the policy value. Then check here whether
> the new policy is present. If not, set the new pref according to what the old
> IncognitoEnabled policy value says.

Done.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/command_li...
File chrome/browser/prefs/command_line_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/command_li...
chrome/browser/prefs/command_line_pref_store.cc:132:
Value::CreateIntegerValue(IncognitoModeAvailabilityPrefs::FORCED));
On 2011/07/28 11:34:03, Bernhard Bauer wrote:
> The --incognito flag opens the first window in incognito mode, but doesn't
force
> other windows into incognito mode. Here you're changing that behavior.

This is now gone.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
File chrome/browser/prefs/incognito_mode_availability_prefs.h (right):

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_mode_availability_prefs.h:6: #define
CHROME_BROWSER_PREFS_INCOGNITO_MODE_AVAILABILITY_PREFS_H_
On 2011/07/28 11:08:50, Mattias Nissler wrote:
> #pragma once

Done.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_mode_availability_prefs.h:8: namespace
IncognitoModeAvailabilityPrefs {
On 2011/07/28 11:08:50, Mattias Nissler wrote:
> I suggest you make this a class, rename the enum to something shorter, and
> provide static functions getting/setting the value given a PrefService. Also,
> moving the registration of the pref here would be nice to have everything in
one
> place. Check out chrome/browser/prefs/session_startup_pref.cc for a similar
> situation.

Done.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_mode_availability_prefs.h:8: namespace
IncognitoModeAvailabilityPrefs {
On 2011/07/28 11:34:03, Bernhard Bauer wrote:
> On 2011/07/28 11:08:50, Mattias Nissler wrote:
> > I suggest you make this a class, rename the enum to something shorter, and
> > provide static functions getting/setting the value given a PrefService.
Also,
> > moving the registration of the pref here would be nice to have everything in
> one
> > place. Check out chrome/browser/prefs/session_startup_pref.cc for a similar
> > situation.
> 
> What Mattias said. How about naming the class just IncognitoModePrefs and the
> enum Availability?

Done.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_mode_availability_prefs.h:18: DISABLED = 1,
On 2011/07/28 11:34:03, Bernhard Bauer wrote:
> Why do you explicitly list the enum values?

To make it clearer and easier to see which policy values they correspond to.
Removed.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/prefs/incognito_...
chrome/browser/prefs/incognito_mode_availability_prefs.h:29: }  // namespace
IncognitoModeAvailability
On 2011/07/28 11:08:50, Mattias Nissler wrote:
> incorrect name.

Done.

http://codereview.chromium.org/7520023/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7520023/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:2259:
prefs->RegisterIntegerPref(prefs::kIncognitoModeAvailability,
On 2011/07/28 11:08:50, Mattias Nissler wrote:
> As mentioned, this should be moved to your helper file.

Done.

[Bernhard Bauer, 2011-07-29 09:11:46]

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.h (right):

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.h:44: IncognitoModePrefs();
DISALLOW_IMPLICIT_CONSTRUCTORS? And I think there is no need for the destructor.

[Mattias Nissler (ping if slow), 2011-07-29 10:33:22]

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:206:
prefs::kIncognitoEnabled },
This should go away. In fact, the entire prefs::kIncognitoEnabled pref should go
away. In ApplyIncognitoModePolicy(), check whether you see the old value and
record it in a member variable. Later, in FinalizeIncognitoModeSettings(), check
whether you have the new-style policy. If so, don't do anything. Otherwise,
check the old policy value and set the correct enum choice in the new-style
pref.

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:14: DCHECK(out_value);
remove this, we'll crash anyway if out_value is NULL.

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs_unittest.cc:9: #include
"chrome/common/pref_names.h"
alphabetize

[Mattias Nissler (ping if slow), 2011-07-29 11:29:08]

Question for Pawel

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs_unittest.cc:62: EXPECT_DEBUG_DEATH({
Pawel, what's the current policy on death tests? I remember there was a
discussion, but do not remember the results and cannot find clear direction from
the chromium-dev archives.

[rustema, 2011-07-30 06:17:37]

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:206:
prefs::kIncognitoEnabled },
On 2011/07/29 10:33:23, Mattias Nissler wrote:
> This should go away. In fact, the entire prefs::kIncognitoEnabled pref should
go
> away. In ApplyIncognitoModePolicy(), check whether you see the old value and
> record it in a member variable. Later, in FinalizeIncognitoModeSettings(),
check
> whether you have the new-style policy. If so, don't do anything. Otherwise,
> check the old policy value and set the correct enum choice in the new-style
> pref.

Yep, the code has become much cleaner this way. Thanks!

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:14: DCHECK(out_value);
On 2011/07/29 10:33:23, Mattias Nissler wrote:
> remove this, we'll crash anyway if out_value is NULL.

Done.

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.h (right):

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.h:44: IncognitoModePrefs();
On 2011/07/29 09:11:46, Bernhard Bauer wrote:
> DISALLOW_IMPLICIT_CONSTRUCTORS? And I think there is no need for the
destructor.

Done.

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/5001/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs_unittest.cc:9: #include
"chrome/common/pref_names.h"
On 2011/07/29 10:33:23, Mattias Nissler wrote:
> alphabetize

Done.

[Mattias Nissler (ping if slow), 2011-08-01 11:13:40]

A couple more issues with the policy->prefs conversion.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_apitest.cc (right):

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_apitest.cc:204:
browser()->profile()->GetPrefs()->SetInteger(
Can we also do the Set operation through a helper in IncognitoModePrefs? That
way it would be symmetric and the fact that we're using integer as the type is
hidden away from client code.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:150: // Saved state of
the deprecated kPolicyIncognitoEnabled. If is still used for
s/If/It/

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:592: if (policy ==
kPolicyIncognitoEnabled) {
else if

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:688: if
(!deprecated_incognito_enabled_) {
It's not correct to treat the cases "not set" and "set to true" the same. If the
legacy policy is set to true, you should set the pref below, but you don't. This
is important s.t. client code asking IsManaged() on the pref will get the right
value.

To accomplish this, I suggest you keep a scoped_ptr<Value> to save the value of
the deprecated policy in instead of the  boolean flag you have at the moment.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:707: }
newline

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:712:
EXPECT_TRUE(Value::CreateIntegerValue(availability)->Equals(value));
You are leaking the created Value here. Just use
FundamentalValue(availability).Equals(value)

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:713: }
newline

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:720: // from
policy to pref "as is".
Shouldn't it rather check that if the new-style policy is set, the old value
gets ignored?

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:743:
EXPECT_EQ(PrefStore::READ_NO_VALUE,
In this case, it should return READ_OK with IncognitoModePrefs::ENABLED.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:749: //
originally was not specified.
grammar

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:769:
DefaultObsoletePolicyDisablesAvailabilityWhenLatterNotSpecified) {
If the policy isn't specified correctly, shouldn't we rather not set the pref at
all?

[rustema, 2011-08-01 23:55:33]

Btw, is there a lint (style issue discovery) tool?

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_apitest.cc (right):

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_apitest.cc:204:
browser()->profile()->GetPrefs()->SetInteger(
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> Can we also do the Set operation through a helper in IncognitoModePrefs? That
> way it would be symmetric and the fact that we're using integer as the type is
> hidden away from client code.

Done.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:150: // Saved state of
the deprecated kPolicyIncognitoEnabled. If is still used for
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> s/If/It/

Done.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:592: if (policy ==
kPolicyIncognitoEnabled) {
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> else if

But there's a return statement in the previous if block. I thought having if
blocks with return statements was cleaner. Do you insist that I change it to
"else if"?

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:688: if
(!deprecated_incognito_enabled_) {
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> It's not correct to treat the cases "not set" and "set to true" the same. If
the
> legacy policy is set to true, you should set the pref below, but you don't.
This
> is important s.t. client code asking IsManaged() on the pref will get the
right
> value.
> 
> To accomplish this, I suggest you keep a scoped_ptr<Value> to save the value
of
> the deprecated policy in instead of the  boolean flag you have at the moment.

Done.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:707: }
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:712:
EXPECT_TRUE(Value::CreateIntegerValue(availability)->Equals(value));
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> You are leaking the created Value here. Just use
> FundamentalValue(availability).Equals(value)

Nuts! Chrome tests do not link in heap checker?!

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:713: }
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:720: // from
policy to pref "as is".
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> Shouldn't it rather check that if the new-style policy is set, the old value
> gets ignored?

Updated.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:743:
EXPECT_EQ(PrefStore::READ_NO_VALUE,
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> In this case, it should return READ_OK with IncognitoModePrefs::ENABLED.

Nope, -1 here means no IncognitoModeAvailability specified. I redefined it as a
constant.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:749: //
originally was not specified.
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> grammar

Updated.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:769:
DefaultObsoletePolicyDisablesAvailabilityWhenLatterNotSpecified) {
On 2011/08/01 11:13:40, Mattias Nissler wrote:
> If the policy isn't specified correctly, shouldn't we rather not set the pref
at
> all?

-1 means "not specified".

[Mattias Nissler (ping if slow), 2011-08-03 09:16:07]

Sorry for the delay. Almost there!

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:592: if (policy ==
kPolicyIncognitoEnabled) {
On 2011/08/01 23:55:33, rustema wrote:
> On 2011/08/01 11:13:40, Mattias Nissler wrote:
> > else if
> 
> But there's a return statement in the previous if block. I thought having if
> blocks with return statements was cleaner. Do you insist that I change it to
> "else if"?

Yes, right, sorry for the noise.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:712:
EXPECT_TRUE(Value::CreateIntegerValue(availability)->Equals(value));
On 2011/08/01 23:55:33, rustema wrote:
> On 2011/08/01 11:13:40, Mattias Nissler wrote:
> > You are leaking the created Value here. Just use
> > FundamentalValue(availability).Equals(value)
> 
> Nuts! Chrome tests do not link in heap checker?!

We do have valgrind and the tcmalloc heap checker. They run on the continuous
build and for local testing, see tools/heapcheck/chrome_tests.sh and
tools/valgrind/chrome_tests.sh

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:743:
EXPECT_EQ(PrefStore::READ_NO_VALUE,
On 2011/08/01 23:55:33, rustema wrote:
> On 2011/08/01 11:13:40, Mattias Nissler wrote:
> > In this case, it should return READ_OK with IncognitoModePrefs::ENABLED.
> 
> Nope, -1 here means no IncognitoModeAvailability specified. I redefined it as
a
> constant.

I was actually referring to the fact that you set kPolicyIncognitoEnabled to
true. That should reflect as READ_OK with IncognitoModePrefs::ENABLED.

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:769:
DefaultObsoletePolicyDisablesAvailabilityWhenLatterNotSpecified) {
On 2011/08/01 23:55:33, rustema wrote:
> On 2011/08/01 11:13:40, Mattias Nissler wrote:
> > If the policy isn't specified correctly, shouldn't we rather not set the
pref
> at
> > all?
> 
> -1 means "not specified".

Right, sorry for the confusion.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:586: DCHECK(result);
I think if the conversion failed, we should just ignore the policy, since it's
invalid instead of defaulting to ENABLED.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:688: DCHECK(result);
Same here. If the check fails, just don't set the pref. Note that this can
actually happen in the wild if admins put invalid values into their
configuration e.g. windows registry.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:715: if
(availability >= 0)
need curlies.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
chrome/browser/prefs/incognito_mode_prefs.cc:13: int in_value, Availability*
out_value) {
Each parameter goes on a separate line (and then they may fit right of the
opening parenthesis in line 12)?

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
File chrome/browser/prefs/incognito_mode_prefs.h (right):

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
chrome/browser/prefs/incognito_mode_prefs.h:41: static void
SetAvailability(const Availability availability,
Nit: I'd expect the arguments in reverse order. I'll leave this decision to you
though.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
File chrome/browser/prefs/incognito_mode_prefs_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
chrome/browser/prefs/incognito_mode_prefs_unittest.cc:18:
scoped_ptr<TestingPrefService> prefs_;
You can just use a regular member, no need to wrap in a scoped_ptr.

[rustema, 2011-08-04 07:00:49]

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:712:
EXPECT_TRUE(Value::CreateIntegerValue(availability)->Equals(value));
On 2011/08/03 09:16:08, Mattias Nissler wrote:
> On 2011/08/01 23:55:33, rustema wrote:
> > On 2011/08/01 11:13:40, Mattias Nissler wrote:
> > > You are leaking the created Value here. Just use
> > > FundamentalValue(availability).Equals(value)
> > 
> > Nuts! Chrome tests do not link in heap checker?!
> 
> We do have valgrind and the tcmalloc heap checker. They run on the continuous
> build and for local testing, see tools/heapcheck/chrome_tests.sh and
> tools/valgrind/chrome_tests.sh

Cool! Thanks for the pointers!

http://codereview.chromium.org/7520023/diff/16001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:743:
EXPECT_EQ(PrefStore::READ_NO_VALUE,
On 2011/08/03 09:16:08, Mattias Nissler wrote:
> On 2011/08/01 23:55:33, rustema wrote:
> > On 2011/08/01 11:13:40, Mattias Nissler wrote:
> > > In this case, it should return READ_OK with IncognitoModePrefs::ENABLED.
> > 
> > Nope, -1 here means no IncognitoModeAvailability specified. I redefined it
as
> a
> > constant.
> 
> I was actually referring to the fact that you set kPolicyIncognitoEnabled to
> true. That should reflect as READ_OK with IncognitoModePrefs::ENABLED.

Yep, ObsoletePolicySetsPreferenceToEnabled verifies that.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:586: DCHECK(result);
On 2011/08/03 09:16:08, Mattias Nissler wrote:
> I think if the conversion failed, we should just ignore the policy, since it's
> invalid instead of defaulting to ENABLED.

Done.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:688: DCHECK(result);
On 2011/08/03 09:16:08, Mattias Nissler wrote:
> Same here. If the check fails, just don't set the pref. Note that this can
> actually happen in the wild if admins put invalid values into their
> configuration e.g. windows registry.

Done.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:715: if
(availability >= 0)
On 2011/08/03 09:16:08, Mattias Nissler wrote:
> need curlies.

Done.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
chrome/browser/prefs/incognito_mode_prefs.cc:13: int in_value, Availability*
out_value) {
On 2011/08/03 09:16:08, Mattias Nissler wrote:
> Each parameter goes on a separate line (and then they may fit right of the
> opening parenthesis in line 12)?

Done.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
File chrome/browser/prefs/incognito_mode_prefs.h (right):

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
chrome/browser/prefs/incognito_mode_prefs.h:41: static void
SetAvailability(const Availability availability,
On 2011/08/03 09:16:08, Mattias Nissler wrote:
> Nit: I'd expect the arguments in reverse order. I'll leave this decision to
you
> though.

Done.

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
File chrome/browser/prefs/incognito_mode_prefs_unittest.cc (right):

http://codereview.chromium.org/7520023/diff/27001/chrome/browser/prefs/incogn...
chrome/browser/prefs/incognito_mode_prefs_unittest.cc:18:
scoped_ptr<TestingPrefService> prefs_;
On 2011/08/03 09:16:08, Mattias Nissler wrote:
> You can just use a regular member, no need to wrap in a scoped_ptr.

Done.

[Mattias Nissler (ping if slow), 2011-08-04 09:03:53]

LGTM with a nit.

http://codereview.chromium.org/7520023/diff/33001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/33001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:695: // If the obsolete
policy says that incognito is disabled,
nit: Seems this comment fell through the cracks. Just remove it?

[rustema, 2011-08-04 17:58:07]

Thanks for review! Submitting.

http://codereview.chromium.org/7520023/diff/33001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/7520023/diff/33001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:695: // If the obsolete
policy says that incognito is disabled,
On 2011/08/04 09:03:53, Mattias Nissler wrote:
> nit: Seems this comment fell through the cracks. Just remove it?

Done.

[commit-bot: I haz the power, 2011-08-05 00:41:47]

Presubmit check for 7520023-39001 failed and returned exit status 1.

Running presubmit commit checks ...
Finished checking
/mnt/data/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json.
0 errors, 0 warnings.

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/profiles/profile.cc

Presubmit checks took 1.7s to calculate.

[rustema, 2011-08-05 00:48:18]

Hi guys,

I need your approval on chrome/browser/profiles/profile.cc.
Can you please take a look?

Thanks,
Rustem

[sail, 2011-08-05 00:49:46]

LGTM

[commit-bot: I haz the power, 2011-08-05 05:28:33]

Change committed as 95580