Move most Feature Policy code into content/

Move most Feature Policy code into content/

This adds a content::FeaturePolicy object to the FrameTreeNode object which is
maintained in parallel with the policy object in the renderer.

This also clarifies the difference between a Feature Policy Header (a set of
declarations), a Declaration (mapping from a feature to a declared whitelist)
and a Policy (a set of defaults inherited from the parent document, as well as a
set of whitelists for each feature).

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[iclelland, 2017-01-16 20:23:17]

Description was changed from

==========
Move most Feature Policy code into content/

This adds a content::FeaturePolicy object to the FrameTreeNode object which is
maintained in parallel with the policy object in the renderer.

This also clarifies the difference between a Feature Policy Header (a set of
declarations), a Declaration (mapping from a feature to a declared whitelist)
and a Policy (a set of defaults inherited from the parent document, as well as a
set of whitelists for each feature).

BUG=
==========

to

==========
Move most Feature Policy code into content/

This adds a content::FeaturePolicy object to the FrameTreeNode object which is
maintained in parallel with the policy object in the renderer.

This also clarifies the difference between a Feature Policy Header (a set of
declarations), a Declaration (mapping from a feature to a declared whitelist)
and a Policy (a set of defaults inherited from the parent document, as well as a
set of whitelists for each feature).

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[iclelland, 2017-01-16 20:23:21]

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

[commit-bot: I haz the power, 2017-01-16 20:23:36]

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-01-16 21:00:23]

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

[commit-bot: I haz the power, 2017-01-16 21:00:24]

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

[iclelland, 2017-01-17 20:44:17]

Patchset #2 (id:20001) has been deleted

[iclelland, 2017-01-17 21:47:11]

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

[iclelland, 2017-01-17 21:47:11]

+r raymes: This is still evolving, but PTAL at the general idea; most of the
code has been moved into content/common -- There are some errors relating to
using WebSecurityOrigin there, though -- if those are legitimate, I may need to
get creative about moving some of it into content/child instead, to deal with
marshalling data between blink and content.

The other big changes are:
  - Features are referenced by enum, rather than pointer; this way they are
trivially shared across processes
  - I've tried to distinguish between whitelists, declared policies, and (full,
composed) feature policies. Let me know if that's still unclear, and I'll try to
distinguish harder :)

Sorry it's so big :(

Thanks!

[iclelland, 2017-01-18 19:07:03]

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

[commit-bot: I haz the power, 2017-01-18 19:07:28]

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-01-18 20:07:53]

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

[commit-bot: I haz the power, 2017-01-18 20:07:55]

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

[raymes, 2017-01-19 02:40:57]

Thanks Ian! Overall this looks great! I was looking at high-level design more
than the details, so the comments are more related to that.

Once we're happy with the design do you think we could try breaking it up for
closer review/landing? Like possibly adding the content versions of the classes
+ test in one CL and then switching over (or something)?

https://codereview.chromium.org/2636843003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2636843003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:260: ResetFeaturePolicy();
Can this be called multiple times for the same frametreenode? I'm not sure but
if not it may be better to DCHECK

https://codereview.chromium.org/2636843003/diff/40001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2636843003/diff/40001/content/child/blink_pla...
content/child/blink_platform_impl.cc:866:
policy->SetHeaderPolicy(FeaturePolicyHeaderFromWeb(policyHeader));
It looks like we never need to access anything in the
WebParsedFeaturePolicyHeader in blink. So I was thinking we could potentially
make it similar to WebFeaturePolicy in the sense that the parsing code lives in
content/ and returns an opaque pointer to the header which can be passed around
in blink, but always just ends up going back into content. This might save us
needing these conversion functions to go to/from blink/content classes. It would
also save needing to expose those structs publicly. What do you think about that
approach? Are there some downsides I'm missing?

