[webkit-reviews] review denied: [Bug 88377] Add new Pointer Lock spec events webkitpointerlockchange and webkitpointerlockerror : [Attachment 146155] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Vincent Scheib
<scheib at chromium.org>'s request for review:
Bug 88377: Add new Pointer Lock spec events webkitpointerlockchange and
webkitpointerlockerror
https://bugs.webkit.org/show_bug.cgi?id=88377

Attachment 146155: Patch
https://bugs.webkit.org/attachment.cgi?id=146155&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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.