Bug 75762

Summary: Pointer Lock: Implement pointer interface
Product: WebKit Reporter: Vincent Scheib <scheib>
Component: New BugsAssignee: Vincent Scheib <scheib>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, gustavo, jchaffraix, tkent, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 76311, 76410, 76411, 77030    
Bug Blocks: 84386    
Attachments:
Description Flags
Patch
none
Tests added
none
Updated big patch, includes all subpatches
none
Small patch, no subpatches
none
Patch
none
Rerun EWS
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
jchaffraix: review+
Patch for landing. none

Description Vincent Scheib 2012-01-06 17:12:22 PST
Pointer Lock: Implement pointer interface
Comment 1 Vincent Scheib 2012-01-06 17:15:28 PST
Created attachment 121524 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-09 10:31:23 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Vincent Scheib 2012-01-11 13:44:31 PST
Created attachment 122087 [details]
Tests added
Comment 4 Gustavo Noronha (kov) 2012-01-11 14:26:06 PST
Comment on attachment 122087 [details]
Tests added

Attachment 122087 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11108580
Comment 5 Gyuyoung Kim 2012-01-11 14:28:22 PST
Comment on attachment 122087 [details]
Tests added

Attachment 122087 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11211140
Comment 6 Dimitri Glazkov (Google) 2012-01-12 10:27:46 PST
Comment on attachment 122087 [details]
Tests added

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

This patch needs to be broken up into smaller bits. Also EWS is sad.

> Source/WebCore/page/PointerLock.cpp:47
> +    return page()->pointerLockController()->lock(target, successCallback, failureCallback);

Can you not just hold on to the pointerLockController in this class?

> Source/WebCore/page/PointerLockController.h:46
> +    void lock(WebCore::Element* target, PassRefPtr<WebCore::VoidCallback> successCallback, PassRefPtr<WebCore::VoidCallback> failureCallback);

Why are these guys all namespace-prefixed?

> Source/WebKit/chromium/public/WebWidgetClient.h:130
> +    virtual bool pointerLock(WebKit::WebWidget*) { return false; }

Again, why are these namespace-prefixed?
Comment 7 Darin Fisher (:fishd, Google) 2012-01-12 13:10:52 PST
Comment on attachment 122087 [details]
Tests added

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

> Source/WebKit/chromium/public/WebWidget.h:175
> +    virtual void pointerLockRequestResult(bool success) { }

nit: didCompletePointerLockRequest  ... verbose, but maybe that's ok?

> Source/WebKit/chromium/public/WebWidget.h:177
> +    virtual void pointerLockLost() { }

prefer "didLoseFoo" over "fooLost"

> Source/WebKit/chromium/public/WebWidgetClient.h:131
> +    virtual void pointerUnlock(WebKit::WebWidget*) { }

You can assume that each WebWidgetClient is associated with a particular WebWidget.
That's why none of the other functions take a WebWidget pointer.

> Tools/DumpRenderTree/LayoutTestController.cpp:2214
> +

nit: nuke extra new line

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:784
> +    webWidget()->pointerLockRequestResult(m_pointerLocked);

why call this on both?
Comment 8 Vincent Scheib 2012-01-16 17:26:42 PST
Created attachment 122697 [details]
Updated big patch, includes all subpatches
Comment 9 Vincent Scheib 2012-01-16 17:35:55 PST
Created attachment 122698 [details]
Small patch, no subpatches
Comment 10 Vincent Scheib 2012-01-16 17:38:34 PST
I've split out two patches:
Bug 76410 - [Chromium] Add WebKit API for Pointer Lock
Bug 76411 - [Chromium] Add Pointer Lock test hooks and mock implementation to DumpRenderTree

I've attached patches that both include and exclude the carved out dependent patches.
Comment 11 Vincent Scheib 2012-01-24 14:07:37 PST
Created attachment 123805 [details]
Patch
Comment 12 WebKit Review Bot 2012-01-24 15:32:07 PST
Comment on attachment 123805 [details]
Patch

Attachment 123805 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11242344

New failing tests:
fast/dom/navigator-detached-no-crash.html
Comment 13 Vincent Scheib 2012-01-24 15:47:55 PST
Created attachment 123832 [details]
Rerun EWS
Comment 14 WebKit Review Bot 2012-01-24 17:34:51 PST
Comment on attachment 123832 [details]
Rerun EWS

Attachment 123832 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11242385

New failing tests:
fast/dom/navigator-detached-no-crash.html
Comment 15 Vincent Scheib 2012-01-25 11:09:28 PST
Created attachment 123976 [details]
Patch
Comment 16 Adam Barth 2012-01-25 11:17:52 PST
Comment on attachment 123976 [details]
Patch

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

I think you have some memory safety problems...

> Source/WebCore/page/PointerLock.h:52
> +    PointerLockController* m_controller;

How do you know whether this object hasn't been deallocated?

> Source/WebCore/page/PointerLockController.cpp:83
> +        callbackToIssue->handleEvent();

