Bug 88377

Created attachment 146155 [details] Patch

Events are patterned after the Fullscreen specification. See http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html.

Comment on attachment 146155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146155&action=review Mostly nits: > Source/WebCore/page/PointerLockController.cpp:55 > + // FIXME: Keep enqueueEvent usage. It's a good idea to list a bug URL in such fixmes, here and elsewhere in this patch: // FIXME: Keep foo (https://bugs.webkit.org/...) > Source/WebCore/page/PointerLockController.cpp:58 > + enqueueEvent(eventNames().webkitpointerlockchangeEvent, m_element.get()); Indentation fail. > LayoutTests/pointer-lock/pointerlockchange-pointerlockerror-events.html:22 > + function changeEventExpected(message, expected_calls, target_document) { Brace on new line and indentation fails elsewhere in this code. Please look over the code carefully and make sure it's in WebKit style. > LayoutTests/pointer-lock/pointerlocklost-event.html:53 > + setTimeout(function () { todo[currentStep++](); }, 0); Why is setTimeout necessary? We typically try to avoid them as they contribue to test flakiness.

Created attachment 146385 [details] Patch

Comment on attachment 146155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146155&action=review >> Source/WebCore/page/PointerLockController.cpp:55 >> + // FIXME: Keep enqueueEvent usage. > > It's a good idea to list a bug URL in such fixmes, here and elsewhere in this patch: > > // FIXME: Keep foo (https://bugs.webkit.org/...) Done. >> Source/WebCore/page/PointerLockController.cpp:58 >> + enqueueEvent(eventNames().webkitpointerlockchangeEvent, m_element.get()); > > Indentation fail. Done. >> LayoutTests/pointer-lock/pointerlockchange-pointerlockerror-events.html:22 >> + function changeEventExpected(message, expected_calls, target_document) { > > Brace on new line and indentation fails elsewhere in this code. Please look over the code carefully and make sure it's in WebKit style. Done. >> LayoutTests/pointer-lock/pointerlocklost-event.html:53 >> + setTimeout(function () { todo[currentStep++](); }, 0); > > Why is setTimeout necessary? We typically try to avoid them as they contribue to test flakiness. The new events are dispatched to the document via an event queue to avoid reentrancy issues. This test must run a step and then exit the script context allowing events to be dispatched. E.g. lock() is called, script must exit, WebCore will then dispatch an onwebkitpointerlockchange. Previous implementation of callbacks executed immediately, reentering the script context. E.g. lock() is called, causes successcallback to be called, calls doNextStep, ... all on one call stack.

> The new events are dispatched to the document via an event queue to avoid reentrancy issues. This test must run a step and then exit the script context allowing events to be dispatched. > > E.g. lock() is called, script must exit, WebCore will then dispatch an onwebkitpointerlockchange. Previous implementation of callbacks executed immediately, reentering the script context. E.g. lock() is called, causes successcallback to be called, calls doNextStep, ... all on one call stack. I see. In this case, I would recommend starting the next test from the success/error callback _plus_ a test to ensure that the callbacks are called -- or not called.

(In reply to comment #6) > > The new events are dispatched to the document via an event queue to avoid reentrancy issues. This test must run a step and then exit the script context allowing events to be dispatched. > > > > E.g. lock() is called, script must exit, WebCore will then dispatch an onwebkitpointerlockchange. Previous implementation of callbacks executed immediately, reentering the script context. E.g. lock() is called, causes successcallback to be called, calls doNextStep, ... all on one call stack. > > I see. In this case, I would recommend starting the next test from the success/error callback _plus_ a test to ensure that the callbacks are called -- or not called. These two concepts are currently done in pointerlocklost-event.html: - Starting the next test step from the callback: Currently still done using the old API's callback. You must expand code in the review mode to see the |locklostHander| which calls |doNextStep|. - Ensure callbacks are called: In the patch, see the change adding |lockchangeToUnlockedHandler|. It verifies the transition to unlocked. - Ensure callbacks not called: In the patch, a |testFailed| is added to |onwebkitpointerlockerror|.

Comment on attachment 146385 [details] Patch ok.

Committed r119763: <http://trac.webkit.org/changeset/119763>