Bug 163801

Created attachment 292383 [details] Patch

Created attachment 292384 [details] Patch

Created attachment 292388 [details] Patch

Comment on attachment 292388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292388&action=review > Source/WebKit/mac/WebView/WebPreferences.mm:565 > +#if ENABLE(POINTER_LOCK) You're only registering the default if ENABLE(POINTER_LOCK), but have the getters and setters always? That seems odd. Maybe just register the default always? > Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:126 > +WK_EXPORT void WKPageDidAllowPointerLock(WKPageRef page); > +WK_EXPORT void WKPageDidDenyPointerLock(WKPageRef page); It seems alarming that there is no way to associate the pointer lock request with the response through this SPI. Perhaps you want something like WKFramePolicyListener (an object that gets handed into the callback and has functions you can call on it to allow/deny)? But then it seems like the lower levels also have no request/response association... which seems similarly concerning. > Source/WebKit2/UIProcess/API/C/WKPreferencesRef.h:288 > +WK_EXPORT void WKPreferencesSetPointerLockEnabled(WKPreferencesRef preferencesRef, bool enabled); No Modern API support?

Comment on attachment 292388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292388&action=review > Source/WTF/ChangeLog:3 > + Add runtime flag to enable pointer lock. Enable pointer lock feature for mac. Technically this just enables the compile time flag, and leaves the runtime flag off by default, right?

