WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
The summary explains the issue, check bugs: -
https://bugs.webkit.org/show_bug.cgi?id=56485
-
https://bugs.webkit.org/show_bug.cgi?id=57515
Attachments
Work in progress patch
(11.42 KB, patch)
2011-10-18 03:47 PDT
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Patch
(19.86 KB, patch)
2011-10-20 00:19 PDT
,
Kaustubh Atrawalkar
mrobinson
: review-
Details
Formatted Diff
Diff
Updated Patch
(16.96 KB, patch)
2011-10-21 05:24 PDT
,
Kaustubh Atrawalkar
mrobinson
: review-
Details
Formatted Diff
Diff
Patch with Review Changes
(24.61 KB, patch)
2011-10-21 20:48 PDT
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Updated Patch
(16.48 KB, patch)
2011-10-21 21:16 PDT
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Patch
(15.67 KB, patch)
2011-10-21 21:31 PDT
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Updated Patch
(16.42 KB, patch)
2011-10-21 22:04 PDT
,
Kaustubh Atrawalkar
mrobinson
: review+
mrobinson
: commit-queue+
Details
Formatted Diff
Diff
Updated Patch
(16.42 KB, patch)
2011-10-21 22:11 PDT
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Patch
(16.88 KB, patch)
2011-10-21 23:03 PDT
,
Kaustubh Atrawalkar
gustavo
: commit-queue-
Details
Formatted Diff
Diff
Fixed Patch
(16.45 KB, patch)
2011-10-22 00:27 PDT
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Patch
(16.45 KB, patch)
2011-10-22 00:48 PDT
,
Kaustubh Atrawalkar
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 112071
[details]
Patch
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
Created
attachment 112077
[details]
Patch
Gustavo Noronha (kov)
Comment 31
2011-10-21 23:19:48 PDT
Comment on
attachment 112077
[details]
Patch
Attachment 112077
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10200169
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
Created
attachment 112082
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug