Bug 54199 – [GTK] Implement NativeWebKeyboard and WebContext classes for WebKit2

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

[Alejandro G. Castro]

Add those classes and compile them.

[Alejandro G. Castro]

Created attachment 81963 [details]
Proposed patch

[Martin Robinson]

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.

[Alejandro G. Castro]

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

[Alejandro G. Castro]

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

[Martin Robinson]

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.

[Alejandro G. Castro]

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

[Alejandro G. Castro]

Created attachment 83802 [details]
Proposed patch

[Martin Robinson]

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.

[Alejandro G. Castro]

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

[Alejandro G. Castro]

Created attachment 83837 [details]
Proposed patch

I've decided to move all to gdkevent not just the API, looks nicer I think.

[Martin Robinson]

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.

[Alejandro G. Castro]

Landed http://trac.webkit.org/changeset/79711

Thanks martin!