[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
bugzilla-daemon at webkit.org
Thu Nov 17 18:58:38 PST 2011
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.
More information about the webkit-reviews
mailing list