Bug 75762 - Pointer Lock: Implement pointer interface
Summary: Pointer Lock: Implement pointer interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vincent Scheib
URL:
Keywords:
Depends on: 76311 76410 76411 77030
Blocks: 84386
  Show dependency treegraph
 
Reported: 2012-01-06 17:12 PST by Vincent Scheib
Modified: 2012-06-19 09:39 PDT (History)
8 users (show)

See Also:


Attachments
Patch (23.38 KB, patch)
2012-01-06 17:15 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Tests added (48.94 KB, patch)
2012-01-11 13:44 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Updated big patch, includes all subpatches (51.05 KB, patch)
2012-01-16 17:26 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Small patch, no subpatches (35.67 KB, patch)
2012-01-16 17:35 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Patch (36.75 KB, patch)
2012-01-24 14:07 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Rerun EWS (36.75 KB, patch)
2012-01-24 15:47 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Patch (36.85 KB, patch)
2012-01-25 11:09 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Patch (37.67 KB, patch)
2012-01-25 17:17 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Patch (37.56 KB, patch)
2012-01-25 22:23 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Patch (38.40 KB, patch)
2012-01-26 16:12 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff
Patch (38.40 KB, patch)
2012-01-26 16:18 PST, Vincent Scheib
jchaffraix: review+
Details | Formatted Diff | Diff
Patch for landing. (38.34 KB, patch)
2012-01-27 10:57 PST, Vincent Scheib
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>