Bug 48510 – [GTK] Implement WebEventFactory, WebErrors classes for WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Amruth Raj]

Implementing WebEventFactory, WebErrors, Module, WebPage, WebContext, NativeKeyboardEvent classes on WebKit2

[Amruth Raj]

Created attachment 72324 [details]
Implements WebEventFactory, WebErrors, Module, WebPage, WebContext, NativeKeyboardEvent classes

[Ravi Phaneendra Kasibhatla]

Adding myself to the CC list for this bug.

[Ravi Phaneendra Kasibhatla]

Created attachment 72684 [details]
WebEventFactory and WebErrors class implementation for GTK port

WebEventFactory and WebErrors class implementation for GTK port

[Ravi Phaneendra Kasibhatla]

Created attachment 72685 [details]
Few other important classes implementation for WebKit2 GTK port

Contains implementation for few other important classes like NativeWebKeyboardEvent, Module, WebContext etc.

[Ravi Phaneendra Kasibhatla]

Created attachment 78235 [details]
WebEventFactory and WebErrors class implementation for GTK port

[Ravi Phaneendra Kasibhatla]

Created attachment 78236 [details]
Few other important classes implementation for WebKit2 GTK port

[Carlos Garcia Campos]

Comment on attachment 78236 [details]
Few other important classes implementation for WebKit2 GTK port

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

I think you could split the patch into different independent pieces.

> WebKit2/ChangeLog:9
> +        * Platform/Module.h:
> +        * Platform/gtk/ModuleGtk.cpp: Added. Initial code using dlopen/dlclose calls (UNIX_X11 specific).

