Implement pref policy for disabling incognito mode.

Implement pref policy for disabling incognito mode.

BUG=66413
TEST=None


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

[danno, 2011-01-24 12:09:10]

here you go...

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

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:209: 'name': 'IncognitoEnabled',
I'm wondering if this is one of those policies where the polarity should be
reversed (like disabling Sync) and should be called "IncognitoDisabled". That
seems to be the pattern we've de facto adopted for policies that could be
phrased as "XXXXEnable" but for which enabling them would have no effect.

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:212: 'features': {'dynamic_refresh': 0},
You use a BooleanPrefMember? That's should properly respond to preference
changes, so can't this be a "1"?

[Finnur, 2011-01-24 13:14:32]

Thanks. Replies:

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

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:209: 'name': 'IncognitoEnabled',
Well, I think we are a bit inconsistent here as we use JavascriptEnabled, for
example, and leaving it default means it is enabled. Personally, I've been
bitten using the term  fooDisabled (in a previous life when designing settings
pages) due to double-negative confusion of when setting them to false. You don't
have that problem with fooEnabled.

On 2011/01/24 12:09:10, danno wrote:
> I'm wondering if this is one of those policies where the polarity should be
> reversed (like disabling Sync) and should be called "IncognitoDisabled". That
> seems to be the pattern we've de facto adopted for policies that could be
> phrased as "XXXXEnable" but for which enabling them would have no effect.

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:212: 'features': {'dynamic_refresh': 0},
Ah, I'm glad you caught this. I meant to ask you about this, as I'm not sure
what this does and it is not well documented at the top of this file nor in the
checkin/associated bug.

But I have flipped the bit here.


On 2011/01/24 12:09:10, danno wrote:
> You use a BooleanPrefMember? That's should properly respond to preference
> changes, so can't this be a "1"?

[danno, 2011-01-24 14:45:42]

LGTM, if you address the two new comments. I leave the Enabled/Disable call up
to you.

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

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:209: 'name': 'IncognitoEnabled',
I don't feel that strongly about it, but . Maybe you might want to ask Mattias
for his opinion.

On 2011/01/24 13:14:32, Finnur wrote:
> Well, I think we are a bit inconsistent here as we use JavascriptEnabled, for
> example, and leaving it default means it is enabled. Personally, I've been
> bitten using the term  fooDisabled (in a previous life when designing settings
> pages) due to double-negative confusion of when setting them to false. You
don't
> have that problem with fooEnabled.
> 
> On 2011/01/24 12:09:10, danno wrote:
> > I'm wondering if this is one of those policies where the polarity should be
> > reversed (like disabling Sync) and should be called "IncognitoDisabled".
That
> > seems to be the pattern we've de facto adopted for policies that could be
> > phrased as "XXXXEnable" but for which enabling them would have no effect.
>

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:211: 'supported_on': ['chrome.win:11-'],
why only on windows? otherwise you need chrome.* BTW, if you get this committed
today, won't it also be in Chrome 10? e.g.:
chrome.*:10-

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:213: 'example_value': True,
Example value should be False, since True doesn't actually do anything.

[Finnur, 2011-01-24 18:43:10]

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

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:209: 'name': 'IncognitoEnabled',
Who? Nissler?

On 2011/01/24 14:45:42, danno wrote:
> I don't feel that strongly about it, but . Maybe you might want to ask Mattias
> for his opinion.
> 
> On 2011/01/24 13:14:32, Finnur wrote:
> > Well, I think we are a bit inconsistent here as we use JavascriptEnabled,
for
> > example, and leaving it default means it is enabled. Personally, I've been
> > bitten using the term  fooDisabled (in a previous life when designing
settings
> > pages) due to double-negative confusion of when setting them to false. You
> don't
> > have that problem with fooEnabled.
> > 
> > On 2011/01/24 12:09:10, danno wrote:
> > > I'm wondering if this is one of those policies where the polarity should
be
> > > reversed (like disabling Sync) and should be called "IncognitoDisabled".
> That
> > > seems to be the pattern we've de facto adopted for policies that could be
> > > phrased as "XXXXEnable" but for which enabling them would have no effect.
> > 
>

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:211: 'supported_on': ['chrome.win:11-'],
Because I don't run any OS' other than Windows at the moment so I can't test any
changes I would make. My Linux box is dead in the water and I've never had a
Mac.

As for v10 or 11, I didn't want to rush it for 10, so I put 11. 


On 2011/01/24 14:45:42, danno wrote:
> why only on windows? otherwise you need chrome.* BTW, if you get this
committed
> today, won't it also be in Chrome 10? e.g.:
> chrome.*:10-

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:213: 'example_value': True,
Done.

On 2011/01/24 14:45:42, danno wrote:
> Example value should be False, since True doesn't actually do anything.

[Mattias Nissler (ping if slow), 2011-01-24 18:47:27]