How do you know the ScriptExecutionContext for this callback is still active?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1019
> +#if ENABLE(POINTER_LOCK)
> +    requestPointerUnlock();
> +#endif

This doesn't seem right.  Is pointer lock scoped to the lifetime of the WebView or to the lifetime of a particular document?  How can a document that's not longer active in a WebView hold the pointer lock?
Comment 17 Vincent Scheib 2012-01-25 15:14:10 PST
Comment on attachment 123976 [details]
Patch

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

>> Source/WebCore/page/PointerLock.h:52
>> +    PointerLockController* m_controller;
> 
> How do you know whether this object hasn't been deallocated?

PointerLockController is owned by the page. If we're concerned about lifetime perhaps PointerLock could be a DOMWindowProperty, and be safely disconnected from the frame, sound good? (I see that in page/Geolocation.h). Other suggestion?

>> Source/WebCore/page/PointerLockController.cpp:83
>> +        callbackToIssue->handleEvent();
> 
> How do you know the ScriptExecutionContext for this callback is still active?

Do you think m_element->inDocument() would be sufficient? 
(Note, To keep this patch smaller I've broken off Bug 77029 - Pointer Lock handles disconnected DOM elements)
Comment 18 Adam Barth 2012-01-25 15:17:27 PST
(In reply to comment #17)
> (From update of attachment 123976 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123976&action=review
> 
> >> Source/WebCore/page/PointerLock.h:52
> >> +    PointerLockController* m_controller;
> > 
> > How do you know whether this object hasn't been deallocated?
> 
> PointerLockController is owned by the page. If we're concerned about lifetime perhaps PointerLock could be a DOMWindowProperty, and be safely disconnected from the frame, sound good? (I see that in page/Geolocation.h). Other suggestion?

Yes.  That's a good idea.

> >> Source/WebCore/page/PointerLockController.cpp:83
> >> +        callbackToIssue->handleEvent();
> > 
> > How do you know the ScriptExecutionContext for this callback is still active?
> 
> Do you think m_element->inDocument() would be sufficient? 
> (Note, To keep this patch smaller I've broken off Bug 77029 - Pointer Lock handles disconnected DOM elements)

Hum...  Maybe checking m_element->document()->frame() would be better.
Comment 19 Vincent Scheib 2012-01-25 17:17:29 PST
Created attachment 124041 [details]
Patch
Comment 20 Vincent Scheib 2012-01-25 17:23:48 PST
- Changed PointerLock to be a DOMWindowProperty to receive a disconnectFrame() notice and clear the reference to PointerLockController. 

- PointerLockController callbacks and events check that the m_element->document()->frame() exists first.

- Removed WebViewImpl call to requestPointerUnlock(). On the WebCore side this should be addressed in Bug 77029 - Pointer Lock handles disconnected DOM elements, the WebViewImpl should trust unlocks are made. The client owning the webwidget should handle paranoia of a misbehaving WebKit and clean up on destruction.
Comment 21 Adam Barth 2012-01-25 19:36:35 PST
Comment on attachment 124041 [details]
Patch

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

Thanks!

> Source/WebCore/page/PointerLock.cpp:41
> +    if (!m_frame)
> +        return;

You're already checking that frame is non-zero when creating this object.  Should we just ASSERT that frame is non-zero?

> Source/WebCore/page/PointerLock.h:48
> +    virtual void disconnectFrame();

Can you add the OVERRIDE keyword?  We're starting to use it in WebKit.

> Source/WebCore/page/PointerLock.h:55
> +    PointerLock(Frame*);

explicit

> Source/WebCore/page/PointerLockController.cpp:81
> +    if (callbackToIssue && elementToNotify && elementToNotify->document() && elementToNotify->document()->frame())
> +        callbackToIssue->handleEvent();

elementToNotify->document() will always be non-zero.  There's no need to check it.

> Source/WebCore/page/PointerLockController.h:44
> +    PointerLockController(Page*);

explicit
Comment 22 Adam Barth 2012-01-25 19:37:52 PST
I'm happy with the low-level mechanics of this patch, but I'd like fishd to take a look as well.
Comment 23 Vincent Scheib 2012-01-25 22:23:54 PST
Created attachment 124064 [details]
Patch
Comment 24 Vincent Scheib 2012-01-25 22:25:35 PST
Comment on attachment 124041 [details]
Patch

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

fishd (or dglazkov?) please take a look.

Adam - thanks for moving this forward.

>> Source/WebCore/page/PointerLock.cpp:41
>> +        return;
> 
> You're already checking that frame is non-zero when creating this object.  Should we just ASSERT that frame is non-zero?

Done.

>> Source/WebCore/page/PointerLock.h:48
>> +    virtual void disconnectFrame();
> 
> Can you add the OVERRIDE keyword?  We're starting to use it in WebKit.

Done.

>> Source/WebCore/page/PointerLock.h:55
>> +    PointerLock(Frame*);
> 
> explicit

Done.

>> Source/WebCore/page/PointerLockController.cpp:81
>> +        callbackToIssue->handleEvent();
> 
> elementToNotify->document() will always be non-zero.  There's no need to check it.

Done.

>> Source/WebCore/page/PointerLockController.h:44
>> +    PointerLockController(Page*);
> 
> explicit

Done.
Comment 25 Julien Chaffraix 2012-01-26 14:54:04 PST
Comment on attachment 124064 [details]
Patch

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

Based on the previous reviews, it looks like this patch has received enough attention from WebCore reviewers. The WebViewImpl seems simple enough to me but I don't usually usually review those changes. If someone with more experience of this part could review it, it would be the best. If no one steps in, I am fine with r+ the change (provided the style comments are addressed).

> Source/WebCore/ChangeLog:9
> +        WebCore::PointerLockController class, which exists per page.

No need for the WebCore prefix here.

Also could you fill the ChangeLog with a bit more with more context:
* What are you doing in this change?
* Some noteworthy information for furture WebKit archeologists?

> Source/WebCore/page/Page.cpp:151
> +    , m_pointerLockController(adoptPtr(new PointerLockController(this)))

Nit: We usually prefer a static create function that wraps this call.

> Source/WebCore/page/PointerLockController.cpp:75
> +    RefPtr<WebCore::Element> elementToNotify(m_element);

Extra WebCore:: (not repeated in the rest of the file).

> Source/WebCore/page/PointerLockController.cpp:108
> +    if (m_element && m_element->document()->frame()) {

We usually prefer early return to nested if.

> Source/WebCore/page/PointerLockController.h:59
> +    RefPtr<WebCore::Element> m_element;
> +    RefPtr<WebCore::VoidCallback> m_successCallback;
> +    RefPtr<WebCore::VoidCallback> m_failureCallback;

Extra WebCore::

> Source/WebKit/chromium/src/WebViewImpl.cpp:3252
>      // FIXME: Implement when PointerLockController lands.

It looks like this comment is unneeded now.
Comment 26 Vincent Scheib 2012-01-26 16:12:29 PST
Created attachment 124205 [details]
Patch
Comment 27 Vincent Scheib 2012-01-26 16:18:10 PST
Created attachment 124206 [details]
Patch
Comment 28 Vincent Scheib 2012-01-26 16:21:23 PST
Comment on attachment 124064 [details]
Patch

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

Thanks, issues addressed.

>> Source/WebCore/ChangeLog:9
>> +        WebCore::PointerLockController class, which exists per page.
> 
> No need for the WebCore prefix here.
> 
> Also could you fill the ChangeLog with a bit more with more context:
> * What are you doing in this change?
> * Some noteworthy information for furture WebKit archeologists?

Done.

>> Source/WebCore/page/Page.cpp:151
>> +    , m_pointerLockController(adoptPtr(new PointerLockController(this)))
> 
> Nit: We usually prefer a static create function that wraps this call.

Done.

>> Source/WebCore/page/PointerLockController.cpp:75
>> +    RefPtr<WebCore::Element> elementToNotify(m_element);
> 
> Extra WebCore:: (not repeated in the rest of the file).

Done (all instances).

>> Source/WebCore/page/PointerLockController.cpp:108
>> +    if (m_element && m_element->document()->frame()) {
> 
> We usually prefer early return to nested if.

Done.

>> Source/WebCore/page/PointerLockController.h:59
>> +    RefPtr<WebCore::VoidCallback> m_failureCallback;
> 
> Extra WebCore::

Done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3252
>>      // FIXME: Implement when PointerLockController lands.
> 
> It looks like this comment is unneeded now.

Done.
Comment 29 Julien Chaffraix 2012-01-27 10:21:18 PST
Comment on attachment 124206 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        Implement the navigator.pointer interface via a new
> +        PointerLockController class, as per
> +        http://dvcs.w3.org/hg/webevents/raw-file/default/mouse-lock.html.
> +
> +        The implementation is being made in steps, the feature is still behind
> +        compile-time and run-time flags, 'webkit' prefixed, and not yet enabled
> +        in any browser. (Chromium has a developer flag required.) Follow-up
> +        work will include handling DOM elements being removed, making all
> +        callbacks asynchronous, iframe permissions (similar to Full Screen),
> +        etc.
> +
> +        PointerLockController maintains state of which Element is the current
> +        lock target and the success and failure callbacks. ChromeClient has
> +        methods added to expose the required state change requests.

Thanks for filing in more details, that's very helpful!

> Source/WebCore/page/PointerLockController.cpp:46
> +PassOwnPtr<PointerLockController> PointerLockController::create(Page* page)
> +{
> +    return adoptPtr(new PointerLockController(page));
> +}

Nit: can be inlined in the class.
Comment 30 Vincent Scheib 2012-01-27 10:57:50 PST
Created attachment 124337 [details]
Patch for landing.
Comment 31 Vincent Scheib 2012-01-27 11:06:54 PST
Committed r106134: <http://trac.webkit.org/changeset/106134>