Bug 48510

Summary: [GTK] Implement WebEventFactory, WebErrors classes for WebKit2
Product: WebKit Reporter: Amruth Raj <amruthraj>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alex, andersca, aroben, cgarcia, eric, gustavo, kbalazs, mrobinson, ravi.kasibhatla, sam, webkit.review.bot, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 52805    
Attachments:
Description Flags
Implements WebEventFactory, WebErrors, Module, WebPage, WebContext, NativeKeyboardEvent classes
none
WebEventFactory and WebErrors class implementation for GTK port
none
Few other important classes implementation for WebKit2 GTK port
none
WebEventFactory and WebErrors class implementation for GTK port
none
Few other important classes implementation for WebKit2 GTK port
none
Module implementation using GModule
none
NativeWebKeyboard and WebContext class implementation for WebKit2 GTK
none
WebPageGtk implementation for WebKit2 GTK
none
Proposed patch
mrobinson: review-
Proposed patch
mrobinson: review-
Proposed patch
none
Proposed patch
mrobinson: review-
Proposed patch mrobinson: review+

Description Amruth Raj 2010-10-28 04:48:08 PDT
Implementing WebEventFactory, WebErrors, Module, WebPage, WebContext, NativeKeyboardEvent classes on WebKit2
Comment 1 Amruth Raj 2010-10-29 06:23:16 PDT
Created attachment 72324 [details]
Implements WebEventFactory, WebErrors, Module, WebPage, WebContext, NativeKeyboardEvent classes
Comment 2 Ravi Phaneendra Kasibhatla 2010-10-29 07:15:25 PDT
Adding myself to the CC list for this bug.
Comment 3 Ravi Phaneendra Kasibhatla 2010-11-02 09:43:51 PDT
Created attachment 72684 [details]
WebEventFactory and WebErrors class implementation for GTK port

WebEventFactory and WebErrors class implementation for GTK port
Comment 4 Ravi Phaneendra Kasibhatla 2010-11-02 09:45:02 PDT
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.
Comment 5 Ravi Phaneendra Kasibhatla 2011-01-07 07:22:02 PST
Created attachment 78235 [details]
WebEventFactory and WebErrors class implementation for GTK port
Comment 6 Ravi Phaneendra Kasibhatla 2011-01-07 07:22:44 PST
Created attachment 78236 [details]
Few other important classes implementation for WebKit2 GTK port
Comment 7 Carlos Garcia Campos 2011-01-12 02:25:12 PST
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.
Comment 8 Ravi Phaneendra Kasibhatla 2011-01-12 21:40:55 PST
(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.
Comment 9 Carlos Garcia Campos 2011-01-12 23:33:41 PST
(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.
Comment 10 Ravi Phaneendra Kasibhatla 2011-01-14 12:27:01 PST
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.
Comment 11 Ravi Phaneendra Kasibhatla 2011-01-14 12:28:50 PST
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.
Comment 12 Ravi Phaneendra Kasibhatla 2011-01-14 12:30:03 PST
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.
Comment 13 Alejandro G. Castro 2011-02-09 09:57:43 PST
Comment on attachment 78978 [details]
Module implementation using GModule

Already landed http://trac.webkit.org/changeset/78072
Comment 14 WebKit Review Bot 2011-02-09 13:23:24 PST
http://trac.webkit.org/changeset/78074 might have broken GTK Linux 64-bit Debug
Comment 15 Alejandro G. Castro 2011-02-10 05:57:53 PST
Comment on attachment 78980 [details]
NativeWebKeyboard and WebContext class implementation for WebKit2 GTK

Updated version uploaded to the bug 54199
Comment 16 Alejandro G. Castro 2011-02-10 11:47:42 PST
Added new bugs to handle the other patches, so I've renamed the bug before uploading the patch for event factory and errors.
Comment 17 Alejandro G. Castro 2011-02-10 11:49:03 PST
Created attachment 82016 [details]
Proposed patch
Comment 18 Carlos Garcia Campos 2011-02-11 00:10:42 PST
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
Comment 19 Martin Robinson 2011-02-11 07:48:56 PST
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.
Comment 20 Alejandro G. Castro 2011-02-11 09:27:01 PST
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)
Comment 21 Alejandro G. Castro 2011-02-15 09:04:39 PST
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.
Comment 22 Martin Robinson 2011-02-15 09:44:35 PST
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.
Comment 23 Alejandro G. Castro 2011-02-16 06:16:09 PST
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.
Comment 24 Alejandro G. Castro 2011-02-16 16:04:43 PST
Created attachment 82715 [details]
Proposed patch
Comment 25 WebKit Review Bot 2011-02-16 16:09:54 PST
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.
Comment 26 Alejandro G. Castro 2011-02-16 16:12:09 PST
Created attachment 82717 [details]
Proposed patch

Fixed style
Comment 27 Martin Robinson 2011-02-16 16:19:07 PST
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...
Comment 28 Alejandro G. Castro 2011-02-17 01:29:12 PST
(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?
Comment 29 Martin Robinson 2011-02-17 07:30:46 PST
(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.
Comment 30 Alejandro G. Castro 2011-02-17 08:44:58 PST
(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.
Comment 31 Alejandro G. Castro 2011-02-22 01:13:37 PST
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.
Comment 32 Martin Robinson 2011-02-22 08:00:49 PST
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.
Comment 33 Alejandro G. Castro 2011-02-24 09:43:04 PST
Landed http://trac.webkit.org/changeset/79581

One less to go :), thanks for the help.
Comment 34 WebKit Review Bot 2011-02-25 17:15:28 PST
http://trac.webkit.org/changeset/79711 might have broken GTK Linux 64-bit Debug