New issue
Advanced search Search tips

Issue 666761 link

Starred by 2 users
Status: Fixed
Owner:
loonyb...@chromium.org
Closed: Sep 2017
Cc:
iclell...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocked on:
issue 718155
issue 729658
Blocking:
issue 623682



Sign in to add a comment

Feature Policy: Support Fullscreen API

Project Member Reported by iclell...@chromium.org, Nov 18 2016 
Add support for the "fullscreen" feature in Feature Policy.

The feature hasn't been defined yet in FeaturePolicy.cpp. It should be defined there, and checked before allowing the use of ELement.requestFullScreen.

If not allowed by policy, the call to requestFullScreen should fail with an error.

Also complicating this is that we need to ensure that the allowfullscreen iframe attribute is *also* supported. This is an alternative method, but the original method should still work in the absence of a Feature-Policy header.

Comment 1 by iclell...@chromium.org, Nov 18 2016

Components: Blink>Fullscreen
Labels: Feature-Policy OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 6 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/719a425cbbbffdc724535a52cd1a507663cf6aad

commit 719a425cbbbffdc724535a52cd1a507663cf6aad
Author: lunalu <lunalu@chromium.org>
Date: Tue Dec 06 18:19:46 2016

Implementation for feature policy - fullscreen
Disable fullscreen API unless enabled through feature policy.

BUG= 666761 

Review-Url: https://codereview.chromium.org/2499373002
Cr-Commit-Position: refs/heads/master@{#436651}

[add] https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled-expected.txt
[add] https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-disabled.php
[add] https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforall.php
[add] https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforself.php
[add] https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen.html
[add] https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/fullscreen-disabled-expected.txt
[modify] https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad/third_party/WebKit/Source/core/dom/Fullscreen.cpp
[modify] https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
[modify] https://crrev.com/719a425cbbbffdc724535a52cd1a507663cf6aad/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h

Comment 3 by lunalu@chromium.org, Dec 12 2016

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39d97d6645829da8b4c402b3112c81ebbe53004d

commit 39d97d6645829da8b4c402b3112c81ebbe53004d
Author: iclelland <iclelland@chromium.org>
Date: Thu Jan 26 19:34:15 2017

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}

[add] https://crrev.com/39d97d6645829da8b4c402b3112c81ebbe53004d/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforall-expected.txt
[modify] https://crrev.com/39d97d6645829da8b4c402b3112c81ebbe53004d/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforall.php
[modify] https://crrev.com/39d97d6645829da8b4c402b3112c81ebbe53004d/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen.html
[modify] https://crrev.com/39d97d6645829da8b4c402b3112c81ebbe53004d/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/fullscreen-disabled-expected.txt
[add] https://crrev.com/39d97d6645829da8b4c402b3112c81ebbe53004d/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/fullscreen-enabledforall-expected.txt
[modify] https://crrev.com/39d97d6645829da8b4c402b3112c81ebbe53004d/third_party/WebKit/Source/core/dom/Fullscreen.cpp

Comment 5 by iclell...@chromium.org, Jan 27 2017

Labels: Merge-Request-57
Project Member

Comment 6 by sheriffbot@chromium.org, Jan 27 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 27 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a505c5e0e74b61d0ab2e194abd3533150f1131f

commit 6a505c5e0e74b61d0ab2e194abd3533150f1131f
Author: Ian Clelland <iclelland@google.com>
Date: Fri Jan 27 19:45:54 2017

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}
(cherry picked from commit 39d97d6645829da8b4c402b3112c81ebbe53004d)

Review-Url: https://codereview.chromium.org/2662443004 .
Cr-Commit-Position: refs/branch-heads/2987@{#152}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[add] https://crrev.com/6a505c5e0e74b61d0ab2e194abd3533150f1131f/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforall-expected.txt
[modify] https://crrev.com/6a505c5e0e74b61d0ab2e194abd3533150f1131f/third_party/WebKit/LayoutTests/http/tests/feature-policy/fullscreen-enabledforall.php
[modify] https://crrev.com/6a505c5e0e74b61d0ab2e194abd3533150f1131f/third_party/WebKit/LayoutTests/http/tests/feature-policy/resources/feature-policy-fullscreen.html
[modify] https://crrev.com/6a505c5e0e74b61d0ab2e194abd3533150f1131f/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/fullscreen-disabled-expected.txt
[add] https://crrev.com/6a505c5e0e74b61d0ab2e194abd3533150f1131f/third_party/WebKit/LayoutTests/virtual/feature-policy/http/tests/feature-policy/fullscreen-enabledforall-expected.txt
[modify] https://crrev.com/6a505c5e0e74b61d0ab2e194abd3533150f1131f/third_party/WebKit/Source/core/dom/Fullscreen.cpp

Comment 8 by lunalu@chromium.org, May 8 2017

Status: Assigned (was: Fixed)
Reopening this bug as "allowfullscreen" can enable requestfullscreen dynamically whereas in FP we would not like to update fullscreen policy without document reload. 

FP implementation breaks current fullscreen tests. So shipping FP without fullscreen for now until all broken tests are fixed.

Comment 9 by iclell...@chromium.org, May 24 2017

Blockedon: 718155

Comment 10 by iclell...@chromium.org, Jun 5 2017

Blockedon: 729658

Comment 11 by lunalu@chromium.org, Jun 12 2017

Owner: loonyb...@chromium.org

Comment 12 by loonyb...@chromium.org, Jul 4 2017

Cc: iclell...@chromium.org
What is the currently status on this one? 

Ian I suppose you fixed all the fullscreen tests?

Should we update IsSupportedInFeaturePolicy() in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp?q=issupportedinfeaturepolicy&dr=CSs&l=87 perhaps?

Comment 13 by loonyb...@chromium.org, Sep 1 2017

Status: Fixed (was: Assigned)
I am pretty sure this has been fixed. Closing it as fixed.
►

Sign in to add a comment