https://codereview.chromium.org/2636843003/diff/40001/content/child/blink_pla...
content/child/blink_platform_impl.cc:870: bool
BlinkPlatformImpl::isFeatureEnabledByPolicy(
I don't have a problem with this being a standalone function at all, but I was
curious why it was easier to do it this way (than to have the function on the
WebFeaturePolicy object)?

https://codereview.chromium.org/2636843003/diff/40001/content/child/blink_pla...
content/child/blink_platform_impl.cc:875: //    return
FeaturePolicy::IsFeatureAllowedByDefault("");
Hmm, what is this bit for?

https://codereview.chromium.org/2636843003/diff/40001/content/child/feature_p...
File content/child/feature_policy/feature_policy_platform.h (right):

https://codereview.chromium.org/2636843003/diff/40001/content/child/feature_p...
content/child/feature_policy/feature_policy_platform.h:6: #define
CONTENT_CHILD_FEATURE_POLICY_FEATURE_POLICY_PLATFORM_H_
Could be good to add yourself as an owner of this directory and
content/common/fp

https://codereview.chromium.org/2636843003/diff/40001/content/common/feature_...
File content/common/feature_policy/feature_policy.h (right):

https://codereview.chromium.org/2636843003/diff/40001/content/common/feature_...
content/common/feature_policy/feature_policy.h:119: const
FeaturePolicyParsedDeclaration& parsed_declaration);
Can this just be an implementation detail (standalone function in the cc file)?

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:134:
WebSecurityOrigin(frame->securityContext()->getSecurityOrigin()))) {
I think it's orthogonal to this CL but I'm a bit confused about how this check
is related to the one below?

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp (left):

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp:5: #include
"platform/feature_policy/FeaturePolicy.h"
I haven't thought hard about it, but do we lose coverage here?

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebFeaturePolicy.h (right):

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebFeaturePolicy.h:41: struct
WebParsedFeature {
Is this class used anymore?

[iclelland, 2017-01-19 05:38:27]

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

[commit-bot: I haz the power, 2017-01-19 05:38:54]

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

[iclelland, 2017-01-19 05:40:40]

https://codereview.chromium.org/2636843003/diff/40001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2636843003/diff/40001/content/child/blink_pla...
content/child/blink_platform_impl.cc:866:
policy->SetHeaderPolicy(FeaturePolicyHeaderFromWeb(policyHeader));
On 2017/01/19 02:40:57, raymes wrote:
> It looks like we never need to access anything in the
> WebParsedFeaturePolicyHeader in blink. So I was thinking we could potentially
> make it similar to WebFeaturePolicy in the sense that the parsing code lives
in
> content/ and returns an opaque pointer to the header which can be passed
around
> in blink, but always just ends up going back into content. This might save us
> needing these conversion functions to go to/from blink/content classes. It
would
> also save needing to expose those structs publicly. What do you think about
that
> approach? Are there some downsides I'm missing?

I think that might make sense as the final step in this move -- I'm not 100%
certain, though, so I haven't done it yet.

Technically moving it isn't a big deal; It means switching to a different JSON
parser, using the one in base, likely, and switching to using all of those
types, which should be a pretty mechanical change.

The bigger difference to me is that it moves header parsing out of blink, which
(I believe) is different than what we do for all other headers.

https://codereview.chromium.org/2636843003/diff/40001/content/child/blink_pla...
content/child/blink_platform_impl.cc:870: bool
BlinkPlatformImpl::isFeatureEnabledByPolicy(
On 2017/01/19 02:40:56, raymes wrote:
> I don't have a problem with this being a standalone function at all, but I was
> curious why it was easier to do it this way (than to have the function on the
> WebFeaturePolicy object)?

That caused me some grief -- it seems like the obvious way to do it, as a
virtual method on WebFeaturePolicy, taking a WebSecurityOrigin. Implementing
that method in content/common requires linking against all of the code for that
class, and  related classes, from Blink -- that's the reason I was getting
compile failures in the first patch.

I figured the best way out of that mess is to define all of the functions that
deal with WebSecurityOrigin in content/{child,renderer} instead, and have
content/common work only with url::Origin.

(I'll talk to mkwst@ about the plans for unifying the two classes so that we can
eliminate a lot of the conversion functions, but until that happens, I think
this is the cleanest way.)

https://codereview.chromium.org/2636843003/diff/40001/content/child/blink_pla...
content/child/blink_platform_impl.cc:875: //    return
FeaturePolicy::IsFeatureAllowedByDefault("");
On 2017/01/19 02:40:56, raymes wrote:
> Hmm, what is this bit for?

Thanks, that's not moved yet -- it was a placeholder to define the behaviour for
features in case there was no feature policy on the document at all.
That behaviour is actually implemented in
ConditionalFeatures::isFeatureEnabledInFrame, for the case where the runtime
flag is disabled, but I was considering moving it into content/.

https://codereview.chromium.org/2636843003/diff/40001/content/child/feature_p...
File content/child/feature_policy/feature_policy_platform.h (right):

https://codereview.chromium.org/2636843003/diff/40001/content/child/feature_p...
content/child/feature_policy/feature_policy_platform.h:6: #define
CONTENT_CHILD_FEATURE_POLICY_FEATURE_POLICY_PLATFORM_H_
On 2017/01/19 02:40:57, raymes wrote:
> Could be good to add yourself as an owner of this directory and
> content/common/fp 

Good idea, thanks. Done. (Added you as well)

https://codereview.chromium.org/2636843003/diff/40001/content/common/feature_...
File content/common/feature_policy/feature_policy.h (right):

https://codereview.chromium.org/2636843003/diff/40001/content/common/feature_...
content/common/feature_policy/feature_policy.h:119: const
FeaturePolicyParsedDeclaration& parsed_declaration);
On 2017/01/19 02:40:57, raymes wrote:
> Can this just be an implementation detail (standalone function in the cc
file)?

Yes it can, thanks. Done.

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:134:
WebSecurityOrigin(frame->securityContext()->getSecurityOrigin()))) {
On 2017/01/19 02:40:57, raymes wrote:
> I think it's orthogonal to this CL but I'm a bit confused about how this check
> is related to the one below?

It is orthogonal, but: this is a bit of a hack until we land support for
constructing the proper policy given the 'allowfullscreen' iframe attribute.
This check is for when the 'allowfullscreen' attribute is present, and checks
whether the *parent* frame had a feature policy which allowed fullscreen. The
check below is for the opposite case, where this frame does *not* have the
allowfullscreen attribute, but it *does* have a policy which correctly enables
fullscreen anyway.

(And actualy, this code turned out to be wrong; I'm waiting on foolip@ to take a
look at the more-correct-replacement in
https://codereview.chromium.org/2585363002/)

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp (left):

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp:5: #include
"platform/feature_policy/FeaturePolicy.h"
On 2017/01/19 02:40:56, raymes wrote:
> I haven't thought hard about it, but do we lose coverage here?

A bit; I want to put some kind of end-to-end test back in place, but it won't be
able to define new features like this one does. I need to either:
  - figure out how to define test-only-features in this test,
  - Add new features to content shell instead, or
  - Restrict this test to existing features like fullscreen -- that doesn't give
us any coverage of the "off-by-default" type, since we haven't defined any of
those yet.

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebFeaturePolicy.h (right):

https://codereview.chromium.org/2636843003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebFeaturePolicy.h:41: struct
WebParsedFeature {
On 2017/01/19 02:40:57, raymes wrote:
> Is this class used anymore?

Looks like not; It was only used during the refactor in the
WebParsedFeaturePolicy vector, but that's unused now, too. Thanks for catching
that.

[commit-bot: I haz the power, 2017-01-19 06:47:29]

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

[commit-bot: I haz the power, 2017-01-19 06:47:30]

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