[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