Skip to content

Conversation

@cdumez
Copy link
Contributor

@cdumez cdumez commented Oct 5, 2022

6331c0f

Add initial implementation of the unprefixed fullscreen API
https://bugs.webkit.org/show_bug.cgi?id=246103

Reviewed by Tim Nguyen.

Add initial implementation of the unprefixed fullscreen API:
- https://fullscreen.spec.whatwg.org/#api

The feature is behind an experimental feature flag, off by default.

This is needed to unblock the WPT testing of the screen orientation API since
screen orientation locking requires being in fullscreen.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/html/capability-delegation/delegate-fullscreen-request-popup-same-origin.https.tentative-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/capability-delegation/delegate-fullscreen-request-subframe-same-origin.https.tentative-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/dom/idlharness.https-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/active-lock-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/event-before-promise-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/lock-basic-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/lock-sandboxed-iframe-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/lock-unlock-check-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/onchange-event-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/onchange-event-subframe-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/orientation-reading-expected.txt:
* Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:
* Source/WebCore/dom/Element+Fullscreen.idl:
* Source/WebCore/dom/Element.cpp:
(WebCore::Element::requestFullscreen):
* Source/WebCore/dom/Element.h:
* Source/WebCore/dom/EventNames.h:
* Source/WebCore/dom/FullscreenManager.cpp:
(WebCore::FullscreenManager::dispatchFullscreenChangeEvents):
* Source/WebCore/html/HTMLAttributeNames.in:

Canonical link: https://commits.webkit.org/255317@main

e5eb020

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe 🛠 🧪 win
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ❌ 🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 tv 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 🧪 merge ✅ 🛠 watch 🧪 mac-AS-debug-wk2
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@cdumez cdumez self-assigned this Oct 5, 2022
@cdumez cdumez added WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). WebKit Nightly Build labels Oct 5, 2022
@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from d6439c9 to 516f9e2 Compare October 5, 2022 23:21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Give this is the standard one, I think you should swap things by putting this at the top of the interface, and annotating the non-standard ones as non-standard with the comments we usually add for such things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, will do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really a win to be inline?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will move to a new cpp file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in its own file as a partial interface mixin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from 516f9e2 to 5ccdaad Compare October 6, 2022 01:52
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 6, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 6, 2022
@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from 5ccdaad to ac24051 Compare October 6, 2022 03:00
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 6, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 6, 2022
@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from ac24051 to 0ca47fc Compare October 6, 2022 19:06
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 6, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 6, 2022
@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from 0ca47fc to c23ba45 Compare October 6, 2022 21:47
@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from c23ba45 to e3d2cd7 Compare October 6, 2022 21:55
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for e3d2cd7. Live statuses available at the PR page, #5045

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 6, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 7, 2022
@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from e3d2cd7 to a60ee94 Compare October 7, 2022 01:58
@cdumez
Copy link
Contributor Author

cdumez commented Oct 7, 2022

Looks generally fine to me, though I'd like less differences between the prefixed and unprefixed APIs, it seems to be mostly about preflight checks at the moment, which I think are reasonable to change in the old API as well.

I really do not want to change the behavior of the shipping API in this large patch. I think this carries risk and such large patches for feature A should not change the behavior of feature B.

@nt1m
Copy link
Member

nt1m commented Oct 7, 2022

Looks generally fine to me, though I'd like less differences between the prefixed and unprefixed APIs, it seems to be mostly about preflight checks at the moment, which I think are reasonable to change in the old API as well.

I really do not want to change the behavior of the shipping API in this large patch. I think this carries risk and such large patches for feature A should not change the behavior of feature B.

@cdumez That's reasonable. Can we move all the new logic into FullscreenManager still and make webkitRequestFullscreen APIs wrap around requestFullscreen? (even if it means adding if/else checks).

I do plan to unify the code for both prefixed/unprefixed APIs anyway, so it would be easier if stuff wasn't scattered around.

@cdumez
Copy link
Contributor Author

cdumez commented Oct 7, 2022

Looks generally fine to me, though I'd like less differences between the prefixed and unprefixed APIs, it seems to be mostly about preflight checks at the moment, which I think are reasonable to change in the old API as well.

I really do not want to change the behavior of the shipping API in this large patch. I think this carries risk and such large patches for feature A should not change the behavior of feature B.

@cdumez That's reasonable. Can we move all the new logic into FullscreenManager still and make webkitRequestFullscreen APIs wrap around requestFullscreen? (even if it means adding if/else checks).

Yes, I'll give this a shot.

@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from 8d97023 to 2ed5b29 Compare October 7, 2022 18:13
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 7, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 7, 2022
@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from 2ed5b29 to b3d2a3f Compare October 7, 2022 19:09
Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, aside from my two comments (the unrelated header file change, and the regression in the test file).

@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label Oct 7, 2022
@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Oct 7, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 8, 2022
@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from b3d2a3f to 7632d71 Compare October 8, 2022 05:59
@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from 7632d71 to 3cc5edc Compare October 8, 2022 06:02
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 8, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 8, 2022
@cdumez cdumez force-pushed the 246103_unprefixed_fullscreen_api branch from 3cc5edc to e5eb020 Compare October 8, 2022 17:34
@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label Oct 8, 2022
https://bugs.webkit.org/show_bug.cgi?id=246103

Reviewed by Tim Nguyen.

Add initial implementation of the unprefixed fullscreen API:
- https://fullscreen.spec.whatwg.org/#api

The feature is behind an experimental feature flag, off by default.

This is needed to unblock the WPT testing of the screen orientation API since
screen orientation locking requires being in fullscreen.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/html/capability-delegation/delegate-fullscreen-request-popup-same-origin.https.tentative-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/capability-delegation/delegate-fullscreen-request-subframe-same-origin.https.tentative-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/dom/idlharness.https-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/active-lock-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/event-before-promise-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/lock-basic-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/lock-sandboxed-iframe-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/lock-unlock-check-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/onchange-event-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/onchange-event-subframe-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/orientation-reading-expected.txt:
* Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:
* Source/WebCore/dom/Element+Fullscreen.idl:
* Source/WebCore/dom/Element.cpp:
(WebCore::Element::requestFullscreen):
* Source/WebCore/dom/Element.h:
* Source/WebCore/dom/EventNames.h:
* Source/WebCore/dom/FullscreenManager.cpp:
(WebCore::FullscreenManager::dispatchFullscreenChangeEvents):
* Source/WebCore/html/HTMLAttributeNames.in:

Canonical link: https://commits.webkit.org/255317@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the 246103_unprefixed_fullscreen_api branch from e5eb020 to 6331c0f Compare October 8, 2022 22:37
@webkit-commit-queue
Copy link
Collaborator

Committed 255317@main (6331c0f): https://commits.webkit.org/255317@main

Reviewed commits have been landed. Closing PR #5045 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 6331c0f into WebKit:main Oct 8, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants