Bug 162745 – Implement basic pointer lock behavior for WebKit and WebKit2.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jeremy Jones]

Implement basic pointer lock behavior for WebKit and WebKit2.

[Jeremy Jones]

rdar://problem/16393663

[Jeremy Jones]

Created attachment 290235 [details]
Patch

[Dean Jackson]

Comment on attachment 290235 [details]
Patch

A WK2 owner should probably take a look too.

[Jon Lee]

Comment on attachment 290235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290235&action=review

r- until we have a WK2 reviewer.

> Source/WebCore/ChangeLog:8
> +        No new tests until feature is ready to enable.

What is the new bug that will deal with test coverage?

[Jon Lee]

Comment on attachment 290235 [details]
Patch

Sorry, better for it to be r? instead.

[Tim Horton]

Please make the patch not red on all the platforms that we care about :|

[Jeremy Jones]

Use this radar instead:
rdar://problem/28550438

[Jeremy Jones]

(In reply to comment #6)
> Please make the patch not red on all the platforms that we care about :|

I failed up upload the project change that adds 'WebCore/PointerLockController.h' as a WebCore private header.

[Jeremy Jones]

Created attachment 290242 [details]
Patch

[Daniel Bates]

Can we write tests for this change?

[Jeremy Jones]

(In reply to comment #10)
> Can we write tests for this change?

New layout tests shouldn't be necessary for this change as there are already many pointer-lock tests in LayoutTests/pointer-lock This change should make some of them pass. I'll annotate with which ones when I finish running the tests.

[Simon Fraser (smfr)]

Comment on attachment 290242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290242&action=review

> Source/WebKit2/Shared/WebEventConversion.cpp:102
> +        m_movementDelta = WebCore::IntPoint(webEvent.deltaX(), webEvent.deltaY());

Should m_movementDelta really be integral? What about mouse positions on retina screens?

[Jeremy Jones]

(In reply to comment #12)
> Comment on attachment 290242 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290242&action=review
> 
> > Source/WebKit2/Shared/WebEventConversion.cpp:102
> > +        m_movementDelta = WebCore::IntPoint(webEvent.deltaX(), webEvent.deltaY());
> 
> Should m_movementDelta really be integral? What about mouse positions on
> retina screens?

That does make sense. It would require also changing m_position and m_globalPosition.

https://bugs.webkit.org/show_bug.cgi?id=163566

[Jeremy Jones]

Created attachment 291880 [details]
Patch

[WebKit Commit Bot]

Attachment 291880 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:0:  Use #pragma once header guard.  [build/header_guard] [5]
ERROR: Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:720:  No space between ^ and block definition.  [whitespace/brackets] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 291880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291880&action=review

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:720
> +    dispatch_async(dispatch_get_main_queue(), ^{

Why is the dispatch_async() required?

[Tim Horton]

Comment on attachment 291880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291880&action=review

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:744
> +    return false;

Always false?

[Jeremy Jones]

(In reply to comment #16)
> Comment on attachment 291880 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291880&action=review
> 
> > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:720
> > +    dispatch_async(dispatch_get_main_queue(), ^{
> 
> Why is the dispatch_async() required?

The lower level logic doesn't handle receiving didAcquirePointerLock() call before requestPointerLock() returns. I'll remove this from this patch and have a better fix in a subsequent patch.

[Jeremy Jones]

(In reply to comment #18)
> (In reply to comment #16)
> > Comment on attachment 291880 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=291880&action=review
> > 
> > > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:720
> > > +    dispatch_async(dispatch_get_main_queue(), ^{
> > 
> > Why is the dispatch_async() required?
> 
> The lower level logic doesn't handle receiving didAcquirePointerLock() call
> before requestPointerLock() returns. I'll remove this from this patch and
> have a better fix in a subsequent patch.

Actually lower level logic is easily fixed. I'll just remove the dispatch_async workaround.

[Jeremy Jones]

(In reply to comment #17)
> Comment on attachment 291880 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291880&action=review
> 
> > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:744
> > +    return false;
> 
> Always false?

Actually isPointerLock() isn't used. Removed.

[Jeremy Jones]

Created attachment 292361 [details]
Patch for landing.

[Jeremy Jones]

Created attachment 292367 [details]
Patch for landing.

[Jeremy Jones]

Created attachment 292373 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 292373 [details]
Patch for landing.

Clearing flags on attachment: 292373

Committed r207689: <http://trac.webkit.org/changeset/207689>