Bug 99036 – [GTK] Enable Pointer Lock feature

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKitGTK

[Zan Dobersek]

Guarded by the ENABLE_POINTER_LOCK feature define. Not currently present in Source/WebCore/GNUmakefile.features.am.

[Anton Obzhirov]

I will have a look on this one.

[Anton Obzhirov]

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

[Anton Obzhirov]

Created attachment 207452 [details]
Patch

[Anton Obzhirov]

Early path for initial review.

[Anton Obzhirov]

Created attachment 207851 [details]
Patch

[Anton Obzhirov]

Updated patch to get latest upstream and to get EWS running.

[WebKit Commit Bot]

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

[Early Warning System Bot]

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

[EFL EWS Bot]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 207851 [details]
Patch

Attachment 207851 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1290805

[Build Bot]

Comment on attachment 207851 [details]
Patch

Attachment 207851 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1297642

[Simon Pena]

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/!

[Simon Pena]

I filed a bug about the Quake 3 Map explorer not working due to a crash in JSC, in bug #119895

[Ben Boeckel]

Should this use permission-request as well? Or is there a guaranteed way to break any pointer lock status?

[Anton Obzhirov]

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

[Anton Obzhirov]

Created attachment 213248 [details]
Patch

[EFL EWS Bot]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 213248 [details]
Patch

Attachment 213248 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3116135

[Anton Obzhirov]

Created attachment 213276 [details]
Patch

[Anton Obzhirov]

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.

[Martin Robinson]

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

[Anton Obzhirov]

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.

[Anton Obzhirov]

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.

[Martin Robinson]

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

[Jinwoo Song]

Is there any more progress? I'd like to start work in EFL port so I'm looking forward this patch to be landed. :)

[Anton Obzhirov]

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

[Jinwoo Song]

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

[ChangSeok Oh]

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

[Michael Catanzaro]

Comment on attachment 213276 [details]
Patch

Looks like this patch needs more work, according to the comments above.

[Carlos Alberto Lopez Perez]

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 ***