-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[Site Isolation] Pointer Lock #46080
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
EWS run on previous version of this PR (hash 418330e) |
EWS run on previous version of this PR (hash 66eebae) |
EWS run on previous version of this PR (hash 23db6a2) |
@@ -142,7 +148,12 @@ void PointerLockController::requestPointerUnlock() | |||
return; | |||
|
|||
m_unlockPending = true; | |||
m_page.chrome().client().requestPointerUnlock(); | |||
m_page.chrome().client().requestPointerUnlock([this] (bool result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is repeated in this file, so I think it could be good to put in a function instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also unsafe use of pointer capture. Nothing is guaranteeing this has not been deallocated when the callback happens.
EWS run on previous version of this PR (hash 792cf13) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs test coverage with site isolation on.
@@ -142,7 +148,12 @@ void PointerLockController::requestPointerUnlock() | |||
return; | |||
|
|||
m_unlockPending = true; | |||
m_page.chrome().client().requestPointerUnlock(); | |||
m_page.chrome().client().requestPointerUnlock([this] (bool result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also unsafe use of pointer capture. Nothing is guaranteeing this has not been deallocated when the callback happens.
@@ -3020,7 +3023,7 @@ void WKPageDidAllowPointerLock(WKPageRef pageRef) | |||
{ | |||
CRASH_IF_SUSPENDED; | |||
#if ENABLE(POINTER_LOCK) | |||
toImpl(pageRef)->didAllowPointerLock(); | |||
toImpl(pageRef)->didAllowPointerLock([](bool) { }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to remove WKPageDidAllowPointerLock instead.
@@ -3040,7 +3043,7 @@ void WKPageDidDenyPointerLock(WKPageRef pageRef) | |||
{ | |||
CRASH_IF_SUSPENDED; | |||
#if ENABLE(POINTER_LOCK) | |||
toImpl(pageRef)->didDenyPointerLock(); | |||
toImpl(pageRef)->didDenyPointerLock([](bool) { }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove WKPageDidDenyPointerLock
EWS run on previous version of this PR (hash 3684e0c) |
EWS run on previous version of this PR (hash 4f9005b) |
Safer C++ Build #41401 (4f9005b)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
Safer C++ Build #41403 (aaa410d)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
EWS run on previous version of this PR (hash 64f7cf8) |
EWS run on previous version of this PR (hash e8b7011) |
I don't see where a non-empty completion handler is passed to Also please add tests similar to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
https://bugs.webkit.org/show_bug.cgi?id=293767 rdar://117910429 Reviewed by NOBODY (OOPS!). Implement Pointer Lock to work under site isolation. * Source/WebCore/page/ChromeClient.h: (WebCore::ChromeClient::requestPointerLock): (WebCore::ChromeClient::requestPointerUnlock): * Source/WebCore/page/PointerLockController.cpp: (WebCore::PointerLockController::requestPointerLock): (WebCore::PointerLockController::requestPointerUnlock): (WebCore::PointerLockController::requestPointerUnlockAndForceCursorVisible): * Source/WebKit/UIProcess/API/APIUIClient.h: (API::UIClient::requestPointerLock): * Source/WebKit/UIProcess/API/C/WKPage.cpp: (WKPageSetPageUIClient): (WKPageDidAllowPointerLock): (WKPageDidDenyPointerLock): * Source/WebKit/UIProcess/Cocoa/UIDelegate.h: * Source/WebKit/UIProcess/Cocoa/UIDelegate.mm: (WebKit::UIDelegate::UIClient::requestPointerLock): * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::dispatchActivityStateChange): (WebKit::WebPageProxy::didCommitLoadForFrame): (WebKit::WebPageProxy::resetState): (WebKit::WebPageProxy::requestPointerLock): (WebKit::WebPageProxy::didAllowPointerLock): (WebKit::WebPageProxy::didDenyPointerLock): (WebKit::WebPageProxy::requestPointerUnlock): * Source/WebKit/UIProcess/WebPageProxy.h: * Source/WebKit/UIProcess/WebPageProxy.messages.in: * Source/WebKit/UIProcess/mac/WebViewImpl.mm: (WebKit::WebViewImpl::windowWillBeginSheet): * Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp: (WebKit::WebChromeClient::requestPointerLock): (WebKit::WebChromeClient::requestPointerUnlock): * Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h: * Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.h: * Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm: (WebChromeClient::requestPointerLock): (WebChromeClient::requestPointerUnlock):
EWS run on current version of this PR (hash 25c3c5c) |
Working on adding an additional test. |
@@ -27,6 +27,7 @@ | |||
#if ENABLE(POINTER_LOCK) | |||
|
|||
#include "ExceptionCode.h" | |||
#include "Page.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should include Page.h
if we're moving the implementation of ref/deref to cpp file.
25c3c5c
25c3c5c