[GTK] Should support key and code properties on keyboard events
Created attachment 298199 [details] Patch
Comment on attachment 298199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298199&action=review Awesome, thanks. :) Do you have any thoughts on not-very-related bug #136430? You probably looked over the code where we can set that GdkEventKey::send_event? Anyway, I guess Carlos will want to review this too. My $0.02: > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:395 > + memset(&utf8, 0, sizeof(utf8)); No need for memset, write char utf8[7] = { 0 }; > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:396 > + g_unichar_to_utf8(unicodeCharacter, reinterpret_cast<gchar*>(&utf8)); Don't cast from char* to gchar*, they are the same type. :P
(In reply to comment #2) > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:396 > > + g_unichar_to_utf8(unicodeCharacter, reinterpret_cast<gchar*>(&utf8)); > > Don't cast from char* to gchar*, they are the same type. :P Sorry, I see it's a cast from char** to gchar*. Hm, why is this? Why do you need a cast there at all?
Comment on attachment 298199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298199&action=review > Source/WebCore/ChangeLog:14 > + - fast/events/arrow-keys-on-body.html > + - fast/events/constructors/keyboard-event-constructor.html > + - fast/events/key-events-in-input-button.html > + - fast/events/key-events-in-input-text.html > + - fast/events/keyboardevent-code.html > + - fast/events/keyboardevent-key.html Shouldn't we unskip them? or are all these currently failing? > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:124 > + return "Backspace"; Why not ASCIILiteral here too? (and below) > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:727 > // FIXME: This is incomplete. We should change this to mirror > // more like what Firefox does, and generate these switch statements > // at build time. This FIXME applies to to the new methods too, I think we should autogenerate all these long switches. Not necessarily in this patch, though.
(In reply to comment #2) > Do you have any thoughts on not-very-related bug #136430? You probably > looked over the code where we can set that GdkEventKey::send_event? Unfortunately no idea =( > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:395 > > + memset(&utf8, 0, sizeof(utf8)); > > No need for memset, write char utf8[7] = { 0 }; > > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:396 > > + g_unichar_to_utf8(unicodeCharacter, reinterpret_cast<gchar*>(&utf8)); > > Don't cast from char* to gchar*, they are the same type. :P Oops, yeah, I changed that code quite a bit and it ended up very weird. Thanks!
(In reply to comment #4) > > Source/WebCore/ChangeLog:14 > > + - fast/events/arrow-keys-on-body.html > > + - fast/events/constructors/keyboard-event-constructor.html > > + - fast/events/key-events-in-input-button.html > > + - fast/events/key-events-in-input-text.html > > + - fast/events/keyboardevent-code.html > > + - fast/events/keyboardevent-key.html > > Shouldn't we unskip them? or are all these currently failing? Currently failing. I thought about skipping and unskipping on this patch but I thought it would go in quick enough =) > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:124 > > + return "Backspace"; > > Why not ASCIILiteral here too? (and below) Ooops, my mistake. I left some of them for a search-and-replace later on and forgot to do it. > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:727 > > // FIXME: This is incomplete. We should change this to mirror > > // more like what Firefox does, and generate these switch statements > > // at build time. > > This FIXME applies to to the new methods too, I think we should autogenerate > all these long switches. Not necessarily in this patch, though. Agreed. I'll take a look at how Firefox does it.
Committed r210504: <http://trac.webkit.org/changeset/210504>