Add runtime flag to enable pointer lock. Enable pointer lock feature for mac.

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

Add runtime flag to enable pointer lock. Enable pointer lock feature for mac.

[Jeremy Jones]

Created attachment 292383 [details]
Patch

[Jeremy Jones]

Created attachment 292384 [details]
Patch

[Jeremy Jones]

Created attachment 292388 [details]
Patch

[Tim Horton]

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?

[Tim Horton]

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?

[Jeremy Jones]

(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.

[Jeremy Jones]

(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?

[Tim Horton]

(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).

[Jeremy Jones]

(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.

[Jeremy Jones]

Created attachment 292503 [details]
Patch

[Jon Lee]

rdar://problem/28550973

[Jon Lee]

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.

[Jeremy Jones]

Created attachment 294068 [details]
Patch

[Simon Fraser (smfr)]

Pointer Lock needs to be added to features.json.

[Jon Lee]

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.

[Jeremy Jones]

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

Added.

[Jeremy Jones]

(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.

[Jeremy Jones]

(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

[Jeremy Jones]

(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.

[Jeremy Jones]

Created attachment 294476 [details]
Patch

[Simon Fraser (smfr)]

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": "[e-mail]"

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.

[Jeremy Jones]

(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": "[e-mail]"
> 
> 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.

[Jeremy Jones]

Created attachment 295166 [details]
Patch

[Jeremy Jones]

Created attachment 295167 [details]
Patch

[Said Abou-Hallawa]

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?

[Jeremy Jones]

(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().

[Jeremy Jones]

Created attachment 295169 [details]
Patch

[Simon Fraser (smfr)]

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?

[Tim Horton]

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.

[Jeremy Jones]

(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

[WebKit Commit Bot]

Comment on attachment 295169 [details]
Patch

Clearing flags on attachment: 295169

Committed r208903: <http://trac.webkit.org/changeset/208903>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.