Bug 69410

Created attachment 111416 [details] Work in progress patch Implementation for KeyDown function for WebKit2 EventSender

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

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.

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 :)

Created attachment 111732 [details] Patch Completed patch with removing skipped tests.

Martin/Alejandro can you review once? I have added changes suggested by you. And patch is completed now.

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.

Looks pretty good! I'm not certain why we need dispatchEvent. Don't we have a handle on the widget in the EventSenderProxy?

(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

(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.

Created attachment 111951 [details] Updated Patch

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?

(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.

Created attachment 112065 [details] Patch with Review Changes

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.

(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!

Created attachment 112070 [details] Updated Patch

(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.

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.

Created attachment 112071 [details] Patch

Comment on attachment 112071 [details] Patch Thanks!

(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 :)

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

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

I'm not sure why the patch is failing to apply. Do you mind rebasing onto the latest from the repository?

(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.

Created attachment 112073 [details] Updated Patch Rebaselined.

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.

Created attachment 112074 [details] Updated Patch Really sorry for that. This wont fail.

Created attachment 112077 [details] Patch

Comment on attachment 112077 [details] Patch Attachment 112077 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10200169

Created attachment 112080 [details] Fixed Patch

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.

Created attachment 112082 [details] Patch

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!

(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.

Hi Martin, Can you r+ & c+ this updated patch? This is final updated patch. THanks.

Comment on attachment 112082 [details] Patch Clearing flags on attachment: 112082 Committed r98200: <http://trac.webkit.org/changeset/98200>

All reviewed patches have been landed. Closing bug.