Rename WebParsedFeaturePolicyHeader to WebParsedFeaturePolicy

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

WebParsedFeaturePolicyHeader is renamed because this DS will
be shared between both header policy and container policy
and in the future meta tag policy and container header policy.

BUG=682256
Review-Url: https://codereview.chromium.org/2738953002
Cr-Commit-Position: refs/heads/master@{#456489}
Committed: https://chromium.googlesource.com/chromium/src/+/0baeb8b4f2a11f29104b2c989a584242acd139f3

[lunalu1, 2017-03-08 22:12:34]

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

[commit-bot: I haz the power, 2017-03-08 22:13:13]

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 22:15:38]

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

==========
WebParsedFeaturePolicyHeader is renamed because this DS will
be shared between both header policy and container policy
and in the future meta tag policy and container header policy.


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

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
3.a Replace string by enum in WebParsedFeaturePolicyDeclaration#feature
(CL: https://codereview.chromium.org/2727803004/)

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

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

Part 5: WebVR

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

[lunalu1, 2017-03-08 22:16:47]

Description was changed from

==========
WebParsedFeaturePolicyHeader is renamed because this DS will
be shared between both header policy and container policy
and in the future meta tag policy and container header policy.


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

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
3.a Replace string by enum in WebParsedFeaturePolicyDeclaration#feature
(CL: https://codereview.chromium.org/2727803004/)

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

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

Part 5: WebVR

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

to

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

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
3.a Replace string by enum in WebParsedFeaturePolicyDeclaration#feature
(CL: https://codereview.chromium.org/2727803004/)

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

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

Part 5: WebVR


WebParsedFeaturePolicyHeader is renamed because this DS will
be shared between both header policy and container policy
and in the future meta tag policy and container header policy.



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

[lunalu1, 2017-03-08 22:17:57]

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

[lunalu1, 2017-03-08 22:17:57]

Hi Raymes, could you PTAL?

Thanks

[commit-bot: I haz the power, 2017-03-08 22:29:10]

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

[commit-bot: I haz the power, 2017-03-08 22:29:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-08 22:43:18]

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

[lunalu1, 2017-03-08 22:43:52]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-08 22:44:05]

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 00:34:20]

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

[commit-bot: I haz the power, 2017-03-09 00:34:24]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[raymes, 2017-03-09 01:00:19]

lgtm, thanks!

In the CL description, you might want to move the description for this
particular CL right to the top and then say "This is a part of a series of CLs:"
just to make it slightly clearer.

Also, will we do this for ParsedFeaturePolicyHeader as well (the content/
version)

[lunalu1, 2017-03-09 01:02:06]

Description was changed from

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

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
3.a Replace string by enum in WebParsedFeaturePolicyDeclaration#feature
(CL: https://codereview.chromium.org/2727803004/)

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

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

Part 5: WebVR


WebParsedFeaturePolicyHeader is renamed because this DS will
be shared between both header policy and container policy
and in the future meta tag policy and container header policy.



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

to

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

WebParsedFeaturePolicyHeader is renamed because this DS will
be shared between both header policy and container policy
and in the future meta tag policy and container header policy.

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

[lunalu1, 2017-03-09 01:14:52]

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

[lunalu1, 2017-03-09 01:14:52]

Hi esprehn@, PTAL at this CL and give me an owner approval. Thanks

[esprehn, 2017-03-09 07:13:50]

All this plumbing is doing is sending legacy IPCs:

https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?q=...

can we onion soup it and use mojo instead so we can remove all the plumbing? I
worry we're just piling on debt here.

[lunalu1, 2017-03-09 20:24:25]

On 2017/03/09 07:13:50, esprehn wrote:
> All this plumbing is doing is sending legacy IPCs:
> 
>
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?q=...
> 
> can we onion soup it and use mojo instead so we can remove all the plumbing? I
> worry we're just piling on debt here.

Yeah we should certainly do that (in a separate CL though). I need to
investigate
a little bit more but it doesn't seem complicated to do. 

Some other container policy information is sent via:
https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_...
Maybe should mojofy that as well (this one seems more complex, so I will not
touch
it for now).

[raymes, 2017-03-12 23:55:12]

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

[raymes, 2017-03-12 23:55:12]

On 2017/03/09 07:13:50, esprehn wrote:
> All this plumbing is doing is sending legacy IPCs:
> 
>
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?q=...
> 
> can we onion soup it and use mojo instead so we can remove all the plumbing? I
> worry we're just piling on debt here.

Hi Elliot, the feature policy is a part of the frame replication state. It
follows all the same conventions as other members of the frame replication
state. Onion souping the frame replication state properly is going to require
quite a bit of thought and work and feels like a project in itself. +alexmos to
comment on that.

In any case all that this CL does is rename some classes. It isn't adding any
technical debt in itself. Would it be ok to land this and the discussion about
onion souping can continue in parallel?

Thanks!

[esprehn, 2017-03-13 00:45:30]

On 2017/03/12 at 23:55:12, raymes wrote:
> On 2017/03/09 07:13:50, esprehn wrote:
> > All this plumbing is doing is sending legacy IPCs:
> > 
> >
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?q=...
> > 
> > can we onion soup it and use mojo instead so we can remove all the plumbing?
I
> > worry we're just piling on debt here.
> 
> Hi Elliot, the feature policy is a part of the frame replication state. It
follows all the same conventions as other members of the frame replication
state. Onion souping the frame replication state properly is going to require
quite a bit of thought and work and feels like a project in itself. +alexmos to
comment on that.
> 
> In any case all that this CL does is rename some classes. It isn't adding any
technical debt in itself. Would it be ok to land this and the discussion about
onion souping can continue in parallel?
> 
> Thanks!

The code is all in content though, and is just proxying through Platform:
https://cs.chromium.org/chromium/src/content/child/blink_platform_impl.cc?l=856

calls into

https://cs.chromium.org/chromium/src/content/common/feature_policy/feature_po...

All of that can be moved into blink instead, those data structures are not
replicated directly.

[esprehn, 2017-03-13 03:22:17]

I discussed with raymes about what's going on here. I think we want to move this
code into a shared component. We definitely want to get rid of all of the static
factory stuff in blink::Platform (and hopefully all of Platform itself) which is
why I was trying to see how we ended up expanding the surface area of Platform
in this case for a brand new feature.

lgtm to rename, but I'd like to see us get some discussions going for how to get
rid of the factories here and put as much of the code into some shared location
and blink platform/ (the directory not the class).

[lunalu1, 2017-03-13 14:24:04]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-03-13 14:24:21]

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

[lunalu1, 2017-03-13 14:24:58]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-03-13 14:25:10]

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

