Bug 75762

Created attachment 121524 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Created attachment 122087 [details] Tests added

Comment on attachment 122087 [details] Tests added Attachment 122087 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11108580

Comment on attachment 122087 [details] Tests added Attachment 122087 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11211140

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

Created attachment 122697 [details] Updated big patch, includes all subpatches

Created attachment 122698 [details] Small patch, no subpatches

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.

Created attachment 123805 [details] Patch

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

Created attachment 123832 [details] Rerun EWS

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

Created attachment 123976 [details] Patch

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

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

Created attachment 124041 [details] Patch

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

I'm happy with the low-level mechanics of this patch, but I'd like fishd to take a look as well.

Created attachment 124064 [details] Patch

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

Created attachment 124205 [details] Patch

Created attachment 124206 [details] Patch

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

Created attachment 124337 [details] Patch for landing.

Committed r106134: <http://trac.webkit.org/changeset/106134>