Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Initial Implementation of Iframe Attribute for Feature Policy (Part 3.a)

Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList

feature is converted from string in parseFeaturePolicy now so we can
drop the unrecognized features earlier instead of doing it in
setHeaderPolicy

BUG=682256
Review-Url: https://codereview.chromium.org/2727803004
Cr-Commit-Position: refs/heads/master@{#457884}
Committed: https://chromium.googlesource.com/chromium/src/+/5fb4be2269ba2a16ebec49edf4f2817517f63ad5

[lunalu1, 2017-03-02 20:54:27]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 20:55:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-02 20:56:00]

Description was changed from

==========
Use enum instead of string in WebParsedFeaturePolicyDeclaration#feature

BUG=682256
==========

to

==========
Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

BUG=682256
==========

[lunalu1, 2017-03-02 20:56:18]

lunalu@chromium.org changed reviewers:
+ iclelland@chromium.org

[commit-bot: I haz the power, 2017-03-02 20:59:00]

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

[commit-bot: I haz the power, 2017-03-02 20:59:00]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2017-03-02 21:07:42]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 21:08:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-02 21:08:13]

The CQ bit was unchecked by lunalu@chromium.org

[lunalu1, 2017-03-02 21:08:21]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 21:08:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 21:12:09]

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

[commit-bot: I haz the power, 2017-03-02 21:12:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iclelland, 2017-03-06 13:55:59]

This looks great! Just a couple of nits.

Let's just make sure to hold off on committing this until
https://codereview.chromium.org/2651883008/ lands, though, so we don't have to
think about any difference between the blink/ and content/ versions of the
classes.

https://codereview.chromium.org/2727803004/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2727803004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:681: ParsedFeaturePolicyHeader
CreateFPHeader(const std::string&.feature,
Looks like there are some extra "." characters in these lines. Bad
search-and-replace, maybe?

https://codereview.chromium.org/2727803004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:684: result[0].feature
=.feature;
Here too

https://codereview.chromium.org/2727803004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:693: const std::string&.feature)
{
And here

https://codereview.chromium.org/2727803004/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:695: result[0].feature
=.feature;
And here.

https://codereview.chromium.org/2727803004/diff/20001/content/common/feature_...
File content/common/feature_policy/feature_policy.cc (right):

https://codereview.chromium.org/2727803004/diff/20001/content/common/feature_...
content/common/feature_policy/feature_policy.cc:14: const FeaturePolicy::Feature
kDocumentCookie{
Removing the namespace declaration moves all of these symbols into the content::
namespace -- was that intentional?

[lunalu1, 2017-03-06 20:57:48]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 20:58:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 21:00:44]

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

[commit-bot: I haz the power, 2017-03-06 21:00:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[lunalu1, 2017-03-06 21:04:26]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-03-06 21:04:32]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-06 21:05:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 21:07:59]

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

[commit-bot: I haz the power, 2017-03-06 21:08:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[lunalu1, 2017-03-07 15:41:03]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-03-07 15:41:23]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-07 15:42:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-07 15:43:10]

Could you PTAL?
Thanks

[lunalu1, 2017-03-07 15:43:14]

Could you PTAL?
Thanks

[commit-bot: I haz the power, 2017-03-07 15:45:48]

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

[commit-bot: I haz the power, 2017-03-07 15:45:49]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2017-03-07 20:42:44]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 20:43:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 21:01:37]

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

[commit-bot: I haz the power, 2017-03-07 21:01:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2017-03-08 19:49:11]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 19:49:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-08 19:54:57]

Description was changed from

==========
Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

BUG=682256
==========

to

==========
Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList


BUG=682256
==========

[lunalu1, 2017-03-08 19:56:27]

lunalu@chromium.org changed reviewers:
+ raymes@chromium.org

[lunalu1, 2017-03-08 19:56:27]

Hi Ian,
After some thinking, I decided to remove FeaturePolicy::Feature (please see the
issue description for more details). PTAL

Hi Raymes,
Since Ian is on vacation, would you mind taking a look at this CL?

Thanks