Will look at it tomorrow.
On Jan 24, 2011 7:43 PM, <finnur@chromium.org> wrote:
>
>
http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
> File chrome/app/policy/policy_templates.json (right):
>
>
http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
> chrome/app/policy/policy_templates.json:209: 'name': 'IncognitoEnabled',
> Who? Nissler?
>
> On 2011/01/24 14:45:42, danno wrote:
>> I don't feel that strongly about it, but . Maybe you might want to ask
> Mattias
>> for his opinion.
>
>> On 2011/01/24 13:14:32, Finnur wrote:
>> > Well, I think we are a bit inconsistent here as we use
> JavascriptEnabled, for
>> > example, and leaving it default means it is enabled. Personally,
> I've been
>> > bitten using the term fooDisabled (in a previous life when
> designing settings
>> > pages) due to double-negative confusion of when setting them to
> false. You
>> don't
>> > have that problem with fooEnabled.
>> >
>> > On 2011/01/24 12:09:10, danno wrote:
>> > > I'm wondering if this is one of those policies where the polarity
> should be
>> > > reversed (like disabling Sync) and should be called
> "IncognitoDisabled".
>> That
>> > > seems to be the pattern we've de facto adopted for policies that
> could be
>> > > phrased as "XXXXEnable" but for which enabling them would have no
> effect.
>> >
>
>
>
http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
> chrome/app/policy/policy_templates.json:211: 'supported_on':
> ['chrome.win:11-'],
> Because I don't run any OS' other than Windows at the moment so I can't
> test any changes I would make. My Linux box is dead in the water and
> I've never had a Mac.
>
> As for v10 or 11, I didn't want to rush it for 10, so I put 11.
>
>
> On 2011/01/24 14:45:42, danno wrote:
>> why only on windows? otherwise you need chrome.* BTW, if you get this
> committed
>> today, won't it also be in Chrome 10? e.g.:
>> chrome.*:10-
>
>
http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
> chrome/app/policy/policy_templates.json:213: 'example_value': True,
> Done.
>
> On 2011/01/24 14:45:42, danno wrote:
>> Example value should be False, since True doesn't actually do
> anything.
>
> http://codereview.chromium.org/6313008/

[Mattias Nissler (ping if slow), 2011-01-25 09:33:43]

LGTM with some comments.

Can you also add the parameterized test case instantiations for your new policy
in configuration_policy_provider_{win|mac}_unittest.cc,
config_dir_policy_provider_unittest.cc and
configuration_policy_pref_store_unittest.cc?

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

http://codereview.chromium.org/6313008/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:209: 'name': 'IncognitoEnabled',
On 2011/01/24 18:43:10, Finnur wrote:
> Who? Nissler?
> 
> On 2011/01/24 14:45:42, danno wrote:
> > I don't feel that strongly about it, but . Maybe you might want to ask
Mattias
> > for his opinion.
> > 
> > On 2011/01/24 13:14:32, Finnur wrote:
> > > Well, I think we are a bit inconsistent here as we use JavascriptEnabled,
> for
> > > example, and leaving it default means it is enabled. Personally, I've been
> > > bitten using the term  fooDisabled (in a previous life when designing
> settings
> > > pages) due to double-negative confusion of when setting them to false. You
> > don't
> > > have that problem with fooEnabled.
> > > 
> > > On 2011/01/24 12:09:10, danno wrote:
> > > > I'm wondering if this is one of those policies where the polarity should
> be
> > > > reversed (like disabling Sync) and should be called "IncognitoDisabled".
> > That
> > > > seems to be the pattern we've de facto adopted for policies that could
be
> > > > phrased as "XXXXEnable" but for which enabling them would have no
effect.
> > > 
> > 
> 

I'm OK with both. For some bits we've just mimicked what the command line and/or
prefs do, otherwise I've loosely followed the rule to name it according to what
admins would intend when they set it, i.e. the main use case.

http://codereview.chromium.org/6313008/diff/13001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:211: 'supported_on': ['chrome.win:11-'],
what about mac and linux?

[Finnur, 2011-01-25 15:28:03]

> Can you also add the parameterized test case instantiations 

Done. Can you take a quick look and see if they are as you expect?

> what about mac and linux?

My Linux box is out of commission and I've never had a Mac.

[Mattias Nissler (ping if slow), 2011-01-25 15:54:23]

On 2011/01/25 15:28:03, Finnur wrote:
> > Can you also add the parameterized test case instantiations 
> 
> Done. Can you take a quick look and see if they are as you expect?

They are, thanks, but Mac and Linux are not covered (see below).

> 
> > what about mac and linux?
> 
> My Linux box is out of commission and I've never had a Mac.

I see. Typically other platforms will work out of the box without additional
work (assuming we don't have custom incognito triggers for other platforms), so
I can offer you just commit it with the Linux and Mac bits enabled and once it's
in I'll verify (and take care of anything that doesn't work). Or if you're not
comfortable with that, let's file a bug about enabling support on Mac and Linux.

[Finnur, 2011-01-25 16:31:13]

No, looking at this again I think you are probably right. I will change the json
to allow Linux and Mac and take you up on the offer to do additional testing.
This is, after all, for M11 (and doesn't take effect untill you flip the bit) so
we should be good.

I'll run it through the try server once more, just for good measure and then
check in if green.

On 2011/01/25 15:54:23, Mattias Nissler wrote:
> On 2011/01/25 15:28:03, Finnur wrote:
> > > Can you also add the parameterized test case instantiations 
> > 
> > Done. Can you take a quick look and see if they are as you expect?
> 
> They are, thanks, but Mac and Linux are not covered (see below).
> 
> > 
> > > what about mac and linux?
> > 
> > My Linux box is out of commission and I've never had a Mac.
> 
> I see. Typically other platforms will work out of the box without additional
> work (assuming we don't have custom incognito triggers for other platforms),
so
> I can offer you just commit it with the Linux and Mac bits enabled and once
it's
> in I'll verify (and take care of anything that doesn't work). Or if you're not
> comfortable with that, let's file a bug about enabling support on Mac and
Linux.

[Mattias Nissler (ping if slow), 2011-01-25 16:35:28]

LGTM

On 2011/01/25 16:31:13, Finnur wrote:
> No, looking at this again I think you are probably right. I will change the
json
> to allow Linux and Mac and take you up on the offer to do additional testing.
> This is, after all, for M11 (and doesn't take effect untill you flip the bit)
so
> we should be good.
> 
> I'll run it through the try server once more, just for good measure and then
> check in if green.
> 
> On 2011/01/25 15:54:23, Mattias Nissler wrote:
> > On 2011/01/25 15:28:03, Finnur wrote:
> > > > Can you also add the parameterized test case instantiations 
> > > 
> > > Done. Can you take a quick look and see if they are as you expect?
> > 
> > They are, thanks, but Mac and Linux are not covered (see below).
> > 
> > > 
> > > > what about mac and linux?
> > > 
> > > My Linux box is out of commission and I've never had a Mac.
> > 
> > I see. Typically other platforms will work out of the box without additional
> > work (assuming we don't have custom incognito triggers for other platforms),
> so
> > I can offer you just commit it with the Linux and Mac bits enabled and once
> it's
> > in I'll verify (and take care of anything that doesn't work). Or if you're
not
> > comfortable with that, let's file a bug about enabling support on Mac and
> Linux.

[Finnur, 2011-01-26 13:23:38]

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

http://codereview.chromium.org/6313008/diff/44001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:229: profile_->GetPrefs(), this);
Daniel/Matthias: I checked this in after the try servers went green and the tree
lit up light a Christmas tree due to crashes in tests (a NOTREACHED).

Then I commented out everything in this CL except the pref definition consts and
left these two lines above in but it still crashes. If I comment these two lines
out, then the tests pass.

The NOTREACHED I'm seeing is in
BookmarkBarView::BookmarkModelBeingDeleted

  // In normal shutdown The bookmark model should never be deleted before us.
  // When X exits suddenly though, it can happen, This code exists
  // to check for regressions in shutdown code and not crash.
  if (!browser_shutdown::ShuttingDownWithoutClosingBrowsers())
    NOTREACHED();

The thing is that ShuttingDownWithoutClosingBrowsers() always returns false,
except on certain scenarios on Linux. 

This makes no sense... Have you guys seen anything like this? Why is adding one
more Init pref call here so disastrous??

[Finnur, 2011-01-26 13:44:35]

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

http://codereview.chromium.org/6313008/diff/44001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:229: profile_->GetPrefs(), this);
Hmm... I believe this is because of a missing Destroy() for the pref I Init...

On 2011/01/26 13:23:38, Finnur wrote:
> Daniel/Matthias: I checked this in after the try servers went green and the
tree
> lit up light a Christmas tree due to crashes in tests (a NOTREACHED).
> 
> Then I commented out everything in this CL except the pref definition consts
and
> left these two lines above in but it still crashes. If I comment these two
lines
> out, then the tests pass.
> 
> The NOTREACHED I'm seeing is in
> BookmarkBarView::BookmarkModelBeingDeleted
> 
>   // In normal shutdown The bookmark model should never be deleted before us.
>   // When X exits suddenly though, it can happen, This code exists
>   // to check for regressions in shutdown code and not crash.
>   if (!browser_shutdown::ShuttingDownWithoutClosingBrowsers())
>     NOTREACHED();
> 
> The thing is that ShuttingDownWithoutClosingBrowsers() always returns false,
> except on certain scenarios on Linux. 
> 
> This makes no sense... Have you guys seen anything like this? Why is adding
one
> more Init pref call here so disastrous??