Bug 60258 – [GTK] Make native events code more consistent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

Currently we have:

 - NativeWebKeyboardEvent: Uses GOwnPtr for the native GdkEvent and ::nativeEvent() returns a const GdkEvent.
 - NativeWebMouseEvent: Doesn't use GOwnPtr, and ::nativeEvent() returns a GdkEvent.
 - NativeWebWheelEvent: Doesn't use GOwnPtr, and ::nativeEvent() returns a GdkEventScroll.

and for WebEventFactory:

 - createWebKeyboardEvent() receives a const GdkEventKey
 - createWebMouseEvent() receives a GdkEvent
 - createWebWheelEvent() receives a GdkEventScroll

I think the we could unify the way events are created following the current keyboard events code. We should use a GOwnPtr for the native event (it could be destroyed before the class is deleted) and use always const GdkEvent in public API.

[Carlos Garcia Campos]

Created attachment 92388 [details]
Patch

[Martin Robinson]

Comment on attachment 92388 [details]
Patch

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

Very nice cleanup, but I do not understand the addition of the copy constructors.

> Source/WebKit2/Shared/NativeWebMouseEvent.h:52
> +    NativeWebMouseEvent(const NativeWebMouseEvent&);

Why is a copy constructor necessary now?

> Source/WebKit2/Shared/NativeWebWheelEvent.h:52
> +    NativeWebWheelEvent(const NativeWebWheelEvent&);

Ditto.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:99
> +WebMouseEvent WebEventFactory::createWebMouseEvent(const GdkEvent *event, int currentClickCount)

The asterisk should move to the left here.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:141
> +    switch (reinterpret_cast<const GdkEventScroll*>(event)->direction) {

It's probably better to write event->scroll.direction.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:175
> +    guint keyval = reinterpret_cast<const GdkEventKey*>(event)->keyval;

Ditto. event->key.keyval.

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 92388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92388&action=review
> 
> Very nice cleanup, but I do not understand the addition of the copy constructors.
> 
> > Source/WebKit2/Shared/NativeWebMouseEvent.h:52
> > +    NativeWebMouseEvent(const NativeWebMouseEvent&);
> 
> Why is a copy constructor necessary now?

Because the default copy constructor doesn't know how to copy a GdkEvent.

> > Source/WebKit2/Shared/NativeWebWheelEvent.h:52
> > +    NativeWebWheelEvent(const NativeWebWheelEvent&);
> 
> Ditto.
> 
> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:99
> > +WebMouseEvent WebEventFactory::createWebMouseEvent(const GdkEvent *event, int currentClickCount)
> 
> The asterisk should move to the left here.

Ok.

> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:141
> > +    switch (reinterpret_cast<const GdkEventScroll*>(event)->direction) {
> 
> It's probably better to write event->scroll.direction.

Ok.

> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:175
> > +    guint keyval = reinterpret_cast<const GdkEventKey*>(event)->keyval;
> 
> Ditto. event->key.keyval.

Ok.

[Martin Robinson]

Comment on attachment 92388 [details]
Patch

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

>>> Source/WebKit2/Shared/NativeWebMouseEvent.h:52
>>> +    NativeWebMouseEvent(const NativeWebMouseEvent&);
>> 
>> Why is a copy constructor necessary now?
> 
> Because the default copy constructor doesn't know how to copy a GdkEvent.

Make sense! Might want to put a small note in the ChangeLog.

[Carlos Garcia Campos]

Committed r85856: <http://trac.webkit.org/changeset/85856>