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

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied 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 82468: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=82468&action=review

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