[lunalu1, 2017-03-08 19:56:46]

Patchset #4 (id:100001) has been deleted

[lunalu1, 2017-03-08 19:57:02]

Patchset #3 (id:80001) has been deleted

[lunalu1, 2017-03-08 20:18:21]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 20:19:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-08 21:20:58]

Description was changed from

==========
Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList


BUG=682256
==========

to

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 3.a)

Enable iframe feature policy by attributes:
  allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.

See design doc:
https://docs.google.com/a/chromium.org/document/d/1sskoBi7Ba7hLuuiJQ6VMQ1KYIG...

Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute
and store featureNames in HTMLIFrameElement
(CL: https://codereview.chromium.org/2680083002/)

Part 2: Propagate featureNames from HTMLIFrameElement to frame owner and
remote frame owner.

Part 3: Set iframe feature policy in FrameLoader::didBeginDocument

Part 4.a: Replace implementation of allowpaymentrequest by feature policy

Part 4.b: Replace implementation of allowfullscreen by feature policy

Part 5: WebVR

Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList


BUG=682256
==========

[lunalu1, 2017-03-08 21:21:39]

Description was changed from

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 3.a)

Enable iframe feature policy by attributes:
  allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.

See design doc:
https://docs.google.com/a/chromium.org/document/d/1sskoBi7Ba7hLuuiJQ6VMQ1KYIG...

Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute
and store featureNames in HTMLIFrameElement
(CL: https://codereview.chromium.org/2680083002/)

Part 2: Propagate featureNames from HTMLIFrameElement to frame owner and
remote frame owner.

Part 3: Set iframe feature policy in FrameLoader::didBeginDocument

Part 4.a: Replace implementation of allowpaymentrequest by feature policy

Part 4.b: Replace implementation of allowfullscreen by feature policy

Part 5: WebVR

Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList


BUG=682256
==========

to

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 3.a)

Enable iframe feature policy by attributes:
  allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.

See design doc:
https://docs.google.com/a/chromium.org/document/d/1sskoBi7Ba7hLuuiJQ6VMQ1KYIG...

Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute
and store featureNames in HTMLIFrameElement
(CL: https://codereview.chromium.org/2680083002/)

Part 2: Propagate featureNames from HTMLIFrameElement to frame owner and
remote frame owner.
(CL: https://codereview.chromium.org/2697713003)

Part 3: Set iframe feature policy in FrameLoader::didBeginDocument

Part 4.a: Replace implementation of allowpaymentrequest by feature policy

Part 4.b: Replace implementation of allowfullscreen by feature policy

Part 5: WebVR

Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList


BUG=682256
==========

[lunalu1, 2017-03-08 21:23:19]

Patchset #4 (id:140001) has been deleted

[lunalu1, 2017-03-08 21:51:54]

Patchset #4 (id:160001) has been deleted

[lunalu1, 2017-03-09 01:07:31]

Description was changed from

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 3.a)

Enable iframe feature policy by attributes:
  allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.

See design doc:
https://docs.google.com/a/chromium.org/document/d/1sskoBi7Ba7hLuuiJQ6VMQ1KYIG...

Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute
and store featureNames in HTMLIFrameElement
(CL: https://codereview.chromium.org/2680083002/)

Part 2: Propagate featureNames from HTMLIFrameElement to frame owner and
remote frame owner.
(CL: https://codereview.chromium.org/2697713003)

Part 3: Set iframe feature policy in FrameLoader::didBeginDocument

Part 4.a: Replace implementation of allowpaymentrequest by feature policy

Part 4.b: Replace implementation of allowfullscreen by feature policy

Part 5: WebVR

Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList


BUG=682256
==========

to

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 3.a)

Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList

feature is converted from string in parseFeaturePolicy now so we can drop the
unrecognized features earlier instead of doing it in setHeaderPolicy

BUG=682256
==========

[lunalu1, 2017-03-09 01:12:42]

Hi,
I added another change to filter out unrecognized features earlier that we don't
need to parse around never-used information in our code. 

PTAL.
Thanks!

[lunalu1, 2017-03-09 01:12:51]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 01:13:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-09 01:15:32]

Description was changed from

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 3.a)

Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList

