Bug 62451

Created attachment 96738 [details] Patch

Comment on attachment 96738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96738&action=review > Source/WebCore/platform/PlatformKeyboardEvent.h:214 > + > + // Used by WebKit2 > + static String keyIdentifierForEvasKeyName(String&); > + static int windowsKeyCodeForEvasKeyName(String&); Do we really need this? What if WebKit2 is not used? I know GTK is using a similar approach to this, but maybe there's a better approach?

(In reply to comment #2) > (From update of attachment 96738 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96738&action=review > > > Source/WebCore/platform/PlatformKeyboardEvent.h:214 > > + > > + // Used by WebKit2 > > + static String keyIdentifierForEvasKeyName(String&); > > + static int windowsKeyCodeForEvasKeyName(String&); > > Do we really need this? What if WebKit2 is not used? I know GTK is using a similar approach to this, but maybe there's a better approach? Yes, I really need this because we have to copy all codes for KeyMap (createKeyMap(), createWindowKeyMap()) to the WebKit2 efl port without this. And I think that this code will not raise any problem without WebKit2. About a better approach, I think it is better to unite WebKit1's PlatformKeyboardEvent and WebKit2's NativeWebKeyboardEvent but it is hard to me because that will cause modifications of all ports's KeyboardEvent. I will be grateful if you share your idea about that :)

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 96738 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=96738&action=review > > > > > Source/WebCore/platform/PlatformKeyboardEvent.h:214 > > > + > > > + // Used by WebKit2 > > > + static String keyIdentifierForEvasKeyName(String&); > > > + static int windowsKeyCodeForEvasKeyName(String&); > > > > Do we really need this? What if WebKit2 is not used? I know GTK is using a similar approach to this, but maybe there's a better approach? > > Yes, I really need this because we have to copy all codes for KeyMap (createKeyMap(), createWindowKeyMap()) to the WebKit2 efl port without this. > And I think that this code will not raise any problem without WebKit2. > About a better approach, > I think it is better to unite WebKit1's PlatformKeyboardEvent and WebKit2's NativeWebKeyboardEvent but it is hard to me because that will cause modifications of all ports's KeyboardEvent. > I will be grateful if you share your idea about that :) I certainly think it's better to share code among all ports, even if it's more difficult to do. However I don't know this piece of code sufficiently to tell you what to do. I'm CC'ing people who know more than me on this area.

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 96738 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=96738&action=review > > > > > > > Source/WebCore/platform/PlatformKeyboardEvent.h:214 > > > > + > > > > + // Used by WebKit2 > > > > + static String keyIdentifierForEvasKeyName(String&); > > > > + static int windowsKeyCodeForEvasKeyName(String&); > > > > > > Do we really need this? What if WebKit2 is not used? I know GTK is using a similar approach to this, but maybe there's a better approach? > > > > Yes, I really need this because we have to copy all codes for KeyMap (createKeyMap(), createWindowKeyMap()) to the WebKit2 efl port without this. > > And I think that this code will not raise any problem without WebKit2. > > About a better approach, > > I think it is better to unite WebKit1's PlatformKeyboardEvent and WebKit2's NativeWebKeyboardEvent but it is hard to me because that will cause modifications of all ports's KeyboardEvent. > > I will be grateful if you share your idea about that :) > > I certainly think it's better to share code among all ports, even if it's more difficult to do. However I don't know this piece of code sufficiently to tell you what to do. I'm CC'ing people who know more than me on this area. The WebCore/platform/PlatformKeyboardEvent and WebKit2/Shared/WebKeyboardEvent looks very similar. but WebKeyboardEvent has the encode and decode function to transmit to the WebProcess. I think it is hard to unify those two classes. So, I have another idea about that => adding the function which creats WebKeyboardEvent from PlatformKeyboardEvent. If that exists, I can create PlatformKeyboardEvent firstly (using PlatformKeyboardEventEfl) and then convert it to the WebKeyboardEvent. in this case, we do not have to open PlatformKeyboardEvent's static functions for KeyMap. How do you think?

Hello Weining, I found your name in the WebKit2/Shared/WebEventConversion.cpp changelog. and I add you because I think you are the expert of WebEvent. I have a problem to implement NativeWebKeyboardEventEfl, because I have to do same thing in the PlatformKeyboardEventEfl. So, I imagine that convert EFL's native event to PlatformEvent firstly, and then convert PlatformEvent to WebEvent. Is that a right approach? Would you give me an advice about that?

I've tried to convert PlatformKeyboardEvent to WebKeyboardEvent, but there are some problems. m_macCharCode and m_timestamp do not exist in the PlatformKeyboardEvent, so I have to add those variables to the PlatformKeyboardEvent in order to convert to WebKeyboardEvent. I think it is not a good idea. So, I think this patch (open keyIdentifierForEvasKeyName and windowsKeyCodeForEvasKeyName) is better than convert PlatformKeyboardEvent to WebKeyboardEvent. Dear Lucas and Weining, Would you review current patch?

Created attachment 115725 [details] EflKeyboardUtilities patch. I moved keyIdentifierForEvasKeyName() and windowsKeyCodeForEvasKeyName() to the separated file - EflKeyboardUtilities.cpp in order to use for WebKit2 EFL's WebKeyboardEvent.

Comment on attachment 115725 [details] EflKeyboardUtilities patch. 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.

Created attachment 115732 [details] EflKeyboardUtilities patch. Modified for Martin's review.

I'm very grateful for your kind review! (In reply to comment #9) > (From update of attachment 115725 [details]) > 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. I removed newline. > > > 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 I replaced global static variables with local ones as an example that you mentioned. Thanks for your kind guidance. > > > 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. > I replaced all comments with full sentences. > > 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. newlines are removed.

Comment on attachment 115732 [details] EflKeyboardUtilities patch. Clearing flags on attachment: 115732 Committed r100726: <http://trac.webkit.org/changeset/100726>

All reviewed patches have been landed. Closing bug.