Move container policy logic to frame owner classes.

Move container policy logic to frame owner classes.

This specifically allows <frame> tags to have an exemption from supporting the
Fullscreen API, even in same-origin contexts, and even when the declared policy
would otherwise enable Fullscreen.

BUG=729658
Review-Url: https://codereview.chromium.org/2923563003
Cr-Commit-Position: refs/heads/master@{#479165}
Committed: https://chromium.googlesource.com/chromium/src/+/53ecf9341f32b7f3746e6f721da9d4dc6839fb98

[iclelland, 2017-06-05 18:35:52]

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

[commit-bot: I haz the power, 2017-06-05 18:36:26]

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-06-05 22:31:36]

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

[commit-bot: I haz the power, 2017-06-05 22:31:36]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-06-06 14:29:06]

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

[commit-bot: I haz the power, 2017-06-06 14:29:31]

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-06-06 16:43:18]

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

[commit-bot: I haz the power, 2017-06-06 16:43:19]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-06-07 17:21:47]

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

[iclelland, 2017-06-07 17:21:47]

+r raymes, could you take a look?

[raymes, 2017-06-07 23:54:38]

Thanks Ian. Just to clarify: does the allow= attribute work for frame tags?

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameElementTest.cpp (right):

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameElementTest.cpp:25:
frame_element->setAttribute(HTMLNames::srcAttr, "http://example.net/");
Just to clarify: adding allow="fullscreen" here would do nothing anyway? Is that
true? Or should we add that to the test as well?

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:211:
container_policy_ = GetContainerPolicyFromAllowedFeatures(
Hmm - should this use the same ConstructContainerPolicy code? I don't know this
code well :( If so, does it mean we can remove
GetContainerPolicyFromAllowedFeatures ?

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:247: }
Is this actually needed or can this be a pure virtual function with subclasses
implementing what they need? If possible that might be preferable, simply just
to reduce the amount of indirection?

[iclelland, 2017-06-12 17:30:41]

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

[commit-bot: I haz the power, 2017-06-12 17:31:09]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-06-12 17:32:17]

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameElementTest.cpp (right):

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameElementTest.cpp:25:
frame_element->setAttribute(HTMLNames::srcAttr, "http://example.net/");
On 2017/06/07 23:54:37, raymes wrote:
> Just to clarify: adding allow="fullscreen" here would do nothing anyway? Is
that
> true? Or should we add that to the test as well?

That's correct -- we've never specified the allow attribute, in IDL or in the
spec, for anything other than iframe elements.

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:211:
container_policy_ = GetContainerPolicyFromAllowedFeatures(
On 2017/06/07 23:54:38, raymes wrote:
> Hmm - should this use the same ConstructContainerPolicy code? I don't know
this
> code well :( If so, does it mean we can remove
> GetContainerPolicyFromAllowedFeatures ?

Good catch -- thanks. Updated.

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:247: }
On 2017/06/07 23:54:38, raymes wrote:
> Is this actually needed or can this be a pure virtual function with subclasses
> implementing what they need? If possible that might be preferable, simply just
> to reduce the amount of indirection?

i had expected that it would be common code to handle the 'allow' attribute, but
that's only been defined on <iframe>, so maybe it makes sense to implement it
there instead.

I've moved this logic to HTMLIFrameElement, and removed the call to this method
(and the code that compensated for <fullscreen> from HTMLFrameElement. There's a
simple no-specific-container-policy implementation for plugin elements as well.

[commit-bot: I haz the power, 2017-06-12 20:28:54]

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

[commit-bot: I haz the power, 2017-06-12 20:28:55]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-06-12 22:54:27]

lgtm with 3 nits :)

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/2923563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:211:
container_policy_ = GetContainerPolicyFromAllowedFeatures(
On 2017/06/12 17:32:17, iclelland wrote:
> On 2017/06/07 23:54:38, raymes wrote:
> > Hmm - should this use the same ConstructContainerPolicy code? I don't know
> this
> > code well :( If so, does it mean we can remove
> > GetContainerPolicyFromAllowedFeatures ?
> 
> Good catch -- thanks. Updated.

Can we remove GetContainerPolicyFromAllowedFeatures from FeaturePolicy.cpp now?
I thin kit's dead code?

https://codereview.chromium.org/2923563003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameElement.cpp (right):

https://codereview.chromium.org/2923563003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameElement.cpp:81: // Frame elements
are not allowed to enable the fullscreen feature. Add an
tiny nit: a link to spec or documentation here might be helpful for a reader but
not too important if there isn't one

https://codereview.chromium.org/2923563003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/2923563003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:273:
Vector<WebParsedFeaturePolicyDeclaration> container_policy;
nit: perhaps add a comment about why we return an empty policy here

[iclelland, 2017-06-13 14:04:54]

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

[commit-bot: I haz the power, 2017-06-13 14:05:11]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-06-13 14:25:40]

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

[commit-bot: I haz the power, 2017-06-13 14:26:01]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-06-13 14:38:33]

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

