Bug 54199

Created attachment 81963 [details] Proposed patch

Comment on attachment 81963 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=81963&action=review > Source/WebKit2/Shared/NativeWebKeyboardEvent.h:38 > +#include <gdk/gdk.h> I recommend using a tyepdef here instead of including all of GDK. You may need to change the member below.

(In reply to comment #2) > (From update of attachment 81963 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81963&action=review > > > Source/WebKit2/Shared/NativeWebKeyboardEvent.h:38 > > +#include <gdk/gdk.h> > > I recommend using a tyepdef here instead of including all of GDK. You may need to change the member below. OK, I'll review it.

(In reply to comment #2) > (From update of attachment 81963 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81963&action=review > > > Source/WebKit2/Shared/NativeWebKeyboardEvent.h:38 > > +#include <gdk/gdk.h> > > I recommend using a tyepdef here instead of including all of GDK. You may need to change the member below. I've tried to use a smart pointer but it also requires the struct definition, I guess the only option would be to use a raw pointer and handle it directly but not sure if avoiding the include deserves adding that kind of code. What do you think?

Comment on attachment 81963 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=81963&action=review >>>> Source/WebKit2/Shared/NativeWebKeyboardEvent.h:38 >>>> +#include <gdk/gdk.h> >>> >>> I recommend using a tyepdef here instead of including all of GDK. You may need to change the member below. >> >> OK, I'll review it. > > I've tried to use a smart pointer but it also requires the struct definition, I guess the only option would be to use a raw pointer and handle it directly but not sure if avoiding the include deserves adding that kind of code. What do you think? You should just be able to do a typedef union _GdkEvent GdkEvent; and have it hold a GOwnPtr<GdkEvent>. I believe we do that in other places. > Source/WebKit2/Shared/gtk/NativeWebKeyboardEventGtk.cpp:36 > + , m_nativeEvent(*event) I think you'll need to do gdk_event_copy here to ensure proper reference counting for the members.

(In reply to comment #5) > > > > I've tried to use a smart pointer but it also requires the struct definition, I guess the only option would be to use a raw pointer and handle it directly but not sure if avoiding the include deserves adding that kind of code. What do you think? > > You should just be able to do a typedef union _GdkEvent GdkEvent; and have it hold a GOwnPtr<GdkEvent>. I believe we do that in other places. > You are right I was just missing the proper constructor to make it work.

Created attachment 83802 [details] Proposed patch

Comment on attachment 83802 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=83802&action=review > Source/WebKit2/Shared/gtk/NativeWebKeyboardEventGtk.cpp:39 > +static inline GdkEventKey* copyGdkEventKey(const GdkEventKey* event) > +{ > + return reinterpret_cast<GdkEventKey*>(gdk_event_copy(reinterpret_cast<const GdkEvent*>(event))); > +} I think this may be uneeded. See below. > Source/WebKit2/Shared/gtk/NativeWebKeyboardEventGtk.cpp:43 > + , m_nativeEvent(copyGdkEventKey(event)) Could you just have these accept a GdkEvent* and then just do: , m_nativeEvent(gdk_even_copy(event)->key) > Source/WebKit2/Shared/gtk/NativeWebKeyboardEventGtk.cpp:49 > + , m_nativeEvent(copyGdkEventKey(event.nativeEvent())) Here I guess you can do: gdk_event_copy(reinterpret_cast<const GdkEvent*>(event.nativeEvent())->key) > Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:47 > + // FIXME: Implement. > + return ""; Let's fill this in with the default we use now.

(In reply to comment #8) > (From update of attachment 83802 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83802&action=review > > > Source/WebKit2/Shared/gtk/NativeWebKeyboardEventGtk.cpp:39 > > +static inline GdkEventKey* copyGdkEventKey(const GdkEventKey* event) > > +{ > > + return reinterpret_cast<GdkEventKey*>(gdk_event_copy(reinterpret_cast<const GdkEvent*>(event))); > > +} > > I think this may be uneeded. See below. > > > Source/WebKit2/Shared/gtk/NativeWebKeyboardEventGtk.cpp:43 > > + , m_nativeEvent(copyGdkEventKey(event)) > > Could you just have these accept a GdkEvent* and then just do: > Yeah, I thought about it but then we will have to change the calls to do the reinterpret_cast, this seemed a better option leaving here all the casts. > , m_nativeEvent(gdk_even_copy(event)->key) > > > Source/WebKit2/Shared/gtk/NativeWebKeyboardEventGtk.cpp:49 > > + , m_nativeEvent(copyGdkEventKey(event.nativeEvent())) > > Here I guess you can do: > gdk_event_copy(reinterpret_cast<const GdkEvent*>(event.nativeEvent())->key) > > > Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:47 > > + // FIXME: Implement. > > + return ""; > > Let's fill this in with the default we use now. Ok, I'll do it.

Created attachment 83837 [details] Proposed patch I've decided to move all to gdkevent not just the API, looks nicer I think.

Comment on attachment 83837 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=83837&action=review > Source/WebKit2/UIProcess/gtk/WebView.cpp:98 > void WebView::handleKeyboardEvent(GdkEventKey* event) > { > - m_page->handleKeyboardEvent(NativeWebKeyboardEvent(event)); > + m_page->handleKeyboardEvent(NativeWebKeyboardEvent(reinterpret_cast<GdkEvent*>(event))); > } Might want to change this call to simply take a GdkEvent.

Landed http://trac.webkit.org/changeset/79711 Thanks martin!