[webkit-reviews] review denied: [Bug 62451] [EFL] Move keyIdentifierForEvasKeyName() and windowsKeyCodeForEvasKeyName() to the EflKeyboardUtilities.cpp to use in the WebKit2 : [Attachment 115725] EflKeyboardUtilities patch.

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied EunMi Lee
<eunmi15.lee at samsung.com>'s request for review:
Bug 62451: [EFL] Move keyIdentifierForEvasKeyName() and
windowsKeyCodeForEvasKeyName() to the EflKeyboardUtilities.cpp to use in the
WebKit2
https://bugs.webkit.org/show_bug.cgi?id=62451

Attachment 115725: EflKeyboardUtilities patch.
https://bugs.webkit.org/attachment.cgi?id=115725&action=review

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


Seems fine overall, but I have a few style nits.

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:33
> +#include "WindowsKeyboardCodes.h"
> +
> +#include <wtf/HashMap.h>

No need for a newline here.

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:44
> +static KeyMap gKeyMap;
> +static WindowsKeyMap gWindowsKeyMap;
> +

Static initializers inrease startup time and can lead to tricky errors. It's
probably better to remove them. For an example see here:
http://trac.webkit.org/changeset/100070

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:80
> +    gWindowsKeyMap.set("Return",	VK_RETURN);
> +    gWindowsKeyMap.set("KP_Return",	VK_RETURN);
> +    gWindowsKeyMap.set("Alt_L",	VK_MENU);
> +    gWindowsKeyMap.set("ISO_Level3_Shift", VK_MENU);
> +    gWindowsKeyMap.set("Menu",	VK_MENU);
> +    gWindowsKeyMap.set("Shift_L",	VK_SHIFT);

This kind of alignment is a violation of WebKit style rules.

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:125
> +    // Alphabet

Comments should be full sentences with a period at the end.

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:132
> +    // Digits

Ditto.

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:138
> +    // Shifted digits

Ditto.

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:154
> +    // F_XX

Ditto.

> Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:34
>  #include "PlatformKeyboardEvent.h"
>  

No newlines necessary here.

> Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:39
>  #include "TextEncoding.h"
> -#include "WindowsKeyboardCodes.h"
>  
>  #include <Evas.h>

Ditto.