(In reply to comment #5) > Comment on attachment 292388 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=292388&action=review > > > Source/WTF/ChangeLog:3 > > + Add runtime flag to enable pointer lock. Enable pointer lock feature for mac. > > Technically this just enables the compile time flag, and leaves the runtime > flag off by default, right? Yes.

(In reply to comment #4) > Comment on attachment 292388 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=292388&action=review > > > Source/WebKit/mac/WebView/WebPreferences.mm:565 > > +#if ENABLE(POINTER_LOCK) > > You're only registering the default if ENABLE(POINTER_LOCK), but have the > getters and setters always? That seems odd. Maybe just register the default > always? Done. > > > Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:126 > > +WK_EXPORT void WKPageDidAllowPointerLock(WKPageRef page); > > +WK_EXPORT void WKPageDidDenyPointerLock(WKPageRef page); > > It seems alarming that there is no way to associate the pointer lock request > with the response through this SPI. Perhaps you want something like > WKFramePolicyListener (an object that gets handed into the callback and has > functions you can call on it to allow/deny)? But then it seems like the > lower levels also have no request/response association... which seems > similarly concerning. Pointer lock approval is for the page, not for specific elements and is disallowed for iframes. While a page has pointer lock, if another element on the page requests pointer lock, it immediately receives it. So long as requests and approvals go to the same page, I don't think it is necessary to pair individual request. > > > Source/WebKit2/UIProcess/API/C/WKPreferencesRef.h:288 > > +WK_EXPORT void WKPreferencesSetPointerLockEnabled(WKPreferencesRef preferencesRef, bool enabled); > > No Modern API support? I'm doing my best to follow the patterns of other properties. I'm sure I've missed something. Where can I look for how to add Modern API support?

(In reply to comment #7) > (In reply to comment #4) > > Comment on attachment 292388 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=292388&action=review > > > > > Source/WebKit/mac/WebView/WebPreferences.mm:565 > > > +#if ENABLE(POINTER_LOCK) > > > > You're only registering the default if ENABLE(POINTER_LOCK), but have the > > getters and setters always? That seems odd. Maybe just register the default > > always? > > Done. > > > > > > Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:126 > > > +WK_EXPORT void WKPageDidAllowPointerLock(WKPageRef page); > > > +WK_EXPORT void WKPageDidDenyPointerLock(WKPageRef page); > > > > It seems alarming that there is no way to associate the pointer lock request > > with the response through this SPI. Perhaps you want something like > > WKFramePolicyListener (an object that gets handed into the callback and has > > functions you can call on it to allow/deny)? But then it seems like the > > lower levels also have no request/response association... which seems > > similarly concerning. > > Pointer lock approval is for the page, not for specific elements and is > disallowed for iframes. While a page has pointer lock, if another element on > the page requests pointer lock, it immediately receives it. > > So long as requests and approvals go to the same page, I don't think it is > necessary to pair individual request. What about navigation? Site 1 requests pointer lock, we pop an alert and ask the user. The user says yes. While the round trip happens, a navigation occurs, site 2 requests pointer lock, round trip completes, UI process gets an "accept", gives site 2 pointer lock. > > > > > Source/WebKit2/UIProcess/API/C/WKPreferencesRef.h:288 > > > +WK_EXPORT void WKPreferencesSetPointerLockEnabled(WKPreferencesRef preferencesRef, bool enabled); > > > > No Modern API support? > > I'm doing my best to follow the patterns of other properties. I'm sure I've > missed something. Where can I look for how to add Modern API support? The Objective C API: WKPreferences (not -Ref) (if you expect it to need to be toggleable) or WKWebViewConfiguration (if you expect it to be fixed for the lifetime of a view, which seems fairly likely).

(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #4) > > > Comment on attachment 292388 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=292388&action=review > > > > > > > Source/WebKit/mac/WebView/WebPreferences.mm:565 > > > > +#if ENABLE(POINTER_LOCK) > > > > > > You're only registering the default if ENABLE(POINTER_LOCK), but have the > > > getters and setters always? That seems odd. Maybe just register the default > > > always? > > > > Done. > > > > > > > > > Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:126 > > > > +WK_EXPORT void WKPageDidAllowPointerLock(WKPageRef page); > > > > +WK_EXPORT void WKPageDidDenyPointerLock(WKPageRef page); > > > > > > It seems alarming that there is no way to associate the pointer lock request > > > with the response through this SPI. Perhaps you want something like > > > WKFramePolicyListener (an object that gets handed into the callback and has > > > functions you can call on it to allow/deny)? But then it seems like the > > > lower levels also have no request/response association... which seems > > > similarly concerning. > > > > Pointer lock approval is for the page, not for specific elements and is > > disallowed for iframes. While a page has pointer lock, if another element on > > the page requests pointer lock, it immediately receives it. > > > > So long as requests and approvals go to the same page, I don't think it is > > necessary to pair individual request. > > What about navigation? > > Site 1 requests pointer lock, we pop an alert and ask the user. The user > says yes. > While the round trip happens, a navigation occurs, site 2 requests pointer > lock, round trip completes, UI process gets an "accept", gives site 2 > pointer lock. Good point. Since that is a larger change. I'll address that in a follow up patch. https://bugs.webkit.org/show_bug.cgi?id=163852 > > > > > > > > Source/WebKit2/UIProcess/API/C/WKPreferencesRef.h:288 > > > > +WK_EXPORT void WKPreferencesSetPointerLockEnabled(WKPreferencesRef preferencesRef, bool enabled); > > > > > > No Modern API support? > > > > I'm doing my best to follow the patterns of other properties. I'm sure I've > > missed something. Where can I look for how to add Modern API support? > > The Objective C API: WKPreferences (not -Ref) (if you expect it to need to > be toggleable) or WKWebViewConfiguration (if you expect it to be fixed for > the lifetime of a view, which seems fairly likely). Done.

Created attachment 292503 [details] Patch

Comment on attachment 292503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292503&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:197 > +@property (nonatomic) BOOL allowsPointerLock WK_API_AVAILABLE(macosx(10.12)); This is wrong. This feature doesn't need to be exposed as API at all. Look at RuntimeEnabledFeatures.

Created attachment 294068 [details] Patch

Pointer Lock needs to be added to features.json.

Comment on attachment 294068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294068&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:6704 > + CGAssociateMouseAndMouseCursorPosition(true); It doesn't seem right that we're making CG-based calls in this class. I could see this 1) be handled in a separate Manager class, 2) pushed to the UIClient, or 3) at least surrounded with #if PLATFORM(MAC) like it currently is in WK1.

(In reply to comment #14) > Pointer Lock needs to be added to features.json. Added.

(In reply to comment #16) > (In reply to comment #14) > > Pointer Lock needs to be added to features.json. > > Added. (In reply to comment #15) > Comment on attachment 294068 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294068&action=review > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:6704 > > + CGAssociateMouseAndMouseCursorPosition(true); > > It doesn't seem right that we're making CG-based calls in this class. I > could see this 1) be handled in a separate Manager class, 2) pushed to the > UIClient, or 3) at least surrounded with #if PLATFORM(MAC) like it currently > is in WK1. Good point. I'll add #if PLATFORM(MAC) for this patch an file a follow up that moves both the WK1 and WK2 CG calls into a manager class.

(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #14) > > > Pointer Lock needs to be added to features.json. > > > > Added. > > (In reply to comment #15) > > Comment on attachment 294068 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=294068&action=review > > > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:6704 > > > + CGAssociateMouseAndMouseCursorPosition(true); > > > > It doesn't seem right that we're making CG-based calls in this class. I > > could see this 1) be handled in a separate Manager class, 2) pushed to the > > UIClient, or 3) at least surrounded with #if PLATFORM(MAC) like it currently > > is in WK1. > > Good point. I'll add #if PLATFORM(MAC) for this patch an file a follow up > that moves both the WK1 and WK2 CG calls into a manager class. https://bugs.webkit.org/show_bug.cgi?id=164634

(In reply to comment #12) > Comment on attachment 292503 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=292503&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:197 > > +@property (nonatomic) BOOL allowsPointerLock WK_API_AVAILABLE(macosx(10.12)); > > This is wrong. This feature doesn't need to be exposed as API at all. Look > at RuntimeEnabledFeatures. Remove this and changed to be a RuntimeEnabledFeature.

Created attachment 294476 [details] Patch

Comment on attachment 294476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294476&action=review > Source/WTF/wtf/FeatureDefines.h:641 > -#define ENABLE_POINTER_LOCK 0 > +#define ENABLE_POINTER_LOCK 1 This would normally be done in all the FeatureDefines.xcconfig files. > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:191 > +#if ENABLE(POINTER_LOCK) > + void setPointerLockEnabled(bool isEnabled) { m_isPointerLockEnabled = isEnabled; } > + bool pointerLockEnabled() const { return m_isPointerLockEnabled; } > +#endif We're trying to move away from RuntimeEnabledFeatures (it's a confusing set of global flags with inconsistent initialization, and the flags often alias Settings). If you can, I would prefer removing this and using a Setting, which it seems you are adding too. > Source/WebCore/features.json:283 > + "name": "Jeremy Jones", > + "email": "jeremyj@apple.com" You don't have to put a contact here if you don't want to. You should, however, make sure there's a bugzilla tracking bug and add the appropriate property. > Source/WebCore/page/PointerLockController.cpp:59 > + if (!RuntimeEnabledFeatures::sharedFeatures().pointerLockEnabled()) { > + enqueueEvent(eventNames().pointerlockerrorEvent, target); > + return; This is wrong. If the feature is disabled, you should behave as if it's unimplemented, so never fire any pointer lock-related events. > Source/WebKit/mac/WebView/WebView.mm:2534 > + RuntimeEnabledFeatures::sharedFeatures().setPointerLockEnabled([preferences pointerLockEnabled]); This would set the setting. > Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:133 > +WK_EXPORT void WKPageDidAllowPointerLock(WKPageRef page); > +WK_EXPORT void WKPageDidDenyPointerLock(WKPageRef page); Do we have to add C API at all? > Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:642 > +typedef struct WKPageUIClientV8 { Ugh. Please talk to Sam about this. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3195 > + RuntimeEnabledFeatures::sharedFeatures().setPointerLockEnabled(store.getBoolValueForKey(WebPreferencesKey::pointerLockEnabledKey())); This should set a setting.

(In reply to comment #21) > Comment on attachment 294476 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294476&action=review > > > Source/WTF/wtf/FeatureDefines.h:641 > > -#define ENABLE_POINTER_LOCK 0 > > +#define ENABLE_POINTER_LOCK 1 > > This would normally be done in all the FeatureDefines.xcconfig files. Done. > > > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:191 > > +#if ENABLE(POINTER_LOCK) > > + void setPointerLockEnabled(bool isEnabled) { m_isPointerLockEnabled = isEnabled; } > > + bool pointerLockEnabled() const { return m_isPointerLockEnabled; } > > +#endif > > We're trying to move away from RuntimeEnabledFeatures (it's a confusing set > of global flags with inconsistent initialization, and the flags often alias > Settings). If you can, I would prefer removing this and using a Setting, > which it seems you are adding too. Will that still allow the feature to be enabled at runtime while the feature is in development? > > > Source/WebCore/features.json:283 > > + "name": "Jeremy Jones", > > + "email": "jeremyj@apple.com" > > You don't have to put a contact here if you don't want to. You should, > however, make sure there's a bugzilla tracking bug and add the appropriate > property. Removed contact, added: https://bugs.webkit.org/show_bug.cgi?id=164652 > > > Source/WebCore/page/PointerLockController.cpp:59 > > + if (!RuntimeEnabledFeatures::sharedFeatures().pointerLockEnabled()) { > > + enqueueEvent(eventNames().pointerlockerrorEvent, target); > > + return; > > This is wrong. If the feature is disabled, you should behave as if it's > unimplemented, so never fire any pointer lock-related events. Done. Moved this to the top of the function; it no longer enqueues an event. I added the same to requestPointerUnlock() > > > Source/WebKit/mac/WebView/WebView.mm:2534 > > + RuntimeEnabledFeatures::sharedFeatures().setPointerLockEnabled([preferences pointerLockEnabled]); > > This would set the setting. I'll change this back to using settings. > > > Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:133 > > +WK_EXPORT void WKPageDidAllowPointerLock(WKPageRef page); > > +WK_EXPORT void WKPageDidDenyPointerLock(WKPageRef page); > > Do we have to add C API at all? Yes. > > > Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:642 > > +typedef struct WKPageUIClientV8 { > > Ugh. Please talk to Sam about this. Will do. > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3195 > > + RuntimeEnabledFeatures::sharedFeatures().setPointerLockEnabled(store.getBoolValueForKey(WebPreferencesKey::pointerLockEnabledKey())); > > This should set a setting. I'll revert this back to using Setting instead of RuntimeEnabledFeatures.

Created attachment 295166 [details] Patch

Created attachment 295167 [details] Patch

Comment on attachment 295167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295167&action=review > Source/WebCore/page/PointerLockController.cpp:55 > + Settings* settings = target->document().settings(); > + if (!settings || !settings->pointerLockEnabled()) > + return; > + Should these lines be moved after if (!target ....) since it checks for (!target) and you are using it to get the settings?

(In reply to comment #25) > Comment on attachment 295167 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295167&action=review > > > Source/WebCore/page/PointerLockController.cpp:55 > > + Settings* settings = target->document().settings(); > > + if (!settings || !settings->pointerLockEnabled()) > > + return; > > + > > Should these lines be moved after if (!target ....) since it checks for > (!target) and you are using it to get the settings? Good catch. I should actually be using m_page->settings().

Created attachment 295169 [details] Patch

Comment on attachment 295169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295169&action=review > Source/WTF/wtf/FeatureDefines.h:634 > #if !defined(ENABLE_POINTER_LOCK) > -#define ENABLE_POINTER_LOCK 0 > +#define ENABLE_POINTER_LOCK 1 > #endif Why is this here? For other platforms?

Comment on attachment 295169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295169&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:58 > +- (void)_webViewRequestPointerLock:(WKWebView *)webView WK_API_AVAILABLE(macosx(10.12)); Not 10.12, should be WK_TBA.

(In reply to comment #29) > Comment on attachment 295169 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295169&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:58 > > +- (void)_webViewRequestPointerLock:(WKWebView *)webView WK_API_AVAILABLE(macosx(10.12)); > > Not 10.12, should be WK_TBA. Too late to stop commit queue, but I'll post a follow up patch to fix this.(In reply to comment #29) > Comment on attachment 295169 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295169&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:58 > > +- (void)_webViewRequestPointerLock:(WKWebView *)webView WK_API_AVAILABLE(macosx(10.12)); > > Not 10.12, should be WK_TBA. https://bugs.webkit.org/show_bug.cgi?id=164962

Comment on attachment 295169 [details] Patch Clearing flags on attachment: 295169 Committed r208903: <http://trac.webkit.org/changeset/208903>

All reviewed patches have been landed. Closing bug.