feature is converted from string in parseFeaturePolicy now so we can drop the
unrecognized features earlier instead of doing it in setHeaderPolicy

BUG=682256
==========

to

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 3.a)

Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList

feature is converted from string in parseFeaturePolicy now so we can
drop the unrecognized features earlier instead of doing it in
setHeaderPolicy

BUG=682256
==========

[commit-bot: I haz the power, 2017-03-09 02:29:28]

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

[commit-bot: I haz the power, 2017-03-09 02:29:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2017-03-09 02:52:00]

Looks good, just one question below. Thanks!

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
File content/common/feature_policy/feature_policy.cc (right):

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
content/common/feature_policy/feature_policy.cc:133: DCHECK(feature !=
blink::WebFeaturePolicyFeature::NotFound);
DCHECK_NE(...)

But could this happen in the browser process if there was a compromised
renderer? I wonder if we should still continue here?

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
content/common/feature_policy/feature_policy.cc:219:
FeaturePolicy::FeatureDefault::EnableForAll}}));
Orhtogonal to this CL: we should think about removing some of these at some
point. Some of them are out of date, like WebRTC and Usermedia. Push and
Notifications ar ealso not needed anymore.

[lunalu1, 2017-03-09 16:05:17]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-03-09 16:05:44]

Hi raymes@, thanks for reviewing my CL. Please see my comments below.

Cheers

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
File content/common/feature_policy/feature_policy.cc (right):

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
content/common/feature_policy/feature_policy.cc:133: DCHECK(feature !=
blink::WebFeaturePolicyFeature::NotFound);
On 2017/03/09 02:52:00, raymes wrote:
> DCHECK_NE(...)
> 
> But could this happen in the browser process if there was a compromised
> renderer? I wonder if we should still continue here?

Please correct me if I am wrong, to get a ParsedFeaturePolicyDeclaration,
ParseFeaturePolicy must have been called, where NotFound feature are dropped.
Could you please specify exactly what case where a NotFound feature is
re-introduced?

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
content/common/feature_policy/feature_policy.cc:219:
FeaturePolicy::FeatureDefault::EnableForAll}}));
On 2017/03/09 02:52:00, raymes wrote:
> Orhtogonal to this CL: we should think about removing some of these at some
> point. Some of them are out of date, like WebRTC and Usermedia. Push and
> Notifications ar ealso not needed anymore.

Yes. Definitely. After I land in the code to add container policy. I will remove
/ update the feature list. 
So a separate CL would make it more clear what we are trying to do. WDYT?

[commit-bot: I haz the power, 2017-03-09 16:05:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 18:17:54]

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

[commit-bot: I haz the power, 2017-03-09 18:17:55]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-03-12 23:43:39]

lgtm

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
File content/common/feature_policy/feature_policy.cc (right):

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
content/common/feature_policy/feature_policy.cc:133: DCHECK(feature !=
blink::WebFeaturePolicyFeature::NotFound);
On 2017/03/09 16:05:44, loonybear wrote:
> On 2017/03/09 02:52:00, raymes wrote:
> > DCHECK_NE(...)
> > 
> > But could this happen in the browser process if there was a compromised
> > renderer? I wonder if we should still continue here?
> 
> Please correct me if I am wrong, to get a ParsedFeaturePolicyDeclaration,
> ParseFeaturePolicy must have been called, where NotFound feature are dropped.
> Could you please specify exactly what case where a NotFound feature is
> re-introduced?

