Bug 78481 – [GTK] DRT doesn't support scheduleAsynchronousKeyDown.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[ChangSeok Oh]

It looks GTK port DRT doesn't support scheduleAsynchronousKeyDown yet, so it causes some layout test failures

[ChangSeok Oh]

Created attachment 127978 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 127978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127978&action=review

r-, although it's just for a few nitpicks and a possible useless include, I think you'll prefer to upload a new patch anyway? =)

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:53
> +#include "PlatformKeyboardEvent.h"

Why do we need this extra include?

> Tools/DumpRenderTree/gtk/EventSender.cpp:761
> +        sendKeyDown(static_cast<gpointer>(pressEvent));

I think I would prefer to have a proper function with the appropriate type for the argument that is called from here and from the idle callback.

[Martin Robinson]

Comment on attachment 127978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127978&action=review

> Tools/DumpRenderTree/gtk/EventSender.cpp:834
> +        g_idle_add(sendKeyDown, static_cast<gpointer>(pressEvent));

Perhaps g_timeout_add after a 0 delay would be better here?

[ChangSeok Oh]

Comment on attachment 127978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127978&action=review

Thanks for the review :)

>> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:53
>> +#include "PlatformKeyboardEvent.h"
> 
> Why do we need this extra include?

Oops. Sorry :p

>> Tools/DumpRenderTree/gtk/EventSender.cpp:761
>> +        sendKeyDown(static_cast<gpointer>(pressEvent));
> 
> I think I would prefer to have a proper function with the appropriate type for the argument that is called from here and from the idle callback.

Do you mean it looks better to wrap sendKeyDown and g_idle_add(sendKeyDown... in a function with arguments?
For example, bool sendKeyDown(bool asynchronous)?

[ChangSeok Oh]

Comment on attachment 127978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127978&action=review

>> Tools/DumpRenderTree/gtk/EventSender.cpp:834
>> +        g_idle_add(sendKeyDown, static_cast<gpointer>(pressEvent));
> 
> Perhaps g_timeout_add after a 0 delay would be better here?

I followed the way of scheduleAsynchronousClickCallback. It has used g_idle_add. Do we need to modify this, too? :)

[ChangSeok Oh]

Created attachment 128132 [details]
Patch

[Gustavo Noronha (kov)]

(In reply to comment #4)
> >> Tools/DumpRenderTree/gtk/EventSender.cpp:761
> >> +        sendKeyDown(static_cast<gpointer>(pressEvent));
> > 
> > I think I would prefer to have a proper function with the appropriate type for the argument that is called from here and from the idle callback.
> 
> Do you mean it looks better to wrap sendKeyDown and g_idle_add(sendKeyDown... in a function with arguments?
> For example, bool sendKeyDown(bool asynchronous)?

No, I mean sendKeyDown should take GdkEvent* pressEvent as the only argument. You would call it directly here, and you would have a handler for the idle that would also call it.

[ChangSeok Oh]

Created attachment 128229 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 128229 [details]
Patch

Yep! Thanks

[WebKit Review Bot]

Comment on attachment 128229 [details]
Patch

Clearing flags on attachment: 128229

Committed r108628: <http://trac.webkit.org/changeset/108628>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.