Bug 99036

Summary: [GTK] Enable Pointer Lock feature
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebKitGTKAssignee: Anton Obzhirov <obzhirov>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: benjamin, bugs-noreply, buildbot, cgarcia, changseok, clopez, cmarcelo, commit-queue, eflews.bot, esprehn+autocc, gustavo, gyuyoung.kim, jinwoo7.song, kangil.han, mathstuf, mrobinson, obzhirov, rniwa, sam, scheib, spenap
Priority: P2 Keywords: Gtk, LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch mcatanzaro: review-

Description Zan Dobersek 2012-10-11 06:00:26 PDT
Guarded by the ENABLE_POINTER_LOCK feature define. Not currently present in Source/WebCore/GNUmakefile.features.am.
Comment 1 Anton Obzhirov 2013-02-05 05:38:05 PST
I will have a look on this one.
Comment 2 Anton Obzhirov 2013-07-25 05:55:27 PDT
The tests that are really testing the feature are located in LayoutTests/pointer-lock.
Some information about pointer lock:
https://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html
https://developer.mozilla.org/en-US/docs/WebAPI/Pointer_Lock
Comment 3 Anton Obzhirov 2013-07-25 06:12:02 PDT
Created attachment 207452 [details]
Patch
Comment 4 Anton Obzhirov 2013-07-25 06:14:16 PDT
Early path for initial review.
Comment 5 Anton Obzhirov 2013-07-31 07:38:33 PDT
Created attachment 207851 [details]
Patch
Comment 6 Anton Obzhirov 2013-07-31 07:39:40 PDT
Updated patch to get latest upstream and to get EWS running.
Comment 7 WebKit Commit Bot 2013-07-31 07:40:42 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 8 Early Warning System Bot 2013-07-31 07:56:03 PDT
Comment on attachment 207851 [details]
Patch

Attachment 207851 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1297611
Comment 9 EFL EWS Bot 2013-07-31 08:14:04 PDT
Comment on attachment 207851 [details]
Patch

Attachment 207851 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1290790
Comment 10 Build Bot 2013-07-31 08:32:45 PDT
Comment on attachment 207851 [details]
Patch

Attachment 207851 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1296730
Comment 11 Build Bot 2013-07-31 09:36:54 PDT
Comment on attachment 207851 [details]
Patch

Attachment 207851 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1290805
Comment 12 Build Bot 2013-07-31 10:40:15 PDT
Comment on attachment 207851 [details]
Patch

Attachment 207851 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1297642
Comment 13 Simon Pena 2013-08-16 06:03:08 PDT
I applied your patch locally and then checked http://threejs.org/examples/misc_controls_pointerlock.html, and it works! :)

(I run it with Tools/Scripts/run-launcher --gtk --release -2 --enable-webgl=TRUE http://threejs.org/examples/misc_controls_pointerlock.html)

However, I noticed a couple of issues:

 * First, the lock wasn't released when I pressed Esc, so I had to close minibrowser to stop.
 * Then, you probably need to do some adjustments on how you calculate the way it turns. In Chromium, you're able to turn 360º, while in Minibrowser the range was something closer to maybe 120º. That also happens when looking up & down.

Regarding the patch itself, I didn't check much, but I think you missed some memory cleanup in the webkitWebViewBaseUnlockMouse and webkitWebViewBaseLockMouse methods.

Pretty nice! I already look forward to trying http://media.tojicode.com/q3bsp/!
Comment 14 Simon Pena 2013-08-19 01:23:36 PDT
I filed a bug about the Quake 3 Map explorer not working due to a crash in JSC, in bug #119895
Comment 15 Ben Boeckel 2013-09-04 17:46:11 PDT
Should this use permission-request as well? Or is there a guaranteed way to break any pointer lock status?
Comment 16 Anton Obzhirov 2013-09-10 03:04:26 PDT
(In reply to comment #15)
> Should this use permission-request as well? Or is there a guaranteed way to break any pointer lock status?
Well ESC should break it - however after checking other browsers they do issue permission request. So probably I need to add it in my patch or create new patch after this one is approved.
Comment 17 Anton Obzhirov 2013-10-03 08:25:55 PDT
Created attachment 213248 [details]
Patch
Comment 18 EFL EWS Bot 2013-10-03 08:46:07 PDT
Comment on attachment 213248 [details]
Patch

Attachment 213248 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2918146
Comment 19 Build Bot 2013-10-03 09:08:44 PDT
Comment on attachment 213248 [details]
Patch

Attachment 213248 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3109155
Comment 20 Build Bot 2013-10-03 09:42:59 PDT
Comment on attachment 213248 [details]
Patch

Attachment 213248 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3116135
Comment 21 Anton Obzhirov 2013-10-03 10:43:17 PDT
Created attachment 213276 [details]
Patch
Comment 22 Anton Obzhirov 2013-10-04 02:30:28 PDT
Summary about my 2013-10-03 patch: I fixed mouse clipping issue, added handling of ESC key, fixed few other small issues and tried to simplify patch a bit. All tests from LayoutTests/pointer-lock/ are pass. Regarding permission request I am planning to submit it after this patch goes upstream.
Comment 23 Martin Robinson 2013-10-17 11:17:19 PDT
Comment on attachment 213276 [details]
Patch

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

Hrm. I'm not sure I understand why so much of this is in the UI process. It seems like a lot of it could move to the web process and be written in a cross-platform way.

> Source/WTF/wtf/FeatureDefines.h:692
>  #if !defined(ENABLE_POINTER_LOCK)
> +#if PLATFORM(GTK)
> +#define ENABLE_POINTER_LOCK 1
> +#else
>  #define ENABLE_POINTER_LOCK 0
>  #endif
> +#endif

This probably isn't the right place to do this. Instead you should define ENABLE_POINTER_LOCK via the Scripts directory.

> Source/WebCore/dom/Document.cpp:5492
>      if (Element* target = page()->pointerLockController()->element()) {
> -        if (target->document() != this)
> +        if (&target->document() != this)
>              return;
>      }

Is this just a build fix? If so, you should split it out into another patch.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:149
>  #endif
> -
>          virtual bool shouldRubberBandInDirection(ScrollDirection) const { return true; }

Looks unecessary
Comment 24 Anton Obzhirov 2013-10-18 01:45:13 PDT
Comment on attachment 213276 [details]
Patch

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

>> Source/WTF/wtf/FeatureDefines.h:692
>> +#endif
> 
> This probably isn't the right place to do this. Instead you should define ENABLE_POINTER_LOCK via the Scripts directory.

Yep, will do that.

>> Source/WebCore/dom/Document.cpp:5492
>>      }
> 
> Is this just a build fix? If so, you should split it out into another patch.

Yes. I'll create another patch for this.

>> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:149
>>          virtual bool shouldRubberBandInDirection(ScrollDirection) const { return true; }
> 
> Looks unecessary

Sorry, forgot to remove white space.
Comment 25 Anton Obzhirov 2013-10-18 02:17:15 PDT
Thanks for the review!

About changes in UIProcess - could you clarify what can be made cross platform?

In general each platform has it its own way of locking pointer and handling mouse events when mouse locked is different as well (for example I have to move mouse back to the the center of the window to avoid clipping of mouse movements in GTK). I also followed the model they use in blink since it already works there.
Comment 26 Martin Robinson 2013-10-30 12:20:44 PDT
(In reply to comment #25)
> Thanks for the review!
> 
> About changes in UIProcess - could you clarify what can be made cross platform?
> 
> In general each platform has it its own way of locking pointer and handling mouse events when mouse locked is different as well (for example I have to move mouse back to the the center of the window to avoid clipping of mouse movements in GTK). I also followed the model they use in blink since it already works there.

I was thinking it might make more sense to just notify that the WebProcess that pointer lock has started and always send the same type of events across the IPC channel. Then the WebProcess can be responsible for interpreting the events properly. I'm not sure it makes sense for the UIProcess to modify the event according to the pointer lock state. Is there a reason this kind of design is infeasible?
Comment 27 Jinwoo Song 2014-03-13 22:02:25 PDT
Is there any more progress? I'd like to start work in EFL port so I'm looking forward this patch to be landed. :)
Comment 28 Anton Obzhirov 2014-03-14 07:08:20 PDT
(In reply to comment #27)
> Is there any more progress? I'd like to start work in EFL port so I'm looking forward this patch to be landed. :)
I have a plan to start working on this patch soon. Probably beginning of April. Please let me know if this would be good enough for you, so we can think of some alternative solution otherwise.
Comment 29 Jinwoo Song 2014-03-14 19:50:09 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > Is there any more progress? I'd like to start work in EFL port so I'm looking forward this patch to be landed. :)
> I have a plan to start working on this patch soon. Probably beginning of April. Please let me know if this would be good enough for you, so we can think of some alternative solution otherwise.

Sounds good! I'll prepare a patch based on yours.
Comment 30 ChangSeok Oh 2015-07-16 01:48:30 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > Is there any more progress? I'd like to start work in EFL port so I'm looking forward this patch to be landed. :)
> I have a plan to start working on this patch soon. Probably beginning of
> April. Please let me know if this would be good enough for you, so we can
> think of some alternative solution otherwise.

Any update on this?
Comment 31 Michael Catanzaro 2016-01-06 19:46:20 PST
Comment on attachment 213276 [details]
Patch

Looks like this patch needs more work, according to the comments above.
Comment 32 Carlos Alberto Lopez Perez 2019-11-08 08:04:42 PST
I just discovered this bug now :\
This feature has been implemented in the end in bugs 202956 and 203896

*** This bug has been marked as a duplicate of bug 202956 ***