Bug 162745 - Implement basic pointer lock behavior for WebKit and WebKit2.
Summary: Implement basic pointer lock behavior for WebKit and WebKit2.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-09-29 12:22 PDT by Jeremy Jones
Modified: 2016-12-10 00:37 PST (History)
9 users (show)

See Also:


Attachments
Patch (17.36 KB, patch)
2016-09-29 12:58 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (19.50 KB, patch)
2016-09-29 14:11 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (19.87 KB, patch)
2016-10-17 15:04 PDT, Jeremy Jones
simon.fraser: review+
Details | Formatted Diff | Diff
Patch for landing. (18.91 KB, patch)
2016-10-21 10:07 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (20.97 KB, patch)
2016-10-21 10:28 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (20.98 KB, patch)
2016-10-21 10:45 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2016-09-29 12:22:22 PDT
Implement basic pointer lock behavior for WebKit and WebKit2.
Comment 1 Jeremy Jones 2016-09-29 12:55:13 PDT
rdar://problem/16393663
Comment 2 Jeremy Jones 2016-09-29 12:58:53 PDT
Created attachment 290235 [details]
Patch
Comment 3 Dean Jackson 2016-09-29 13:38:21 PDT
Comment on attachment 290235 [details]
Patch

A WK2 owner should probably take a look too.
Comment 4 Jon Lee 2016-09-29 13:39:57 PDT
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?
Comment 5 Jon Lee 2016-09-29 13:40:24 PDT
Comment on attachment 290235 [details]
Patch

Sorry, better for it to be r? instead.
Comment 6 Tim Horton 2016-09-29 13:48:36 PDT
Please make the patch not red on all the platforms that we care about :|
Comment 7 Jeremy Jones 2016-09-29 13:55:21 PDT
Use this radar instead:
rdar://problem/28550438
Comment 8 Jeremy Jones 2016-09-29 14:07:54 PDT
(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.
Comment 9 Jeremy Jones 2016-09-29 14:11:01 PDT
Created attachment 290242 [details]
Patch
Comment 10 Daniel Bates 2016-09-29 20:44:47 PDT
Can we write tests for this change?
Comment 11 Jeremy Jones 2016-10-05 13:45:31 PDT
(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.
Comment 12 Simon Fraser (smfr) 2016-10-13 13:56:47 PDT
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?
Comment 13 Jeremy Jones 2016-10-17 14:48:50 PDT
(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
Comment 14 Jeremy Jones 2016-10-17 15:04:10 PDT
Created attachment 291880 [details]
Patch
Comment 15 WebKit Commit Bot 2016-10-17 15:06:44 PDT
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.
Comment 16 Simon Fraser (smfr) 2016-10-20 16:42:59 PDT
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?
Comment 17 Tim Horton 2016-10-20 16:47:55 PDT
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?
Comment 18 Jeremy Jones 2016-10-21 09:25:05 PDT
(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.
Comment 19 Jeremy Jones 2016-10-21 09:46:31 PDT
(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.
Comment 20 Jeremy Jones 2016-10-21 09:47:12 PDT
(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.
Comment 21 Jeremy Jones 2016-10-21 10:07:27 PDT
Created attachment 292361 [details]
Patch for landing.
Comment 22 Jeremy Jones 2016-10-21 10:28:21 PDT
Created attachment 292367 [details]
Patch for landing.
Comment 23 Jeremy Jones 2016-10-21 10:45:54 PDT
Created attachment 292373 [details]
Patch for landing.
Comment 24 WebKit Commit Bot 2016-10-21 11:48:25 PDT
Comment on attachment 292373 [details]
Patch for landing.

Clearing flags on attachment: 292373

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