Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2585363002:
Use FeaturePolicy to implement iframe[allowfullscreen] (Closed)
DescriptionUse FeaturePolicy to implement iframe[allowfullscreen]
The previous CL (https://crrev.com/719a425c) tried to handle this in fullscreenIsSupported(), which misses the important case where the Feature Policy API is enabled, but the site uses 'allowfullscreen' iframe attributes.
This CL roughly mimics the intended behaviour of feature-policy allowfullscreen, by allowing access to fullscreen in same-origin iframes, and in cross-origin iframes where either a Feature-Policy header or an 'allowfullscreen' attribute is used, and the parent frame allows fullscreen.
BUG=666761
Review-Url: https://codereview.chromium.org/2585363002
Cr-Commit-Position: refs/heads/master@{#446393}
Committed: https://chromium.googlesource.com/chromium/src/+/39d97d6645829da8b4c402b3112c81ebbe53004d
Patch Set 1 #Patch Set 2 : Remove FP code in fullscreenIsSupported #Patch Set 3 : Update test expectations #
Total comments: 3
Patch Set 4 : Rebase #Patch Set 5 : Avoid extra change event notification #Messages
Total messages: 49 (30 generated)
The CQ bit was checked by iclelland@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 iclelland@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...
Description was changed from ========== Use FeaturePolicy to implement iframe[allowfullscreen] BUG=666761 ========== to ========== Use FeaturePolicy to implement iframe[allowfullscreen] The previous CL (https://crrev.com/719a425c) tried to handle this in fullscreenIsSupported(), which misses the important case where the Feature Policy API is enabled, but the site uses 'allowfullscreen' iframe attributes. This CL roughly mimics the intended behaviour of feature-policy allowfullscreen, by allowing access to fullscreen in same-origin iframes, and in cross-origin iframes where either a Feature-Policy header or an 'allowfullscreen' attribute is used, and the parent frame allows fullscreen. BUG=666761 ==========
iclelland@chromium.org changed reviewers: + foolip@chromium.org
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_...)
The CQ bit was checked by iclelland@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...
iclelland@chromium.org changed reviewers: + lunalu@chromium.org
+r foolip, lulalu -- PTAL, thanks!
On 2016/12/20 16:14:11, iclelland wrote: > +r foolip, lulalu -- PTAL, thanks! s/lulalu/lunalu/, oops
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_...)
lgtm Thanks for doing that Ian. LGTM to me!
On 2016/12/20 19:00:01, loonybear (OOO until Jan 6) wrote: > lgtm > > Thanks for doing that Ian. > LGTM to me! Post-holiday ping, foolip :)
-lgtm https://codereview.chromium.org/2585363002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (left): https://codereview.chromium.org/2585363002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Fullscreen.cpp:140: JSMessageSource, WarningMessageLevel, Do we want to be more implicit when fullscreen is disabled by FP? In that case, we need to be able to return different console messages when allowedToUseFullscreen returns false.
I also noticed that the comments in LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php // This test ensures that fullscreen feature when enabled for self only works // in the same orgin but not cross origins when allowfullscreen is set. No // iframe may call it when allowfullscreen is not set. ==> // This test ensures that fullscreen feature when enabled for self works when allowfullscreen is set or in the same origin. No cross-origin iframe may call it when allowfullscreen is not set.
On 2017/01/03 17:05:11, iclelland wrote: > On 2016/12/20 19:00:01, loonybear (OOO until Jan 6) wrote: > > lgtm > > > > Thanks for doing that Ian. > > LGTM to me! > > Post-holiday ping, foolip :) ping ping :)
On 2017/01/19 14:59:01, iclelland wrote: > On 2017/01/03 17:05:11, iclelland wrote: > > On 2016/12/20 19:00:01, loonybear (OOO until Jan 6) wrote: > > > lgtm > > > > > > Thanks for doing that Ian. > > > LGTM to me! > > > > Post-holiday ping, foolip :) > > ping ping :) Eh, excuses excuses. I have a feeling of deja vu, but what's the spec-side plan for this? This CL as it would change the behavior in a way that doesn't match the Fullscreen spec. Can you link to the spec issues discussing these changes? Ideally we have a rough agreement in there that we will try to change the behavior like this, and if successful we change the spec.
https://codereview.chromium.org/2585363002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2585363002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Fullscreen.cpp:92: // 2. Otherwise, if the embedding frame's document is allowed to use Can this part be turned into a step 4 before the return false, conditional on FP being enabled? The duplication of the allowedToUseFullscreen calls looks unfortunate and can perhaps be avoided.
On 2017/01/19 17:10:38, foolip_slow_very_sorry wrote: > On 2017/01/19 14:59:01, iclelland wrote: > > On 2017/01/03 17:05:11, iclelland wrote: > > > On 2016/12/20 19:00:01, loonybear (OOO until Jan 6) wrote: > > > > lgtm > > > > > > > > Thanks for doing that Ian. > > > > LGTM to me! > > > > > > Post-holiday ping, foolip :) > > > > ping ping :) > > Eh, excuses excuses. I have a feeling of deja vu, but what's the spec-side plan > for this? This CL as it would change the behavior in a way that doesn't match > the Fullscreen spec. Can you link to the spec issues discussing these changes? > Ideally we have a rough agreement in there that we will try to change the > behavior like this, and if successful we change the spec. The ideas behind this are in the FP explainer; I'm currently writing them up in the spec, which will be monkeypatching HTML's "allowed-to-use" algorithm to support FP in a generic way. That is, something to the effect of "if the document has an active feature policy which allows the use of the 'fullscreen' feature for the current origin, then the document object is allowed to use requestFullscreen()", and the "allowfullscreen" attribute is similarly specified in such a way that its presence on an iframe element containing cross-origin frame allows the framed document to use fullscreen if the parent document was allowed to. Unfortunately, since the FP spec isn't finished, there aren't issues for it there, and it's too early to reference it in issues on the HTML repo, where the changes will ultimately need to land. This CL deliberately doesn't change any behavior if the runtime flag for FP is turned off (which is why it's structured the way that it is). If the flag is turned on, then it gets us closer to the desired FP behaviour (which should ship in a proper, specced form in M58.)
https://codereview.chromium.org/2585363002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/2585363002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Fullscreen.cpp:92: // 2. Otherwise, if the embedding frame's document is allowed to use On 2017/01/19 17:14:18, foolip_slow_very_sorry wrote: > Can this part be turned into a step 4 before the return false, conditional on FP > being enabled? The duplication of the allowedToUseFullscreen calls looks > unfortunate and can perhaps be avoided. I was trying to maintain the integrity of the original algorithm as much as possible, by just wrapping it in "if(!fp-enabled)". We could certainly avoid duplicating the recursive call, but I thought it would make the whole method less clear. There are two things coming that will allow us to make this code simpler: - I've proposed the change to fullscreen to allow it in same-origin-subframes (if allowed in the parent). There's been no I2S for that, though. If we could ship that, then the two sides of this method will be almost identical, and we could easily just wrap the call to isFeatureEnabled like you say. - The other thing that we want to get done very soon (won't be for M57, but definitely for M58) is supporting iframe attributes directly in the feature policy framework. Once we have *that*, then the FP side becomes literally "return isFeatureEnabled(fullscreen)" and the rest of the hack goes away.
lgtm, sorry for the delay. I would like to think about this again before FP is launched, since that'll be when the change effectively happens. In particular, when do you think we can fully implement allowfullscreen in terms of FP? This would mean that the allowfullscreen attribute is processed at the same time as other FP directives and sets some internal flag. This algorithm would then not look at the attribute at all, but at the flag, without knowing why it is set or unset.
On 2017/01/24 11:02:36, foolip_UTC7 wrote: > lgtm, sorry for the delay. I would like to think about this again before FP is > launched, since that'll be when the change effectively happens. > > In particular, when do you think we can fully implement allowfullscreen in terms > of FP? This would mean that the allowfullscreen attribute is processed at the > same time as other FP directives and sets some internal flag. This algorithm > would then not look at the attribute at all, but at the flag, without knowing > why it is set or unset. Agreed. That's exactly the plan; we're going to have it in place before shipping. crbug.com/682282 is tracking that, and crbug.com/682258 is the master "support iframe-based-policy generally" issue, and both are being actively worked on. Thanks
The CQ bit was checked by iclelland@chromium.org
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: 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 iclelland@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org, lunalu@chromium.org Link to the patchset: https://codereview.chromium.org/2585363002/#ps60001 (title: "Rebase")
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 iclelland@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.
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org, lunalu@chromium.org Link to the patchset: https://codereview.chromium.org/2585363002/#ps80001 (title: "Avoid extra change event notification")
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": 80001, "attempt_start_ts": 1485455425651140,
"parent_rev": "5c4e9645c2655b652161b180728ff1cc48e1225a", "commit_rev":
"39d97d6645829da8b4c402b3112c81ebbe53004d"}
Message was sent while issue was closed.
Description was changed from ========== Use FeaturePolicy to implement iframe[allowfullscreen] The previous CL (https://crrev.com/719a425c) tried to handle this in fullscreenIsSupported(), which misses the important case where the Feature Policy API is enabled, but the site uses 'allowfullscreen' iframe attributes. This CL roughly mimics the intended behaviour of feature-policy allowfullscreen, by allowing access to fullscreen in same-origin iframes, and in cross-origin iframes where either a Feature-Policy header or an 'allowfullscreen' attribute is used, and the parent frame allows fullscreen. BUG=666761 ========== to ========== Use FeaturePolicy to implement iframe[allowfullscreen] The previous CL (https://crrev.com/719a425c) tried to handle this in fullscreenIsSupported(), which misses the important case where the Feature Policy API is enabled, but the site uses 'allowfullscreen' iframe attributes. This CL roughly mimics the intended behaviour of feature-policy allowfullscreen, by allowing access to fullscreen in same-origin iframes, and in cross-origin iframes where either a Feature-Policy header or an 'allowfullscreen' attribute is used, and the parent frame allows fullscreen. BUG=666761 Review-Url: https://codereview.chromium.org/2585363002 Cr-Commit-Position: refs/heads/master@{#446393} Committed: https://chromium.googlesource.com/chromium/src/+/39d97d6645829da8b4c402b3112c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/39d97d6645829da8b4c402b3112c... |
