Allow extensions to specify that they are not allowed in incognito mode.

Allow extensions to specify that they are not allowed in incognito mode.

Add "not_allowed" as a valid value to the "incognito" field in the
manifest. Track incognito behavior using an enum instead of a boolean
to distinguish between "split", "spanning", and "not_allowed". Add
CanBeIncognitoEnabled method to utils which checks the manifest entry
in addition to checking if extension is platform app or component.

BUG=455756
Committed: https://crrev.com/e548e74412d0a18f8829ce60201eb953a700b09b
Cr-Commit-Position: refs/heads/master@{#349806}

[Not at Google. Contact bengr, 2015-09-17 22:45:42]

kundaji@chromium.org changed reviewers:
+ kalman@chromium.org

[Not at Google. Contact bengr, 2015-09-17 22:45:42]

kalman:
  extensions/common/extension.cc
  extensions/common/manifest_constants.cc
  extensions/common/manifest_constants.h
  extensions/common/manifest_handlers/incognito_info.cc
  extensions/common/manifest_handlers/incognito_info.h

[not at google - send to devlin, 2015-09-17 22:59:55]

Thanks for doing this.

A test would be nice.

https://codereview.chromium.org/1351223003/diff/20001/extensions/common/exten...
File extensions/common/extension.cc (right):

https://codereview.chromium.org/1351223003/diff/20001/extensions/common/exten...
extensions/common/extension.cc:444: return
IncognitoInfo::IsIncognitoAllowed(this) &&
This function has too much logic, especially for one called
can_be_incognito_enabled (supposed to be reserved for simple accessors). Let's
move this to... extension_util.h maybe (in /extensions), as
CanBeIncognitoEnabled, to mirror the IsIncognitoEnabled function (in /chrome).

https://codereview.chromium.org/1351223003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handlers/incognito_info.cc (right):

https://codereview.chromium.org/1351223003/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/incognito_info.cc:46: IncognitoInfo::Mode
default_mode =
It would be slightly nicer to assign this before the this if-block, then below
where you declare |mode| assign it to |default_mode| first.

Or just assign it to |mode| to start off with. Either way, initializing to
SPANNING on line 55 looks odd.

https://codereview.chromium.org/1351223003/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/incognito_info.cc:68: else {
don't mix blocks with braces and blocks without them. I guess the old code was
style-illegal.

see
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals

[Not at Google. Contact bengr, 2015-09-18 21:24:31]

Thanks for the quick review.

Added tests and addressed comments.

https://codereview.chromium.org/1351223003/diff/20001/extensions/common/exten...
File extensions/common/extension.cc (right):

https://codereview.chromium.org/1351223003/diff/20001/extensions/common/exten...
extensions/common/extension.cc:444: return
IncognitoInfo::IsIncognitoAllowed(this) &&
On 2015/09/17 22:59:54, kalman wrote:
> This function has too much logic, especially for one called
> can_be_incognito_enabled (supposed to be reserved for simple accessors). Let's
> move this to... extension_util.h maybe (in /extensions), as
> CanBeIncognitoEnabled, to mirror the IsIncognitoEnabled function (in /chrome).

Done.

https://codereview.chromium.org/1351223003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handlers/incognito_info.cc (right):

https://codereview.chromium.org/1351223003/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/incognito_info.cc:46: IncognitoInfo::Mode
default_mode =
On 2015/09/17 22:59:54, kalman wrote:
> It would be slightly nicer to assign this before the this if-block, then below
> where you declare |mode| assign it to |default_mode| first.
> 
> Or just assign it to |mode| to start off with. Either way, initializing to
> SPANNING on line 55 looks odd.

Done.

https://codereview.chromium.org/1351223003/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/incognito_info.cc:68: else {
On 2015/09/17 22:59:54, kalman wrote:
> don't mix blocks with braces and blocks without them. I guess the old code was
> style-illegal.
> 
> see
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals

Done.

[not at google - send to devlin, 2015-09-18 21:45:17]

lgtm

[not at google - send to devlin, 2015-09-18 21:49:20]

https://codereview.chromium.org/1351223003/diff/40001/extensions/common/manif...
File extensions/common/manifest_handlers/incognito_manifest_unittest.cc (right):

https://codereview.chromium.org/1351223003/diff/40001/extensions/common/manif...
extensions/common/manifest_handlers/incognito_manifest_unittest.cc:15:
LoadAndExpectSuccess("incognito_not_allowed.json"));
actually can you also make assertions about the IncognitoInfo here? I know that
CanBeIncognitoEnabled is what you're going for at the end of the day, but there
are other methods/properties to test, namely everything public on
https://codereview.chromium.org/1351223003/diff/40001/extensions/common/manif...
(so I'd expect to see assertions for IsSplitMode and IsIncognitoAllowed here).

(we could go further - there are other untested things - but this is far enough)

[Not at Google. Contact bengr, 2015-09-18 22:07:33]

https://codereview.chromium.org/1351223003/diff/40001/extensions/common/manif...
File extensions/common/manifest_handlers/incognito_manifest_unittest.cc (right):

https://codereview.chromium.org/1351223003/diff/40001/extensions/common/manif...
extensions/common/manifest_handlers/incognito_manifest_unittest.cc:15:
LoadAndExpectSuccess("incognito_not_allowed.json"));
On 2015/09/18 21:49:20, kalman wrote:
> actually can you also make assertions about the IncognitoInfo here? I know
that
> CanBeIncognitoEnabled is what you're going for at the end of the day, but
there
> are other methods/properties to test, namely everything public on
>
https://codereview.chromium.org/1351223003/diff/40001/extensions/common/manif...
> (so I'd expect to see assertions for IsSplitMode and IsIncognitoAllowed here).
> 
> (we could go further - there are other untested things - but this is far
enough)

Done.

[not at google - send to devlin, 2015-09-18 22:08:21]

(lgtm)

[Not at Google. Contact bengr, 2015-09-18 22:32:57]

The CQ bit was checked by kundaji@chromium.org

[commit-bot: I haz the power, 2015-09-18 22:34:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1351223003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1351223003/60001

[commit-bot: I haz the power, 2015-09-18 23:19:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-18 23:19:55]

Patchset 4 (id:??) landed as
https://crrev.com/e548e74412d0a18f8829ce60201eb953a700b09b
Cr-Commit-Position: refs/heads/master@{#349806}