[webkit-reviews] review granted: [Bug 57515] [WK2] Implement KeyDown function for WebKit2 EventSender : [Attachment 94789] fix patch 3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chang Shu <cshu at webkit.org>'s request
for review:
Bug 57515: [WK2] Implement KeyDown function for WebKit2 EventSender
https://bugs.webkit.org/show_bug.cgi?id=57515

Attachment 94789: fix patch 3
https://bugs.webkit.org/attachment.cgi?id=94789&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=94789&action=review

> Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:71
> +WK_EXPORT void WKPageSetSendEventSynchronously(WKPageRef page, bool sync);

The name here is not ideal. Given what this does, normally we’d name this
WKPageSetShouldSendKeyboardEventsSynchronously.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1091
> +    // FIXME: Platform default behaviors should be performed during normal
DOM event dispatch (in most cases, in default keydown event handler).

Do we really need this FIXME?

> Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:36
> +    KeyEventSync(WebKit::WebKeyboardEvent event) -> (bool handled)

I think it would be best if there was some way to indicate this was intended
for testing only.

> Tools/WebKitTestRunner/EventSenderProxy.h:36
> +    EventSenderProxy(TestController* testController) :
m_testController(testController) { };

This semicolon is unnecessary.

> Tools/WebKitTestRunner/EventSenderProxy.h:38
> +    void keyDown(WKStringRef key, WKEventModifiers, unsigned int location,
double timestamp);

It should be "unsigned", not "unsigned int".

> Tools/WebKitTestRunner/TestController.cpp:31
> +#if PLATFORM(MAC)
> +#include "EventSenderProxy.h"
> +#endif

Conditional includes should normally be separate and under unconditional
includes.

> Tools/WebKitTestRunner/TestController.cpp:502
> +#if PLATFORM(MAC)

What is Mac-specific here?

> Tools/WebKitTestRunner/TestController.cpp:518
> +	       unsigned int location = static_cast<unsigned
int>(WKUInt64GetValue(static_cast<WKUInt64Ref>(WKDictionaryGetItemForKey(messag
eBodyDictionary, locationKey.get()))));

Should just be "unsigned" not "unsigned int".