Skip to content

Remove ProcessSyncDataType::HasFullscreenElement#40462

Merged
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
achristensen07:eng/Remove-ProcessSyncDataType-HasFullscreenElement
Feb 12, 2025
Merged

Remove ProcessSyncDataType::HasFullscreenElement#40462
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
achristensen07:eng/Remove-ProcessSyncDataType-HasFullscreenElement

Conversation

@achristensen07
Copy link
Copy Markdown
Contributor

@achristensen07 achristensen07 commented Feb 12, 2025

ae35ee5

Remove ProcessSyncDataType::HasFullscreenElement
https://bugs.webkit.org/show_bug.cgi?id=287541
rdar://144667685

Reviewed by Tim Nguyen.

We don't need all site isolation processes to know that the top document has
a fullscreen element.  We just need to check if the fullscreen element in this
process is in the top document when a request comes to exit fullscreen.

* Source/WebCore/dom/Document.cpp:
(WebCore::Document::populateDocumentSyncDataForNewlyConstructedDocument):
* Source/WebCore/dom/FullscreenManager.cpp:
(WebCore::clearFullscreenFlags):
(WebCore::FullscreenManager::willEnterFullscreen):
(WebCore::FullscreenManager::updatePageFullscreenStatusIfTopDocument): Deleted.
* Source/WebCore/dom/FullscreenManager.h:
* Source/WebCore/page/Page.cpp:
(WebCore::Page::updateProcessSyncData):
(WebCore::Page::setTopDocumentHasFullscreenElement): Deleted.
(WebCore::Page::topDocumentHasFullscreenElement): Deleted.
* Source/WebCore/page/Page.h:
* Source/WebCore/page/ProcessSyncData.in:
* Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.cpp:
(WebKit::WebFullScreenManager::requestExitFullScreen):

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

ed34119

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2 🧪 win-tests
✅ 🧪 webkitperl 🧪 ios-wk2 🧪 api-mac 🧪 api-wpe
🧪 ios-wk2-wpt 🧪 mac-wk1 ✅ 🛠 wpe-cairo
🧪 api-ios 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision 🧪 mac-AS-debug-wk2 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress 🧪 api-gtk
🧪 vision-wk2 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv 🛠 mac-safer-cpp
🛠 tv-sim
🛠 watch
🛠 watch-sim

@achristensen07 achristensen07 self-assigned this Feb 12, 2025
@achristensen07 achristensen07 added the WebKit Process Model Bugs related to WebKit's multi-process architecture label Feb 12, 2025
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 12, 2025
Copy link
Copy Markdown
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.

As much as I'd love getting rid of this code, fullscreen/iframe-fullscreen-system-exit.html seems to be regressing (e.g. https://bugs.webkit.org/show_bug.cgi?id=287309 )

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm guessing there's a mismatch between WebFullscreenManager::m_element and FullscreenManager::fullscreenElement(). If I had to guess, it's the same mismatch than between FullscreenManager::fullscreenElement() and FullscreenManager::m_fullscreenElement

@achristensen07 achristensen07 removed the merging-blocked Applied to prevent a change from being merged label Feb 12, 2025
@achristensen07 achristensen07 force-pushed the eng/Remove-ProcessSyncDataType-HasFullscreenElement branch from 6116ba9 to dbe238d Compare February 12, 2025 18:12
Copy link
Copy Markdown
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 modulo leaving the move to Document.cpp

Comment thread Source/WebCore/dom/Document.cpp Outdated
Comment on lines 8585 to 8593
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason to move this to Document ? Can't we just leave this on FullscreenManager and access it from there? I'd prefer fullscreen logic to be contained.

@achristensen07 achristensen07 force-pushed the eng/Remove-ProcessSyncDataType-HasFullscreenElement branch from dbe238d to ed34119 Compare February 12, 2025 19:29
@achristensen07 achristensen07 added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 12, 2025
https://bugs.webkit.org/show_bug.cgi?id=287541
rdar://144667685

Reviewed by Tim Nguyen.

We don't need all site isolation processes to know that the top document has
a fullscreen element.  We just need to check if the fullscreen element in this
process is in the top document when a request comes to exit fullscreen.

* Source/WebCore/dom/Document.cpp:
(WebCore::Document::populateDocumentSyncDataForNewlyConstructedDocument):
* Source/WebCore/dom/FullscreenManager.cpp:
(WebCore::clearFullscreenFlags):
(WebCore::FullscreenManager::willEnterFullscreen):
(WebCore::FullscreenManager::updatePageFullscreenStatusIfTopDocument): Deleted.
* Source/WebCore/dom/FullscreenManager.h:
* Source/WebCore/page/Page.cpp:
(WebCore::Page::updateProcessSyncData):
(WebCore::Page::setTopDocumentHasFullscreenElement): Deleted.
(WebCore::Page::topDocumentHasFullscreenElement): Deleted.
* Source/WebCore/page/Page.h:
* Source/WebCore/page/ProcessSyncData.in:
* Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.cpp:
(WebKit::WebFullScreenManager::requestExitFullScreen):

Canonical link: https://commits.webkit.org/290288@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Remove-ProcessSyncDataType-HasFullscreenElement branch from ed34119 to ae35ee5 Compare February 12, 2025 20:13
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 290288@main (ae35ee5): https://commits.webkit.org/290288@main

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

@webkit-commit-queue webkit-commit-queue merged commit ae35ee5 into WebKit:main Feb 12, 2025
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebKit Process Model Bugs related to WebKit's multi-process architecture

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants