Initial Implementation of Iframe Attribute for Feature Policy (Part 4)

Initial Implementation of Iframe Attribute for Feature Policy (Part 4)

Replace implementation of allowpaymentrequest and allowfullscreen by feature policy.

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

See design doc: https://docs.google.com/a/chromium.org/document/d/1sskoBi7Ba7hLuuiJQ6VMQ1KYIGBP8Lr87hLbkF76c2I/edit?usp=sharing

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/)
3.b Rename WebParsedFeaturePolicyHeader to WebParsedFeaturePolicy
(CL: https://codereview.chromium.org/2738953002/)

Part 4: Replace implementation of allowpaymentrequest and allowfullscreen by feature policy.

BUG=682256
Review-Url: https://codereview.chromium.org/2767983003
Cr-Commit-Position: refs/heads/master@{#467699}
Committed: https://chromium.googlesource.com/chromium/src/+/44b442a4f245e3fbfa17478bf0807cc139ab39ef

[lunalu1, 2017-03-22 20:30:40]

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

[commit-bot: I haz the power, 2017-03-22 20:31: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-22 20:31:25]

Description was changed from

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 4)

Replace implementation of allowpaymentrequest and allowfullscreen by
feature policy.

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

to

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 4)

Replace implementation of allowpaymentrequest and allowfullscreen by feature
policy.

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/)
3.b Rename WebParsedFeaturePolicyHeader to WebParsedFeaturePolicy
(CL: https://codereview.chromium.org/2738953002/)

Part 4: Replace implementation of allowpaymentrequest and allowfullscreen by
feature policy.

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

[lunalu1, 2017-03-22 21:06:20]

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

[commit-bot: I haz the power, 2017-03-22 21:06:38]

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-22 22:41:09]

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

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

Dry run: This issue passed the CQ dry run.

[lunalu1, 2017-03-23 00:22:13]

Patchset #1 (id:1) has been deleted

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

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

[commit-bot: I haz the power, 2017-03-28 15:57:50]

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-28 15:58:10]

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

[lunalu1, 2017-03-28 15:58:11]

Hi Ian, 

I added some layout tests for container policy in this patch too. Please lemme
know if I should split this into a separate CL.

Thanks,
Luna

[lunalu1, 2017-03-28 17:32:53]

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

[lunalu1, 2017-03-28 17:33:03]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-28 17:33: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-28 18:39:23]

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

[commit-bot: I haz the power, 2017-03-28 18:39:24]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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-28 19:53:08]

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

[commit-bot: I haz the power, 2017-03-28 19:53:38]

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-28 21:14:58]

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

[commit-bot: I haz the power, 2017-03-28 21:15:00]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[iclelland, 2017-04-11 19:04:21]

Since the great blink rename, you'll need to update this to follow the new
style.

I've left a few comments on the getContainerPolicyFromAllowedFeatures method,
but I think this is looking pretty good, thanks!

https://codereview.chromium.org/2767983003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2767983003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:142:
DLOG(WARNING) << origin->toString();
Nit: Remove the warning

https://codereview.chromium.org/2767983003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:144: // If
allowfullscreen attribute or allowpayment attribute is(are) present,
I think you can get away with just using "are" here

(Or you could re-word it into two different comments: "If the allowfullscreen
attribute is present, enable the feature for all origins", and similarly for
allowpaymentrequest.)

https://codereview.chromium.org/2767983003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:147:
whitelists);
Could you move the if(isFeatureAllowed) out of |addAllowFeatureToList|, and just
check them here? It would make it clearer that we only do something if the
condition is true.

https://codereview.chromium.org/2767983003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:151: //
allow policy will override the allow* attribute policy.
This logic is really hard to follow; it took me a while to figure out why it's
hard coding the idea of "first" and "second".

Could we make it clearer by adding the "allow" policies first, and if you see
fullscreen (or paymentrequest) in there, just set a flag. Then afterwards, you
can process the "allowfullscreen" and "allowpaymentrequest" policies, and ignore
them if their flag is set. Would that make sense?

