|
|
Created:
3 years, 9 months ago by lunalu1 Modified:
3 years, 9 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, blink-reviews-frames_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dcheng, dglazkov+blink, eae+blinkwatch, iclelland, jam, kinuko+watch, lunalu1, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, rwlbuis, sof Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial Implementation of Iframe Attribute for Feature Policy (Part 3)
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.a: Replace implementation of allowpaymentrequest by feature policy
Part 4.b: Replace implementation of allowfullscreen by feature policy
Part 5: WebVR
WebParsedFeaturePolicyHeader is renamed because this DS will
be shared between both header policy and container policy
and in the future meta tag policy and container header policy.
BUG=682256
Review-Url: https://codereview.chromium.org/2739203002
Cr-Commit-Position: refs/heads/master@{#458425}
Committed: https://chromium.googlesource.com/chromium/src/+/1a466f064ef609c227ef1fa840ec812591af053b
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebase update #Patch Set 3 : Codereview: added TODO comment for implementing allow* attributes in the container policy parser #
Total comments: 3
Patch Set 4 : Codereview: nit #
Total comments: 1
Patch Set 5 : Rebase update #
Total comments: 1
Patch Set 6 : Clean up after rebase #
Total comments: 2
Patch Set 7 : Bug fix: frame owner does not always exist. #
Total comments: 3
Messages
Total messages: 68 (45 generated)
lunalu@chromium.org changed reviewers: + iclelland@chromium.org
lunalu@chromium.org changed reviewers: + iclelland@chromium.org
Hi Ian, once you reviewed the other 2 CL (3.a, and 3.b), please take a look at this CL. Please note: the renaming part is patched from 3.b
Hi Ian, once you reviewed the other 2 CL (3.a, and 3.b), please take a look at this CL. Please note: the renaming part is patched from 3.b
I think this is looking pretty good -- do you think you could rebase this now that the CLs it depends on have landed? That would make the changes in this CL more apparent. Thanks! https://codereview.chromium.org/2739203002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2739203002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:117: const WebVector<WebFeaturePolicyFeature>& features, In part 4, I think this method should also take a couple of bools -- in order to handle the "allowfullscreen" and "allowpayment" attributes, which need to be treated differently than 'allow="..."', maybe we should pass them in here. If that's the case, can you add a TODO here, so that it's clear that that's what's going to happen?
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
Added TODO comments as suggested. PTAL
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Almost there -- looks great, other than a couple of nits and what looks like some unrelated code that made it in as part of the rebase https://codereview.chromium.org/2739203002/diff/40001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2739203002/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:746: double GetFrameDeviceScaleFactor(const ToRenderFrameHost& adapter) { Do you know why this showed up in this CL? It looks totally unrelated. https://codereview.chromium.org/2739203002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2739203002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:126: Vector<WebSecurityOrigin>(static_cast<size_t>(1), {origin}); I think that "1UL" is usually enough to avoid the implicit static_cast on a constant like that. https://codereview.chromium.org/2739203002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h (left): https://codereview.chromium.org/2739203002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h:33: } // namespace blink Nit: blank line before closing the namespace
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Thanks. PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Other than the one unrelated change that's still sticking around, this LGTM https://codereview.chromium.org/2739203002/diff/60001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (left): https://codereview.chromium.org/2739203002/diff/60001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:238: double GetFrameDeviceScaleFactor(const ToRenderFrameHost& adapter) { This is still a bit weird -- I think you might need to reset this file; bring it back to tip-of-tree. It shouldn't actually appear in this CL, I think.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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 #5 (id:80001) has been deleted
lunalu@chromium.org changed reviewers: + jochen@chromium.org
Hi jochen, would you mind owner approve this CL? Thanks
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
the CL appears to be very crashy, and sadly it's not obvious from the bots why. Would you mind investigating this? Not sure whether it's a small change only or something is fundamentally missing https://codereview.chromium.org/2739203002/diff/100001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2739203002/diff/100001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:737: double GetFrameDeviceScaleFactor(const ToRenderFrameHost& adapter) { why did you move this?
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Patchset #6 (id:120001) has been deleted
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
dglazkov@chromium.org changed reviewers: + dglazkov@chromium.org
https://codereview.chromium.org/2739203002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2739203002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:622: m_frame->owner()->allowedFeatures(), It crashes here.
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Patchset #7 (id:160001) has been deleted
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Fixed it. Thanks https://codereview.chromium.org/2739203002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2739203002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:622: m_frame->owner()->allowedFeatures(), On 2017/03/20 16:49:50, dglazkov wrote: > It crashes here. Thanks! My bad, frame owner doesn't always exit.
lgtm
https://codereview.chromium.org/2739203002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2739203002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:621: if (m_frame->owner()) { I wonder if even need to do anything with feature policy in situations when we don't have a frame owner?
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/2739203002/#ps180001 (title: "Bug fix: frame owner does not always exist.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Please see my comment below. https://codereview.chromium.org/2739203002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2739203002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:621: if (m_frame->owner()) { On 2017/03/20 20:57:12, dglazkov wrote: > I wonder if even need to do anything with feature policy in situations when we > don't have a frame owner? There are two ways to define feature policy. One is though HTTP header (in this case, a frame owner is not required). The other is through iframe attribute "allow" (see this CL). icelland@ please correct me if I am wrong
https://codereview.chromium.org/2739203002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2739203002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:621: if (m_frame->owner()) { On 2017/03/20 at 21:07:01, loonybear wrote: > On 2017/03/20 20:57:12, dglazkov wrote: > > I wonder if even need to do anything with feature policy in situations when we > > don't have a frame owner? > > There are two ways to define feature policy. One is though HTTP header (in this case, a frame owner is not required). The other is through iframe attribute "allow" (see this CL). icelland@ please correct me if I am wrong I could be wrong, but if I remember correctly, the only time when FrameOwner is nullptr is when it's an SVGImage or a popup (or maybe something similar). Would we still need to initialize feature policy for those?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I think we do need to initialize *some* policy in those cases, both of those are ExecutionContexts. For the moment, I think that using nullptr for the container policy (or equivalently, an empty policy) is probably the right thing to do; it will essentially treat them as top-level documents. On Mar 20, 2017 6:18 PM, <dglazkov@chromium.org> wrote: https://codereview.chromium.org/2739203002/diff/180001/ third_party/WebKit/Source/core/loader/FrameLoader.cpp File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2739203002/diff/180001/ third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode621 third_party/WebKit/Source/core/loader/FrameLoader.cpp:621: if (m_frame->owner()) { On 2017/03/20 at 21:07:01, loonybear wrote: > On 2017/03/20 20:57:12, dglazkov wrote: > > I wonder if even need to do anything with feature policy in situations when we > > don't have a frame owner? > > There are two ways to define feature policy. One is though HTTP header (in this case, a frame owner is not required). The other is through iframe attribute "allow" (see this CL). icelland@ please correct me if I am wrong I could be wrong, but if I remember correctly, the only time when FrameOwner is nullptr is when it's an SVGImage or a popup (or maybe something similar). Would we still need to initialize feature policy for those? https://codereview.chromium.org/2739203002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I think we do need to initialize *some* policy in those cases, both of those are ExecutionContexts. For the moment, I think that using nullptr for the container policy (or equivalently, an empty policy) is probably the right thing to do; it will essentially treat them as top-level documents. On Mar 20, 2017 6:18 PM, <dglazkov@chromium.org> wrote: https://codereview.chromium.org/2739203002/diff/180001/ third_party/WebKit/Source/core/loader/FrameLoader.cpp File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2739203002/diff/180001/ third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode621 third_party/WebKit/Source/core/loader/FrameLoader.cpp:621: if (m_frame->owner()) { On 2017/03/20 at 21:07:01, loonybear wrote: > On 2017/03/20 20:57:12, dglazkov wrote: > > I wonder if even need to do anything with feature policy in situations when we > > don't have a frame owner? > > There are two ways to define feature policy. One is though HTTP header (in this case, a frame owner is not required). The other is through iframe attribute "allow" (see this CL). icelland@ please correct me if I am wrong I could be wrong, but if I remember correctly, the only time when FrameOwner is nullptr is when it's an SVGImage or a popup (or maybe something similar). Would we still need to initialize feature policy for those? https://codereview.chromium.org/2739203002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/20 at 22:59:51, iclelland wrote: > I think we do need to initialize *some* policy in those cases, both of > those are ExecutionContexts. For the moment, I think that using nullptr for > the container policy (or equivalently, an empty policy) is probably the > right thing to do; it will essentially treat them as top-level documents. SG.
The CQ bit was checked by lunalu@chromium.org
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": 180001, "attempt_start_ts": 1490106458349600, "parent_rev": "588b7bac5c4289654f11b6834e53c9de4177dd06", "commit_rev": "1a466f064ef609c227ef1fa840ec812591af053b"}
Message was sent while issue was closed.
Description was changed from ========== Initial Implementation of Iframe Attribute for Feature Policy (Part 3) 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.a: Replace implementation of allowpaymentrequest by feature policy Part 4.b: Replace implementation of allowfullscreen by feature policy Part 5: WebVR WebParsedFeaturePolicyHeader is renamed because this DS will be shared between both header policy and container policy and in the future meta tag policy and container header policy. BUG=682256 ========== to ========== Initial Implementation of Iframe Attribute for Feature Policy (Part 3) 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.a: Replace implementation of allowpaymentrequest by feature policy Part 4.b: Replace implementation of allowfullscreen by feature policy Part 5: WebVR WebParsedFeaturePolicyHeader is renamed because this DS will be shared between both header policy and container policy and in the future meta tag policy and container header policy. BUG=682256 Review-Url: https://codereview.chromium.org/2739203002 Cr-Commit-Position: refs/heads/master@{#458425} Committed: https://chromium.googlesource.com/chromium/src/+/1a466f064ef609c227ef1fa840ec... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/1a466f064ef609c227ef1fa840ec... |