[Webkit-unassigned] [Bug 69410] [WK2] [GTK] Implement KeyDown function for WebKit2 EventSender.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=69410


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #111416|review?                     |
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2011-10-18 06:54:10 PST ---
(From update of attachment 111416)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.