ParsedFeaturePolicyHeader is a member of FrameReplicationState (see
https://cs.chromium.org/chromium/src/content/common/frame_replication_state.h...)

It gets passed from the renderer to the browser process and then back to other
renderers. If a renderer is compromised, it could lie about its
ParsedFeaturePolicyHeader and introduce something different which is where this
could become an unexpected value.

Actually though, it turns out that we already check this with enum traits:
https://cs.chromium.org/chromium/src/content/common/frame_messages.h?rcl=a054...

So I think what you have is safe :) You can probably even remove this DHECK.

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
content/common/feature_policy/feature_policy.cc:219:
FeaturePolicy::FeatureDefault::EnableForAll}}));
On 2017/03/09 16:05:44, loonybear wrote:
> On 2017/03/09 02:52:00, raymes wrote:
> > Orhtogonal to this CL: we should think about removing some of these at some
> > point. Some of them are out of date, like WebRTC and Usermedia. Push and
> > Notifications ar ealso not needed anymore.
> 
> Yes. Definitely. After I land in the code to add container policy. I will
remove
> / update the feature list. 
> So a separate CL would make it more clear what we are trying to do. WDYT? 

Sounds great :)

[lunalu1, 2017-03-13 15:23:29]

lunalu@chromium.org changed reviewers:
+ nasko@chromium.org, pfeldman@chromium.org

[lunalu1, 2017-03-13 15:23:30]

