Bug 88377 – Add new Pointer Lock spec events webkitpointerlockchange and webkitpointerlockerror

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vincent Scheib]

Add new Pointer Lock spec events webkitpointerlockchange and webkitpointerlockerror

[Vincent Scheib]

Created attachment 146155 [details]
Patch

[Vincent Scheib]

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

[Dimitri Glazkov (Google)]

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.

[Vincent Scheib]

Created attachment 146385 [details]
Patch

[Vincent Scheib]

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.

[Dimitri Glazkov (Google)]

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

[Vincent Scheib]

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

[Dimitri Glazkov (Google)]

Comment on attachment 146385 [details]
Patch

ok.

[Vincent Scheib]

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