Bug 69410 - [WK2] [GTK] Implement KeyDown function for WebKit2 EventSender.
Summary: [WK2] [GTK] Implement KeyDown function for WebKit2 EventSender.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 69523
  Show dependency treegraph
 
Reported: 2011-10-05 04:06 PDT by Alejandro G. Castro
Modified: 2011-10-23 00:27 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 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
Comment 1 Kaustubh Atrawalkar 2011-10-18 03:47:03 PDT
Created attachment 111416 [details]
Work in progress patch

Implementation for KeyDown function for WebKit2 EventSender
Comment 2 Martin Robinson 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
Comment 3 Alejandro G. Castro 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.
Comment 4 Kaustubh Atrawalkar 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 :)
Comment 5 Kaustubh Atrawalkar 2011-10-20 00:19:39 PDT
Created attachment 111732 [details]
Patch

Completed patch with removing skipped tests.
Comment 6 Kaustubh Atrawalkar 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.
Comment 7 Martin Robinson 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.
Comment 8 Martin Robinson 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?
Comment 9 Kaustubh Atrawalkar 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
Comment 10 Martin Robinson 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.
Comment 11 Kaustubh Atrawalkar 2011-10-21 05:24:28 PDT
Created attachment 111951 [details]
Updated Patch
Comment 12 Martin Robinson 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?
Comment 13 Kaustubh Atrawalkar 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.
Comment 14 Kaustubh Atrawalkar 2011-10-21 20:48:48 PDT
Created attachment 112065 [details]
Patch with Review Changes
Comment 15 WebKit Review Bot 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.
Comment 16 Martin Robinson 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!
Comment 17 Kaustubh Atrawalkar 2011-10-21 21:16:35 PDT
Created attachment 112070 [details]
Updated Patch
Comment 18 Kaustubh Atrawalkar 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.
Comment 19 Martin Robinson 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.
Comment 20 Kaustubh Atrawalkar 2011-10-21 21:31:16 PDT
Created attachment 112071 [details]
Patch
Comment 21 Martin Robinson 2011-10-21 21:35:22 PDT
Comment on attachment 112071 [details]
Patch

Thanks!
Comment 22 Kaustubh Atrawalkar 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 :)
Comment 23 WebKit Review Bot 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
Comment 24 WebKit Review Bot 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
Comment 25 Martin Robinson 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?
Comment 26 Kaustubh Atrawalkar 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.
Comment 27 Kaustubh Atrawalkar 2011-10-21 22:04:24 PDT
Created attachment 112073 [details]
Updated Patch

Rebaselined.
Comment 28 WebKit Review Bot 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.
Comment 29 Kaustubh Atrawalkar 2011-10-21 22:11:59 PDT
Created attachment 112074 [details]
Updated Patch

Really sorry for that. This wont fail.
Comment 30 Kaustubh Atrawalkar 2011-10-21 23:03:54 PDT
Created attachment 112077 [details]
Patch
Comment 31 Gustavo Noronha (kov) 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
Comment 32 Kaustubh Atrawalkar 2011-10-22 00:27:53 PDT
Created attachment 112080 [details]
Fixed Patch
Comment 33 WebKit Review Bot 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.
Comment 34 Kaustubh Atrawalkar 2011-10-22 00:48:35 PDT
Created attachment 112082 [details]
Patch
Comment 35 Alejandro G. Castro 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!
Comment 36 Kaustubh Atrawalkar 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.
Comment 37 Kaustubh Atrawalkar 2011-10-22 20:34:05 PDT
Hi Martin, Can you r+ & c+ this updated patch? This is final updated patch. THanks.
Comment 38 WebKit Review Bot 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>
Comment 39 WebKit Review Bot 2011-10-23 00:27:00 PDT
All reviewed patches have been landed.  Closing bug.