Bug 166759 – [GTK] Should support key and code properties on keyboard events

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

[GTK] Should support key and code properties on keyboard events

[Gustavo Noronha (kov)]

Created attachment 298199 [details]
Patch

[Michael Catanzaro]

Comment on attachment 298199 [details]
Patch

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

Awesome, thanks. :)

Do you have any thoughts on not-very-related bug #136430? You probably looked over the code where we can set that GdkEventKey::send_event?

Anyway, I guess Carlos will want to review this too. My $0.02:

> Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:395
> +            memset(&utf8, 0, sizeof(utf8));

No need for memset, write char utf8[7] = { 0 };

> Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:396
> +            g_unichar_to_utf8(unicodeCharacter, reinterpret_cast<gchar*>(&utf8));

Don't cast from char* to gchar*, they are the same type. :P

[Michael Catanzaro]

(In reply to comment #2)
> > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:396
> > +            g_unichar_to_utf8(unicodeCharacter, reinterpret_cast<gchar*>(&utf8));
> 
> Don't cast from char* to gchar*, they are the same type. :P

Sorry, I see it's a cast from char** to gchar*. Hm, why is this? Why do you need a cast there at all?

[Carlos Garcia Campos]

Comment on attachment 298199 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        - fast/events/arrow-keys-on-body.html
> +        - fast/events/constructors/keyboard-event-constructor.html
> +        - fast/events/key-events-in-input-button.html
> +        - fast/events/key-events-in-input-text.html
> +        - fast/events/keyboardevent-code.html
> +        - fast/events/keyboardevent-key.html

Shouldn't we unskip them? or are all these currently failing?

> Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:124
> +        return "Backspace";

Why not ASCIILiteral here too? (and below)

> Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:727
>  // FIXME: This is incomplete.  We should change this to mirror
>  // more like what Firefox does, and generate these switch statements
>  // at build time.

This FIXME applies to to the new methods too, I think we should autogenerate all these long switches. Not necessarily in this patch, though.

[Gustavo Noronha (kov)]

(In reply to comment #2)
> Do you have any thoughts on not-very-related bug #136430? You probably
> looked over the code where we can set that GdkEventKey::send_event?

Unfortunately no idea =(

> > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:395
> > +            memset(&utf8, 0, sizeof(utf8));
> 
> No need for memset, write char utf8[7] = { 0 };
> 
> > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:396
> > +            g_unichar_to_utf8(unicodeCharacter, reinterpret_cast<gchar*>(&utf8));
> 
> Don't cast from char* to gchar*, they are the same type. :P

Oops, yeah, I changed that code quite a bit and it ended up very weird. Thanks!

[Gustavo Noronha (kov)]

(In reply to comment #4)
> > Source/WebCore/ChangeLog:14
> > +        - fast/events/arrow-keys-on-body.html
> > +        - fast/events/constructors/keyboard-event-constructor.html
> > +        - fast/events/key-events-in-input-button.html
> > +        - fast/events/key-events-in-input-text.html
> > +        - fast/events/keyboardevent-code.html
> > +        - fast/events/keyboardevent-key.html
> 
> Shouldn't we unskip them? or are all these currently failing?

Currently failing. I thought about skipping and unskipping on this patch but I thought it would go in quick enough =)
 
> > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:124
> > +        return "Backspace";
> 
> Why not ASCIILiteral here too? (and below)

Ooops, my mistake. I left some of them for a search-and-replace later on and forgot to do it.

> > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:727
> >  // FIXME: This is incomplete.  We should change this to mirror
> >  // more like what Firefox does, and generate these switch statements
> >  // at build time.
> 
> This FIXME applies to to the new methods too, I think we should autogenerate
> all these long switches. Not necessarily in this patch, though.

Agreed. I'll take a look at how Firefox does it.

[Gustavo Noronha (kov)]

Committed r210504: <http://trac.webkit.org/changeset/210504>