Bug 45942

Summary: [GTK] fast/forms/listbox-selection.html fails
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dbates, eric, webkit.review.bot, xan.lopez
Priority: P3 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch for this issue dbates: review+

Description Martin Robinson 2010-09-16 20:05:05 PDT
It appears the issue is that keyDown in the EventSender does not handle all the appropriate modifier strings.
Comment 1 Martin Robinson 2010-09-16 20:07:55 PDT
Created attachment 67877 [details]
Patch for this issue
Comment 2 Daniel Bates 2010-09-17 14:57:40 PDT
Comment on attachment 67877 [details]
Patch for this issue

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

This patch looks sane to me.

r=me

> LayoutTests/platform/gtk/Skipped:-1169
> -fast/forms/listbox-selection.html

I did a brief search for all layout tests that call eventSender.keyDown() with a non-empty list of modifiers that are also in the GTK Skipped list and found fast/events/keydown-1.html. Does this still fail?

> WebKitTools/DumpRenderTree/gtk/EventSender.cpp:463
> +    // Get the modifiers from the second argument.
> +    guint modifiers = argumentCount >= 2 ? gdkModifersFromJSValue(context, arguments[1]) : 0;

I noticed that you did not put a similar comment "// Get the modifiers from the second argument." above the other call sites in mouseUpCallback and mouseDownCallback. We may want to consider putting such a comment at these other call sites.
Comment 3 Martin Robinson 2010-09-20 10:43:05 PDT
(In reply to comment #2)
> I did a brief search for all layout tests that call eventSender.keyDown() with a non-empty list of modifiers that are also in the GTK Skipped list and found fast/events/keydown-1.html. Does this still fail?

This one just needed platform-specific results. I've added them.

> I noticed that you did not put a similar comment "// Get the modifiers from the second argument." above the other call sites in mouseUpCallback and mouseDownCallback. We may want to consider putting such a comment at these other call sites.

I just removed this comment. After further consideration it really answers "What?" instead of "Why?" It's superfluous, I think.
Comment 4 Martin Robinson 2010-09-20 10:43:25 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > I did a brief search for all layout tests that call eventSender.keyDown() with a non-empty list of modifiers that are also in the GTK Skipped list and found fast/events/keydown-1.html. Does this still fail?
> 
> This one just needed platform-specific results. I've added them.
> 
> > I noticed that you did not put a similar comment "// Get the modifiers from the second argument." above the other call sites in mouseUpCallback and mouseDownCallback. We may want to consider putting such a comment at these other call sites.
> 
> I just removed this comment. After further consideration it really answers "What?" instead of "Why?" It's superfluous, I think.

By the way, thanks for the awesome review!
Comment 5 Martin Robinson 2010-09-20 10:44:20 PDT
Committed r67860: <http://trac.webkit.org/changeset/67860>
Comment 6 WebKit Review Bot 2010-09-20 11:16:08 PDT
http://trac.webkit.org/changeset/67860 might have broken GTK Linux 32-bit Release