Bug 60258 - [GTK] Make native events code more consistent
Summary: [GTK] Make native events code more consistent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-05-05 01:49 PDT by Carlos Garcia Campos
Modified: 2011-05-05 09:02 PDT (History)
0 users

See Also:


Attachments
Patch (11.75 KB, patch)
2011-05-05 01:53 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-05-05 01:49:10 PDT
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.
Comment 1 Carlos Garcia Campos 2011-05-05 01:53:15 PDT
Created attachment 92388 [details]
Patch
Comment 2 Martin Robinson 2011-05-05 08:25:16 PDT
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.
Comment 3 Carlos Garcia Campos 2011-05-05 08:38:37 PDT
(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.
Comment 4 Martin Robinson 2011-05-05 08:49:58 PDT
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.
Comment 5 Carlos Garcia Campos 2011-05-05 09:02:11 PDT
Committed r85856: <http://trac.webkit.org/changeset/85856>