[iclelland, 2017-06-13 14:38:34]

+r foolip -- can you PTAL?

https://codereview.chromium.org/2923563003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameElement.cpp (right):

https://codereview.chromium.org/2923563003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameElement.cpp:81: // Frame elements
are not allowed to enable the fullscreen feature. Add an
On 2017/06/12 22:54:26, raymes wrote:
> tiny nit: a link to spec or documentation here might be helpful for a reader
but
> not too important if there isn't one

Done.

https://codereview.chromium.org/2923563003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/2923563003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:273:
Vector<WebParsedFeaturePolicyDeclaration> container_policy;
On 2017/06/12 22:54:26, raymes wrote:
> nit: perhaps add a comment about why we return an empty policy here

Done, though I actually duplicated the <frame> logic here as well, since it is
really only <iframe> elements that can enable the fullscreen flag, according to
the spec. (And my local tests confirm there really is no way, even in same
origin, to enable fullscreen from an <embed> or <object>-embedded document.)

[foolip, 2017-06-13 14:59:30]

lgtm % nit. It'd be great to also have web-platform-tests for this, checking
that document.fullscreenEnabled has the right value in various context, if they
don't already exist. Could be a follow-up though.

https://codereview.chromium.org/2923563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameElementTest.cpp (right):

https://codereview.chromium.org/2923563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameElementTest.cpp:32:
EXPECT_EQ(WebFeaturePolicyFeature::kFullscreen, container_policy[0].feature);
In addition to testing that the low-level fields were set correctly, is it
possible to test that the effective policy is to disallow, using the same style
of check as a call site in Fullscreen.cpp would?

https://codereview.chromium.org/2923563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2923563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:217: bool
can_override_fullscreen = true;
Maybe call this any_fullscreen_policy or similar (with inverted semantics, any
instead of none) to match how the comment is phrased?

[commit-bot: I haz the power, 2017-06-13 16:27:57]

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

[commit-bot: I haz the power, 2017-06-13 16:27:58]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-06-13 18:36:47]

https://codereview.chromium.org/2923563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameElementTest.cpp (right):

https://codereview.chromium.org/2923563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameElementTest.cpp:32:
EXPECT_EQ(WebFeaturePolicyFeature::kFullscreen, container_policy[0].feature);
On 2017/06/13 14:59:30, foolip wrote:
> In addition to testing that the low-level fields were set correctly, is it
> possible to test that the effective policy is to disallow, using the same
style
> of check as a call site in Fullscreen.cpp would?

Without actually loading a document into the frame, we don't have the final
composed policy, so we can't check it here :(. Browsertests would be a better
place to do that, and we do have browsertests for the container policy, though
not for this exact situation. I'm writing a CL with some other browsertests
around container policy, so I'll make sure that the same-origin
disabled-by-container case is handled.

This also gets caught by LayoutTests/fullscreen/full-screen-frameset.html as
soon as I re-enable Feature Policy for Fullscreen.

The combination of container policy with the header policy of the embedded frame
is covered by unit tests in content/common/feature_policy, which is why the
tests here are just to ensure that the correct container policy is set in the
first place.

https://codereview.chromium.org/2923563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2923563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:217: bool
can_override_fullscreen = true;
On 2017/06/13 14:59:30, foolip wrote:
> Maybe call this any_fullscreen_policy or similar (with inverted semantics, any
> instead of none) to match how the comment is phrased?

Done.

[iclelland, 2017-06-13 18:41:47]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2017-06-13 18:41:48]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2923563003/#ps100001
(title: "Addressing nits")

[commit-bot: I haz the power, 2017-06-13 18:42:19]

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-06-13 21:45:05]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497379307675760,
"parent_rev": "048e61d50c55b8708794673546b546de626c8e16", "commit_rev":
"53ecf9341f32b7f3746e6f721da9d4dc6839fb98"}

[commit-bot: I haz the power, 2017-06-13 21:45:16]

Description was changed from

==========
Move container policy logic to frame owner classes.

This specifically allows <frame> tags to have an exemption from supporting the
Fullscreen API, even in same-origin contexts, and even when the declared policy
would otherwise enable Fullscreen.

BUG=729658
==========

to

==========
Move container policy logic to frame owner classes.

This specifically allows <frame> tags to have an exemption from supporting the
Fullscreen API, even in same-origin contexts, and even when the declared policy
would otherwise enable Fullscreen.

BUG=729658

Review-Url: https://codereview.chromium.org/2923563003
Cr-Commit-Position: refs/heads/master@{#479165}
Committed:
https://chromium.googlesource.com/chromium/src/+/53ecf9341f32b7f3746e6f721da9...
==========

[commit-bot: I haz the power, 2017-06-13 21:45:17]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/53ecf9341f32b7f3746e6f721da9...