Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2499373002:
Implementation for feature policy - fullscreen (Closed)
DescriptionImplementation for feature policy - fullscreen
Disable fullscreen API unless enabled through feature policy.
BUG=666761
Committed: https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad
Cr-Commit-Position: refs/heads/master@{#436651}
Patch Set 1 : Initial checkin #Patch Set 2 : Modified layout tests #
Total comments: 4
Patch Set 3 : Modified layout tests #
Total comments: 21
Patch Set 4 : Codereview update and added layout tests #
Total comments: 14
Patch Set 5 : Codereview update #
Total comments: 4
Patch Set 6 : Codereview update #
Total comments: 4
Patch Set 7 : Codereview update #Patch Set 8 : Bug fix: handling the case when frame parent does not exist #
Total comments: 1
Messages
Total messages: 97 (69 generated)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Patchset #1 (id:1) 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...)
Description was changed from ========== Implementation for feature policy - fullscreen Disable fullscreen API unless enabled through feature policy. BUG=623682 ========== to ========== Implementation for feature policy - fullscreen Disable fullscreen API unless enabled through feature policy. BUG=623682 ==========
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== Implementation for feature policy - fullscreen Disable fullscreen API unless enabled through feature policy. BUG=623682 ========== to ========== Implementation for feature policy - fullscreen Disable fullscreen API unless enabled through feature policy. BUG=666761 ==========
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_asan_rel_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
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
Patchset #6 (id:120001) has been deleted
foolip@chromium.org changed reviewers: + foolip@chromium.org
https://codereview.chromium.org/2499373002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php (right): https://codereview.chromium.org/2499373002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php:28: <iframe id="f3" src="resources/feature-policy-vibrate-enabled.html"></iframe> typo here? https://codereview.chromium.org/2499373002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html (right): https://codereview.chromium.org/2499373002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:8: test(function(test) { Remove the unused test argument. (For async_test it's usually needed, but rarely for sync tests.)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled.php (right): https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled.php:13: <html> Tests can omit html, head and body. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html (right): https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:8: test(function(test) { Should be async_test? https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:9: document.onwebkitfullscreenerror = function ( event ) { If async, test, wrap in test.step_func. You can also omit the unused argument. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:10: console.log('FULL SCREEN DENIED'); Is this needed? If the result of the test matters, then passing the test should be enough. And if the test result in the iframes don't matter, it would be more clear to not use testharness.js here. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:15: document.documentElement.appendChild(div); A div in the markup and document.querySelector('div') would work, and makes it clear that there's nothing special about the parent of the div. (Being a sibling of head and body is unusual.) https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:16: assert_false(document.fullscreenEnabled); Using the prefixed or unprefixed API consistently would be good. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:17: div.webkitRequestFullscreen(); Without a user gesture the request will fail with or without feature policy. The tests deleted in https://codereview.chromium.org/2466333002 are one way of writing fullscreen tests that you could copy, but more directly you can just use eventSender. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:312: Frame* frame = document.frame(); Making this part of fullscreenSupported should reduce code duplication. If you need to make the argument non-const, that's fine. Can you add a TODO to update the placement of the code once it's in https://fullscreen.spec.whatwg.org/? (I assume that the plan for Feature Policy is to eventually have each affected spec have the steps.) https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:319: "Fullscreen API is not enabled in feature policy for this frame")); Is this the wording used for other features? "Fullscreen API is disabled by feature policy for this frame" would read slightly better I think.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php (right): https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php:21: testRunner.dumpChildFramesAsText(); It turns out that this is the magic I couldn't understand about child frames. Introduced in https://trac.webkit.org/changeset/24863 and not part of testharness.js as I thought. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php:28: <iframe id="f3" src="resources/feature-policy-vibrate-enabled.html"></iframe> typo here, shouldn't be testing vibrate
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Patchset #7 (id:160001) has been deleted
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Patchset #7 (id:180001) 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...
lunalu@chromium.org changed reviewers: + iclelland@chromium.org
Hi foolip@, thank you for reviewing my CL. I have made the changes upon your comment. Could you please take a second look? Hi iclelland@, could you please take a look at the changes in FeaturePolicy.cpp? Thanks https://codereview.chromium.org/2499373002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php (right): https://codereview.chromium.org/2499373002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php:28: <iframe id="f3" src="resources/feature-policy-vibrate-enabled.html"></iframe> On 2016/11/28 23:18:30, foolip wrote: > typo here? Done. https://codereview.chromium.org/2499373002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html (right): https://codereview.chromium.org/2499373002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:8: test(function(test) { On 2016/11/28 23:18:30, foolip wrote: > Remove the unused test argument. (For async_test it's usually needed, but rarely > for sync tests.) Done. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled.php (right): https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled.php:13: <html> On 2016/11/28 23:33:22, foolip wrote: > Tests can omit html, head and body. Done. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php (right): https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php:21: testRunner.dumpChildFramesAsText(); On 2016/11/29 16:06:36, foolip wrote: > It turns out that this is the magic I couldn't understand about child frames. > Introduced in https://trac.webkit.org/changeset/24863 and not part of > testharness.js as I thought. Done. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php:28: <iframe id="f3" src="resources/feature-policy-vibrate-enabled.html"></iframe> On 2016/11/29 16:06:36, foolip wrote: > typo here, shouldn't be testing vibrate Done. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html (right): https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:8: test(function(test) { On 2016/11/28 23:33:22, foolip wrote: > Should be async_test? Done. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:9: document.onwebkitfullscreenerror = function ( event ) { On 2016/11/28 23:33:22, foolip wrote: > If async, test, wrap in test.step_func. You can also omit the unused argument. Done. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:10: console.log('FULL SCREEN DENIED'); On 2016/11/28 23:33:22, foolip wrote: > Is this needed? If the result of the test matters, then passing the test should > be enough. And if the test result in the iframes don't matter, it would be more > clear to not use testharness.js here. Done. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:15: document.documentElement.appendChild(div); On 2016/11/28 23:33:22, foolip wrote: > A div in the markup and document.querySelector('div') would work, and makes it > clear that there's nothing special about the parent of the div. (Being a sibling > of head and body is unusual.) Done. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen-disabled.html:16: assert_false(document.fullscreenEnabled); On 2016/11/28 23:33:22, foolip wrote: > Using the prefixed or unprefixed API consistently would be good. Done. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:312: Frame* frame = document.frame(); On 2016/11/28 23:33:22, foolip wrote: > Making this part of fullscreenSupported should reduce code duplication. If you > need to make the argument non-const, that's fine. > > Can you add a TODO to update the placement of the code once it's in > https://fullscreen.spec.whatwg.org/ (I assume that the plan for Feature Policy > is to eventually have each affected spec have the steps.) Done. https://codereview.chromium.org/2499373002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:319: "Fullscreen API is not enabled in feature policy for this frame")); On 2016/11/28 23:33:22, foolip wrote: > Is this the wording used for other features? "Fullscreen API is disabled by > feature policy for this frame" would read slightly better I think. Done.
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled-expected.txt (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled-expected.txt:4: FAIL Fullscreen disabled on URL: resources/feature-policy-fullscreen.html with allowfullscreen = true assert_false: expected false got true Should it fail? https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself-expected.txt (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself-expected.txt:4: FAIL Fullscreen enabled for self on URL: resources/feature-policy-fullscreen.html with allowfullscreen = true assert_false: expected false got true To make it easier to see which assert this was that failed, you can add a short description to each, like assert_false(data.enabled, 'enabled'). In general I only bother when there are mulitple asserts of the same type though. https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php:35: if (iframe.src === srcs[0] && iframe.id === "f2") { Will `iframe.src === srcs[0]` ever be true? Getters like src and href resolve URLs, and srcs[0] is a relative URL. Even it were absolute one should be vigilant against some kind of normalization, e.g. with assert_true(iframe.src === srcs[0] || iframe.src === srcs[1]). https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen.html (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen.html:2: <head> head can be omitted https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen.html:18: }, { once: true }); { once: true } is harmless here, but also not needed. https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/fullscreen-enabledforall-expected.txt (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/fullscreen-enabledforall-expected.txt:2: PASS Fullscreen enabled for all on URL: resources/feature-policy-fullscreen.html with allowfullscreen = false Didn't you get a presubmit warning when upload all-PASS expectations? Even if this can be landed, there's a good chance someone will come along and remove it, so if it's important you'll need some console output so that it can't be removed. https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:107: Frame* frame = document.frame(); document.frame() returns a LocalFrame*, so you can use that and skip the DCHECK and cast. I think all call sites guarantee that document.frame() can't be null here, but that's a bit brittle, so I'd say also throw in `if (!frame) return false`;
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...
Done. Thanks! Hi iclelland@, could you please lgtm to WebKit/Source/platform/feature_policy/FeaturePolicy* so that I can land the CL? Thanks https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled-expected.txt (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled-expected.txt:4: FAIL Fullscreen disabled on URL: resources/feature-policy-fullscreen.html with allowfullscreen = true assert_false: expected false got true On 2016/12/02 10:52:09, foolip wrote: > Should it fail? Yes, without the feature policy runtime flag turned on, fullscreen should be enabled when allowfullscreen attribute is set. https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself-expected.txt (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself-expected.txt:4: FAIL Fullscreen enabled for self on URL: resources/feature-policy-fullscreen.html with allowfullscreen = true assert_false: expected false got true On 2016/12/02 10:52:09, foolip wrote: > To make it easier to see which assert this was that failed, you can add a short > description to each, like assert_false(data.enabled, 'enabled'). In general I > only bother when there are mulitple asserts of the same type though. Done. https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php:35: if (iframe.src === srcs[0] && iframe.id === "f2") { On 2016/12/02 10:52:09, foolip wrote: > Will `iframe.src === srcs[0]` ever be true? Getters like src and href resolve > URLs, and srcs[0] is a relative URL. Even it were absolute one should be > vigilant against some kind of normalization, e.g. with assert_true(iframe.src > === srcs[0] || iframe.src === srcs[1]). I changed it to src === srcs[0] instead https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen.html (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen.html:2: <head> On 2016/12/02 10:52:09, foolip wrote: > head can be omitted Done. https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen.html:18: }, { once: true }); On 2016/12/02 10:52:09, foolip wrote: > { once: true } is harmless here, but also not needed. Done. https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/fullscreen-enabledforall-expected.txt (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/fullscreen-enabledforall-expected.txt:2: PASS Fullscreen enabled for all on URL: resources/feature-policy-fullscreen.html with allowfullscreen = false On 2016/12/02 10:52:09, foolip wrote: > Didn't you get a presubmit warning when upload all-PASS expectations? Even if > this can be landed, there's a good chance someone will come along and remove it, > so if it's important you'll need some console output so that it can't be > removed. Done. https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2499373002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:107: Frame* frame = document.frame(); On 2016/12/02 10:52:09, foolip wrote: > document.frame() returns a LocalFrame*, so you can use that and skip the DCHECK > and cast. > > I think all call sites guarantee that document.frame() can't be null here, but > that's a bit brittle, so I'd say also throw in `if (!frame) return false`; 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...
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_...)
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:220001) has been deleted
Patchset #5 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2499373002/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php (right): https://codereview.chromium.org/2499373002/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php:35: if (src === srcs[0] && iframe.id === "f2") { Does this mean that with fullscreen:self, the frame has to be same-origin, *and* the "allowfullscreen" attribute has to be present? That seems wrong -- I would think that either one should be sufficient. https://codereview.chromium.org/2499373002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2499373002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:103: "fullscreen", FeaturePolicy::FeatureDefault::EnableForAll}; I think this should be EnableForSelf, to closely match the existing behaviour of fullscreen (where you have to explicitly allow it for frames) That also makes me think that we need one more test, without an explicit feature policy, to test what happens in that case. It should essentially be exactly the same as the 'allowforself' test, without the HTTP header.
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Done. Please take a second look :) Thanks https://codereview.chromium.org/2499373002/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php (right): https://codereview.chromium.org/2499373002/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php:35: if (src === srcs[0] && iframe.id === "f2") { On 2016/12/05 16:27:34, iclelland wrote: > Does this mean that with fullscreen:self, the frame has to be same-origin, *and* > the "allowfullscreen" attribute has to be present? That seems wrong -- I would > think that either one should be sufficient. Done. https://codereview.chromium.org/2499373002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp (right): https://codereview.chromium.org/2499373002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:103: "fullscreen", FeaturePolicy::FeatureDefault::EnableForAll}; On 2016/12/05 16:27:35, iclelland wrote: > I think this should be EnableForSelf, to closely match the existing behaviour of > fullscreen (where you have to explicitly allow it for frames) > > That also makes me think that we need one more test, without an explicit feature > policy, to test what happens in that case. It should essentially be exactly the > same as the 'allowforself' test, without the HTTP header. Done.
Done. Please take a second look :) Thanks
Done. Please take a second look :) Thanks
Done. Please take a second look :) Thanks
Done. Please take a second look :) Thanks
Done. Please take a second look :) Thanks
Done. Please take a second look :) Thanks
Done. Please take a second look :) Thanks
Done. Please take a second look :) Thanks
Done. Please take a second look :) Thanks
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/05 20:09:24, loonybear wrote: > Done. Please take a second look :) > > Thanks LGTM, thanks!
https://codereview.chromium.org/2499373002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2499373002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:120: if (fullscreenSupported) { Can we add a comment here? Something like "If FeaturePolicy is enabled, then check that fullscreen was not disabled by policy in the parent frame" (And a TODO comment to clean all of this up once iframe attributes are supported for feature policy) https://codereview.chromium.org/2499373002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:128: } else if (isFeatureEnabledInFrame(kFullscreenFeature, frame)) { And I'd add a comment here that says "Even if the iframe attribute is not present, allow fullscreen to be enabled by feature policy"
Done. Thanks https://codereview.chromium.org/2499373002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2499373002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:120: if (fullscreenSupported) { On 2016/12/05 20:48:30, iclelland wrote: > Can we add a comment here? Something like > > "If FeaturePolicy is enabled, then check that fullscreen was not disabled by > policy in the parent frame" > > (And a TODO comment to clean all of this up once iframe attributes are supported > for feature policy) Done. https://codereview.chromium.org/2499373002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:128: } else if (isFeatureEnabledInFrame(kFullscreenFeature, frame)) { On 2016/12/05 20:48:30, iclelland wrote: > And I'd add a comment here that says "Even if the iframe attribute is not > present, allow fullscreen to be enabled by feature policy" Done.
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org, iclelland@chromium.org Link to the patchset: https://codereview.chromium.org/2499373002/#ps300001 (title: "Codereview update")
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
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_...)
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org, iclelland@chromium.org Link to the patchset: https://codereview.chromium.org/2499373002/#ps320001 (title: "Bug fix: handling the case when frame parent does not exist")
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": 320001, "attempt_start_ts": 1481042692761050,
"parent_rev": "4fd9d74a598f288a3195d4f3ce57506b6ed78d15", "commit_rev":
"222b7509ef651e2c303dd70393a79123c7c61bd9"}
Message was sent while issue was closed.
Description was changed from ========== Implementation for feature policy - fullscreen Disable fullscreen API unless enabled through feature policy. BUG=666761 ========== to ========== Implementation for feature policy - fullscreen Disable fullscreen API unless enabled through feature policy. BUG=666761 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Implementation for feature policy - fullscreen Disable fullscreen API unless enabled through feature policy. BUG=666761 ========== to ========== Implementation for feature policy - fullscreen Disable fullscreen API unless enabled through feature policy. BUG=666761 Committed: https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad Cr-Commit-Position: refs/heads/master@{#436651} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad Cr-Commit-Position: refs/heads/master@{#436651}
Message was sent while issue was closed.
https://codereview.chromium.org/2499373002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2499373002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Fullscreen.cpp:126: parent->securityContext()->getFeaturePolicy()->isFeatureEnabled( I was working on https://codereview.chromium.org/2557943002/ today when I had to wonder if it was OK to skip this test in the case of cross-process iframes. Will this check that fullscreen is allowed by the policy for all parent frames up the chain? If not, then it will be possible for some intermediate but cross-process frame to go fullscreen in violation of the feature policy.
Message was sent while issue was closed.
On 2016/12/07 16:09:26, foolip wrote: > https://codereview.chromium.org/2499373002/diff/320001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): > > https://codereview.chromium.org/2499373002/diff/320001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Fullscreen.cpp:126: > parent->securityContext()->getFeaturePolicy()->isFeatureEnabled( > I was working on https://codereview.chromium.org/2557943002/ today when I had to > wonder if it was OK to skip this test in the case of cross-process iframes. Will > this check that fullscreen is allowed by the policy for all parent frames up the > chain? If not, then it will be possible for some intermediate but cross-process > frame to go fullscreen in violation of the feature policy. This implicitly checks that it's allowed by policy all the way to the main frame -- each frame has a policy which is built from it's parent, plus it's own headers. What we're checking here is whether the parent frame had access to the Fullscreen API, since if it didn't, it should not be allowed to enable it via an iframe attribute in the child. (This block is a hack, intended only to provide the correct behaviour for this one case until we have general support for feature policy on iframes) Was there a reason you were wanting to skip it? (It should work in OOPIFs, since the feature policy headers are replicated to remote frames, and the policy object can still be queried on remote frames)
Message was sent while issue was closed.
On 2016/12/07 19:23:02, iclelland wrote: > On 2016/12/07 16:09:26, foolip wrote: > > > https://codereview.chromium.org/2499373002/diff/320001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): > > > > > https://codereview.chromium.org/2499373002/diff/320001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/dom/Fullscreen.cpp:126: > > parent->securityContext()->getFeaturePolicy()->isFeatureEnabled( > > I was working on https://codereview.chromium.org/2557943002/ today when I had > to > > wonder if it was OK to skip this test in the case of cross-process iframes. > Will > > this check that fullscreen is allowed by the policy for all parent frames up > the > > chain? If not, then it will be possible for some intermediate but > cross-process > > frame to go fullscreen in violation of the feature policy. > > This implicitly checks that it's allowed by policy all the way to the main frame > -- each frame has a policy which is built from it's parent, plus it's own > headers. What we're checking here is whether the parent frame had access to the > Fullscreen API, since if it didn't, it should not be allowed to enable it via an > iframe attribute in the child. > > (This block is a hack, intended only to provide the correct behaviour for this > one case until we have general support for feature policy on iframes) > > Was there a reason you were wanting to skip it? (It should work in OOPIFs, since > the feature policy headers are replicated to remote frames, and the policy > object can still be queried on remote frames) All of the checks are skipped in the forCrossProcessDescendant case, because the checks should have already been performed, and if any now fail it's too late to do anything about. If this check is implicitly recursive (like allowedToUseFullscreen) then all is well. |