[lunalu1, 2017-04-21 21:35:57]

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

[commit-bot: I haz the power, 2017-04-21 21:36:30]

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-04-21 23:33:11]

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

[commit-bot: I haz the power, 2017-04-21 23:33:12]

Dry run: This issue passed the CQ dry run.

[lunalu1, 2017-04-24 20:23:39]

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

[lunalu1, 2017-04-24 20:24:21]

Hi Ian. 
I added container policy tests for vibrate and made changes based on your
previous review. Please take another look.

Thanks!

[commit-bot: I haz the power, 2017-04-24 20:24: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-04-24 22:00:40]

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

[commit-bot: I haz the power, 2017-04-24 22:00:41]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-04-25 17:19:55]

This is looking great, thanks! Just a couple of comments.

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

https://codereview.chromium.org/2767983003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:84: if
(DeprecatedEqualIgnoringCase(target_string, "self")) {
Is there a simple replacement for the recently-deprecated method? I suppose we
could just lowercase target_string and then compare to 'self'; we'd just have to
make sure that the lowercasing method is utf8-safe

https://codereview.chromium.org/2767983003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:113: // If
allowfullscreen attribute is present, enable the feature for all
I still think we could make this simpler -- could it be structured something
like this? (this is extremely sketchy, but just the general outline):

bool override_payment = false;
bool override_fullscreen = false;
for (feature : features) {
  if (feature == kPayment) override_payment = true;
  if (feature == kFullscreen) override_fullscreen = true;
  // Add "allow" feature to whitelists for |origin| here
}
if (allowpayment && !override_payment) AddAllowFeatureToList(kPayment,
whitelists);
if (allowfullscreen && !override_fullscreen) AddAllowFeatureToList(kFullscreen,
whitelists);

https://codereview.chromium.org/2767983003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:159:
default_feature_name_map.Set("eme", WebFeaturePolicyFeature::kEme);
This one is new with this CL -- ddorwin@ asked for the feature name string to be
spelled "encrypted-media" in https://github.com/WICG/feature-policy/issues/40 --
can you change it to that here?

https://codereview.chromium.org/2767983003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:168:
default_feature_name_map.Set("docwrit",
typo: "docwrite"

https://codereview.chromium.org/2767983003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp
(right):

https://codereview.chromium.org/2767983003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp:155:
TEST_F(FeaturePolicyTest, ParseContainerPolicyWithAllowAttributes) {
Can you add two more similar tests here? One where only allowfullscreen is true,
and one where only allowpaymentrequest is true? (Without the corresponding allow
attributes?) I'd like to have coverage that the right flags are being tested.

[lunalu1, 2017-04-25 17:55:47]

Thanks Ian. PTAL

[lunalu1, 2017-04-25 18:31:10]

Patchset #6 (id:140001) has been deleted

[lunalu1, 2017-04-25 18:31:13]

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

[lunalu1, 2017-04-25 18:31:42]

Hi Ian,
Sorry I missed some of your comments. Please review this patch instead.

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

https://codereview.chromium.org/2767983003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:84: if
(DeprecatedEqualIgnoringCase(target_string, "self")) {
On 2017/04/25 at 17:19:54, iclelland wrote:
> Is there a simple replacement for the recently-deprecated method? I suppose we
could just lowercase target_string and then compare to 'self'; we'd just have to
make sure that the lowercasing method is utf8-safe

Replaced by EqualIgnoringASCIICase

[commit-bot: I haz the power, 2017-04-25 18:32:26]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-04-25 18:38:05]

Thanks for the update! This LGTM

[commit-bot: I haz the power, 2017-04-25 20:13:37]

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

[commit-bot: I haz the power, 2017-04-25 20:13:38]

Dry run: This issue passed the CQ dry run.

[lunalu1, 2017-04-25 21:10:41]

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

[lunalu1, 2017-04-25 21:10:42]

Hi haraken, could you please owner approve this CL?

Files to be reviewed:
  third_party/WebKit/Source/core/dom/Fullscreen.cpp
  third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp
  third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
  third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
  third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h
  third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp
  third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp

[haraken, 2017-04-26 02:34:34]

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

https://codereview.chromium.org/2767983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:103: bool
allowpayment,

This looks a bit too ad-hoc to me. Is there any way to avoid hand-writing these
features here?

(In general I'm a bit concerned that FeaturePolicy & OriginTrials are
introducing a bunch of hand-written code.)

[iclelland, 2017-04-26 02:59:47]

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

https://codereview.chromium.org/2767983003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:103: bool
allowpayment,
On 2017/04/26 02:34:34, haraken wrote:
> 
> This looks a bit too ad-hoc to me. Is there any way to avoid hand-writing
these
> features here?
> 
> (In general I'm a bit concerned that FeaturePolicy & OriginTrials are
> introducing a bunch of hand-written code.)

I see how this could seem like we're requiring ad-hoc code for every feature;
however, these are the only two such defined attributes, and they are present
for compatibility, since allowfullscreen and allowpaymentrequest have
pre-existing behavior that we need to continue to support. (In the spec, this is
called out in https://wicg.github.io/feature-policy/#legacy-attributes)

The intention is that no other features in the future will require this
treatment. All of the other features are supported purely by the "allow" iframe
attribute, which is passed here as the |features| vector.

[haraken, 2017-04-26 03:07:35]

On 2017/04/26 02:59:47, iclelland wrote:
>
https://codereview.chromium.org/2767983003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
> (right):
> 
>
https://codereview.chromium.org/2767983003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:103: bool
> allowpayment,
> On 2017/04/26 02:34:34, haraken wrote:
> > 
> > This looks a bit too ad-hoc to me. Is there any way to avoid hand-writing
> these
> > features here?
> > 
> > (In general I'm a bit concerned that FeaturePolicy & OriginTrials are
> > introducing a bunch of hand-written code.)
> 
> I see how this could seem like we're requiring ad-hoc code for every feature;
> however, these are the only two such defined attributes, and they are present
> for compatibility, since allowfullscreen and allowpaymentrequest have
> pre-existing behavior that we need to continue to support. (In the spec, this
is
> called out in https://wicg.github.io/feature-policy/#legacy-attributes)

Ah, this sounds reasonable. Let's add the spec link as a comment.

LGTM.

[lunalu1, 2017-04-27 14:03:03]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-04-27 14:03:33]

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-04-27 16:09:44]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493301783814310,
"parent_rev": "7e61913aaeda21fcfdbefb435d6085efdc1aa9fb", "commit_rev":
"44b442a4f245e3fbfa17478bf0807cc139ab39ef"}

[commit-bot: I haz the power, 2017-04-27 16:11:10]

Description was changed from

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 4)

Replace implementation of allowpaymentrequest and allowfullscreen by feature
policy.

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/)
3.b Rename WebParsedFeaturePolicyHeader to WebParsedFeaturePolicy
(CL: https://codereview.chromium.org/2738953002/)

Part 4: Replace implementation of allowpaymentrequest and allowfullscreen by
feature policy.

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

to

==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 4)

Replace implementation of allowpaymentrequest and allowfullscreen by feature
policy.

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/)
3.b Rename WebParsedFeaturePolicyHeader to WebParsedFeaturePolicy
(CL: https://codereview.chromium.org/2738953002/)

Part 4: Replace implementation of allowpaymentrequest and allowfullscreen by
feature policy.

BUG=682256

Review-Url: https://codereview.chromium.org/2767983003
Cr-Commit-Position: refs/heads/master@{#467699}
Committed:
https://chromium.googlesource.com/chromium/src/+/44b442a4f245e3fbfa17478bf080...
==========

[commit-bot: I haz the power, 2017-04-27 16:11:11]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/44b442a4f245e3fbfa17478bf080...