We should use GModule instead since it's portable.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #7)
> (From update of attachment 78236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78236&action=review
> 
> I think you could split the patch into different independent pieces.
The patch has majorly stubbed implementations except for WebPageGtk.cpp. Do you want us split WebPageGtk.cpp to separate patch & keep rest of the files as separate patch?
> 
> > WebKit2/ChangeLog:9
> > +        * Platform/Module.h:
> > +        * Platform/gtk/ModuleGtk.cpp: Added. Initial code using dlopen/dlclose calls (UNIX_X11 specific).
> 
> We should use GModule instead since it's portable.
We will address this.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 78236 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=78236&action=review
> > 
> > I think you could split the patch into different independent pieces.
> The patch has majorly stubbed implementations except for WebPageGtk.cpp. Do you want us split WebPageGtk.cpp to separate patch & keep rest of the files as separate patch?

The point of having different patches, is that a reviewer can r+ one patch and r- all others, for example,  making the review process easier for you too.

[Ravi Phaneendra Kasibhatla]

Created attachment 78978 [details]
Module implementation using GModule

Implemented Module class for WebKit2 GTK port using GModule and split the patch to 3 separate parts for review convenience.

[Ravi Phaneendra Kasibhatla]

Created attachment 78980 [details]
NativeWebKeyboard and WebContext class implementation for WebKit2 GTK

Implemented NativeWebKeyboardEvent & WebContext classes for WebKit2 GTK port. Split the patch to separate parts for review convenience.

[Ravi Phaneendra Kasibhatla]

Created attachment 78981 [details]
WebPageGtk implementation for WebKit2 GTK

Implemented WebPage class for WebKit2 GTK port. Split the patch to separate part for review convenience.

[Alejandro G. Castro]

Comment on attachment 78978 [details]
Module implementation using GModule

Already landed http://trac.webkit.org/changeset/78072

[WebKit Review Bot]

http://trac.webkit.org/changeset/78074 might have broken GTK Linux 64-bit Debug

[Alejandro G. Castro]

Comment on attachment 78980 [details]
NativeWebKeyboard and WebContext class implementation for WebKit2 GTK

Updated version uploaded to the bug 54199

[Alejandro G. Castro]

Added new bugs to handle the other patches, so I've renamed the bug before uploading the patch for event factory and errors.

[Alejandro G. Castro]

Created attachment 82016 [details]
Proposed patch

[Carlos Garcia Campos]

Comment on attachment 82016 [details]
Proposed patch

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

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:40
> +static inline GdkPoint positionForEvent(GdkEventAny* event)

We don't need this, we can use gdk_event_get_coords()

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:67
> +static inline GdkPoint globalPositionForEvent(GdkEventAny* event)

We don't need this either, we can use gdk_event_get_root_coords()

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:113
> +    switch (event->type) {
> +    case GDK_BUTTON_PRESS:
> +        sClickCount = 1;
> +        break;
> +    case GDK_2BUTTON_PRESS:
> +        sClickCount = 2;
> +        break;
> +    case GDK_3BUTTON_PRESS:
> +        sClickCount = 3;
> +        break;
> +    case GDK_MOTION_NOTIFY:
> +    case GDK_BUTTON_RELEASE:
> +        sClickCount = 0;
> +        break;
> +    default:
> +        ASSERT_NOT_REACHED();

I would return the click count for every case instead of using a variable

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:114
> +    };

There's ; there

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:143
> +    case GDK_MOTION_NOTIFY:
> +        motion = (GdkEventMotion*) event;

You can avoid all the casts here by using event->motion.state passing GdkEvent * instead of GdkEventAny to this function

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:156
> +    case GDK_BUTTON_PRESS:
> +    case GDK_2BUTTON_PRESS:
> +    case GDK_3BUTTON_PRESS:
> +    case GDK_BUTTON_RELEASE:
> +        bEvent = (GdkEventButton *)event;

Same here using event->button.button

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:174
> +    guint timestamp = 0;

why not using event->time (you could use gdk_event_get_time()) instead of 0?

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:180
> +    GdkPoint globalPosition = globalPositionForEvent((GdkEventAny *)event);

event is already GdkEventAny, that cast is redundant. I recommend using GdkEvent instead of GdkEventAny anyway.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:257
> +    if ((state & GDK_SHIFT_MASK) || (state == GDK_KEY_3270_BackTab))

That's wrong, GDK_KEY_3270_BackTab is a keyval, not a state. I think we could use a single funcion modifiersForEvent() that receives an event instead of a state. Then gdk_event_get_state() and if event is a key press then check whether event->keyval is GDK_KEY_3270_BackTab

[Martin Robinson]

Comment on attachment 82016 [details]
Proposed patch

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

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:33
> +

No newline needed here.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:111
> +    int sClickCount = 0;
> +
> +    switch (event->type) {
> +    case GDK_BUTTON_PRESS:
> +        sClickCount = 1;
> +        break;
> +    case GDK_2BUTTON_PRESS:
> +        sClickCount = 2;
> +        break;
> +    case GDK_3BUTTON_PRESS:
> +        sClickCount = 3;
> +        break;
> +    case GDK_MOTION_NOTIFY:
> +    case GDK_BUTTON_RELEASE:
> +        sClickCount = 0;
> +        break;

This code is very suspicious. We need to support click counts higher than 3 and also to detect up front situations where GDK is sending both a GDK_BUTTON_PRESS followed closely by a GDK_3BUTTON_PRESS/GDK_2BUTTON_PRESS. See the code in webkitwebview.cpp to do this. My guess is that we'll need similar code in the widget and to pass in the click count manually.

Please don't use Hungarian notation (sClickCount).

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:139
> +    GdkEventButton* bEvent;

Please do not abbreviate. Use buttonEvent or eventButton.

> Source/WebKit2/Shared/gtk/WebEventFactory.h:32
> +#include <gdk/gdk.h>

Please use typedefs instead of including gdk.h here.

> Source/WebKit2/Shared/gtk/WebEventFactory.h:37
> +    public:

No indentation needed here.

[Alejandro G. Castro]

Thanks for both reviews, I'll check the remarks.

(In reply to comment #19)
> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:111
> > +    int sClickCount = 0;
> > +
> > +    switch (event->type) {
> > +    case GDK_BUTTON_PRESS:
> > +        sClickCount = 1;
> > +        break;
> > +    case GDK_2BUTTON_PRESS:
> > +        sClickCount = 2;
> > +        break;
> > +    case GDK_3BUTTON_PRESS:
> > +        sClickCount = 3;
> > +        break;
> > +    case GDK_MOTION_NOTIFY:
> > +    case GDK_BUTTON_RELEASE:
> > +        sClickCount = 0;
> > +        break;
> 
> This code is very suspicious. We need to support click counts higher than 3 and also to detect up front situations where GDK is sending both a GDK_BUTTON_PRESS followed closely by a GDK_3BUTTON_PRESS/GDK_2BUTTON_PRESS. See the code in webkitwebview.cpp to do this. My guess is that we'll need similar code in the widget and to pass in the click count manually.

We have a quite similar code already working in WebCore in MouseEventGtk.cpp. I'll check if it could cause any issue. Basic click combination at least work for the tests I did (except right click and menu popup)

[Alejandro G. Castro]

Created attachment 82468 [details]
Proposed patch

Thanks for the reviews, I've added all the comments and other issues I've spotted. Regarding the click count we had the same code to generate the events in webkit1. Basically this should be used inside the widget event handler to generate the webkit platform event that we are going to pass to webcore, so in theory we could add the code we have in the widget in the same place for webkit2. I guess this starts to be a proper first version but still things to check and finish.

[Martin Robinson]

Comment on attachment 82468 [details]
Proposed patch

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

> Source/WebCore/platform/PlatformKeyboardEvent.h:182
> +        static String keyIdentifierForGdkKeyCode(guint keyCode);
> +        static int windowsKeyCodeForKeyEvent(unsigned int keycode);
> +        static String singleCharacterString(guint val);

I'd remove the argument names here. Shouldn't windowsKeyCodeForKeyEvent be called windowsKeyCodeForKeyCode?

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:34
> +#include "WindowsKeyboardCodes.h"
> +
> +#include <gdk/gdk.h>

Please remove this extra newline.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:78
> +    if ((state & GDK_SHIFT_MASK)
> +        || ((event->type == GDK_KEY_PRESS || event->type == GDK_KEY_RELEASE) && (reinterpret_cast<GdkEventKey*>(event)->keyval == GDK_KEY_3270_BackTab)))
> +        modifiers |= WebEvent::ShiftKey;

Why do we special case BackTab?  Instead of casting to a GdkEventKey, it's better to use event->key.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:163
> +    float deltaX = 0;
> +    float deltaY = 0;

Why have a pair of floats and a FloatSize? It would make more sense just to keep a FloatSize and fiddle that.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:165
> +    float wheelTicksX = 0;
> +    float wheelTicksY = 0;

You should be able to remove these, seee below.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:166
> +    float scrollbarPixelsPerLine = 40;

This should be static const and there should be a comment explaining where it comes from, since it's a magic number. I'd move it down closer to where it's used to. There's no need to declare all the variables at the beginning of the method.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:187
> +        deltaY = delta;
> +        break;
> +    case GDK_SCROLL_DOWN:
> +        deltaY = -delta;
> +        break;
> +    case GDK_SCROLL_LEFT:
> +        deltaX = delta;
> +        break;
> +    case GDK_SCROLL_RIGHT:
> +        deltaX = -delta;
> +        break;

If delta is always 1, it's much clearer just to say 1 or -1. Even better, just say eventDelta = FloatSize(1, 0); etc.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:195
> +    FloatSize eventWheelTicks(wheelTicksX, wheelTicksY);

Why not just move this definition above where you adjust deltaX and deltaY so that you can dispense with wheelTicks{X,Y}?

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebErrorsGtk.cpp:47
> +#define WebURLErrorDomain   "URLErrorDomain"
> +
> +enum {
> +    WebURLErrorUnknown =                         -1,
> +    WebURLErrorCancelled =                       -999,
> +    WebURLErrorBadURL =                          -1000,
> +    WebURLErrorTimedOut =                        -1001,
> +    WebURLErrorUnsupportedURL =                  -1002,
> +    WebURLErrorCannotFindHost =                  -1003,
> +    WebURLErrorCannotConnectToHost =             -1004,
> +    WebURLErrorNetworkConnectionLost =           -1005,

I believe this file has totally changed since this patch was written. You should now be calling WebError::webKitErrorDomain and Windows and Mac do not define all these enums.

[Alejandro G. Castro]

Thanks for the review.

(In reply to comment #22)
> (From update of attachment 82468 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82468&action=review
> 
> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:78
> > +    if ((state & GDK_SHIFT_MASK)
> > +        || ((event->type == GDK_KEY_PRESS || event->type == GDK_KEY_RELEASE) && (reinterpret_cast<GdkEventKey*>(event)->keyval == GDK_KEY_3270_BackTab)))
> > +        modifiers |= WebEvent::ShiftKey;
> 
> Why do we special case BackTab?  Instead of casting to a GdkEventKey, it's better to use event->key.
>

I did not check the code when reviewing, apparently it was the bug 51587 that added that code to the original file this code comes from.

> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:163
> > +    float deltaX = 0;
> > +    float deltaY = 0;
> 
> Why have a pair of floats and a FloatSize? It would make more sense just to keep a FloatSize and fiddle that.
> 

I agree, I'll change this code.

> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:165
> > +    float wheelTicksX = 0;
> > +    float wheelTicksY = 0;
> 
> You should be able to remove these, seee below.
> 
> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:166
> > +    float scrollbarPixelsPerLine = 40;
> 
> This should be static const and there should be a comment explaining where it comes from, since it's a magic number. I'd move it down closer to where it's used to. There's no need to declare all the variables at the beginning of the method.
> 
> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:187
> > +        deltaY = delta;
> > +        break;
> > +    case GDK_SCROLL_DOWN:
> > +        deltaY = -delta;
> > +        break;
> > +    case GDK_SCROLL_LEFT:
> > +        deltaX = delta;
> > +        break;
> > +    case GDK_SCROLL_RIGHT:
> > +        deltaX = -delta;
> > +        break;
> 
> If delta is always 1, it's much clearer just to say 1 or -1. Even better, just say eventDelta = FloatSize(1, 0); etc.
> 
> > Source/WebKit2/Shared/gtk/WebEventFactory.cpp:195
> > +    FloatSize eventWheelTicks(wheelTicksX, wheelTicksY);
> 
> Why not just move this definition above where you adjust deltaX and deltaY so that you can dispense with wheelTicks{X,Y}?
>

I agree, I'm going to review it. This code comes from the old WheelEventEvent, I guess that is the reason for most of the decisions there.

> > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebErrorsGtk.cpp:47
> > +#define WebURLErrorDomain   "URLErrorDomain"
> > +
> > +enum {
> > +    WebURLErrorUnknown =                         -1,
> > +    WebURLErrorCancelled =                       -999,
> > +    WebURLErrorBadURL =                          -1000,
> > +    WebURLErrorTimedOut =                        -1001,
> > +    WebURLErrorUnsupportedURL =                  -1002,
> > +    WebURLErrorCannotFindHost =                  -1003,
> > +    WebURLErrorCannotConnectToHost =             -1004,
> > +    WebURLErrorNetworkConnectionLost =           -1005,
> 
> I believe this file has totally changed since this patch was written. You should now be calling WebError::webKitErrorDomain and Windows and Mac do not define all these enums.

Yep, good point.

Thanks again.

[Alejandro G. Castro]

Created attachment 82715 [details]
Proposed patch

[WebKit Review Bot]

Attachment 82715 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebErrorsGtk.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alejandro G. Castro]

Created attachment 82717 [details]
Proposed patch

Fixed style

[Martin Robinson]

Comment on attachment 82715 [details]
Proposed patch

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

Sorry. There were many things that I missed in my previous review. Below are some suggestions.

> Source/WebCore/platform/PlatformKeyboardEvent.h:182
> +        static String keyIdentifierForGdkKeyCode(guint);
> +        static int windowsKeyCodeForKeyCode(unsigned int);
> +        static String singleCharacterString(guint);

Sorry that I missed this before! For consistency, the second should probably be windowsKeyCodeForGdkKeyCode(guint) and the third should probably be singleCharacterStringForGdkKeyCode.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:44
> +static inline int clickCount(GdkEvent* event)
> +{
> +    switch (event->type) {
> +    case GDK_BUTTON_PRESS:

Please open a bug for proper click counting and link to the bug here with a FIXME.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:61
> +    ASSERT_NOT_REACHED();

Might as well put this in the default case above.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:72
> +    if (!gdk_event_get_state(event, &state))
> +        ASSERT_NOT_REACHED();

Why not just ASSERT(gdk_event_get_state(event, &state)) ?

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:78
> +    if ((state & GDK_SHIFT_MASK)
> +        || ((event->type == GDK_KEY_PRESS || event->type == GDK_KEY_RELEASE) && (event->key.keyval == GDK_KEY_3270_BackTab)))
> +        modifiers |= WebEvent::ShiftKey;

Still probably should put a comment here explaining where this comes from / why it's needed.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:121
> +    double timestamp = gdk_event_get_time(event);

No need to cache the timestamp, just call this when you use it.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:124
> +    WebMouseEvent::Button button = buttonForEvent(event);

Ditto.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:129
> +    IntPoint position(x, y);

Ditto.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:130
> +    IntPoint globalPosition(xRoot, yRoot);

Ditto.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:131
> +    int clickcount = clickCount(event);

Ditto.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:146
> +    default :

No space after default.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:155
> +    WebWheelEvent::Granularity granularity = WebWheelEvent::ScrollByPixelWheelEvent;

No need to cache this.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:157
> +    unsigned timestamp = gdk_event_get_time(reinterpret_cast<GdkEvent*>(event));

Ditto.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:162
> +    IntPoint position(x, y);

Ditto.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:163
> +    IntPoint globalPosition(xRoot, yRoot);

Ditto.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:164
> +    WebEvent::Modifiers modifiers = modifiersForEvent(reinterpret_cast<GdkEvent*>(event));

Ditto.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:179
> +    case GDK_SCROLL_RIGHT:
> +        wheelTicks = FloatSize(-1, 0);
> +        break;

This should also have a case default: with ASSERT_NOT_REACHED();

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:182
> +    // FIXME: retrieve the user setting for the number of lines to scroll on each wheel event

Please open a bug for this issue and link to the bug here.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:184
> +    FloatSize delta(wheelTicks.width()*step, wheelTicks.height()*step);

Need spaces around the asterisks here.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:199
> +    bool isKeypad = event->keyval >= GDK_KEY_KP_Space && event->keyval <= GDK_KEY_KP_9;

I'd like to see this split out into a static function isKeyCodeFromKeyPad or something like that.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:205
> +    return WebKeyboardEvent(type, text, unmodifiedText, keyIdentifier, windowsVirtualKeyCode, nativeVirtualKeyCode, macCharCode,
> +                            autoRepeat, isKeypad, isSystemKey, modifiers, timestamp);

Instead of caching all these things locally, I think it makes sense to just split them out here, like so:

 return WebKeyboardEvent((event->type == GDK_KEY_RELEASE) ? WebEvent::KeyUp : WebEvent::KeyDown,
                                                    PlatformKeyboardEvent::singleCharacterString(event->keyval),
                                                    PlatformKeyboardEvent::singleCharacterString(event->keyval),
                                                    PlatformKeyboardEvent::keyIdentifierForGdkKeyCode(event->keyval),
                                                    PlatformKeyboardEvent::windowsKeyCodeForKeyCode(event->keyval),
                                                    static_cast<int>(event->keyval),
                                                    0 /* macCharCode */,
                                                    ... etc...

[Alejandro G. Castro]

(In reply to comment #27)
> (From update of attachment 82715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82715&action=review
> 
> Sorry. There were many things that I missed in my previous review. Below are some suggestions.
> 

No worries :), I was thinking that maybe we should apply other solution to this code, when you added the first comments improving the parts of the code that comes from webkit1 (PlatformMouseEvent, PlatformKeyboardEvent, etc.) it started to feel weird that we do not fix the code there. Initially I discarded it because creating a object just to create another one does not seem a good option (I guess that was the idea of the first implementation of this patch considering other ports are also copying the code) but I now think improving the code here a leaving the other code duplicated there seems worse.

So I'm going to try that option and check what we could have, in that case we can open new bugs improving the old code and add patches there. Does this solution sound good?

[Martin Robinson]

(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 82715 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=82715&action=review
> > 
> > Sorry. There were many things that I missed in my previous review. Below are some suggestions.
> > 
> 
> No worries :), I was thinking that maybe we should apply other solution to this code, when you added the first comments improving the parts of the code that comes from webkit1 (PlatformMouseEvent, PlatformKeyboardEvent, etc.) it started to feel weird that we do not fix the code there. Initially I discarded it because creating a object just to create another one does not seem a good option (I guess that was the idea of the first implementation of this patch considering other ports are also copying the code) but I now think improving the code here a leaving the other code duplicated there seems worse.
> 
> So I'm going to try that option and check what we could have, in that case we can open new bugs improving the old code and add patches there. Does this solution sound good?

Sure. Some of the things we use in WebKit1 could be abstracted up into WebCore.

[Alejandro G. Castro]

(In reply to comment #29)
> (In reply to comment #28)
> > 
> > So I'm going to try that option and check what we could have, in that case we can open new bugs improving the old code and add patches there. Does this solution sound good?
> 
> Sure. Some of the things we use in WebKit1 could be abstracted up into WebCore.

I've checked current situation with the events and it is not natural to use the platform events because we have enums redefined in webkit2, so it seems it would require a bigger patch to allow the use of the platformfooevents as webevents, but not sure how complex it could be, does anyone know about this? I've added other Apple people that could know about it.

So now without more information I think it is easier to finish the first support for gtk using a solution like the one we are implementing and leave the use of platformfooevents for other more general patch.

[Alejandro G. Castro]

Created attachment 83283 [details]
Proposed patch

I think all of the points are updated. I got rid of the 3270 keyval check, it is not clear when someone using that kind of terminal wants to have the Shift pressed also. I also added a new clickCount parameter and added the code to calculate it to the webview patch that I'm going to upload to the other bug, that way we do not use the count of the event but the one we have in our widget.

[Martin Robinson]

Comment on attachment 83283 [details]
Proposed patch

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

Great! I have a few consistency nits below.

> Source/WebCore/platform/PlatformKeyboardEvent.h:182
> +        static String keyIdentifierForGdkKeyCode(guint);
> +        static int windowsKeyCodeForGdkKeyCode(unsigned int);
> +        static String singleCharacterString(guint);

Please use either allk "guint" here or all "unsigned int" or all "unsigned." Change singleCharacterString to singleCharacterStringForGdkKeyCode for consistency.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:42
> +static inline bool isKeyCodeFromKeyPad(unsigned keyval)

Just for consistency, I think this should be isGdkKeyCodeFromKeyPad(<guint or unsigned int or unsigned> keyCode)

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:122
> +    return WebMouseEvent(type, buttonForEvent(event), IntPoint(x, y), IntPoint(xRoot, yRoot), 0 /* deltaX */, 0 /* delta Y*/,
> +                         0 /*deltaZ */, currentClickCount, static_cast<WebEvent::Modifiers>(0), gdk_event_get_time(event));

Pretty nitful here, but I think it makes sense to break this up to one or two per line for readability. :)

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:169
> +    return WebKeyboardEvent((event->type == GDK_KEY_RELEASE) ? WebEvent::KeyUp : WebEvent::KeyDown,
> +                            PlatformKeyboardEvent::singleCharacterString(event->keyval),
> +                            PlatformKeyboardEvent::singleCharacterString(event->keyval),
> +                            PlatformKeyboardEvent::keyIdentifierForGdkKeyCode(event->keyval),
> +                            PlatformKeyboardEvent::windowsKeyCodeForGdkKeyCode(event->keyval),
> +                            static_cast<int>(event->keyval), 0 /* macCharCode */, false /* isAutoRepeat */,
> +                            isKeyCodeFromKeyPad(event->keyval), false /* isSystemKey */,
> +                            modifiersForEvent(reinterpret_cast<GdkEvent*>(event)),
> +                            gdk_event_get_time(reinterpret_cast<GdkEvent*>(event)));

Ditto.

[Alejandro G. Castro]

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

One less to go :), thanks for the help.

[WebKit Review Bot]

http://trac.webkit.org/changeset/79711 might have broken GTK Linux 64-bit Debug