Add the basic infrastructure for the Behavior feature type: BehaviorFeature and

Add the basic infrastructure for the Behavior feature type: BehaviorFeature and
_behavior_features.json. Arbitrarily use it to implement the allow-in-incognito
whitelist.

BUG=440194
R=rockot@chromium.org

Committed: https://crrev.com/64210328fe91376da6cf6a21ac16db72efa6118a
Cr-Commit-Position: refs/heads/master@{#308311}

[not at google - send to devlin, 2014-12-10 23:57:18]

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

[not at google - send to devlin, 2014-12-10 23:57:18]

Ken, seeking initial impressions here. It compiles and I can't see why it
wouldn't work, but I haven't tested it.

I don't know about the name "switch features". It's pretty confusing because
there is already a FeatureSwitch class - which I plan on gradually removing in
favour of whatever this turns out to be - but nevertheless, confusing.

I'm also not happy about:
- needing to spell out every feature name in a constants file.
- the amount of work needed to check it.

Ideas? Another other name I considered was "flag features" but that also sounds
wrong. Switch works but only if you remember to omit "command line".

Other considered names:
- "misc" (don't like this, they have a defined purpose so not misc)
- "runtime" (don't like this, all the other features are "runtime" as well)
- "capability" (don't like this, tautological)

[not at google - send to devlin, 2014-12-11 00:07:27]

Another question is whether you need separate of these for extension concerns vs
chrome concerns. I think no, because it's simpler, and there is no compile
issue, but I didn't design this stuff.

[Ken Rockot(use gerrit already), 2014-12-11 00:38:58]

Nice. I wanted exactly this eons ago when looking at all the magic whitelisting
cases scattered around the AppWindow implementation. I don't remember what I
wanted to call it, but I don't like switch_features.

*thinks real hard*

*more coffee*

Ugh, switch_features is no worse than anything else I can think of. Other
considerations I didn't immediately discard:

binary_features
boolean_features
conditional_features
predicate_features

The biggest problem I have with "switch" is that I immediately associate it with
command-line arguments in the chromium vernacular.

On 2014/12/11 00:07:27, kalman wrote:
> Another question is whether you need separate of these for extension concerns
vs
> chrome concerns. I think no, because it's simpler, and there is no compile
> issue, but I didn't design this stuff.

I see no good reason to bother with separate features for //extensions vs
//chrome. If the need arises, it's trivial to adapt.

[Ken Rockot(use gerrit already), 2014-12-11 00:40:02]

On 2014/12/11 00:38:58, Ken Rockot wrote:
> Nice. I wanted exactly this eons ago when looking at all the magic
whitelisting
> cases scattered around the AppWindow implementation. I don't remember what I
> wanted to call it, but I don't like switch_features.
> 
> *thinks real hard*
> 
> *more coffee*
> 
> Ugh, switch_features is no worse than anything else I can think of. Other
> considerations I didn't immediately discard:
> 
> binary_features
> boolean_features
> conditional_features
> predicate_features
> 
> The biggest problem I have with "switch" is that I immediately associate it
with
> command-line arguments in the chromium vernacular.
> 
> On 2014/12/11 00:07:27, kalman wrote:
> > Another question is whether you need separate of these for extension
concerns
> vs
> > chrome concerns. I think no, because it's simpler, and there is no compile
> > issue, but I didn't design this stuff.
> 
> I see no good reason to bother with separate features for //extensions vs
> //chrome. If the need arises, it's trivial to adapt.

And apart from the naming bikeshed, the code here looks fine

[not at google - send to devlin, 2014-12-11 17:15:47]

On 2014/12/11 00:38:58, Ken Rockot wrote:
> Nice. I wanted exactly this eons ago when looking at all the magic
whitelisting
> cases scattered around the AppWindow implementation. I don't remember what I
> wanted to call it, but I don't like switch_features.
> 
> *thinks real hard*
> 
> *more coffee*
> 
> Ugh, switch_features is no worse than anything else I can think of. Other
> considerations I didn't immediately discard:
> 
> binary_features
> boolean_features
> conditional_features
> predicate_features
> 
> The biggest problem I have with "switch" is that I immediately associate it
with
> command-line arguments in the chromium vernacular.

Yeah, that's my issue too. That's what I meant by "only if you remember to omit
command line", that is, switches are really *command line* switches, but that's
like arguing that you shouldn't say "literally" anymore.

I just thought of one: "behavior", or "behavioral", features. Something along
those lines? Or "custom"?

[not at google - send to devlin, 2014-12-11 17:51:00]

https://codereview.chromium.org/789383002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/789383002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_util.cc:48: return
FeatureProvider::GetSwitchFeatures()
Btw, wdyt about renaming all of these to "GetSwitch", "GetAPI", etc? The
"Features" seems tautological given that it's on the "FeatureProvider"
namespace.

[not at google - send to devlin, 2014-12-11 22:09:04]

Ok, for realz.

[Ken Rockot(use gerrit already), 2014-12-11 22:32:21]

lgtm

https://codereview.chromium.org/789383002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/789383002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_util.cc:87: if
(IsWhitelistedForIncognito(extension))
This doesn't seem equivalent to the previous condition, but I guess it's
pointless to specifically check for component extensions if it's whitelist-only.

[not at google - send to devlin, 2014-12-11 22:41:23]

https://codereview.chromium.org/789383002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/789383002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_util.cc:87: if
(IsWhitelistedForIncognito(extension))
On 2014/12/11 22:32:21, Ken Rockot wrote:
> This doesn't seem equivalent to the previous condition, but I guess it's
> pointless to specifically check for component extensions if it's
whitelist-only.

Yeah, exactly. I would need to add support for external component extensions to
the feature files, which I could do, but it doesn't really seem that worthwhile
given it's a whitelist.

[not at google - send to devlin, 2014-12-11 22:41:27]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-12-11 23:11:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789383002/40001

[commit-bot: I haz the power, 2014-12-12 01:02:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 01:02:13]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[not at google - send to devlin, 2014-12-12 17:59:38]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-12-12 18:01:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789383002/40001

[commit-bot: I haz the power, 2014-12-12 18:50:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 18:50:18]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[not at google - send to devlin, 2014-12-12 20:42:28]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-12-12 20:43:10]

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

[commit-bot: I haz the power, 2014-12-12 21:29:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 21:29:36]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[not at google - send to devlin, 2014-12-15 03:12:20]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-12-15 03:12:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789383002/100001

[not at google - send to devlin, 2014-12-15 03:18:14]

Well, I needed to end up maintaining that external_component restriction after
all.

[commit-bot: I haz the power, 2014-12-15 03:59:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-15 04:00:01]

Patchset 6 (id:??) landed as
https://crrev.com/64210328fe91376da6cf6a21ac16db72efa6118a
Cr-Commit-Position: refs/heads/master@{#308311}