RESOLVED FIXED 69410
[WK2] [GTK] Implement KeyDown function for WebKit2 EventSender.
https://bugs.webkit.org/show_bug.cgi?id=69410
Summary [WK2] [GTK] Implement KeyDown function for WebKit2 EventSender.
Alejandro G. Castro
Reported 2011-10-05 04:06:48 PDT
Attachments
Work in progress patch (11.42 KB, patch)
2011-10-18 03:47 PDT, Kaustubh Atrawalkar
no flags
Patch (19.86 KB, patch)
2011-10-20 00:19 PDT, Kaustubh Atrawalkar
mrobinson: review-
Updated Patch (16.96 KB, patch)
2011-10-21 05:24 PDT, Kaustubh Atrawalkar
mrobinson: review-
Patch with Review Changes (24.61 KB, patch)
2011-10-21 20:48 PDT, Kaustubh Atrawalkar
no flags
Updated Patch (16.48 KB, patch)
2011-10-21 21:16 PDT, Kaustubh Atrawalkar
no flags
Patch (15.67 KB, patch)
2011-10-21 21:31 PDT, Kaustubh Atrawalkar
no flags
Updated Patch (16.42 KB, patch)
2011-10-21 22:04 PDT, Kaustubh Atrawalkar
mrobinson: review+
mrobinson: commit-queue+
Updated Patch (16.42 KB, patch)
2011-10-21 22:11 PDT, Kaustubh Atrawalkar
no flags
Patch (16.88 KB, patch)
2011-10-21 23:03 PDT, Kaustubh Atrawalkar
gustavo: commit-queue-
Fixed Patch (16.45 KB, patch)
2011-10-22 00:27 PDT, Kaustubh Atrawalkar
no flags
Patch (16.45 KB, patch)
2011-10-22 00:48 PDT, Kaustubh Atrawalkar
no flags
Kaustubh Atrawalkar
Comment 1 2011-10-18 03:47:03 PDT
Created attachment 111416 [details] Work in progress patch Implementation for KeyDown function for WebKit2 EventSender
Martin Robinson
Comment 2 2011-10-18 06:54:10 PDT
Comment on attachment 111416 [details] Work in progress patch 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
Alejandro G. Castro
Comment 3 2011-10-18 08:18:33 PDT
Thanks for the patch, I would add to Martin comments that you should try to add the modification of the Skipped file, because we are passing more tests with this feature implemented. Test which ones and remove them from the Skipped file and check the reason we are not passing other tests.
Kaustubh Atrawalkar
Comment 4 2011-10-18 23:06:40 PDT
Thanks Martin & Alejandro for quick review. I will incorporate all the suggested changes and will update the patch soon. The only reason I added r+ for the patch was to get the bot testing done :)
Kaustubh Atrawalkar
Comment 5 2011-10-20 00:19:39 PDT
Created attachment 111732 [details] Patch Completed patch with removing skipped tests.
Kaustubh Atrawalkar
Comment 6 2011-10-20 22:59:41 PDT
Martin/Alejandro can you review once? I have added changes suggested by you. And patch is completed now.
Martin Robinson
Comment 7 2011-10-20 23:08:31 PDT
Comment on attachment 111732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111732&action=review > Tools/WebKitTestRunner/GNUmakefile.am:15 > Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp \ > + Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp \ > Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp \ We try to keep the source lists in alphabetical order. That means that EventSenderProxyGtk.cpp would be before PlatformWebViewGtk.cpp. > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:83 > + if (WKStringIsEqualToUTF8CString(keyRef, "leftArrow")) > + gdkKeySym = GDK_KEY_KP_Left; > + else if (WKStringIsEqualToUTF8CString(keyRef, "rightArror")) > + gdkKeySym = GDK_KEY_KP_Right; > + else if (WKStringIsEqualToUTF8CString(keyRef, "upArrow")) > + gdkKeySym = GDK_KEY_KP_Up; Typically we use early returns to do this sort of thing. Do you mind doing that and modifying the "else if" statements to be simply if? > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:96 > + gdkKeySym = GDK_KEY_KP_Insert; > + else if (WKStringIsEqualToUTF8CString(keyRef, "delete")) You'll need a final else here to return GDK_KEY_VoidSymbol. You might want to ASSERT_NOT_REACHED. > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:101 > + } else { > + if (WKStringIsEqualToUTF8CString(keyRef, "leftArrow")) > + gdkKeySym = GDK_KEY_Left; > + else if (WKStringIsEqualToUTF8CString(keyRef, "rightArror")) After which, you can move this out of the else block. > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:153 > + size_t stringSize = WKStringGetLength(keyRef); > + char* buffer = new char[stringSize]; > + WKStringGetUTF8CString(keyRef, buffer, stringSize); > + int charCode = buffer[0]; > + > + if (charCode == '\n' || charCode == '\r') And this will also be out of the else block as well. > Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:127 > +bool PlatformWebView::dispatchEvent(GdkEvent* event) > +{ > + if (event->type == GDK_KEY_PRESS || event->type == GDK_KEY_RELEASE) { > + event->key.window = gtk_widget_get_window(GTK_WIDGET(m_view)); > + g_object_ref(event->key.window); Why can't this be in the EventSender? > Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:130 > +#ifndef GTK_API_VERSION_2 > + gdk_event_set_device(event, getDefaultGDKPointerDevice(event->key.window)); > +#endif There is no GTK+ 2 support in Webkit2, so you can remove this #ifdef.
Martin Robinson
Comment 8 2011-10-20 23:18:04 PDT
Looks pretty good! I'm not certain why we need dispatchEvent. Don't we have a handle on the widget in the EventSenderProxy?
Kaustubh Atrawalkar
Comment 9 2011-10-20 23:22:29 PDT
(In reply to comment #8) > Looks pretty good! I'm not certain why we need dispatchEvent. Don't we have a handle on the widget in the EventSenderProxy? We have WebView handle from testController. But somehow when i had that code in EventSenderProxyGtk.cpp I was getting assert for not a GtkWidget. Then I looked at qt port code how then have done and found that they also have made implementation for dispatching event in PlatformWebViewQt.cpp
Martin Robinson
Comment 10 2011-10-20 23:30:43 PDT
(In reply to comment #9) > (In reply to comment #8) > > Looks pretty good! I'm not certain why we need dispatchEvent. Don't we have a handle on the widget in the EventSenderProxy? > > We have WebView handle from testController. But somehow when i had that code in EventSenderProxyGtk.cpp I was getting assert for not a GtkWidget. Then I looked at qt port code how then have done and found that they also have made implementation for dispatching event in PlatformWebViewQt.cpp Do you mind investigating a bit further? If the WebView doesn't have a widget than moving this code to WebView is still an issue.
Kaustubh Atrawalkar
Comment 11 2011-10-21 05:24:28 PDT
Created attachment 111951 [details] Updated Patch
Martin Robinson
Comment 12 2011-10-21 09:36:39 PDT
Comment on attachment 111951 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111951&action=review Looks good, but I have a couple questions below. > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:63 > +bool dispatchEvent(GdkEvent* event) > +{ > + gtk_main_do_event(event); > + gdk_event_free(event); > + return 0; > +} This function should be static. Why does it return a bool, while you unconditionally return 0? > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:193 > + GdkKeymapKey* keys; > + gint nKeys; > + if (gdk_keymap_get_entries_for_keyval(gdk_keymap_get_default(), gdkKeySym, &keys, &nKeys)) { > + pressEvent->key.hardware_keycode = keys[0].keycode; > + g_free(keys); > + } Can you use GOwnPtr and .outPtr() for keys?
Kaustubh Atrawalkar
Comment 13 2011-10-21 20:12:32 PDT
(In reply to comment #12) > (From update of attachment 111951 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111951&action=review > > Looks good, but I have a couple questions below. > > > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:63 > > +bool dispatchEvent(GdkEvent* event) > > +{ > > + gtk_main_do_event(event); > > + gdk_event_free(event); > > + return 0; > > +} > > This function should be static. Why does it return a bool, while you unconditionally return 0? > Yes, I can make it static. I was returning bool only considering further more addition of Events like Mouse click/move/drag might add return values/error codes. > > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:193 > > + GdkKeymapKey* keys; > > + gint nKeys; > > + if (gdk_keymap_get_entries_for_keyval(gdk_keymap_get_default(), gdkKeySym, &keys, &nKeys)) { > > + pressEvent->key.hardware_keycode = keys[0].keycode; > > + g_free(keys); > > + } > > Can you use GOwnPtr and .outPtr() for keys? Yes, I will make that change.
Kaustubh Atrawalkar
Comment 14 2011-10-21 20:48:48 PDT
Created attachment 112065 [details] Patch with Review Changes
WebKit Review Bot
Comment 15 2011-10-21 20:54:03 PDT
Attachment 112065 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:234: "config.h" already included at Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:33 [build/include] [4] Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:235: "EventSenderProxy.h" already included at Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:34 [build/include] [4] Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:237: "PlatformWebView.h" already included at Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:36 [build/include] [4] Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:238: "TestController.h" already included at Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:37 [build/include] [4] Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:239: "gdk/gdk.h" already included at Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:39 [build/include] [4] Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:240: "gdk/gdkkeysyms.h" already included at Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:40 [build/include] [4] Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:241: "gtk/gtk.h" already included at Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:41 [build/include] [4] Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:242: "wtf/text/WTFString.h" already included at Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:42 [build/include] [4] Total errors found: 8 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Martin Robinson
Comment 16 2011-10-21 21:03:25 PDT
(In reply to comment #13) > > This function should be static. Why does it return a bool, while you unconditionally return 0? > > > Yes, I can make it static. I was returning bool only considering further more addition of Events like Mouse click/move/drag might add return values/error codes. For now, since the return value is not used, just have it return void. Furthermore, bool functions should return either "true" or "false" :) Thanks!
Kaustubh Atrawalkar
Comment 17 2011-10-21 21:16:35 PDT
Created attachment 112070 [details] Updated Patch
Kaustubh Atrawalkar
Comment 18 2011-10-21 21:17:25 PDT
(In reply to comment #16) > > For now, since the return value is not used, just have it return void. Furthermore, bool functions should return either "true" or "false" :) > > Thanks! Ok Done :) Will change it later when required.
Martin Robinson
Comment 19 2011-10-21 21:29:02 PDT
Comment on attachment 112070 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112070&action=review > Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:63 > +static void dispatchEvent(GdkEvent* event) > +{ > + gtk_main_do_event(event); > + gdk_event_free(event); > + return 0; > +} You also need to remove the "return 0" to avoid a warning.
Kaustubh Atrawalkar
Comment 20 2011-10-21 21:31:16 PDT
Martin Robinson
Comment 21 2011-10-21 21:35:22 PDT
Comment on attachment 112071 [details] Patch Thanks!
Kaustubh Atrawalkar
Comment 22 2011-10-21 21:38:04 PDT
(In reply to comment #21) > (From update of attachment 112071 [details]) > Thanks! Thanks Martin :) I will start extending now for the bug https://bugs.webkit.org/show_bug.cgi?id=69411. You can assign that to me if u wish :)
WebKit Review Bot
Comment 23 2011-10-21 21:38:42 PDT
Comment on attachment 112071 [details] Patch Rejecting attachment 112071 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: tTestRunner/TestController.cpp patching file Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp patch: **** unexpected end of file in patch fatal: pathspec 'Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp' did not match any files Failed to git add Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp. at /mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply line 439. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Martin Robinson', u'--..." exit_code: 2 Full output: http://queues.webkit.org/results/10198159
WebKit Review Bot
Comment 24 2011-10-21 21:44:13 PDT
Comment on attachment 112071 [details] Patch Rejecting attachment 112071 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: tTestRunner/TestController.cpp patching file Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp patch: **** unexpected end of file in patch fatal: pathspec 'Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp' did not match any files Failed to git add Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp. at /mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply line 439. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Martin Robinson', u'--..." exit_code: 2 Full output: http://queues.webkit.org/results/10200143
Martin Robinson
Comment 25 2011-10-21 21:50:51 PDT
I'm not sure why the patch is failing to apply. Do you mind rebasing onto the latest from the repository?
Kaustubh Atrawalkar
Comment 26 2011-10-21 21:54:33 PDT
(In reply to comment #25) > I'm not sure why the patch is failing to apply. Do you mind rebasing onto the latest from the repository? Yep doing that only.
Kaustubh Atrawalkar
Comment 27 2011-10-21 22:04:24 PDT
Created attachment 112073 [details] Updated Patch Rebaselined.
WebKit Review Bot
Comment 28 2011-10-21 22:08:06 PDT
Attachment 112073 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:191: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kaustubh Atrawalkar
Comment 29 2011-10-21 22:11:59 PDT
Created attachment 112074 [details] Updated Patch Really sorry for that. This wont fail.
Kaustubh Atrawalkar
Comment 30 2011-10-21 23:03:54 PDT
Gustavo Noronha (kov)
Comment 31 2011-10-21 23:19:48 PDT
Kaustubh Atrawalkar
Comment 32 2011-10-22 00:27:53 PDT
Created attachment 112080 [details] Fixed Patch
WebKit Review Bot
Comment 33 2011-10-22 00:30:08 PDT
Attachment 112080 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:39: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kaustubh Atrawalkar
Comment 34 2011-10-22 00:48:35 PDT
Alejandro G. Castro
Comment 35 2011-10-22 03:10:12 PDT
Comment on attachment 112082 [details] Patch Just one comment, you are removing the tests from the Keydown section of the Skipped file, some of the tests are still failing it is because some other reason, I think if we want to keep the things a little bit tidy in the file it would be great to reclassify the tests, even if it means adding them to the "Unchecked failure" section. If you can do a small review and add them to other sections in the patch it would be great. Thanks for the work!
Kaustubh Atrawalkar
Comment 36 2011-10-22 04:53:42 PDT
(In reply to comment #35) > (From update of attachment 112082 [details]) > Just one comment, you are removing the tests from the Keydown section of the Skipped file, some of the tests are still failing it is because some other reason, I think if we want to keep the things a little bit tidy in the file it would be great to reclassify the tests, even if it means adding them to the "Unchecked failure" section. If you can do a small review and add them to other sections in the patch it would be great. Thanks for the work! Sure Alejandro, I will make the patch and will upload in different bug. Thanks :) This current patch is final and ready to be committed now.
Kaustubh Atrawalkar
Comment 37 2011-10-22 20:34:05 PDT
Hi Martin, Can you r+ & c+ this updated patch? This is final updated patch. THanks.
WebKit Review Bot
Comment 38 2011-10-23 00:26:54 PDT
Comment on attachment 112082 [details] Patch Clearing flags on attachment: 112082 Committed r98200: <http://trac.webkit.org/changeset/98200>
WebKit Review Bot
Comment 39 2011-10-23 00:27:00 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.