[lunalu1, 2017-03-13 14:28:18]

The CQ bit was unchecked by lunalu@chromium.org

[lunalu1, 2017-03-13 15:17:19]

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

[lunalu1, 2017-03-13 15:17:40]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2017-03-13 15:17:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2738953002/#ps40001
(title: "ParsedFeaturePolicyHeader => ParsedFeaturePolicy (content version)")

[commit-bot: I haz the power, 2017-03-13 15:17:45]

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-13 16:53:40]

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

[commit-bot: I haz the power, 2017-03-13 16:53:41]

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

[alexmos, 2017-03-13 17:45:23]

On 2017/03/12 23:55:12, raymes wrote:
> On 2017/03/09 07:13:50, esprehn wrote:
> > All this plumbing is doing is sending legacy IPCs:
> > 
> >
>
https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?q=...
> > 
> > can we onion soup it and use mojo instead so we can remove all the plumbing?
I
> > worry we're just piling on debt here.
> 
> Hi Elliot, the feature policy is a part of the frame replication state. It
> follows all the same conventions as other members of the frame replication
> state. Onion souping the frame replication state properly is going to require
> quite a bit of thought and work and feels like a project in itself. +alexmos
to
> comment on that.
> 
> In any case all that this CL does is rename some classes. It isn't adding any
> technical debt in itself. Would it be ok to land this and the discussion about
> onion souping can continue in parallel?
> 
> Thanks!

On the OOPIF side, we've chatted about mojofying the frame replication state but
haven't really thought through all the issues.  See https://crbug.com/686188 for
some discussion that dcheng@ started around that.

[lunalu1, 2017-03-13 20:35:04]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2017-03-13 20:35:33]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-03-13 20:35:36]

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-13 21:24:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489437333793460,
"parent_rev": "1d64628a6b9a59469503b6a82a68606a09950d9a", "commit_rev":
"0baeb8b4f2a11f29104b2c989a584242acd139f3"}

[commit-bot: I haz the power, 2017-03-13 21:25:29]

Description was changed from

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

WebParsedFeaturePolicyHeader is renamed because this DS will
be shared between both header policy and container policy
and in the future meta tag policy and container header policy.

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

to

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

WebParsedFeaturePolicyHeader is renamed because this DS will
be shared between both header policy and container policy
and in the future meta tag policy and container header policy.

BUG=682256

Review-Url: https://codereview.chromium.org/2738953002
Cr-Commit-Position: refs/heads/master@{#456489}
Committed:
https://chromium.googlesource.com/chromium/src/+/0baeb8b4f2a11f29104b2c989a58...
==========

[commit-bot: I haz the power, 2017-03-13 21:25:35]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0baeb8b4f2a11f29104b2c989a58...