[webkit-reviews] review denied: [Bug 93979] [WK2] Implement eventSender.scheduleAsynchronousKeyDown : [Attachment 180989] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Christophe Dumez
<christophe.dumez at intel.com>'s request for review:
Bug 93979: [WK2] Implement eventSender.scheduleAsynchronousKeyDown
https://bugs.webkit.org/show_bug.cgi?id=93979

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=180989&action=review


The patch looks good. Some nitpick:

> Tools/ChangeLog:14
> +	   (WTR):

You can remove this.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:269
> +    return EventSenderMessageBody.leakRef();

I think it would be reasonable to return a WKRetainPtr here to avoid accidents
if someone forgets to use Adopt.

> Tools/WebKitTestRunner/TestController.cpp:800
> +	       WKRetainPtr<WKStringRef> keyKey =
adoptWK(WKStringCreateWithUTF8CString("Key"));
> +	       WKStringRef key =
static_cast<WKStringRef>(WKDictionaryGetItemForKey(messageBodyDictionary,
keyKey.get()));
> +
> +	       WKRetainPtr<WKStringRef> modifiersKey =
adoptWK(WKStringCreateWithUTF8CString("Modifiers"));
> +	       WKEventModifiers modifiers =
static_cast<WKEventModifiers>(WKUInt64GetValue(static_cast<WKUInt64Ref>(WKDicti
onaryGetItemForKey(messageBodyDictionary, modifiersKey.get()))));
> +
> +	       WKRetainPtr<WKStringRef> locationKey =
adoptWK(WKStringCreateWithUTF8CString("Location"));
> +	       unsigned location =
static_cast<unsigned>(WKUInt64GetValue(static_cast<WKUInt64Ref>(WKDictionaryGet
ItemForKey(messageBodyDictionary, locationKey.get()))));
> +
> +	       // Forward to WebProcess
> +	       WKPageSetShouldSendEventsSynchronously(mainWebView()->page(),
false);
> +	       m_eventSenderProxy->keyDown(key, modifiers, location);
> +
> +	       return;

This is duplicated code from the synchronous message handler.
Please refactor to make a common path as much as possible.