raymes LGTM'ed content/common/feature_policy/* and
content/child_feature_policy/*

nasko, could you please owner LGTM frame_messages.h
pfeldman, could you please owner LGTM everything else?

Thanks

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
File content/common/feature_policy/feature_policy.cc (right):

https://codereview.chromium.org/2727803004/diff/180001/content/common/feature...
content/common/feature_policy/feature_policy.cc:133: DCHECK(feature !=
blink::WebFeaturePolicyFeature::NotFound);
On 2017/03/12 23:43:39, raymes wrote:
> On 2017/03/09 16:05:44, loonybear wrote:
> > On 2017/03/09 02:52:00, raymes wrote:
> > > DCHECK_NE(...)
> > > 
> > > But could this happen in the browser process if there was a compromised
> > > renderer? I wonder if we should still continue here?
> > 
> > Please correct me if I am wrong, to get a ParsedFeaturePolicyDeclaration,
> > ParseFeaturePolicy must have been called, where NotFound feature are
dropped.
> > Could you please specify exactly what case where a NotFound feature is
> > re-introduced?
> 
> ParsedFeaturePolicyHeader is a member of FrameReplicationState (see
>
https://cs.chromium.org/chromium/src/content/common/frame_replication_state.h...)
> 
> It gets passed from the renderer to the browser process and then back to other
> renderers. If a renderer is compromised, it could lie about its
> ParsedFeaturePolicyHeader and introduce something different which is where
this
> could become an unexpected value.
> 
> Actually though, it turns out that we already check this with enum traits:
>
https://cs.chromium.org/chromium/src/content/common/frame_messages.h?rcl=a054...
> 
> So I think what you have is safe :) You can probably even remove this DHECK.

Lemme keep DCHECK here for now til most part of feature policy is implemented
and thoroughly tested.

[nasko, 2017-03-13 22:07:10]

On 2017/03/13 15:23:30, loonybear wrote:
> raymes LGTM'ed content/common/feature_policy/* and
> content/child_feature_policy/*
> 
> nasko, could you please owner LGTM frame_messages.h

I'm happy to. Since I'm also content/ OWNER, I'll wait for pfeldman's review and
only stamp the CL at that point in time.

> pfeldman, could you please owner LGTM everything else?
> 
> Thanks

[iclelland, 2017-03-15 15:33:02]

Other than the message, which I don't think should be an error, this LGTM

https://codereview.chromium.org/2727803004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2727803004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:77:
messages->push_back("Feature name is unrecognized.");
I don't think we should report this like an error -- it is likely that other
browers, or even other versions of chrome, will support feature names that we
don't currently recognize. In that case, we should just ignore those names, as
silently as possible.

[lunalu1, 2017-03-15 16:08:57]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 16:09:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-15 16:09:30]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:34]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:35]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:35]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:38]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:41]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:41]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:41]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:41]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:42]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:42]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:42]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[lunalu1, 2017-03-15 16:09:43]

Done. Thanks
pfeldman@ could you please owner LGTM?

Thanks

[commit-bot: I haz the power, 2017-03-15 17:49:21]

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

[commit-bot: I haz the power, 2017-03-15 17:49:22]

Dry run: This issue passed the CQ dry run.

[lunalu1, 2017-03-16 21:58:48]

Ping
Hi pfeldman and nasko, could you please owner lgtm?

Thanks

[pfeldman, 2017-03-17 01:12:21]

content/ and WebKit/ lgtm % nits

https://codereview.chromium.org/2727803004/diff/220001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2727803004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:720: const
blink::WebFeaturePolicyFeature feature,
drop the const - it is a value type enum

https://codereview.chromium.org/2727803004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:732: const
blink::WebFeaturePolicyFeature feature) {
ditto

[lunalu1, 2017-03-17 14:35:33]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 14:35:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-17 14:35:54]

hi nasko, would you mind take a look at the ipc messages?

Thanks

[commit-bot: I haz the power, 2017-03-17 16:28:13]

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

[commit-bot: I haz the power, 2017-03-17 16:28:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nasko, 2017-03-17 16:35:56]

IPC LGTM

https://codereview.chromium.org/2727803004/diff/240001/content/common/feature...
File content/common/feature_policy/feature_policy.h (left):

https://codereview.chromium.org/2727803004/diff/240001/content/common/feature...
content/common/feature_policy/feature_policy.h:34: // Features
nit: I don't understand why this section is removed fully, as it seems
"Features" is still a concept, just it is implemented differently now.

[lunalu1, 2017-03-17 18:44:13]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2017-03-17 18:44:13]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
iclelland@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2727803004/#ps240001
(title: "Codereview: nit")

[commit-bot: I haz the power, 2017-03-17 18:45:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-17 18:51:09]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2017-03-17 18:51:10]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
iclelland@chromium.org, pfeldman@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2727803004/#ps260001
(title: "Codereview: nit -- add comments to introduce features")

[commit-bot: I haz the power, 2017-03-17 18:51:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-17 18:52:55]

https://codereview.chromium.org/2727803004/diff/240001/content/common/feature...
File content/common/feature_policy/feature_policy.h (left):

https://codereview.chromium.org/2727803004/diff/240001/content/common/feature...
content/common/feature_policy/feature_policy.h:34: // Features
On 2017/03/17 16:35:56, nasko (slow) wrote:
> nit: I don't understand why this section is removed fully, as it seems
> "Features" is still a concept, just it is implemented differently now.

My bad, I will add it back.

[commit-bot: I haz the power, 2017-03-17 20:57:22]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1489776669957780,
"parent_rev": "7f27d3a538e1b0244723630dd221a14e935f28ba", "commit_rev":
"5fb4be2269ba2a16ebec49edf4f2817517f63ad5"}

[commit-bot: I haz the power, 2017-03-17 20:58:15]

Description was changed from

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 3.a)

Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList

feature is converted from string in parseFeaturePolicy now so we can
drop the unrecognized features earlier instead of doing it in
setHeaderPolicy

BUG=682256
==========

to

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 3.a)

Replace string by enum in WebParsedFeaturePolicyDeclaration#feature

Use enum in WebParsedFeaturePolicyDeclaration for 2 reasons:
1. Memory efficient
2. Make code shareable for header policy and container policy

Removed FeaturePolicy::Feature as we no longer need to store feature
name (string) once the policy is parsed. Updated FeatureList (now a
a mapping between feature to default policy) and tests as a result.
Only downside is we no longer have explicit definition of each feature
(e.g. kDocumentCookie{"cookie", EnableForAll}), we define the default
policy directly in DefaultFeatureList

feature is converted from string in parseFeaturePolicy now so we can
drop the unrecognized features earlier instead of doing it in
setHeaderPolicy

BUG=682256

Review-Url: https://codereview.chromium.org/2727803004
Cr-Commit-Position: refs/heads/master@{#457884}
Committed:
https://chromium.googlesource.com/chromium/src/+/5fb4be2269ba2a16ebec49edf4f2...
==========

[commit-bot: I haz the power, 2017-03-17 20:58:17]

Committed patchset #8 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/5fb4be2269ba2a16ebec49edf4f2...