[webkit-reviews] review canceled: [Bug 69410] [WK2] [GTK] Implement KeyDown function for WebKit2 EventSender. : [Attachment 111416] Work in progress patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has canceled 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 111416: Work in progress patch
https://bugs.webkit.org/attachment.cgi?id=111416&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111416&action=review


Just a couple quick comments. Don't set the review bit on WIP patches, since
they aren't ready for commit.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:8
> +/*
> + * Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
modification,
> + * are permitted provided that the following conditions are met:
> + *
> + * Redistributions of source code must retain the above copyright notice,
> + * this list of conditions and the following disclaimer.

This code appears to be based on code from EventSender.cpp. You need to copy
the header from that file and add your own Copyright line.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:48
> +    DOM_KEY_LOCATION_STANDARD      = 0x00,
> +    DOM_KEY_LOCATION_LEFT	      = 0x01,
> +    DOM_KEY_LOCATION_RIGHT	      = 0x02,
> +    DOM_KEY_LOCATION_NUMPAD	      = 0x03
> +};

The enum values here are incorrectly named. enums values must be camel case,
DOMKeyLocationStandard, etc.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:52
> +static void dispatchEvent(GdkEvent*);
> +static guint getModifiers(WKEventModifiers);
> +

You don't need these forward declarations at this point, I think.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:82
> +    size_t size = WKStringGetLength(keyRef);

Would prefer stringSize.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:94
> +    if (location == DOM_KEY_LOCATION_NUMPAD) {
> +	   if (WKStringIsEqualToUTF8CString(keyRef, "leftArrow"))
> +	       gdkKeySym = GDK_KEY_KP_Left;
> +	   else if (WKStringIsEqualToUTF8CString(keyRef, "rightArror"))
> +	       gdkKeySym = GDK_KEY_KP_Right;
> +	   else if (WKStringIsEqualToUTF8CString(keyRef, "upArrow"))

It'd be better to split this off into a helper function called something like
getGDKKeySymForKeyRef.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:175
> +    // create and send the event

This comment needs to begin with a capital letter and end with a period. You
can probably just omit it though. It doesn't provide much over the code itself.


> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:183
> +    gint n_keys;

n_keys does not follow WebKit naming conventions. Take a look at:
http://www.webkit.org/coding/coding-style.html