[webkit-reviews] review denied: [Bug 69410] [WK2] [GTK] Implement KeyDown function for WebKit2 EventSender. : [Attachment 111732] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 20 23:08:30 PDT 2011
Martin Robinson <mrobinson at webkit.org> has denied Kaustubh Atrawalkar
<kaustubh at motorola.com>'s request for review:
Bug 69410: [WK2] [GTK] Implement KeyDown function for WebKit2 EventSender.
https://bugs.webkit.org/show_bug.cgi?id=69410
Attachment 111732: Patch
https://bugs.webkit.org/attachment.cgi?id=111732&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111732&action=review
> Tools/WebKitTestRunner/GNUmakefile.am:15
> Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp \
> + Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp \
> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp \
We try to keep the source lists in alphabetical order. That means that
EventSenderProxyGtk.cpp would be before PlatformWebViewGtk.cpp.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:83
> + if (WKStringIsEqualToUTF8CString(keyRef, "leftArrow"))
> + gdkKeySym = GDK_KEY_KP_Left;
> + else if (WKStringIsEqualToUTF8CString(keyRef, "rightArror"))
> + gdkKeySym = GDK_KEY_KP_Right;
> + else if (WKStringIsEqualToUTF8CString(keyRef, "upArrow"))
> + gdkKeySym = GDK_KEY_KP_Up;
Typically we use early returns to do this sort of thing. Do you mind doing that
and modifying the "else if" statements to be simply if?
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:96
> + gdkKeySym = GDK_KEY_KP_Insert;
> + else if (WKStringIsEqualToUTF8CString(keyRef, "delete"))
You'll need a final else here to return GDK_KEY_VoidSymbol. You might want to
ASSERT_NOT_REACHED.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:101
> + } else {
> + if (WKStringIsEqualToUTF8CString(keyRef, "leftArrow"))
> + gdkKeySym = GDK_KEY_Left;
> + else if (WKStringIsEqualToUTF8CString(keyRef, "rightArror"))
After which, you can move this out of the else block.
> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:153
> + size_t stringSize = WKStringGetLength(keyRef);
> + char* buffer = new char[stringSize];
> + WKStringGetUTF8CString(keyRef, buffer, stringSize);
> + int charCode = buffer[0];
> +
> + if (charCode == '\n' || charCode == '\r')
And this will also be out of the else block as well.
> Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:127
> +bool PlatformWebView::dispatchEvent(GdkEvent* event)
> +{
> + if (event->type == GDK_KEY_PRESS || event->type == GDK_KEY_RELEASE) {
> + event->key.window = gtk_widget_get_window(GTK_WIDGET(m_view));
> + g_object_ref(event->key.window);
Why can't this be in the EventSender?
> Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:130
> +#ifndef GTK_API_VERSION_2
> + gdk_event_set_device(event,
getDefaultGDKPointerDevice(event->key.window));
> +#endif
There is no GTK+ 2 support in Webkit2, so you can remove this #ifdef.
More information about the webkit-reviews
mailing list