[WK2] [GTK] Implement KeyDown function for WebKit2 EventSender.

[Alejandro G. Castro]

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

[Kaustubh Atrawalkar]

Created attachment 111416 [details]
Work in progress patch

Implementation for KeyDown function for WebKit2 EventSender

[Martin Robinson]

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]

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]

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]

Created attachment 111732 [details]
Patch

Completed patch with removing skipped tests.

[Kaustubh Atrawalkar]

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

[Martin Robinson]

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]

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]

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

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

Created attachment 111951 [details]
Updated Patch

[Martin Robinson]

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]

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

Created attachment 112065 [details]
Patch with Review Changes

[WebKit Review Bot]

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]

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

Created attachment 112070 [details]
Updated Patch

[Kaustubh Atrawalkar]

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

Created attachment 112071 [details]
Patch

[Martin Robinson]

Comment on attachment 112071 [details]
Patch

Thanks!

[Kaustubh Atrawalkar]

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

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

[Kaustubh Atrawalkar]

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

Created attachment 112073 [details]
Updated Patch

Rebaselined.

[WebKit Review Bot]

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]

Created attachment 112074 [details]
Updated Patch

Really sorry for that. This wont fail.

[Kaustubh Atrawalkar]

Created attachment 112077 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 112077 [details]
Patch

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

[Kaustubh Atrawalkar]

Created attachment 112080 [details]
Fixed Patch

[WebKit Review Bot]

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]

Created attachment 112082 [details]
Patch

[Alejandro G. Castro]

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]

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

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

[WebKit Review Bot]

Comment on attachment 112082 [details]
Patch

Clearing flags on attachment: 112082

Committed r98200: <http://trac.webkit.org/changeset/98200>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.