Prevent incognito windows from opening when incognito mode is disabled.

Prevent incognito windows from opening when incognito mode is disabled.
Prevent normal windows from opening when incognito mode is forced.


BUG=97677
TEST=Added a test to chrome/browser/extensions/extension_tabs_test.cc.

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

[Mattias Nissler (ping if slow), 2011-10-24 09:20:45]

policy-wise LGTM, but please also get an LGTM from a person that knows the
extension side of things.

[Finnur, 2011-10-24 10:08:07]

Extension side LGTM. You need to fill in the TEST= line in the changelist
description.

Question: There are other places in the product where you can open incognito
windows (ie. right-clicking links). Is this believed to be the only place where
this is a problem?

http://codereview.chromium.org/8373027/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8373027/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_tabs_module.cc:390: const
IncognitoModePrefs::Availability incognito_avail =
nit: Please don't abbreviate the variable names.

[Aaron Boodman, 2011-10-24 16:03:47]

Where is this enforced normally (e.g., for the UI?) Maybe our code should call
through there so that we don't duplicate this check? It seems weird to be
checking UI policy way up here in the extension API.

In any case, you need a test. Please see extension_tabs_test.cc.

[rustema, 2011-10-25 03:35:17]

Test added.

Incognito windows can be opened in multiple places: right-clicking, opening new
empty windows, typing certain urls (e.g. chrome://settings), etc. The original
approach was to disallow these actions (e.g. disable menu items etc.) in order
not to make users confused. Here's the original CL that introduced the policy:
http://codereview.chromium.org/7520023
Basically, there's no single place where enforcement of incognito happens. We
could potentially add such a check into Browser::Create* methods and make these
methods return NULL when the profile conflicts with the policy. That sounds like
a big refactoring, though.

http://codereview.chromium.org/8373027/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8373027/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_tabs_module.cc:390: const
IncognitoModePrefs::Availability incognito_avail =
On 2011/10/24 10:08:08, Finnur wrote:
> nit: Please don't abbreviate the variable names.

Done.

[rustema, 2011-10-25 03:44:02]

On 2011/10/24 16:03:47, Aaron Boodman wrote:
> Where is this enforced normally (e.g., for the UI?) Maybe our code should call
> through there so that we don't duplicate this check? It seems weird to be
> checking UI policy way up here in the extension API.
> 
> In any case, you need a test. Please see extension_tabs_test.cc.

I have actually added a test into extension_tabs_apitest.cc. Is
extension_tabs_test.cc more preferable?

[Aaron Boodman, 2011-10-25 03:53:42]

On 2011/10/25 03:44:02, rustema wrote:
> On 2011/10/24 16:03:47, Aaron Boodman wrote:
> > Where is this enforced normally (e.g., for the UI?) Maybe our code should
call
> > through there so that we don't duplicate this check? It seems weird to be
> > checking UI policy way up here in the extension API.
> > 
> > In any case, you need a test. Please see extension_tabs_test.cc.
> 
> I have actually added a test into extension_tabs_apitest.cc. Is
> extension_tabs_test.cc more preferable?

Yes, it is. It should be a lot more compact that way.

[Aaron Boodman, 2011-10-25 03:56:15]

On 2011/10/25 03:53:42, Aaron Boodman wrote:
> On 2011/10/25 03:44:02, rustema wrote:
> > On 2011/10/24 16:03:47, Aaron Boodman wrote:
> > > Where is this enforced normally (e.g., for the UI?) Maybe our code should
> call
> > > through there so that we don't duplicate this check? It seems weird to be
> > > checking UI policy way up here in the extension API.
> > > 
> > > In any case, you need a test. Please see extension_tabs_test.cc.
> > 
> > I have actually added a test into extension_tabs_apitest.cc. Is
> > extension_tabs_test.cc more preferable?
> 
> Yes, it is. It should be a lot more compact that way.

While you are in there, can you add this note to the bottom of
extension_tabs_apitest.cc, so that others don't make the same mistake:

// Adding a new test? Awesome. But API tests are the old hotness. The
// new hotness is extension_test_utils. See extension_tabs_test.cc for
// an example. We are trying to phase out many uses of API tests as 
// they tend to be flakey.

[rustema, 2011-10-25 08:09:11]

On 2011/10/25 03:56:15, Aaron Boodman wrote:
> On 2011/10/25 03:53:42, Aaron Boodman wrote:
> > On 2011/10/25 03:44:02, rustema wrote:
> > > On 2011/10/24 16:03:47, Aaron Boodman wrote:
> > > > Where is this enforced normally (e.g., for the UI?) Maybe our code
should
> > call
> > > > through there so that we don't duplicate this check? It seems weird to
be
> > > > checking UI policy way up here in the extension API.
> > > > 
> > > > In any case, you need a test. Please see extension_tabs_test.cc.
> > > 
> > > I have actually added a test into extension_tabs_apitest.cc. Is
> > > extension_tabs_test.cc more preferable?
> > 
> > Yes, it is. It should be a lot more compact that way.
> 
> While you are in there, can you add this note to the bottom of
> extension_tabs_apitest.cc, so that others don't make the same mistake:
> 
> // Adding a new test? Awesome. But API tests are the old hotness. The
> // new hotness is extension_test_utils. See extension_tabs_test.cc for
> // an example. We are trying to phase out many uses of API tests as 
> // they tend to be flakey.

All done. I guess I can also delete ExtensionApiTest.IncognitoDisabledByPref in
chrome/browser/extensions/extension_tabs_apitest.cc since I added its equivalent
to extension_tabs_test.cc?

[Aaron Boodman, 2011-10-25 08:24:12]

On 2011/10/25 08:09:11, rustema wrote:
> On 2011/10/25 03:56:15, Aaron Boodman wrote:
> > On 2011/10/25 03:53:42, Aaron Boodman wrote:
> > > On 2011/10/25 03:44:02, rustema wrote:
> > > > On 2011/10/24 16:03:47, Aaron Boodman wrote:
> > > > > Where is this enforced normally (e.g., for the UI?) Maybe our code
> should
> > > call
> > > > > through there so that we don't duplicate this check? It seems weird to
> be
> > > > > checking UI policy way up here in the extension API.
> > > > > 
> > > > > In any case, you need a test. Please see extension_tabs_test.cc.
> > > > 
> > > > I have actually added a test into extension_tabs_apitest.cc. Is
> > > > extension_tabs_test.cc more preferable?
> > > 
> > > Yes, it is. It should be a lot more compact that way.
> > 
> > While you are in there, can you add this note to the bottom of
> > extension_tabs_apitest.cc, so that others don't make the same mistake:
> > 
> > // Adding a new test? Awesome. But API tests are the old hotness. The
> > // new hotness is extension_test_utils. See extension_tabs_test.cc for
> > // an example. We are trying to phase out many uses of API tests as 
> > // they tend to be flakey.
> 
> All done. I guess I can also delete ExtensionApiTest.IncognitoDisabledByPref
in
> chrome/browser/extensions/extension_tabs_apitest.cc since I added its
equivalent
> to extension_tabs_test.cc?

Yes.

[Aaron Boodman, 2011-10-25 08:35:09]

Looking at this again, if it is important that this policy be followed you
should really pull the checks and defaulting down into Browser.

Otherwise it will be too easy for someone moving code in the higher layers to
not know they are supposed to check this preference.

http://codereview.chromium.org/8373027/diff/5005/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_test.cc (right):

http://codereview.chromium.org/8373027/diff/5005/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_test.cc:145:
kArgsWithoutExplicitIncognitoParam,
In this case, shouldn't we default to incognito? Seems like most extension
developers won't be aware of this case and won't code to it. So without a
sensible default, you're essentially guaranteeing that any extension that calls
window.create() won't work.

[Mattias Nissler (ping if slow), 2011-10-25 08:56:04]

On 2011/10/25 08:35:09, Aaron Boodman wrote:
> Looking at this again, if it is important that this policy be followed you
> should really pull the checks and defaulting down into Browser.
> 
> Otherwise it will be too easy for someone moving code in the higher layers to
> not know they are supposed to check this preference.

I agree with Aaron, but let me add some additional perspective on what we've
done in other situations like this:
- In a central place, put in a policy check that will make absolutely sure the
feature you're disabling is actually disabled. Usually, that place in the code
is to deep inside a subsystem to allow for good UX should the check fail.
- To get better UX, write code to disable UI elements that trigger the feature.
As Aaron said, it's usually not a good idea to rely on these UI-level checks to
actually enforce a policy.

[rustema, 2011-10-26 07:13:41]

I agree that enforcing the policy at the Browser level is needed. I'm going to
prepare a follow-up CL in which I will modify Browser factory methods to perform
necessary checks.

http://codereview.chromium.org/8373027/diff/5005/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_test.cc (right):

http://codereview.chromium.org/8373027/diff/5005/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_test.cc:145:
kArgsWithoutExplicitIncognitoParam,
On 2011/10/25 08:35:09, Aaron Boodman wrote:
> In this case, shouldn't we default to incognito? Seems like most extension
> developers won't be aware of this case and won't code to it. So without a
> sensible default, you're essentially guaranteeing that any extension that
calls
> window.create() won't work.

Makes sense. Done.

[pastarmovj, 2011-10-27 09:14:31]

Seems like most has been said for this CL already. I only got  two small nits on
the comments. 
I am good with this CL as long as Mattias and Aaron give you green light.

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

http://codereview.chromium.org/8373027/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_apitest.cc:229: // they tend to be
flakey.
Isn't it flaky instead of flakey? :)

http://codereview.chromium.org/8373027/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8373027/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:406: // incognito.
maybe extend this sentence to include "...when forced so by policy". Because it
sounds like this is the default always to me. The code does the right thing but
only by reading the comments one gets the wrong idea.

[Joao da Silva, 2011-10-28 11:05:29]

http://codereview.chromium.org/8373027/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8373027/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:390: const
IncognitoModePrefs::Availability incognito_availability =
These checks are scoped inside an "if (args)" block; but should't the incognito
availability checks be done even if there are no args?

I'd move this block, from here down to the "window_profile = .." line, to
outside the "if (args)" block. The line that checks the args here could be
rewritten as "if (args && args->HasKey(...))".

[rustema, 2011-10-29 06:43:20]

Thanks for comments and suggestions, guys!
Aaron, can you please take another look?

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

http://codereview.chromium.org/8373027/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_apitest.cc:229: // they tend to be
flakey.
On 2011/10/27 09:14:31, pastarmovj wrote:
> Isn't it flaky instead of flakey? :)

Yeah, that may be confusing. Changed.

http://codereview.chromium.org/8373027/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8373027/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:390: const
IncognitoModePrefs::Availability incognito_availability =
On 2011/10/28 11:05:29, Joao da Silva wrote:
> These checks are scoped inside an "if (args)" block; but should't the
incognito
> availability checks be done even if there are no args?
> 
> I'd move this block, from here down to the "window_profile = .." line, to
> outside the "if (args)" block. The line that checks the args here could be
> rewritten as "if (args && args->HasKey(...))".

Done.

http://codereview.chromium.org/8373027/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:406: // incognito.
On 2011/10/27 09:14:31, pastarmovj wrote:
> maybe extend this sentence to include "...when forced so by policy". Because
it
> sounds like this is the default always to me. The code does the right thing
but
> only by reading the comments one gets the wrong idea.

Done.

[Aaron Boodman, 2011-10-29 20:29:00]

lgtm

You don't need to wait for another review from me, but factoring this out a
little better would be nice.

http://codereview.chromium.org/8373027/diff/19001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8373027/diff/19001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:355: // Decide whether we are
opening a normal window or an incognito window.
This function is getting a little long. Could you pull these lines out into a
GetIncognitoMode() helper method?

If you see anything else that can be easily pulled out at the same time, that
would also be nice.

[rustema, 2011-10-31 07:59:57]

Thanks for the review everyone! Will submit tomorrow morning.

http://codereview.chromium.org/8373027/diff/19001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8373027/diff/19001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:355: // Decide whether we are
opening a normal window or an incognito window.
On 2011/10/29 20:29:00, Aaron Boodman wrote:
> This function is getting a little long. Could you pull these lines out into a
> GetIncognitoMode() helper method?
> 
> If you see anything else that can be easily pulled out at the same time, that
> would also be nice.

Done.

[commit-bot: I haz the power, 2011-10-31 20:06:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rustema@google.com/8373027/23001

[commit-bot: I haz the power, 2011-10-31 21:05:50]

Try job failure for 8373027-23001 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-01 00:10:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rustema@google.com/8373027/23001

[commit-bot: I haz the power, 2011-11-01 02:01:13]

Change committed as 108058