Bug 88377 - Add new Pointer Lock spec events webkitpointerlockchange and webkitpointerlockerror
Summary: Add new Pointer Lock spec events webkitpointerlockchange and webkitpointerloc...
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:
Blocks: 84402
  Show dependency treegraph
 
Reported: 2012-06-05 17:09 PDT by Vincent Scheib
Modified: 2012-06-07 15:40 PDT (History)
5 users (show)

See Also:


Attachments
Patch (23.99 KB, patch)
2012-06-06 17:22 PDT, Vincent Scheib
no flags Details | Formatted Diff | Diff
Patch (26.45 KB, patch)
2012-06-07 14:05 PDT, Vincent Scheib
dglazkov: review+
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-06-05 17:09:00 PDT
Add new Pointer Lock spec events webkitpointerlockchange and webkitpointerlockerror
Comment 1 Vincent Scheib 2012-06-06 17:22:37 PDT
Created attachment 146155 [details]
Patch
Comment 2 Vincent Scheib 2012-06-06 17:25:46 PDT
Events are patterned after the Fullscreen specification. See http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html.
Comment 3 Dimitri Glazkov (Google) 2012-06-07 09:19:36 PDT
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.
Comment 4 Vincent Scheib 2012-06-07 14:05:15 PDT
Created attachment 146385 [details]
Patch
Comment 5 Vincent Scheib 2012-06-07 14:05:38 PDT
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.
Comment 6 Dimitri Glazkov (Google) 2012-06-07 14:26:27 PDT
> 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.
Comment 7 Vincent Scheib 2012-06-07 15:13:06 PDT
(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 8 Dimitri Glazkov (Google) 2012-06-07 15:27:19 PDT
Comment on attachment 146385 [details]
Patch

ok.
Comment 9 Vincent Scheib 2012-06-07 15:40:30 PDT
Committed r119763: <http://trac.webkit.org/changeset/119763>