Summary: | [GTK] Enable Pointer Lock feature | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||||||||
Component: | WebKitGTK | Assignee: | 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
Zan Dobersek
2012-10-11 06:00:26 PDT
I will have a look on this one. 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 Created attachment 207452 [details]
Patch
Early path for initial review. Created attachment 207851 [details]
Patch
Updated patch to get latest upstream and to get EWS running. 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 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 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 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 on attachment 207851 [details] Patch Attachment 207851 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1290805 Comment on attachment 207851 [details] Patch Attachment 207851 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1297642 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/! I filed a bug about the Quake 3 Map explorer not working due to a crash in JSC, in bug #119895 Should this use permission-request as well? Or is there a guaranteed way to break any pointer lock status? (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. Created attachment 213248 [details]
Patch
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 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 on attachment 213248 [details] Patch Attachment 213248 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/3116135 Created attachment 213276 [details]
Patch
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 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 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. 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. (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? Is there any more progress? I'd like to start work in EFL port so I'm looking forward this patch to be landed. :) (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. (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. (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 on attachment 213276 [details]
Patch
Looks like this patch needs more work, according to the comments above.
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 *** |