Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2680083002:
Initial Implementation of Iframe Attribute for Feature Policy (Part 1) (Closed)
|
Created:
3 years, 10 months ago by lunalu1 Modified:
3 years, 10 months ago CC:
jbroman, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, kinuko+watch, lunalu1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc: https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtPYxK30wWLaZ7Jslk/edit?usp=sharing
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute
and store featureNames in HTMLIFrameElement
Part 2: Pass 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
BUG=682256
Review-Url: https://codereview.chromium.org/2680083002
Cr-Commit-Position: refs/heads/master@{#449847}
Committed: https://chromium.googlesource.com/chromium/src/+/37f156fe308e0f249d087f48b046a93938be0cd9
Patch Set 1 #Patch Set 2 : Removed getAllowedFeatureNames() from HTMLIFrameElement #
Total comments: 21
Patch Set 3 : Codereview: nit, added RuntimeFeatures::FeaturePolicyExperimentalFeatures #
Total comments: 6
Patch Set 4 : Codereview: nit #Patch Set 5 : Moved hash function for WebFeaturePolicyFeature to where the enum is defined for ODR purpose #
Total comments: 9
Patch Set 6 : Codereview: nit, extracted a helper function to process error message for parsing DOMTokenList el… #Patch Set 7 : Update rebase #
Total comments: 5
Patch Set 8 : Used Vector (sorted, unique) instead of HashSet for the list of allowed feature names #Patch Set 9 : Codereview: nit + undid the change to generalize code for parseAllow/Permissions/Sandbox since ther… #Patch Set 10 : Nit: replace std::vector by WTF::Vector in the implementation of HTMLIFrameElementAllow::parseAllow… #
Total comments: 2
Patch Set 11 : Codereview: nit #Messages
Total messages: 69 (48 generated)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Description was changed from
==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc:
https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtP...
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute and
store featureNames in HTMLIFrameElement
BUG=682256
==========
to
==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc:
https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtP...
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute and
store featureNames in HTMLIFrameElement
BUG=682256
==========
lunalu@chromium.org changed reviewers: + iclelland@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Hi Ian, Could you please take a look at this CL. A few tests are failing because I need to add "allow" to a few global lists. I will do that asap. Thanks
Description was changed from
==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc:
https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtP...
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute and
store featureNames in HTMLIFrameElement
BUG=682256
==========
to
==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc:
https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtP...
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute and
store featureNames in HTMLIFrameElement
BUG=682256
==========
I think this is looking really good -- thanks for doing this! https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:232: // frameOwnerPropertiesChanged(); I presume this is for a future CL -- if so, can you change this to a "TODO: call frameOwnerPropertiesChanged after implementing XXX" (for some value of XXX)? https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:34: [CEReactions, PutForwards=value] readonly attribute DOMTokenList allow; We should add "RuntimeEnabled=FeaturePolicy" to this attribute to ensure that it's not available to JavaScript until we ship the allow attribute. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp:69: return isTokenSupported(tokenValue); Could this just be return FeaturePolicy::getWebFeaturePolicyFeature(token.getString()) != WebFeaturePolicyFeature::NotFound; and then we wouldn't need the separate function above? https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp:73: m_element->allowValueWasSet(); HTMLIframeElementPermissions.cpp has a check -- if(m_element) -- here. Do you know if that check is ever necessary, if m_element can ever be null? https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.h (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllowTest.cpp (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllowTest.cpp:1: // Copyright 2016 The Chromium Authors. All rights reserved. Nit: 2017 https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/WebFeaturePolicyFeatureHash.h (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/WebFeaturePolicyFeatureHash.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 here, too https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/WebFeaturePolicyFeatureHash.h:21: : UnsignedWithZeroKeyHashTraits<blink::WebFeaturePolicyFeature> { I think you could use a GenericHashTraits here, since we've explicitly reserved the zero-value for NotFound, it should actually be suitable as an empty value. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:159: static WebFeaturePolicyFeature getWebFeaturePolicyFeature( Can you add a comment for this? https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebFeaturePolicy.h (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebFeaturePolicy.h:25: enum class WebFeaturePolicyFeature { This certainly conflicts with https://codereview.chromium.org/2655663004/patch/100001/110010 -- it shouldn't be a problem, since the declarations are identical, but it's something to be aware of; whichever lands first will need to merge the changes from the other.
On 2017/02/08 00:12:44, loonybear wrote: > Hi Ian, > > Could you please take a look at this CL. > A few tests are failing because I need to add "allow" to a few global lists. I > will do that asap. I think it will be easier for you if you add the RuntimeEnabled extended IDL attribute to "allow" before doing this; that way it won't show up on the "stable" interface lists, just the ones where Feature Policy is enabled.
Thanks for reviewing the CL. I have made the change based on your comments. Please take another look. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:232: // frameOwnerPropertiesChanged(); On 2017/02/08 15:06:24, iclelland wrote: > I presume this is for a future CL -- if so, can you change this to a "TODO: call > frameOwnerPropertiesChanged after implementing XXX" (for some value of XXX)? Done. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:34: [CEReactions, PutForwards=value] readonly attribute DOMTokenList allow; On 2017/02/08 15:06:24, iclelland wrote: > We should add "RuntimeEnabled=FeaturePolicy" to this attribute to ensure that > it's not available to JavaScript until we ship the allow attribute. Done. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/08 15:06:24, iclelland wrote: > 2017 Done. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp:69: return isTokenSupported(tokenValue); On 2017/02/08 15:06:24, iclelland wrote: > Could this just be > > return FeaturePolicy::getWebFeaturePolicyFeature(token.getString()) != > WebFeaturePolicyFeature::NotFound; > > and then we wouldn't need the separate function above? Done. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp:73: m_element->allowValueWasSet(); On 2017/02/08 15:06:24, iclelland wrote: > HTMLIframeElementPermissions.cpp has a check -- if(m_element) -- here. Do you > know if that check is ever necessary, if m_element can ever be null? Done. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllowTest.cpp (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllowTest.cpp:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/08 15:06:24, iclelland wrote: > Nit: 2017 Done. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/WebFeaturePolicyFeatureHash.h (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/WebFeaturePolicyFeatureHash.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/08 15:06:24, iclelland wrote: > 2017 here, too Done. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/WebFeaturePolicyFeatureHash.h:21: : UnsignedWithZeroKeyHashTraits<blink::WebFeaturePolicyFeature> { On 2017/02/08 15:06:24, iclelland wrote: > I think you could use a GenericHashTraits here, since we've explicitly reserved > the zero-value for NotFound, it should actually be suitable as an empty value. Done. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:159: static WebFeaturePolicyFeature getWebFeaturePolicyFeature( On 2017/02/08 15:06:24, iclelland wrote: > Can you add a comment for this? Done. https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebFeaturePolicy.h (right): https://codereview.chromium.org/2680083002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebFeaturePolicy.h:25: enum class WebFeaturePolicyFeature { On 2017/02/08 15:06:24, iclelland wrote: > This certainly conflicts with > https://codereview.chromium.org/2655663004/patch/100001/110010 -- it shouldn't > be a problem, since the declarations are identical, but it's something to be > aware of; whichever lands first will need to merge the changes from the other. Done.
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, Luna -- just a couple more comments. I think that WebFeaturePolicyFeatureHash.h needs to be mentioned in a BUILD.gn file somewhere, even though it's just a header. https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right): https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:234: // frameOwnerPropertiesChanged(); You can just remove this commented-out code, with the TODO in place. https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right): https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:34: [CEReactions, PutForwards=value, RuntimeEnabled=FeaturePolicy] readonly attribute DOMTokenList allow; Should we remove the readonly modifier here? It's not in the spec yet, but I presume that we'd want to be able to set it at runtime. (And I thought that's what the JS-path tests were doing, but maybe I'm mistaken there)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
lunalu@chromium.org changed reviewers: + rbyers@chromium.org
Thanks Ian. Please take another look https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right): https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:234: // frameOwnerPropertiesChanged(); On 2017/02/08 20:11:35, iclelland wrote: > You can just remove this commented-out code, with the TODO in place. Done. https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right): https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:34: [CEReactions, PutForwards=value, RuntimeEnabled=FeaturePolicy] readonly attribute DOMTokenList allow; On 2017/02/08 20:11:35, iclelland wrote: > Should we remove the readonly modifier here? It's not in the spec yet, but I > presume that we'd want to be able to set it at runtime. (And I thought that's > what the JS-path tests were doing, but maybe I'm mistaken there) I believe this is what you need to do for DomTokenList. At least all current DomTokenList's in our codebase are readonly.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc:
https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtP...
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute and
store featureNames in HTMLIFrameElement
BUG=682256
==========
to
==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc:
https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtP...
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute
and store featureNames in HTMLIFrameElement
Part 2: Pass featureNames from HTMLIFrameElement to frame owner and
remote frame owner.
Part 3: Set iframe feature policy in FrameLoader::didBeginDocument
Part 4: Replace implementation of allowpaymentrequest by feature policy
Part 5: Replace implementation of allowfullscreen by feature policy
BUG=682256
==========
https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right): https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:34: [CEReactions, PutForwards=value, RuntimeEnabled=FeaturePolicy] readonly attribute DOMTokenList allow; On 2017/02/08 21:06:11, loonybear wrote: > On 2017/02/08 20:11:35, iclelland wrote: > > Should we remove the readonly modifier here? It's not in the spec yet, but I > > presume that we'd want to be able to set it at runtime. (And I thought that's > > what the JS-path tests were doing, but maybe I'm mistaken there) > > I believe this is what you need to do for DomTokenList. At least all current > DomTokenList's in our codebase are readonly. You can still add things to a readonly DOMTokenList using the 'add' method. You just can't swap it out for a new DOMTokenList. So yea, I think 'readonly' with PutForwards is right (that way assigning a string to right hand site will go through the DOMTokenList value setter).
https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right): https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:34: [CEReactions, PutForwards=value, RuntimeEnabled=FeaturePolicy] readonly attribute DOMTokenList allow; nit: put RuntimeEnabled first to be consistent with the others below (and make it more obvious) https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp (right): https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp:37: tokenErrors.append(numTokenErrors ? ", '" : "\'"); why escape the single-quote in one place but not the other? AFAIK no escape is necessary when inside double quotes like this. This code is basically copied from the similar code for 'sandbox', right? If there an opportunity (here or elsewhere) to avoid the duplication and share some common helper code somewhere? If that's strictly more complicated than what you've done then leaving it as-is is fine. But whenever we copy/paste we should ask ourselves if it really is the simplest way to express what we want. https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllowTest.cpp (right): https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllowTest.cpp:16: TEST(HTMLIFrameElementAllowTest, ParseAllowedFeatureNamesValid) { We're going to (eventually) need a web-platform-test for this (and the stuff covered by the other unit test). It's kind of nice having a unit test, but if it can be tested nearly as well from a testharness.js test then I'd suggest doing that instead (with a plan to upstream the tests once there is an official spec). https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebFeaturePolicy.h (right): https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebFeaturePolicy.h:28: enum class WebFeaturePolicyFeature { iclelland is landing this now in https://codereview.chromium.org/2655663004/, you'll want merge with his patch.
Description was changed from
==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc:
https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtP...
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute
and store featureNames in HTMLIFrameElement
Part 2: Pass featureNames from HTMLIFrameElement to frame owner and
remote frame owner.
Part 3: Set iframe feature policy in FrameLoader::didBeginDocument
Part 4: Replace implementation of allowpaymentrequest by feature policy
Part 5: Replace implementation of allowfullscreen by feature policy
BUG=682256
==========
to
==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc:
https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtP...
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute
and store featureNames in HTMLIFrameElement
Part 2: Pass 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
BUG=682256
==========
Hi Rick, Thanks for the comments. I made the change based on your comments, although I am a little skeptical about adding the helper function in HTMLIFrameElement to process the error message. I couldn't think of a good place to place it. Please let me know what you think. Thanks https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right): https://codereview.chromium.org/2680083002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:34: [CEReactions, PutForwards=value, RuntimeEnabled=FeaturePolicy] readonly attribute DOMTokenList allow; On 2017/02/09 19:00:35, Rick Byers wrote: > On 2017/02/08 21:06:11, loonybear wrote: > > On 2017/02/08 20:11:35, iclelland wrote: > > > Should we remove the readonly modifier here? It's not in the spec yet, but I > > > presume that we'd want to be able to set it at runtime. (And I thought > that's > > > what the JS-path tests were doing, but maybe I'm mistaken there) > > > > I believe this is what you need to do for DomTokenList. At least all current > > DomTokenList's in our codebase are readonly. > > You can still add things to a readonly DOMTokenList using the 'add' method. You > just can't swap it out for a new DOMTokenList. So yea, I think 'readonly' with > PutForwards is right (that way assigning a string to right hand site will go > through the DOMTokenList value setter). Done. https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right): https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:34: [CEReactions, PutForwards=value, RuntimeEnabled=FeaturePolicy] readonly attribute DOMTokenList allow; On 2017/02/09 20:59:58, Rick Byers wrote: > nit: put RuntimeEnabled first to be consistent with the others below (and make > it more obvious) My bad, should have noticed that. Thanks https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp (right): https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp:37: tokenErrors.append(numTokenErrors ? ", '" : "\'"); On 2017/02/09 20:59:59, Rick Byers wrote: > why escape the single-quote in one place but not the other? AFAIK no escape is > necessary when inside double quotes like this. > > This code is basically copied from the similar code for 'sandbox', right? If > there an opportunity (here or elsewhere) to avoid the duplication and share some > common helper code somewhere? If that's strictly more complicated than what > you've done then leaving it as-is is fine. But whenever we copy/paste we should > ask ourselves if it really is the simplest way to express what we want. Good point! We can have a helper function that process the error messages. https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllowTest.cpp (right): https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllowTest.cpp:16: TEST(HTMLIFrameElementAllowTest, ParseAllowedFeatureNamesValid) { On 2017/02/09 20:59:59, Rick Byers wrote: > We're going to (eventually) need a web-platform-test for this (and the stuff > covered by the other unit test). It's kind of nice having a unit test, but if > it can be tested nearly as well from a testharness.js test then I'd suggest > doing that instead (with a plan to upstream the tests once there is an official > spec). Yes, I was planning to do the layout tests (I mean web-platform-test :P) once the entire feature policy iframe attribute is implemented. This test is only testing the sanity of adding DOMTokenList attribute allow and how it is correctly parses into a set of enums. I guess once part 5 is done. I can remove those tests and add web-platform-tests instead? https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebFeaturePolicy.h (right): https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebFeaturePolicy.h:28: enum class WebFeaturePolicyFeature { On 2017/02/09 20:59:59, Rick Byers wrote: > iclelland is landing this now in https://codereview.chromium.org/2655663004/, > you'll want merge with his patch. Yeah, but I think it might be easier for me to land this first because his CL is moving stuff around and might make lots of conflicts with my CL.
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Waiting for https://codereview.chromium.org/2687923002/ to land, so that I can remove the hash traits added for enum WebFeaturePolicyFeature
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:120001) has been deleted
LGTM from a feature-policy perspective
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllowTest.cpp (right): https://codereview.chromium.org/2680083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLIFrameElementAllowTest.cpp:16: TEST(HTMLIFrameElementAllowTest, ParseAllowedFeatureNamesValid) { On 2017/02/09 23:19:04, loonybear wrote: > On 2017/02/09 20:59:59, Rick Byers wrote: > > We're going to (eventually) need a web-platform-test for this (and the stuff > > covered by the other unit test). It's kind of nice having a unit test, but if > > it can be tested nearly as well from a testharness.js test then I'd suggest > > doing that instead (with a plan to upstream the tests once there is an > official > > spec). > > Yes, I was planning to do the layout tests (I mean web-platform-test :P) once > the entire feature policy iframe attribute is implemented. > This test is only testing the sanity of adding DOMTokenList attribute allow and > how it is correctly parses into a set of enums. > I guess once part 5 is done. I can remove those tests and add web-platform-tests > instead? Ok SGTM. Maybe just add a "// TODO(lunalu) Move to web-platform-tests. crbug.com/XXX"? https://codereview.chromium.org/2680083002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right): https://codereview.chromium.org/2680083002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.h:59: static void addTokenError(StringBuilder& tokenErrors, I think hanging this off HTMLIFrameElement is fine (though I'd put the implementation in the C++ file, and ideally make it a private instance method instead of a public static). But there's still a lot of redundant logic between the different cases. I was thinking something more like a template method over some enum T which takes a DOMTokenList, an error-message substring like "feature names", a parsing functor that attempts to convert a string to a 'T', and just returns the Vector<T> with all the parsed enums (and generates the appropriate console warning if any fail to parse). But I haven't studied the three different cases in as much detail as you have - perhaps there's not as much in common here as it naively seems? But the general concept of "turn a DOMTokenList into a set of enums, generating an appropriate error if any are invalid" seems like a reasonable abstraction to me. WDYT? BTW I'm not sure offhand what the best style is for the parsing functor - maybe just a callback function, or maybe we have a common pattern we should copy. https://codereview.chromium.org/2680083002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right): https://codereview.chromium.org/2680083002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:33: // Feature Policy allow attribute sorry, one more nit - include a https://wicg.github.io/feature-policy/ link here and move this down to, eg., just below the 'csp' line. In general we try to put spec links above everything defined in that spec (so eg. contentWindow below is still covered by the spec link on line 21).
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Rick, Thanks for your comments. Please see the comment below why what you were suggesting won't work. I also made some changes to how the list of parsed feature name is created. So please take another look at that as well. Cheers, https://codereview.chromium.org/2680083002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right): https://codereview.chromium.org/2680083002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.h:59: static void addTokenError(StringBuilder& tokenErrors, On 2017/02/10 22:09:20, Rick Byers wrote: > I think hanging this off HTMLIFrameElement is fine (though I'd put the > implementation in the C++ file, and ideally make it a private instance method > instead of a public static). But there's still a lot of redundant logic between > the different cases. > > I was thinking something more like a template method over some enum T which > takes a DOMTokenList, an error-message substring like "feature names", a parsing > functor that attempts to convert a string to a 'T', and just returns the > Vector<T> with all the parsed enums (and generates the appropriate console > warning if any fail to parse). But I haven't studied the three different cases > in as much detail as you have - perhaps there's not as much in common here as it > naively seems? But the general concept of "turn a DOMTokenList into a set of > enums, generating an appropriate error if any are invalid" seems like a > reasonable abstraction to me. WDYT? > > BTW I'm not sure offhand what the best style is for the parsing functor - maybe > just a callback function, or maybe we have a common pattern we should copy. It is not as trivial as it seems. parseSandboxPolicy returns an int (a bit map), and if you look at the code in core/dom/SandboxFlags.cpp, there is no trivial parsing functor that will serve this purpose. I can sort of generalize parseDelegatedPermissions and parseAllowedFeatureNames, but I will not only need a parsing functor (string -> enum), but also a validating functor on the returend enum to verify if each string token was valid or not. I also changed the implementation for parseAllowedFeatureNames (I want a unique set of feature names, so I have to sort the feature names first, which can't be generalized). We can write a base class for the three token lists, but the only methods can be easily generalized are: constructor and destructor. If we want to make the helper function a private method, then we have to move it to DOMTokenList. HTMLIFrameElement owns the three DOMTokenList but there is no easy way for the three token lists to access a private method of HTMLIFrameElement. But keeping the helper function in HTMLIFrameElement doesn't really make sense to me. Even though it will cause duplicated code, I think it is still better to write them out individually. WDYT? https://codereview.chromium.org/2680083002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.idl (right): https://codereview.chromium.org/2680083002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.idl:33: // Feature Policy allow attribute On 2017/02/10 22:09:21, Rick Byers wrote: > sorry, one more nit - include a https://wicg.github.io/feature-policy/ link here > and move this down to, eg., just below the 'csp' line. In general we try to put > spec links above everything defined in that spec (so eg. contentWindow below is > still covered by the spec link on line 21). Will do. Thanks
Hi Rick, Thanks for your comments. Please see the comment below why what you were suggesting won't work. I also made some changes to how the list of parsed feature name is created. So please take another look at that as well. Cheers,
LGTM https://codereview.chromium.org/2680083002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right): https://codereview.chromium.org/2680083002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.h:59: static void addTokenError(StringBuilder& tokenErrors, On 2017/02/10 23:04:12, loonybear wrote: > On 2017/02/10 22:09:20, Rick Byers wrote: > > I think hanging this off HTMLIFrameElement is fine (though I'd put the > > implementation in the C++ file, and ideally make it a private instance method > > instead of a public static). But there's still a lot of redundant logic > between > > the different cases. > > > > I was thinking something more like a template method over some enum T which > > takes a DOMTokenList, an error-message substring like "feature names", a > parsing > > functor that attempts to convert a string to a 'T', and just returns the > > Vector<T> with all the parsed enums (and generates the appropriate console > > warning if any fail to parse). But I haven't studied the three different > cases > > in as much detail as you have - perhaps there's not as much in common here as > it > > naively seems? But the general concept of "turn a DOMTokenList into a set of > > enums, generating an appropriate error if any are invalid" seems like a > > reasonable abstraction to me. WDYT? > > > > BTW I'm not sure offhand what the best style is for the parsing functor - > maybe > > just a callback function, or maybe we have a common pattern we should copy. > > It is not as trivial as it seems. > > parseSandboxPolicy returns an int (a bit map), and if you look at the code in > core/dom/SandboxFlags.cpp, there is no trivial parsing functor that will serve > this purpose. > > I can sort of generalize parseDelegatedPermissions and parseAllowedFeatureNames, > but I will not only need a parsing functor (string -> enum), but also a > validating functor on the returend enum to verify if each string token was valid > or not. > > I also changed the implementation for parseAllowedFeatureNames (I want a unique > set of feature names, so I have to sort the feature names first, which can't be > generalized). > > We can write a base class for the three token lists, but the only methods can be > easily generalized are: constructor and destructor. > > If we want to make the helper function a private method, then we have to move it > to DOMTokenList. HTMLIFrameElement owns the three DOMTokenList but there is no > easy way for the three token lists to access a private method of > HTMLIFrameElement. > > But keeping the helper function in HTMLIFrameElement doesn't really make sense > to me. Even though it will cause duplicated code, I think it is still better to > write them out individually. > > WDYT? Thanks for digging into this! Ok, I see how this isn't easy, and this duplicated logic is straight forward enough that I don't think it's worth any more than the time we've already put in to try to unify it. Sorry to waste your time on it, thanks for exploring the options!
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iclelland@chromium.org Link to the patchset: https://codereview.chromium.org/2680083002/#ps180001 (title: "Codereview: nit + undid the change to generalize code for parseAllow/Permissions/Sandbox since ther…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by lunalu@chromium.org
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iclelland@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2680083002/#ps200001 (title: "Nit: replace std::vector by WTF::Vector in the implementation of HTMLIFrameElementAllow::parseAllow…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by lunalu@chromium.org
jbroman@chromium.org changed reviewers: + jbroman@chromium.org
https://codereview.chromium.org/2680083002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp (right): https://codereview.chromium.org/2680083002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp:57: for (auto const feature : featureNames) { nit: STL has an efficient uniqueness thing that is more efficient: std::sort(featureNames.begin(), featureNames.end()); auto it = std::unique(featureNames.begin(), featureNames.end()); featureNames.shrink(featureNames.end() - it); return featureNames; // then allowedFeatureNames is redundant
https://codereview.chromium.org/2680083002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp (right): https://codereview.chromium.org/2680083002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElementAllow.cpp:57: for (auto const feature : featureNames) { On 2017/02/11 at 05:34:29, jbroman wrote: > nit: STL has an efficient uniqueness thing that is more efficient: > > std::sort(featureNames.begin(), featureNames.end()); > auto it = std::unique(featureNames.begin(), featureNames.end()); > featureNames.shrink(featureNames.end() - it); This line should be: featureNames.shrink(it - featureNames.begin()); > return featureNames; > > // then allowedFeatureNames is redundant
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iclelland@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2680083002/#ps220001 (title: "Codereview: nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1486791666763740,
"parent_rev": "ebc34ac470e32c079aad209b5eff7d0a06d13182", "commit_rev":
"37f156fe308e0f249d087f48b046a93938be0cd9"}
Message was sent while issue was closed.
Description was changed from
==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc:
https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtP...
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute
and store featureNames in HTMLIFrameElement
Part 2: Pass 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
BUG=682256
==========
to
==========
Initial Implementation of Iframe Attribute for Feature Policy (Part 1)
Enable iframe feature policy by attributes:
allow="feature1 feature2", allowfullscreen, and allowpaymentrequest.
See design doc:
https://docs.google.com/a/google.com/document/d/1ZTuCRKNnWKQUn8gzufuRsCnjVXtP...
Part 1: Introduce iframe allowAttr in HTMLIFrameElement::parseAttribute
and store featureNames in HTMLIFrameElement
Part 2: Pass 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
BUG=682256
Review-Url: https://codereview.chromium.org/2680083002
Cr-Commit-Position: refs/heads/master@{#449847}
Committed:
https://chromium.googlesource.com/chromium/src/+/37f156fe308e0f249d087f48b046...
==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/37f156fe308e0f249d087f48b046... |
