[webkit-reviews] review granted: [Bug 48510] [GTK] Implement WebEventFactory, WebErrors classes for WebKit2 : [Attachment 83283] Proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 48510: [GTK] Implement WebEventFactory, WebErrors classes for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=48510

Attachment 83283: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=83283&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.